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

[Alexander Lobakin <aleksander.lobakin@intel.com>]

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

> 
>>> +{
>>> +	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