[PATCH net-next] net: core: rework skb_probe_transport_header()

[PATCH net-next] net: core: rework skb_probe_transport_header()

[Paolo Abeni]

When the transport header is not available, skb_probe_transport_header()
resorts to fully dissect the flow keys, even if it only needs the
ransport offset. We can obtain the latter using a simpler flow dissector -
flow_keys_buf_dissector - and a smaller struct for key storage.

The above gives ~50% performance improvement in micro benchmarking around
skb_probe_transport_header(), mostly due to the smaller memset. Small, but
measurable improvement is measured also in macro benchmarking - raw xmit
tput from a VM.

Signed-off-by: Paolo Abeni <pabeni@redhat.com>
---
 include/linux/skbuff.h       | 7 +++++--
 include/net/flow_dissector.h | 5 +++++
 net/core/flow_dissector.c    | 1 +
 3 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 908d66e55b14..63cb523d3519 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2350,11 +2350,14 @@ static inline void skb_pop_mac_header(struct sk_buff *skb)
 static inline void skb_probe_transport_header(struct sk_buff *skb,
 					      const int offset_hint)
 {
-	struct flow_keys keys;
+	struct flow_keys_basic keys;
 
 	if (skb_transport_header_was_set(skb))
 		return;
-	else if (skb_flow_dissect_flow_keys(skb, &keys, 0))
+
+	memset(&keys, 0, sizeof(keys));
+	if (__skb_flow_dissect(skb, &flow_keys_buf_dissector, &keys,
+			       0, 0, 0, 0, 0))
 		skb_set_transport_header(skb, keys.control.thoff);
 	else
 		skb_set_transport_header(skb, offset_hint);
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 9a074776f70b..e81dab6e9ac6 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -226,6 +226,11 @@ struct flow_dissector {
 	unsigned short int offset[FLOW_DISSECTOR_KEY_MAX];
 };
 
+struct flow_keys_basic {
+	struct flow_dissector_key_control control;
+	struct flow_dissector_key_basic basic;
+};
+
 struct flow_keys {
 	struct flow_dissector_key_control control;
 #define FLOW_KEYS_HASH_START_FIELD basic
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index d29f09bc5ff9..ac7b4de4a0f0 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1418,6 +1418,7 @@ struct flow_dissector flow_keys_dissector __read_mostly;
 EXPORT_SYMBOL(flow_keys_dissector);
 
 struct flow_dissector flow_keys_buf_dissector __read_mostly;
+EXPORT_SYMBOL(flow_keys_buf_dissector);
 
 static int __init init_default_flow_dissectors(void)
 {
-- 
2.14.3

Re: [PATCH net-next] net: core: rework skb_probe_transport_header()

[Jason Wang]

On 2018年05月03日 17:35, Paolo Abeni wrote:
> When the transport header is not available, skb_probe_transport_header()
> resorts to fully dissect the flow keys, even if it only needs the
> ransport offset. We can obtain the latter using a simpler flow dissector -
> flow_keys_buf_dissector - and a smaller struct for key storage.
>
> The above gives ~50% performance improvement in micro benchmarking around
> skb_probe_transport_header(), mostly due to the smaller memset. Small, but
> measurable improvement is measured also in macro benchmarking - raw xmit
> tput from a VM.
>
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> ---
>   include/linux/skbuff.h       | 7 +++++--
>   include/net/flow_dissector.h | 5 +++++
>   net/core/flow_dissector.c    | 1 +
>   3 files changed, 11 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 908d66e55b14..63cb523d3519 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2350,11 +2350,14 @@ static inline void skb_pop_mac_header(struct sk_buff *skb)
>   static inline void skb_probe_transport_header(struct sk_buff *skb,
>   					      const int offset_hint)
>   {
> -	struct flow_keys keys;
> +	struct flow_keys_basic keys;
>   
>   	if (skb_transport_header_was_set(skb))
>   		return;
> -	else if (skb_flow_dissect_flow_keys(skb, &keys, 0))
> +
> +	memset(&keys, 0, sizeof(keys));
> +	if (__skb_flow_dissect(skb, &flow_keys_buf_dissector, &keys,
> +			       0, 0, 0, 0, 0))
>   		skb_set_transport_header(skb, keys.control.thoff);
>   	else
>   		skb_set_transport_header(skb, offset_hint);
> diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> index 9a074776f70b..e81dab6e9ac6 100644
> --- a/include/net/flow_dissector.h
> +++ b/include/net/flow_dissector.h
> @@ -226,6 +226,11 @@ struct flow_dissector {
>   	unsigned short int offset[FLOW_DISSECTOR_KEY_MAX];
>   };
>   
> +struct flow_keys_basic {
> +	struct flow_dissector_key_control control;
> +	struct flow_dissector_key_basic basic;
> +};
> +
>   struct flow_keys {
>   	struct flow_dissector_key_control control;
>   #define FLOW_KEYS_HASH_START_FIELD basic
> diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> index d29f09bc5ff9..ac7b4de4a0f0 100644
> --- a/net/core/flow_dissector.c
> +++ b/net/core/flow_dissector.c
> @@ -1418,6 +1418,7 @@ struct flow_dissector flow_keys_dissector __read_mostly;
>   EXPORT_SYMBOL(flow_keys_dissector);
>   
>   struct flow_dissector flow_keys_buf_dissector __read_mostly;
> +EXPORT_SYMBOL(flow_keys_buf_dissector);
>   
>   static int __init init_default_flow_dissectors(void)
>   {

Acked-by: Jason Wang <jasowang@redhat.com>

Just curious, I believe this happens only when csum offload is disabled 
which is not the common case?

Thanks

Re: [PATCH net-next] net: core: rework skb_probe_transport_header()

[Paolo Abeni]

On Thu, 2018-05-03 at 20:55 +0800, Jason Wang wrote:
> 
> On 2018年05月03日 17:35, Paolo Abeni wrote:
> > When the transport header is not available, skb_probe_transport_header()
> > resorts to fully dissect the flow keys, even if it only needs the
> > ransport offset. We can obtain the latter using a simpler flow dissector -
> > flow_keys_buf_dissector - and a smaller struct for key storage.
> > 
> > The above gives ~50% performance improvement in micro benchmarking around
> > skb_probe_transport_header(), mostly due to the smaller memset. Small, but
> > measurable improvement is measured also in macro benchmarking - raw xmit
> > tput from a VM.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> > ---
> >   include/linux/skbuff.h       | 7 +++++--
> >   include/net/flow_dissector.h | 5 +++++
> >   net/core/flow_dissector.c    | 1 +
> >   3 files changed, 11 insertions(+), 2 deletions(-)
> > 
> > diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> > index 908d66e55b14..63cb523d3519 100644
> > --- a/include/linux/skbuff.h
> > +++ b/include/linux/skbuff.h
> > @@ -2350,11 +2350,14 @@ static inline void skb_pop_mac_header(struct sk_buff *skb)
> >   static inline void skb_probe_transport_header(struct sk_buff *skb,
> >   					      const int offset_hint)
> >   {
> > -	struct flow_keys keys;
> > +	struct flow_keys_basic keys;
> >   
> >   	if (skb_transport_header_was_set(skb))
> >   		return;
> > -	else if (skb_flow_dissect_flow_keys(skb, &keys, 0))
> > +
> > +	memset(&keys, 0, sizeof(keys));
> > +	if (__skb_flow_dissect(skb, &flow_keys_buf_dissector, &keys,
> > +			       0, 0, 0, 0, 0))
> >   		skb_set_transport_header(skb, keys.control.thoff);
> >   	else
> >   		skb_set_transport_header(skb, offset_hint);
> > diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
> > index 9a074776f70b..e81dab6e9ac6 100644
> > --- a/include/net/flow_dissector.h
> > +++ b/include/net/flow_dissector.h
> > @@ -226,6 +226,11 @@ struct flow_dissector {
> >   	unsigned short int offset[FLOW_DISSECTOR_KEY_MAX];
> >   };
> >   
> > +struct flow_keys_basic {
> > +	struct flow_dissector_key_control control;
> > +	struct flow_dissector_key_basic basic;
> > +};
> > +
> >   struct flow_keys {
> >   	struct flow_dissector_key_control control;
> >   #define FLOW_KEYS_HASH_START_FIELD basic
> > diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
> > index d29f09bc5ff9..ac7b4de4a0f0 100644
> > --- a/net/core/flow_dissector.c
> > +++ b/net/core/flow_dissector.c
> > @@ -1418,6 +1418,7 @@ struct flow_dissector flow_keys_dissector __read_mostly;
> >   EXPORT_SYMBOL(flow_keys_dissector);
> >   
> >   struct flow_dissector flow_keys_buf_dissector __read_mostly;
> > +EXPORT_SYMBOL(flow_keys_buf_dissector);
> >   
> >   static int __init init_default_flow_dissectors(void)
> >   {
> 
> Acked-by: Jason Wang <jasowang@redhat.com>
> 
> Just curious, I believe this happens only when csum offload is disabled 
> which is not the common case?

This also happens if the VM is doing L2 forwarding (and L3 I think)
with offload enabled.

Cheers,

Paolo

Re: [PATCH net-next] net: core: rework skb_probe_transport_header()

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Thu,  3 May 2018 11:35:35 +0200

> When the transport header is not available, skb_probe_transport_header()
> resorts to fully dissect the flow keys, even if it only needs the
> ransport offset. We can obtain the latter using a simpler flow dissector -
> flow_keys_buf_dissector - and a smaller struct for key storage.
> 
> The above gives ~50% performance improvement in micro benchmarking around
> skb_probe_transport_header(), mostly due to the smaller memset. Small, but
> measurable improvement is measured also in macro benchmarking - raw xmit
> tput from a VM.
> 
> Signed-off-by: Paolo Abeni <pabeni@redhat.com>

Please make this optimization generally, then every driver using
eth_get_headlen() (11 or so) will get the same improvement, for all
traffic!

Something like this:

diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index 908d66e55b14..e635e9ab818a 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -1171,7 +1171,7 @@ void __skb_get_hash(struct sk_buff *skb);
 u32 __skb_get_hash_symmetric(const struct sk_buff *skb);
 u32 skb_get_poff(const struct sk_buff *skb);
 u32 __skb_get_poff(const struct sk_buff *skb, void *data,
-		   const struct flow_keys *keys, int hlen);
+		   const struct flow_keys_basic *keys, int hlen);
 __be32 __skb_flow_get_ports(const struct sk_buff *skb, int thoff, u8 ip_proto,
 			    void *data, int hlen_proto);
 
@@ -1208,7 +1208,7 @@ static inline bool skb_flow_dissect_flow_keys(const struct sk_buff *skb,
 				  NULL, 0, 0, 0, flags);
 }
 
-static inline bool skb_flow_dissect_flow_keys_buf(struct flow_keys *flow,
+static inline bool skb_flow_dissect_flow_keys_buf(struct flow_keys_basic *flow,
 						  void *data, __be16 proto,
 						  int nhoff, int hlen,
 						  unsigned int flags)
@@ -2350,11 +2350,14 @@ static inline void skb_pop_mac_header(struct sk_buff *skb)
 static inline void skb_probe_transport_header(struct sk_buff *skb,
 					      const int offset_hint)
 {
-	struct flow_keys keys;
+	struct flow_keys_basic keys;
 
 	if (skb_transport_header_was_set(skb))
 		return;
-	else if (skb_flow_dissect_flow_keys(skb, &keys, 0))
+
+	memset(&keys, 0, sizeof(keys));
+	if (__skb_flow_dissect(skb, &flow_keys_buf_dissector, &keys,
+			       NULL, 0, 0, 0, 0))
 		skb_set_transport_header(skb, keys.control.thoff);
 	else
 		skb_set_transport_header(skb, offset_hint);
diff --git a/include/net/flow_dissector.h b/include/net/flow_dissector.h
index 9a074776f70b..e81dab6e9ac6 100644
--- a/include/net/flow_dissector.h
+++ b/include/net/flow_dissector.h
@@ -226,6 +226,11 @@ struct flow_dissector {
 	unsigned short int offset[FLOW_DISSECTOR_KEY_MAX];
 };
 
+struct flow_keys_basic {
+	struct flow_dissector_key_control control;
+	struct flow_dissector_key_basic basic;
+};
+
 struct flow_keys {
 	struct flow_dissector_key_control control;
 #define FLOW_KEYS_HASH_START_FIELD basic
diff --git a/net/core/flow_dissector.c b/net/core/flow_dissector.c
index d29f09bc5ff9..50d68b54f03a 100644
--- a/net/core/flow_dissector.c
+++ b/net/core/flow_dissector.c
@@ -1253,7 +1253,7 @@ __u32 skb_get_hash_perturb(const struct sk_buff *skb, u32 perturb)
 EXPORT_SYMBOL(skb_get_hash_perturb);
 
 u32 __skb_get_poff(const struct sk_buff *skb, void *data,
-		   const struct flow_keys *keys, int hlen)
+		   const struct flow_keys_basic *keys, int hlen)
 {
 	u32 poff = keys->control.thoff;
 
@@ -1314,9 +1314,11 @@ u32 __skb_get_poff(const struct sk_buff *skb, void *data,
  */
 u32 skb_get_poff(const struct sk_buff *skb)
 {
-	struct flow_keys keys;
+	struct flow_keys_basic keys;
 
-	if (!skb_flow_dissect_flow_keys(skb, &keys, 0))
+	memset(&keys, 0, sizeof(keys));
+	if (!__skb_flow_dissect(skb, &flow_keys_buf_dissector, &keys,
+				NULL, 0, 0, 0, 0))
 		return 0;
 
 	return __skb_get_poff(skb, skb->data, &keys, skb_headlen(skb));
@@ -1418,6 +1420,7 @@ struct flow_dissector flow_keys_dissector __read_mostly;
 EXPORT_SYMBOL(flow_keys_dissector);
 
 struct flow_dissector flow_keys_buf_dissector __read_mostly;
+EXPORT_SYMBOL(flow_keys_buf_dissector);
 
 static int __init init_default_flow_dissectors(void)
 {
diff --git a/net/ethernet/eth.c b/net/ethernet/eth.c
index eaeba9b99a73..873eca643f99 100644
--- a/net/ethernet/eth.c
+++ b/net/ethernet/eth.c
@@ -128,7 +128,7 @@ u32 eth_get_headlen(void *data, unsigned int len)
 {
 	const unsigned int flags = FLOW_DISSECTOR_F_PARSE_1ST_FRAG;
 	const struct ethhdr *eth = (const struct ethhdr *)data;
-	struct flow_keys keys;
+	struct flow_keys_basic keys;
 
 	/* this should never happen, but better safe than sorry */
 	if (unlikely(len < sizeof(*eth)))

Re: [PATCH net-next] net: core: rework skb_probe_transport_header()

[Paolo Abeni]

On Thu, 2018-05-03 at 13:32 -0400, David Miller wrote:
> From: Paolo Abeni <pabeni@redhat.com>
> Date: Thu,  3 May 2018 11:35:35 +0200
> 
> > When the transport header is not available, skb_probe_transport_header()
> > resorts to fully dissect the flow keys, even if it only needs the
> > ransport offset. We can obtain the latter using a simpler flow dissector -
> > flow_keys_buf_dissector - and a smaller struct for key storage.
> > 
> > The above gives ~50% performance improvement in micro benchmarking around
> > skb_probe_transport_header(), mostly due to the smaller memset. Small, but
> > measurable improvement is measured also in macro benchmarking - raw xmit
> > tput from a VM.
> > 
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
> 
> Please make this optimization generally, then every driver using
> eth_get_headlen() (11 or so) will get the same improvement, for all
> traffic!

Sure! I though about sending a follow-up patch for other
flow_keys_buf_dissector users, but if you prefer a single patch, I'm
fine with your version (just give me a little time tomorrow to test it)

Please let me know,

Paolo

Re: [PATCH net-next] net: core: rework skb_probe_transport_header()

[David Miller]

From: Paolo Abeni <pabeni@redhat.com>
Date: Thu, 03 May 2018 19:59:16 +0200

> On Thu, 2018-05-03 at 13:32 -0400, David Miller wrote:
>> From: Paolo Abeni <pabeni@redhat.com>
>> Date: Thu,  3 May 2018 11:35:35 +0200
>> 
>> > When the transport header is not available, skb_probe_transport_header()
>> > resorts to fully dissect the flow keys, even if it only needs the
>> > ransport offset. We can obtain the latter using a simpler flow dissector -
>> > flow_keys_buf_dissector - and a smaller struct for key storage.
>> > 
>> > The above gives ~50% performance improvement in micro benchmarking around
>> > skb_probe_transport_header(), mostly due to the smaller memset. Small, but
>> > measurable improvement is measured also in macro benchmarking - raw xmit
>> > tput from a VM.
>> > 
>> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>> 
>> Please make this optimization generally, then every driver using
>> eth_get_headlen() (11 or so) will get the same improvement, for all
>> traffic!
> 
> Sure! I though about sending a follow-up patch for other
> flow_keys_buf_dissector users, but if you prefer a single patch, I'm
> fine with your version (just give me a little time tomorrow to test it)

Ok, please give is a spin and let me know how the testing goes.

Re: [PATCH net-next] net: core: rework skb_probe_transport_header()

[kbuild test robot]

Hi Paolo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:    https://github.com/0day-ci/linux/commits/Paolo-Abeni/net-core-rework-skb_probe_transport_header/20180504-041345
reproduce:
        # apt-get install sparse
        make ARCH=x86_64 allmodconfig
        make C=1 CF=-D__CHECK_ENDIAN__


sparse warnings: (new ones prefixed by >>)

>> include/linux/skbuff.h:2360:32: sparse: Using plain integer as NULL pointer
   drivers/net/tun.c:2088:40: sparse: expression using sizeof(void)
   drivers/net/tun.c:2221:15: sparse: expression using sizeof(void)
   drivers/net/tun.c:2221:15: sparse: expression using sizeof(void)
   drivers/net/tun.c:2846:36: sparse: incorrect type in argument 2 (different address spaces) @@    expected struct tun_prog [noderef] <asn:4>**prog_p @@    got noderef] <asn:4>**prog_p @@
   drivers/net/tun.c:2846:36:    expected struct tun_prog [noderef] <asn:4>**prog_p
   drivers/net/tun.c:2846:36:    got struct tun_prog **prog_p
   drivers/net/tun.c:3142:42: sparse: incorrect type in argument 2 (different address spaces) @@    expected struct tun_prog **prog_p @@    got struct tun_prog [struct tun_prog **prog_p @@
   drivers/net/tun.c:3142:42:    expected struct tun_prog **prog_p
   drivers/net/tun.c:3142:42:    got struct tun_prog [noderef] <asn:4>**<noident>
   drivers/net/tun.c:3146:42: sparse: incorrect type in argument 2 (different address spaces) @@    expected struct tun_prog **prog_p @@    got struct tun_prog [struct tun_prog **prog_p @@
   drivers/net/tun.c:3146:42:    expected struct tun_prog **prog_p
   drivers/net/tun.c:3146:42:    got struct tun_prog [noderef] <asn:4>**<noident>
--
>> include/linux/skbuff.h:2360:32: sparse: Using plain integer as NULL pointer
   drivers/net/tap.c:879:15: sparse: expression using sizeof(void)
   drivers/net/tap.c:879:15: sparse: expression using sizeof(void)
--
   drivers/net/xen-netback/netback.c:175:21: sparse: expression using sizeof(void)
   drivers/net/xen-netback/netback.c:182:35: sparse: expression using sizeof(void)
   drivers/net/xen-netback/netback.c:182:35: sparse: expression using sizeof(void)
>> include/linux/skbuff.h:2360:32: sparse: Using plain integer as NULL pointer
   drivers/net/xen-netback/netback.c:1632:37: sparse: expression using sizeof(void)

vim +2360 include/linux/skbuff.h

  2349	
  2350	static inline void skb_probe_transport_header(struct sk_buff *skb,
  2351						      const int offset_hint)
  2352	{
  2353		struct flow_keys_basic keys;
  2354	
  2355		if (skb_transport_header_was_set(skb))
  2356			return;
  2357	
  2358		memset(&keys, 0, sizeof(keys));
  2359		if (__skb_flow_dissect(skb, &flow_keys_buf_dissector, &keys,
> 2360				       0, 0, 0, 0, 0))
  2361			skb_set_transport_header(skb, keys.control.thoff);
  2362		else
  2363			skb_set_transport_header(skb, offset_hint);
  2364	}
  2365	

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation