[PATCH net-next] net: add a prefetch in socket backlog processing

[PATCH net-next] net: add a prefetch in socket backlog processing

[Eric Dumazet]

From: Eric Dumazet <edumazet@google.com>

TCP or UDP stacks have big enough latencies that prefetching next
pointer is worth it.

Signed-off-by: Eric Dumazet <edumazet@google.com>
---
 net/core/sock.c |    1 +
 1 file changed, 1 insertion(+)

diff --git a/net/core/sock.c b/net/core/sock.c
index 836bca6..1a88351 100644
--- a/net/core/sock.c
+++ b/net/core/sock.c
@@ -1700,6 +1700,7 @@ static void __release_sock(struct sock *sk)
 		do {
 			struct sk_buff *next = skb->next;
 
+			prefetch(next);
 			WARN_ON_ONCE(skb_dst_is_noref(skb));
 			skb->next = NULL;
 			sk_backlog_rcv(sk, skb);

Re: [PATCH net-next] net: add a prefetch in socket backlog processing

[Joe Perches]

On Tue, 2012-05-01 at 04:07 +0200, Eric Dumazet wrote:
> From: Eric Dumazet <edumazet@google.com>
> 
> TCP or UDP stacks have big enough latencies that prefetching next
> pointer is worth it.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> ---
>  net/core/sock.c |    1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/net/core/sock.c b/net/core/sock.c
> index 836bca6..1a88351 100644
> --- a/net/core/sock.c
> +++ b/net/core/sock.c
> @@ -1700,6 +1700,7 @@ static void __release_sock(struct sock *sk)
>  		do {
>  			struct sk_buff *next = skb->next;
>  
> +			prefetch(next);
>  			WARN_ON_ONCE(skb_dst_is_noref(skb));
>  			skb->next = NULL;
>  			sk_backlog_rcv(sk, skb);

Hi Eric.

Why should next be "prefetch"ed when
two lines below it's set to null and
the only use is as a pointer not as
an apparently undereferenced pointer?

Re: [PATCH net-next] net: add a prefetch in socket backlog processing

[Eric Dumazet]

On Mon, 2012-04-30 at 20:24 -0700, Joe Perches wrote:
> On Tue, 2012-05-01 at 04:07 +0200, Eric Dumazet wrote:
> > From: Eric Dumazet <edumazet@google.com>
> > 
> > TCP or UDP stacks have big enough latencies that prefetching next
> > pointer is worth it.
> > 
> > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > ---
> >  net/core/sock.c |    1 +
> >  1 file changed, 1 insertion(+)
> > 
> > diff --git a/net/core/sock.c b/net/core/sock.c
> > index 836bca6..1a88351 100644
> > --- a/net/core/sock.c
> > +++ b/net/core/sock.c
> > @@ -1700,6 +1700,7 @@ static void __release_sock(struct sock *sk)
> >  		do {
> >  			struct sk_buff *next = skb->next;
> >  
> > +			prefetch(next);
> >  			WARN_ON_ONCE(skb_dst_is_noref(skb));
> >  			skb->next = NULL;
> >  			sk_backlog_rcv(sk, skb);
> 
> Hi Eric.
> 
> Why should next be "prefetch"ed when
> two lines below it's set to null and
> the only use is as a pointer not as
> an apparently undereferenced pointer?
> 
> 

Thats because you have no idea of what is happening ?

next points to the next skb in list (after this skb)

prefetch(next) instructs CPU to preload its cache with the memory
content of first cache line of next skb (it contains its own ->next
pointer)

After prefetch(next), we clear skb->next before continuing, but we later
will need the memory we preloaded in cpu cache at next iteration.

Basically this patch avoids one memory cache miss per iteration.

Re: [PATCH net-next] net: add a prefetch in socket backlog processing

[Eric Dumazet]

On Tue, 2012-05-01 at 08:34 +0200, Eric Dumazet wrote:

> Basically this patch avoids one memory cache miss per iteration.
> 

This patch was the easy part.

I am now working on refining __skb_dequeue() API to avoid the cache line
miss when we perform the __skb_unlink() and allow a prefetch(next) as
well, to speedup process_backlog().

Re: [PATCH net-next] net: add a prefetch in socket backlog processing

[Joe Perches]

On Tue, 2012-05-01 at 08:34 +0200, Eric Dumazet wrote:
> On Mon, 2012-04-30 at 20:24 -0700, Joe Perches wrote:
> > On Tue, 2012-05-01 at 04:07 +0200, Eric Dumazet wrote:
> > > From: Eric Dumazet <edumazet@google.com>
> > > TCP or UDP stacks have big enough latencies that prefetching next
> > > pointer is worth it.
> > > 
> > > Signed-off-by: Eric Dumazet <edumazet@google.com>
> > > ---
> > >  net/core/sock.c |    1 +
> > >  1 file changed, 1 insertion(+)
> > > 
> > > diff --git a/net/core/sock.c b/net/core/sock.c
> > > index 836bca6..1a88351 100644
> > > --- a/net/core/sock.c
> > > +++ b/net/core/sock.c
> > > @@ -1700,6 +1700,7 @@ static void __release_sock(struct sock *sk)
> > >  		do {
> > >  			struct sk_buff *next = skb->next;
> > >  
> > > +			prefetch(next);
> > >  			WARN_ON_ONCE(skb_dst_is_noref(skb));
> > >  			skb->next = NULL;
> > >  			sk_backlog_rcv(sk, skb);
> > 
> > Hi Eric.
> > 
> > Why should next be "prefetch"ed when
> > two lines below it's set to null and
> > the only use is as a pointer not as
> > an apparently undereferenced pointer?
> Thats because you have no idea of what is happening ?

Sometimes true.  Ask my wife though and you
might get a "almost always true" reply.

Here it's true because I just glossed over the
code and didn't notice the loop control variable
was actually next (skb).

> next points to the next skb in list (after this skb)
> 
> prefetch(next) instructs CPU to preload its cache with the memory
> content of first cache line of next skb (it contains its own ->next
> pointer)
> 
> After prefetch(next), we clear skb->next before continuing, but we later
> will need the memory we preloaded in cpu cache at next iteration.
> 
> Basically this patch avoids one memory cache miss per iteration.

That's true for cpus with sufficient cache but prefetch
might be wasteful for cpus without (like some ARMs).

Some of the sk_backlog_rcv functions like tcp_v4_do_rcv
can be relatively large.

It might be useful to have a target cpu compile time
test precede this prefetch.

cheers, Joe

Re: [PATCH net-next] net: add a prefetch in socket backlog processing

[Eric Dumazet]

On Tue, 2012-05-01 at 04:52 -0700, Joe Perches wrote:

> That's true for cpus with sufficient cache but prefetch
> might be wasteful for cpus without (like some ARMs).
> 

> Some of the sk_backlog_rcv functions like tcp_v4_do_rcv
> can be relatively large.

You speak of icache here. Thats different matter.

My patch does a prefetch of data (dcache)

> 
> It might be useful to have a target cpu compile time
> test precede this prefetch.

How this prefetch() is different than other ones in kernel ?

We optimize linux for cpus with a minimum cache, not for the ones with
less than 16KB caches.

For old cpus, you can use linux 2.4 it works much better.

If you believe there is an issue on a particular arch, I suggest you
talk with arch maintainer about ARCH_HAS_PREFETCH being undefined on
this arch.

Re: [PATCH net-next] net: add a prefetch in socket backlog processing

[Joe Perches]

On Tue, 2012-05-01 at 14:29 +0200, Eric Dumazet wrote:
> On Tue, 2012-05-01 at 04:52 -0700, Joe Perches wrote:
> 
> > That's true for cpus with sufficient cache but prefetch
> > might be wasteful for cpus without (like some ARMs).
> > 
> > Some of the sk_backlog_rcv functions like tcp_v4_do_rcv
> > can be relatively large.
> 
> You speak of icache here. Thats different matter.
> 
> My patch does a prefetch of data (dcache)

Actually I meant cpus with an integrated cache
like the old arm 710 and the sh3/7710.

I think those are still possible compilation
targets, but perhaps no one cares anymore.

> How this prefetch() is different than other ones in kernel ?

I'm not suggesting prefetch isn't useful.

If prefetch improves performance for the general case
it's good.  If the prefetch can also be trivially
compile time wrapped to not impact older supported
targets, I think that's good too.

> For old cpus, you can use linux 2.4 it works much better.

Deprecating older targets may not be a bad thing either.

cheers, Joe

Re: [PATCH net-next] net: add a prefetch in socket backlog processing

[David Miller]

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Tue, 01 May 2012 04:07:09 +0200

> From: Eric Dumazet <edumazet@google.com>
> 
> TCP or UDP stacks have big enough latencies that prefetching next
> pointer is worth it.
> 
> Signed-off-by: Eric Dumazet <edumazet@google.com>

Applied.