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: <3f8f0fd8-b75c-5666-797b-315ebb632ca2@intel.com>
Date: Thu, 14 Sep 2023 18:38:04 +0200
From: Alexander Lobakin <aleksander.lobakin@...el.com>
To: Larysa Zaremba <larysa.zaremba@...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

From: Larysa Zaremba <larysa.zaremba@...el.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@...el.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