Re: [PATCH net-next 8/9] net: hns3: add support for queue bonding mode of flow director

From: Alexander Duyck <alexander.duyck@gmail.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: Huazhong Tan <tanhuazhong@huawei.com>,
	David Miller <davem@davemloft.net>,
	Netdev <netdev@vger.kernel.org>,
	Salil Mehta <salil.mehta@huawei.com>,
	yisen.zhuang@huawei.com, huangdaode@huawei.com,
	linuxarm@openeuler.org, LinuxArm <linuxarm@huawei.com>,
	Jian Shen <shenjian15@huawei.com>
Subject: Re: [PATCH net-next 8/9] net: hns3: add support for queue bonding mode of flow director
Date: Wed, 17 Mar 2021 20:30:59 -0700	[thread overview]
Message-ID: <CAKgT0UdD_s_99nVAXBmYgKhzdt+YQxgT6UsYMgEc4TwwgMHw-g@mail.gmail.com> (raw)
In-Reply-To: <20210317182828.70fcc61d@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>

On Wed, Mar 17, 2021 at 6:28 PM Jakub Kicinski <kuba@kernel.org> wrote:
>
> On Thu, 18 Mar 2021 09:02:54 +0800 Huazhong Tan wrote:
> > On 2021/3/16 4:04, Jakub Kicinski wrote:
> > > On Mon, 15 Mar 2021 20:23:50 +0800 Huazhong Tan wrote:
> > >> From: Jian Shen <shenjian15@huawei.com>
> > >>
> > >> For device version V3, it supports queue bonding, which can
> > >> identify the tuple information of TCP stream, and create flow
> > >> director rules automatically, in order to keep the tx and rx
> > >> packets are in the same queue pair. The driver set FD_ADD
> > >> field of TX BD for TCP SYN packet, and set FD_DEL filed for
> > >> TCP FIN or RST packet. The hardware create or remove a fd rule
> > >> according to the TX BD, and it also support to age-out a rule
> > >> if not hit for a long time.
> > >>
> > >> The queue bonding mode is default to be disabled, and can be
> > >> enabled/disabled with ethtool priv-flags command.
> > > This seems like fairly well defined behavior, IMHO we should have a full
> > > device feature for it, rather than a private flag.
> >
> > Should we add a NETIF_F_NTUPLE_HW feature for it?
>
> It'd be better to keep the configuration close to the existing RFS
> config, no? Perhaps a new file under
>
>   /sys/class/net/$dev/queues/rx-$id/
>
> to enable the feature would be more appropriate?
>
> Otherwise I'd call it something like NETIF_F_RFS_AUTO ?
>
> Alex, any thoughts? IIRC Intel HW had a similar feature?

Yeah, this is pretty much what Intel used to put out as ATR aka Flow
Director. Although with that there was also a component of XPS. Flow
Director was the name of the hardware feature and ATR, Application
Targeted Routing, was the software feature that had the Tx path adding
rules by default.

The i40e driver supports disabling it via the "flow-director-atr" private flag.

As far as tying this into NTUPLE that is definitely a no-go. Generally
NTUPLE rules and ATR are mutually exclusive since they compete for
resources within the same device.

> > > Does the device need to be able to parse the frame fully for this
> > > mechanism to work? Will it work even if the TCP segment is encapsulated
> > > in a custom tunnel?
> >
> > no, custom tunnel is not supported.
>
> Hm, okay, it's just queue mapping, if device gets it wrong not the end
> of the world (provided security boundaries are preserved).

So yes/no in terms of this not causing serious issues. Where this
tends to get ugly is if it is combined with something like XPS, which
appears to be enabled for hns3. In that case the flow can jump queues
and when it does that can lead to the Rx either jumping to follow
causing an out of order issue on the Rx side, or being left behind,
with being left behind which is the safer case.

Really I think this feature would be better served by implementing
Accelerated RFS and adding support for ndo_rx_flow_steer.