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

[Paolo Abeni <pabeni@redhat.com>]

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