Could we avoid touching dst->refcount in some cases ?

tbench has hard time incrementing decrementing the route cache refcount shared by all communications on localhost.

On real world, we also have this problem on RTP servers sending many UDP frames to mediagateways, especially big ones handling thousand of streams.

Given that route entries are using RCU, we probably can avoid incrementing their refcount in case of connected sockets ?

Quoted 2 lines · Eric Dumazet

tbench has hard time incrementing decrementing the route cache refcount shared by all communications on localhost.

iirc there was a patch some time ago to use per CPU loopback devices to avoid this, but it was considered too much a benchmark hack. As core counts increase it might stop being that though.

Quoted 6 lines

On real world, we also have this problem on RTP servers sending many UDP frames to mediagateways, especially big ones handling thousand of streams.

Given that route entries are using RCU, we probably can avoid incrementing their refcount in case of connected sockets ?

Normally they can be hold over sleeps or queuing of skbs too, and RCU
doesn't handle that. To make it handle that you would need to define a
custom RCU period designed for this case, but this would be probably
tricky and fragile: especially I'm not sure even if you had a "any
packet queued" RCU method it be guaranteed to always finish because there is no fixed upper livetime of a packet.

The other issue is that on preemptible kernels you would need to disable preemption all the time such a routing entry is hold, which could be potentially quite long.

Quoted 5 lines · Andi Kleen

Quoted 2 lines · Eric Dumazet

tbench has hard time incrementing decrementing the route cache refcount shared by all communications on localhost.

Quoted 3 lines

iirc there was a patch some time ago to use per CPU loopback devices to avoid this, but it was considered too much a benchmark hack. As core counts increase it might stop being that though.

Well, you probably mention Stephen patch to avoid dirtying other contended cache lines (one napi structure per cpu)

Quoted 6 lines

Quoted 5 lines · Andi Kleen

Quoted 2 lines · Eric Dumazet

tbench has hard time incrementing decrementing the route cache refcount shared by all communications on localhost.

Quoted 6 lines

Quoted 3 lines

iirc there was a patch some time ago to use per CPU loopback devices to avoid this, but it was considered too much a benchmark hack. As core counts increase it might stop being that though.

Well, you probably mention Stephen patch to avoid dirtying other contended cache lines (one napi structure per cpu)

No that patch wasn't from Stephen. iirc it was from someone at SGI.

Quoted 7 lines · Andi Kleen

Quoted 2 lines · Eric Dumazet

tbench has hard time incrementing decrementing the route cache refcount shared by all communications on localhost.

iirc there was a patch some time ago to use per CPU loopback devices to avoid this, but it was considered too much a benchmark hack. As core counts increase it might stop being that though.

Well, you probably mention Stephen patch to avoid dirtying other contended cache lines (one napi structure per cpu)

Having multiple loopback dev would really be a hack I agree.

Quoted 7 lines

Quoted 6 lines

On real world, we also have this problem on RTP servers sending many UDP frames to mediagateways, especially big ones handling thousand of streams.

Given that route entries are using RCU, we probably can avoid incrementing their refcount in case of connected sockets ?

Quoted 9 lines

Normally they can be hold over sleeps or queuing of skbs too, and RCU
doesn't handle that. To make it handle that you would need to define a
custom RCU period designed for this case, but this would be probably
tricky and fragile: especially I'm not sure even if you had a "any
packet queued" RCU method it be guaranteed to always finish because there is no fixed upper livetime of a packet.

The other issue is that on preemptible kernels you would need to disable preemption all the time such a routing entry is hold, which could be potentially quite long.

Well, in case of UDP, we call ip_push_pending_frames() and this one does the increment of refcount (again). I was not considering avoiding the refcount hold we do when queing a skb in transmit queue, only during a short period of time. Oh well, ip_append_data() might sleep, so this cannot work...

I agree avoiding one refcount increment/decrement is probably not a huge gain, considering we *have* to do the increment, but when many cpus are using UDP send/receive in //, this might show a gain somehow.

So maybe we could make ip_append_data() (or its callers) a litle bit smarter, avoiding increment/decrement if possible.

We can reduce pressure on dst entry refcount that slowdown UDP transmit path on SMP machines. This pressure is visible on RTP servers when delivering content to mediagateways, especially big ones, handling thousand of streams. Several cpus send UDP frames to the same destination, hence use the same dst entry.

This patch makes ip_push_pending_frames() steal the refcount its callers had to take when filling inet->cork.dst.

This doesnt avoid all refcounting, but still gives speedups on SMP, on UDP/RAW transmit path.

Signed-off-by Eric Dumazet <[email protected]>

[PATCH] net: avoid a pair of dst_hold()/dst_release() in ip_append_data()

We can reduce pressure on dst entry refcount that slowdown UDP transmit path on SMP machines. This pressure is visible on RTP servers when delivering content to mediagateways, especially big ones, handling thousand of streams. Several cpus send UDP frames to the same destination, hence use the same dst entry.

This patch makes ip_append_data() eventually steal the refcount its callers had to take on the dst entry.

This doesnt avoid all refcounting, but still gives speedups on SMP, on UDP/RAW transmit path

Signed-off-by Eric Dumazet <[email protected]>

Thinking more I believe we can do similar tricks for all TCP transmit traffic.

Packets bound to sockets never outlive those sockets (and thus their cached routes) unless we skb_orphan().

The only not covered case is where the socket cached route is reset or changed. We could defer the dst put until the transmit queue reaches a certain point, kind of like a retransmit queue RCU :-)

Just some ideas...

So maybe we could make ip_append_data() (or its callers) a litle bit smarter, avoiding increment/decrement if possible.

