From: Paolo Abeni <hidden> Date: 2016-09-22 16:14:24
On Thu, 2016-09-22 at 16:21 +0100, Edward Cree wrote:
On 22/09/16 11:33, Paolo Abeni wrote:
quoted
Hi Eric,
On Wed, 2016-09-21 at 16:31 -0700, Eric Dumazet wrote:
quoted
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.
quoted
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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
From: Eric Dumazet <hidden> Date: 2016-09-22 16:30:51
On Thu, 2016-09-22 at 18:14 +0200, Paolo Abeni wrote:
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.
But do we want any additional atomic to begin with ?
Given typical number of UDP sockets on a host, we could reserve/forward
alloc at socket creation time, and when SO_RCVBUF is changed.
I totally understand that this schem does not work with TCP, because TCP
BDP mandates xx MB of buffers per socket, and we really handle millions
of TCP sockets per host,
But for UDP, most applications are working well with a default limit,
and we hardly have more than 1000 UDP sockets.
cat /proc/sys/net/core/rmem_default
212992
From: Paolo Abeni <pabeni@redhat.com> Date: 2016-09-22 20:27:06
On Thu, 2016-09-22 at 09:30 -0700, Eric Dumazet wrote:
On Thu, 2016-09-22 at 18:14 +0200, Paolo Abeni wrote:
quoted
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.
But do we want any additional atomic to begin with ?
Given typical number of UDP sockets on a host, we could reserve/forward
alloc at socket creation time, and when SO_RCVBUF is changed.
That would be very efficient and would probably work on most scenario,
but if/when the system will reach udp memory pressure things will be
very bad: forward allocation on open() will fail and nobody will be able
to create any new udp socket, right ?
We are working on a v2 incorporating the feedback of your previous email
- still keeping the new udp_sock fields.
It looks quite simpler than v1, will work reasonably well in memory
pressure scenario, and performance are measurably better than v1, most
probably comparable with the above solution, since usually no additional
atomic operations (beyond sk_rmem_alloc updating) are performed on
enqueue/dequeue.
Paolo
From: Eric Dumazet <hidden> Date: 2016-09-22 20:34:10
On Thu, 2016-09-22 at 22:27 +0200, Paolo Abeni wrote:
On Thu, 2016-09-22 at 09:30 -0700, Eric Dumazet wrote:
quoted
On Thu, 2016-09-22 at 18:14 +0200, Paolo Abeni wrote:
quoted
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.
But do we want any additional atomic to begin with ?
Given typical number of UDP sockets on a host, we could reserve/forward
alloc at socket creation time, and when SO_RCVBUF is changed.
That would be very efficient and would probably work on most scenario,
but if/when the system will reach udp memory pressure things will be
very bad: forward allocation on open() will fail and nobody will be able
to create any new udp socket, right ?
No, we could allow one page per socket (udp_mem[0]) and applications
would still work.
TCP has the notion of memory pressure, and behaves roughly the same in
this case (one skb is allowed to be received)
The other (fat) sockets could notice udp_memory_pressure is set and
start reclaiming their forward allocations for other sockets.
We have a counter of UDP sockets, so probably doable to compute
udp_mem[2]/number
Anyway, just an idea.
We are working on a v2 incorporating the feedback of your previous email
- still keeping the new udp_sock fields.
It looks quite simpler than v1, will work reasonably well in memory
pressure scenario, and performance are measurably better than v1, most
probably comparable with the above solution, since usually no additional
atomic operations (beyond sk_rmem_alloc updating) are performed on
enqueue/dequeue.
From: Eric Dumazet <hidden> Date: 2016-09-22 20:37:23
On Thu, 2016-09-22 at 13:34 -0700, Eric Dumazet wrote:
On Thu, 2016-09-22 at 22:27 +0200, Paolo Abeni wrote:
quoted
On Thu, 2016-09-22 at 09:30 -0700, Eric Dumazet wrote:
quoted
On Thu, 2016-09-22 at 18:14 +0200, Paolo Abeni wrote:
quoted
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.
But do we want any additional atomic to begin with ?
Given typical number of UDP sockets on a host, we could reserve/forward
alloc at socket creation time, and when SO_RCVBUF is changed.
That would be very efficient and would probably work on most scenario,
but if/when the system will reach udp memory pressure things will be
very bad: forward allocation on open() will fail and nobody will be able
to create any new udp socket, right ?
No, we could allow one page per socket (udp_mem[0]) and applications
would still work.
I meant udp_rmem_min, not udp_mem[0]
udp_rmem_min - INTEGER
Minimal size of receive buffer used by UDP sockets in moderation.
Each UDP socket is able to use the size for receiving data, even if
total pages of UDP sockets exceed udp_mem pressure. The unit is byte.
Default: 1 page
From: Paolo Abeni <pabeni@redhat.com> Date: 2016-09-23 09:56:11
On Thu, 2016-09-22 at 13:34 -0700, Eric Dumazet wrote:
On Thu, 2016-09-22 at 22:27 +0200, Paolo Abeni wrote:
quoted
On Thu, 2016-09-22 at 09:30 -0700, Eric Dumazet wrote:
quoted
On Thu, 2016-09-22 at 18:14 +0200, Paolo Abeni wrote:
quoted
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.
But do we want any additional atomic to begin with ?
Given typical number of UDP sockets on a host, we could reserve/forward
alloc at socket creation time, and when SO_RCVBUF is changed.
That would be very efficient and would probably work on most scenario,
but if/when the system will reach udp memory pressure things will be
very bad: forward allocation on open() will fail and nobody will be able
to create any new udp socket, right ?
No, we could allow one page per socket (udp_mem[0]) and applications
would still work.
TCP has the notion of memory pressure, and behaves roughly the same in
this case (one skb is allowed to be received)
The other (fat) sockets could notice udp_memory_pressure is set and
start reclaiming their forward allocations for other sockets.
I agree, that would work. Anyway I think it will more invasive than the
proposed code, after the currently ongoing rework. With the above, on
memory pressure, reclaim will be needed on dequeue, and scheduling will
still needed even on enqueue.
The overall behavior of the two approach will be similar: lazy
reclaiming under memory pressure or on sk closing, and rare memory
scheduling.