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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <1481046434.18162.599.camel@edumazet-glaptop3.roam.corp.google.com>
Date:   Tue, 06 Dec 2016 09:47:14 -0800
From:   Eric Dumazet <eric.dumazet@...il.com>
To:     Paolo Abeni <pabeni@...hat.com>
Cc:     David Miller <davem@...emloft.net>,
        netdev <netdev@...r.kernel.org>,
        Willem de Bruijn <willemb@...gle.com>
Subject: 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:
> > On Mon, 2016-12-05 at 09:57 -0800, Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@...gle.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@...gle.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 !