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 -- 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