Message-ID: <1c368b57-be21-5c37-ef38-e23fe344b70a@nbd.name>
Date:   Thu, 19 May 2022 17:37:12 +0200
From:   Felix Fietkau <nbd@....name>
To:     Pablo Neira Ayuso <pablo@...filter.org>
Cc:     netfilter-devel@...r.kernel.org, netdev@...r.kernel.org,
        Jo-Philipp Wich <jo@...n.io>
Subject: Re: [RFC] netfilter: nf_tables: ignore errors on flowtable device hw
 offload setup


On 16.05.22 02:57, Pablo Neira Ayuso wrote:
> On Fri, May 13, 2022 at 11:09:51AM +0200, Felix Fietkau wrote:
>> > > Assuming that is the case, wouldn't it be better if we simply have
>> > > an API that indicates, which flowtable members hardware offload was
>> > > actually enabled for?
>> > 
>> > What are you proposing?
>> > 
>> > I think it would be good to expose through netlink interface what the
>> > device can actually do according to the existing supported flowtable
>> > software datapath features.
>> In addition to the NFTA_FLOWTABLE_HOOK_DEVS array, the netlink API could
>> also return another array, e.g. NFTA_FLOWTABLE_HOOK_OFFLOAD_DEVS which
>> indicates devices for which hw offload is enabled.
>> 
>> What I really don't like about the current state of the flowtable offload
>> API is the (in my opinion completely unnecessary) complexity that is
>> required for the simple use case of enabling hw/sw flow offloading on a best
>> effort basis for all devices.
>> What I like even less is the number of implementation details that it has to
>> consider.
>> 
>> For example: Let's assume we have a machine with several devices, some of
>> which support hw offload, some of which don't. We have a mix of VLANs and
>> bridges in there as well, maybe even PPPoE.
>> Now the admin of that machine wants to enable best-effort hardware +
>> software flow offloading for that configuration.
>> Now he (or a piece of user space software dealing with the config) has to do
>> these things:
>> - figure out which devices could support hw offload, create a separate flow
>> table for them
>> - be aware of which of these devices are actually used by looking at the
>> stack of bridges, vlans, dsa devices, etc.
>> - if an error occurs, test them individually just to see which one actually
>> failed and leave it out of the flowtable
>> - for sw offload be aware that there is limited support for offloading decap
>> of vlans/pppoe, count the number of decaps and figure out the right input
>> device to add based on the behavior of nft_dev_path_info, so that the
>> 'indev' it selects matches the device you put in the flow table.
>> 
>> So I'm asking you: Am I getting any of this completely wrong? Do you
>> consider it to be a reasonable trade-off to force the admin (or intermediate
>> user space layer) to jump through these hoops for such a simple use case,
>> just because somebody might want more fine grained control?
>> 
>> I consider this patch to be a first step towards making simple use cases
>> easier to configure. I'd also be fine with adding a flag to make the
>> fallback behavior opt-in, even though I think it would make a much better
>> default.
>> 
>> Eventually I'd also like to add a flag that makes it unnecessary to even
>> specify the devices in the flow table by making the code auto-create hooks
>> for devices with active flows, just like I did in my xtables target.
>> You correctly pointed out to me in the past that this comes at the cost of a
>> few packets delay before offloading kicks in, but I'm still wondering: who
>> actually cares about that?
>> 
>> If I'm completely off-base with this, please let me know. I'm simply trying
>> to make sense of all of this...
> 
> Maybe only fail if _none_ of the selected devices support for hardware
> offload, ie. instead of silently accepting all devices, count of the
> number of devices for which a block has been set up, if it is == 0
> then bail out with EOPNOTSUPP.
I've thought about this some more. The problem with that is if you start 
by having only non-hw-offload devices in the flow table, you can't 
create it with the offload flag.
If you then want to add another device that is capable of doing hw 
offload, it means you have to delete the flowtable and recreate it 
(along with the rules that depend on it), since adding the offload flag 
at runtime isn't supported.

I still think the best course of action is to silently accept the 
offload flag even if none of the devices support hw offload.

- Felix

Powered by blists - more mailing lists