On 22/09/16 11:33, Paolo Abeni wrote:
> Hi Eric,
>
> On Wed, 2016-09-21 at 16:31 -0700, Eric Dumazet wrote:
>> Also does inet_diag properly give the forward_alloc to user ?
>>
>> $ ss -mua
>> State Recv-Q Send-Q Local Address:Port Peer Addres
>> s:Port
>> UNCONN 51584 0 *:52460 *:*
>> skmem:(r51584,rb327680,t0,tb327680,f1664,w0,o0,bl0,d575)
> Thank you very much for reviewing this!
>
> My bad, there is still a race which leads to temporary negative values
> of fwd. I feel the fix is trivial but it needs some investigation.
>
>> Couldn't we instead use an union of an atomic_t and int for
>> sk->sk_forward_alloc ?
> That was our first attempt, but we had some issue on mem scheduling; if
> we use:
>
> if (atomic_sub_return(size, &sk->sk_forward_alloc_atomic) < 0) {
> // fwd alloc
> }
>
> that leads to inescapable, temporary, negative value for
> sk->sk_forward_alloc.
>
> Another option would be:
>
> again:
> fwd = atomic_read(&sk->sk_forward_alloc_atomic);
> if (fwd > size) {
> if (atomic_cmpxchg(&sk->sk_forward_alloc_atomic, fwd, fwd -
> size) != fwd)
> goto again;
> } else
> // fwd alloc
>
> which would be bad under high contention.
Apologies if I'm misunderstanding the problem, but couldn't you have two
atomic_t fields, 'internal' and 'external' forward_alloc. Then
if (atomic_sub_return(size, &sk->sk_forward_alloc_internal) < 0) {
atomic_sub(size, &sk->sk_forward_alloc);
// fwd alloc
} else {
atomic_add(size, &sk->sk_forward_alloc_internal);
}
or something like that. Then sk->sk_forward_alloc never sees a negative
value, and is always >= sk->sk_forward_alloc_internal. Of course places
that go the other way would have to add to sk->sk_forward_alloc first,
before adding to sk->sk_forward_alloc_internal, to maintain that invariant.
Would that help matters at all?
-Ed