From: Eric Dumazet <[email protected]>
Date: Mon, 24 Nov 2008 11:14:29 +0100

Quoted 2 lines

So maybe we could make ip_append_data() (or its callers) a litle bit smarter, avoiding increment/decrement if possible.

These ideas are interesting but hard to make work.

I think the receive path has more chance of getting gains from this, to be honest.

One third (effectively) of TCP stream packets are ACKs and freed immediately. This means that the looked up route does not escape the packet receive path. So we could elide the counter increment in that case.

In fact, once we queue even TCP data, there is no need for that cached skb->dst route any longer.

So pretty much all TCP packets could avoid the dst refcounting on receive.

Very interesting. So we could try the following path :

1) First try to release dst when queueing skb to various queues (UDP, TCP, ...) while its hot. Reader wont have to release it while its cold.

2) Check if we can handle the input path without any refcount dirtying ?

To make the transition easy, we could use a bit on skb to mark dst being not refcounted (ie no dst_release() should be done on it)

From: Eric Dumazet <[email protected]>
Date: Tue, 25 Nov 2008 05:43:32 +0100

Quoted 11 lines

Very interesting. So we could try the following path :

1) First try to release dst when queueing skb to various queues (UDP, TCP, ...) while its hot. Reader wont have to release it while its cold.

2) Check if we can handle the input path without any refcount dirtying ?

To make the transition easy, we could use a bit on skb to mark dst being not refcounted (ie no dst_release() should be done on it)

It is possible to make this self-auditing. For example, by using the usual trick where we encode a pointer in an unsigned long and use the low bits for states.

In the first step, make each skb->dst access go through some accessor inline function.

Next, audit the paths where skb->dst's can "escape" the pure packet input path. Add annotations, in the form of a inline function call, for these locations.

Also, audit the other locations where we enqueue into a socket queue and no longer care about the skb->dst, and annotate those with another inline function.

Finally, the initial skb->dst assignment in the input path doesn't grab a reference, but sets the low bit ("refcount pending") in the encoded skb->dst pointer. The skb->dst "escape" inline function performs the deferred refcount grab. And kfree_skb() is taught to not dst_release() on skb->dst's which have the low bit set.

Anyways, something like that.

Hum... for a tcp connection, dst refcount should already be pinned by a sk->sk_dst_cache. Maybe test refcount value, and if this value is > 1, dont take a reference. (given rcu_read_lock() is done before calling ip_rcv_finish())

In the meantime, what do you think of the following patch ?

[PATCH] net: release skb->dst in sock_queue_rcv_skb()

When queuing a skb to sk->sk_receive_queue, we can release its dst, not anymore needed.
Since current cpu did the dst_hold(), refcount is probably still hot int this cpu caches.

This avoids readers to access the original dst to decrement its refcount, possibly a long time after packet reception. This should speedup UDP and RAW receive path.

Signed-off-by Eric Dumazet <[email protected]>

From: Eric Dumazet <[email protected]>
Date: Wed, 26 Nov 2008 01:00:30 +0100

Quoted 9 lines

In the meantime, what do you think of the following patch ?

[PATCH] net: release skb->dst in sock_queue_rcv_skb()

When queuing a skb to sk->sk_receive_queue, we can release its dst, not anymore needed.
Since current cpu did the dst_hold(), refcount is probably still hot int this cpu caches.

Quoted 5 lines

This avoids readers to access the original dst to decrement its refcount, possibly a long time after packet reception. This should speedup UDP and RAW receive path.

Signed-off-by Eric Dumazet <[email protected]>

I guess the idea is that if we release quickly we'll not have to reget the cacheline in owned state.

I wonder if this might actually slightly hurt loads like tbench where we are banging on the refcnt constantly on every cpu anyways.

Can you do a quick check?

Quoted 5 lines · David Miller

From: Eric Dumazet <[email protected]>
Date: Wed, 26 Nov 2008 01:00:30 +0100

Can you do a quick check?

Sure I can do a check :)

No impact at all on tbench, unless this bench can run in UDP mode :)

[PATCH] net: release skb->dst in sock_queue_rcv_skb()

When queuing a skb to sk->sk_receive_queue, we can release its dst, not anymore needed.
Since current cpu did the dst_hold(), refcount is probably still hot int this cpu caches.

This avoids readers to access the original dst to decrement its refcount, possibly a long time after packet reception. This should speedup UDP and RAW receive path.

Signed-off-by Eric Dumazet <[email protected]>

plain text document attachment (sock_queue_rcv_skb.patch)

net/core/sock.c +0 -0

index a4e840e..b287645 100644

Quoted 2 lines

[PATCH] net: release skb->dst in sock_queue_rcv_skb()

IP_PKTINFO cmsg data is one post-queueing user:

From: Mark McLoughlin <[email protected]>
Date: Wed, 17 Dec 2008 11:25:01 +0000

Quoted 3 lines · Eric Dumazet

Quoted 2 lines

[PATCH] net: release skb->dst in sock_queue_rcv_skb()

...

Quoted 1 line

IP_PKTINFO cmsg data is one post-queueing user:

Eric, we'll need to rever this change I think.

Quoted 1 line · David Miller

Eric, we'll need to rever this change I think.

I am afraid we have to revert it, yes.

About ip_cmsg_recv_pktinfo() :

iif can be found in skb->iif instead of rt->rt_iif, but I am not sure about rt_spec_dst : Shouldnt we find it in ip_hdr(skb)->saddr ?

Do you know if we really need rtable in ip_cmsg_recv_pktinfo() ?