2007-12-14 14:28:39

by Chuck Lever

[permalink] [raw]
Subject: Re: [PATCH 09/38] svc: Add a transport function that checks for write space

On Dec 13, 2007, at 5:20 PM, Tom Tucker wrote:
> On Thu, 2007-12-13 at 16:45 -0500, Chuck Lever wrote:
>> On Dec 13, 2007, at 4:33 PM, J. Bruce Fields wrote:
>>> On Wed, Dec 12, 2007 at 01:10:17PM -0500, Chuck Lever wrote:
>>>> On Dec 11, 2007, at 6:32 PM, Tom Tucker wrote:
>>>>> + required = atomic_read(&svsk->sk_reserved) + serv->sv_max_mesg;
>>>>> + wspace = sk_stream_wspace(svsk->sk_sk);
>>>>> +
>>>>> + if (wspace < sk_stream_min_wspace(svsk->sk_sk))
>>>>> + return 0;
>>>>> + if (required * 2 > wspace)
>>>>> + return 0;
>>>>
>>>> Since "required" is an int, this test can behave differently than
>>>> the one
>>>> it replaces.
>>>
>>> If sk_reserved can approach half of 2^31, for example, then
>>> surely we
>>> have bigger problems?
>>
>>
>> What stops sk_reserved from going negative?
>
> Nothing actively _stops_ it from going negative. Indirectly it is
> prevented by the "fact" that sv_max_mesg is always greater than the
> amount returned by any read on the socket. If this is not true, then
> sk_reserved can go negative. That would occur in svc_recv when the
> difference between the amount currently reserved (worst case
> sv_max_mesg) and the amount read is subtracted from the amount
> reserved.
> If the amount read were greater than sv_max_mesg then the result would
> be negative (both meanings intended).
>
> I could add a BUG_ON and run some tests.

My (limited) understanding is that the amount of socket output buffer
space the server thinks it needs for sending a reply is subtracted
from sk_reserved. So it seems like svc_reserve() would be the place
to assert that sk_reserved does not go negative.

As this is used mostly by svc_reserve_auth(), that suggests that a
reasonable test case would be running a read/write intensive workload
with some security flavor that uses large authenticators.

--
Chuck Lever
chuck[dot]lever[at]oracle[dot]com