[xdp-hints] Re: [RFC bpf-next 10/23] ice: Implement VLAN tag hint

[Larysa Zaremba <larysa.zaremba@intel.com>]

On Thu, Sep 14, 2023 at 06:25:04PM +0200, Alexander Lobakin wrote:
> From: Larysa Zaremba <larysa.zaremba@intel.com>
> Date: Thu, 24 Aug 2023 21:26:49 +0200
> 
> > Implement .xmo_rx_vlan_tag callback to allow XDP code to read
> > packet's VLAN tag.
> > 
> > At the same time, use vlan_tci instead of vlan_tag in touched code,
> > because vlan_tag is misleading.
> > 
> > Signed-off-by: Larysa Zaremba <larysa.zaremba@intel.com>
> > ---
> >  drivers/net/ethernet/intel/ice/ice_main.c     | 22 ++++++++++++++++
> >  drivers/net/ethernet/intel/ice/ice_txrx.c     |  6 ++---
> >  drivers/net/ethernet/intel/ice/ice_txrx.h     |  1 +
> >  drivers/net/ethernet/intel/ice/ice_txrx_lib.c | 26 +++++++++++++++++++
> >  drivers/net/ethernet/intel/ice/ice_txrx_lib.h |  4 +--
> >  drivers/net/ethernet/intel/ice/ice_xsk.c      |  6 ++---
> >  6 files changed, 57 insertions(+), 8 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/intel/ice/ice_main.c b/drivers/net/ethernet/intel/ice/ice_main.c
> > index 557c6326ff87..aff4fa1a75f8 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_main.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_main.c
> > @@ -6007,6 +6007,23 @@ ice_fix_features(struct net_device *netdev, netdev_features_t features)
> >  	return features;
> >  }
> >  
> > +/**
> > + * ice_set_rx_rings_vlan_proto - update rings with new stripped VLAN proto
> > + * @vsi: PF's VSI
> > + * @vlan_ethertype: VLAN ethertype (802.1Q or 802.1ad) in network byte order
> > + *
> > + * Store current stripped VLAN proto in ring packet context,
> > + * so it can be accessed more efficiently by packet processing code.
> > + */
> > +static void
> > +ice_set_rx_rings_vlan_proto(struct ice_vsi *vsi, __be16 vlan_ethertype)
> 
> @vsi can be const (I hope).

I will try to make it const.

> Line can be broken on arguments, not type (I hope).
> 

This is how we break the lines everywhere in this file though :/

> > +{
> > +	u16 i;
> > +
> > +	ice_for_each_alloc_rxq(vsi, i)
> > +		vsi->rx_rings[i]->pkt_ctx.vlan_proto = vlan_ethertype;
> > +}
> > +
> >  /**
> >   * ice_set_vlan_offload_features - set VLAN offload features for the PF VSI
> >   * @vsi: PF's VSI
> > @@ -6049,6 +6066,11 @@ ice_set_vlan_offload_features(struct ice_vsi *vsi, netdev_features_t features)
> >  	if (strip_err || insert_err)
> >  		return -EIO;
> >  
> > +	if (enable_stripping)
> > +		ice_set_rx_rings_vlan_proto(vsi, htons(vlan_ethertype));
> > +	else
> > +		ice_set_rx_rings_vlan_proto(vsi, 0);
> 
> Ternary?

Would look ugly in this particular case, I think, too long expressions and no 
return values.

> 
> > +
> >  	return 0;
> >  }
> >  
> > diff --git a/drivers/net/ethernet/intel/ice/ice_txrx.c b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > index 4e6546d9cf85..4fd7614f243d 100644
> > --- a/drivers/net/ethernet/intel/ice/ice_txrx.c
> > +++ b/drivers/net/ethernet/intel/ice/ice_txrx.c
> > @@ -1183,7 +1183,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget)
> >  		struct sk_buff *skb;
> >  		unsigned int size;
> >  		u16 stat_err_bits;
> > -		u16 vlan_tag = 0;
> > +		u16 vlan_tci;
> >  
> >  		/* get the Rx desc from Rx ring based on 'next_to_clean' */
> >  		rx_desc = ICE_RX_DESC(rx_ring, ntc);
> > @@ -1278,7 +1278,7 @@ int ice_clean_rx_irq(struct ice_rx_ring *rx_ring, int budget)
> >  			continue;
> >  		}
> >  
> > -		vlan_tag = ice_get_vlan_tag_from_rx_desc(rx_desc);
> > +		vlan_tci = ice_get_vlan_tci(rx_desc);
> 
> Unrelated: I never was a fan of scattering rx_desc parsing across
> several files, I remember I moved it to process_skb_fields() in both ice
> (Hints series) and iavf (libie), maybe do that here as well? Or way too
> out of context?

A little bit too unrelated to the purpose of the series, but a thing we must do 
in the future.

> 
> >  
> >  		/* pad the skb if needed, to make a valid ethernet frame */
> >  		if (eth_skb_pad(skb))
> 
> [...]
> 
> Thanks,
> Olek