Re: [PATCH] net/udp: do not touch skb->peeked unless really needed

On Tue, 2016-12-06 at 18:08 +0100, Paolo Abeni wrote:

On Tue, 2016-12-06 at 11:34 +0100, Paolo Abeni wrote:

quoted

On Mon, 2016-12-05 at 09:57 -0800, Eric Dumazet wrote:

quoted

From: Eric Dumazet <edumazet@google.com>

In UDP recvmsg() path we currently access 3 cache lines from an skb
while holding receive queue lock, plus another one if packet is
dequeued, since we need to change skb->next->prev

1st cache line (contains ->next/prev pointers, offsets 0x00 and 0x08)
2nd cache line (skb->len & skb->peeked, offsets 0x80 and 0x8e)
3rd cache line (skb->truesize/users, offsets 0xe0 and 0xe4)

skb->peeked is only needed to make sure 0-length packets are properly
handled while MSG_PEEK is operated.

I had first the intent to remove skb->peeked but the "MSG_PEEK at
non-zero offset" support added by Sam Kumar makes this not possible.

This patch avoids one cache line miss during the locked section, when
skb->len and skb->peeked do not have to be read.

It also avoids the skb_set_peeked() cost for non empty UDP datagrams.

Signed-off-by: Eric Dumazet <edumazet@google.com>

Thank you for all the good work.

After all your improvement, I see the cacheline miss in inet_recvmsg()
as a major perf offender for the user space process in the udp flood
scenario due to skc_rxhash sharing the same sk_drops cacheline.

Using an udp-specific drop counter (and an sk_drops accessor to wrap
sk_drops access where needed), we could avoid such cache miss. With that
- patch for udp.h only below - I get 3% improvement on top of all the
pending udp patches, and the gain should be more relevant after the 2
queues rework. What do you think ?

Here follow what I'm experimenting. 

Well, new socket layout makes this kind of patches not really needed ?

	/* --- cacheline 2 boundary (128 bytes) was 8 bytes ago --- */
	socket_lock_t              sk_lock;              /*  0x88  0x20 */
	atomic_t                   sk_drops;             /*  0xa8   0x4 */
	int                        sk_rcvlowat;          /*  0xac   0x4 */
	struct sk_buff_head        sk_error_queue;       /*  0xb0  0x18 */
	/* --- cacheline 3 boundary (192 bytes) was 8 bytes ago --- */

Nothing in UDP fast path needs to access a field from this cache line, but sk_drops.

So wherever we put sk_drops (even in a cache line of its own), we will still have
false sharing on it.

And really this should be fine.
Eventually we could have per NUMA node counter to help a bit.

I mentioned at some point that we can very easily instruct 
sock_skb_set_dropcount() to not read sk_drops if application
does not care about getting sk_drops ;)

https://patchwork.kernel.org/patch/9405677/

Now sk_drops was moved, the plan is to submit this patch in an official way.

Thanks !