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

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: <ZQM0lzXSsseZTmOy@lincoln>
Date: Thu, 14 Sep 2023 18:28:07 +0200
From: Larysa Zaremba <larysa.zaremba@...el.com>
To: Alexander Lobakin <aleksander.lobakin@...el.com>
CC: <bpf@...r.kernel.org>, <ast@...nel.org>, <daniel@...earbox.net>,
	<andrii@...nel.org>, <martin.lau@...ux.dev>, <song@...nel.org>, <yhs@...com>,
	<john.fastabend@...il.com>, <kpsingh@...nel.org>, <sdf@...gle.com>,
	<haoluo@...gle.com>, <jolsa@...nel.org>, David Ahern <dsahern@...il.com>,
	Jakub Kicinski <kuba@...nel.org>, Willem de Bruijn <willemb@...gle.com>,
	Jesper Dangaard Brouer <brouer@...hat.com>, Anatoly Burakov
	<anatoly.burakov@...el.com>, Alexander Lobakin <alexandr.lobakin@...el.com>,
	Magnus Karlsson <magnus.karlsson@...il.com>, Maryam Tahhan
	<mtahhan@...hat.com>, <xdp-hints@...-project.net>, <netdev@...r.kernel.org>,
	Willem de Bruijn <willemdebruijn.kernel@...il.com>, Alexei Starovoitov
	<alexei.starovoitov@...il.com>, Simon Horman <simon.horman@...igine.com>,
	Tariq Toukan <tariqt@...lanox.com>, Saeed Mahameed <saeedm@...lanox.com>
Subject: Re: [xdp-hints] [RFC bpf-next 10/23] ice: Implement VLAN tag hint

On Thu, Sep 14, 2023 at 06:25:04PM +0200, Alexander Lobakin wrote:
> From: Larysa Zaremba <larysa.zaremba@...el.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@...el.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