Re: [PATCH net-next] net/sched: cls_flower: fix resetting of ether proto mask

From: Vladimir Oltean <olteanv@gmail.com>
To: Boris Sukholitko <boris.sukholitko@broadcom.com>
Cc: Vadym Kochan <vadym.kochan@plvision.eu>,
	"David S. Miller" <davem@davemloft.net>,
	Jakub Kicinski <kuba@kernel.org>,
	Jamal Hadi Salim <jhs@mojatatu.com>,
	Cong Wang <xiyou.wangcong@gmail.com>,
	Andrew Lunn <andrew@lunn.ch>,
	Serhiy Boiko <serhiy.boiko@plvision.eu>,
	Volodymyr Mytnyk <volodymyr.mytnyk@plvision.eu>,
	netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
	jiri@resnulli.us, idosch@idosch.org, ilya.lifshits@broadcom.com
Subject: Re: [PATCH net-next] net/sched: cls_flower: fix resetting of ether proto mask
Date: Mon, 21 Jun 2021 13:09:49 +0300	[thread overview]
Message-ID: <20210621100949.dkzvv4mdgopcenab@skbuf> (raw)
In-Reply-To: <20210621083037.GA9665@builder>

On Mon, Jun 21, 2021 at 11:32:27AM +0300, Boris Sukholitko wrote:
> On Thu, Jun 17, 2021 at 10:51:02PM +0300, Vladimir Oltean wrote:
> > On Thu, Jun 17, 2021 at 07:41:55PM +0300, Vladimir Oltean wrote:
> > > On Thu, Jun 17, 2021 at 07:14:35PM +0300, Vadym Kochan wrote:
> 
> [snip excellent problem analysis]
> 
> > So maybe it is the flow dissector we need to fix, to make it give us an
> > additional pure EtherType if asked for, make tc-flower use that
> > dissector key instead, and then revert Jamal's user space patch, and we
> > should all install our tc filters as:
> > 
> > tc filter add dev sw1p0 ingress handle 11 protocol all flower eth_type 0x8864 skip_hw action drop
> > 
> > ?
> 
> I like this solution. To be more explicit, the plan becomes:
> 
> 1. Add FLOW_DISSECTOR_KEY_ETH_TYPE and struct flow_dissector_key_eth_type.
> 2. Have skb flow dissector use it.
> 3. Userspace does not set TCA_FLOWER_KEY_ETH_TYPE automagically
>    anymore. cls_flower takes basic.n_proto from struct tcf_proto.
> 4. Add eth_type to the userspace and use it to set TCA_FLOWER_KEY_ETH_TYPE
> 5. Existence of TCA_FLOWER_KEY_ETH_TYPE triggers new eth_type dissector.
> 
> IMHO this neatly solves non-vlan protocol match case.
> 
> What should we do with the VLANs then? Should we have vlan_pure_ethtype
> and cvlan_pure_ethtype as additional keys?

Yeah, I don't know about the "_pure_" part (the current name of the
options in tc user space seems fine), but the flow dissector should have
some parsing keys for the C-VLAN and S-VLAN EthType too, since the
FLOW_DISSECTOR_KEY_ETH_TYPE should match on, well, the EtherType.

> > 
> > Or maybe just be like you, say I don't care about any of that, I just
> > want it to behave as before, and simply revert Boris's patch. Ok, maybe
> 
> FTR I fully support reverting the patch. Please accept my apologies for
> breaking the HW offload and big thanks to Vadym for finding it.
> 
> I will send the revert shortly.
> 
> Thanks,
> Boris.

Thanks.

Please note that I haven't used tc for long enough to know what changes
are for its own good, so there is still place for expert feedback from
the maintainers, but this solution seems common sense to me.