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

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

On Thu, Sep 14, 2023 at 06:38:04PM +0200, Alexander Lobakin wrote:
> From: Larysa Zaremba <larysa.zaremba@intel.com>
> Date: Thu, 14 Sep 2023 18:28:07 +0200
> 
> > 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
> 
> [...]
> 
> >>> +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 :/
> 
> I know and would really like us stop at least adding new such
> occurrences when not needed :s

I think with such minor stuff, it is more important to keep style consistent in 
files.

> 
> > 
> >>> +{
> >>> +	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.
> 
> 	ice_set_rx_rings_vlan_proto(vsi, strip ? htons(vlan_ethertype) : 0);
> 
> ?
> 
> [...]
> 
> >>> -		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.
> 
> Sure, +
> 
> > 
> >>
> >>>  
> >>>  		/* pad the skb if needed, to make a valid ethernet frame */
> >>>  		if (eth_skb_pad(skb))
> >>
> >> [...]
> >>
> >> Thanks,
> >> Olek
> 
> Thanks,
> Olek