On Thu, 2016-09-22 at 16:21 +0100, Edward Cree wrote:
> 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.
I think that the idea behind using atomic ops directly on
sk_forward_alloc is to avoid adding other fields to the udp_socket.
If we can add some fields to the udp_sock structure, the schema proposed
in this patch should fit better (modulo bugs ;-), always requiring a
single atomic operation at memory reclaiming time and at memory
allocation time.
Paolo
