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 -- To unsubscribe from this list: send the line "unsubscribe linux-nfs" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html