Message-Id: <20210711010244.1709329-1-martin.blumenstingl@googlemail.com>
Date:   Sun, 11 Jul 2021 03:02:44 +0200
From:   Martin Blumenstingl <martin.blumenstingl@...glemail.com>
To:     pablo@...filter.org
Cc:     nbd@....name, coreteam@...filter.org, davem@...emloft.net,
        fw@...len.de, kadlec@...filter.org, kuba@...nel.org,
        linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
        netfilter-devel@...r.kernel.org, olek2@...pl, roid@...dia.com
Subject: Re: [PATCH nf] Revert "netfilter: flowtable: Remove redundant hw refresh bit"

Hi Aleksander,

> The xt_flowoffload module is inconditionally setting on the hardware
> offload flag:
[...]
>
> which is triggering the slow down because packet path is allocating
> work to offload the entry to hardware, however, this driver does not
> support for hardware offload.
> 
> Probably this module can be updated to unset the flowtable flag if the
> harware does not support hardware offload.

yesterday there was a discussion about this on the #openwrt-devel IRC
channel. I am adding the IRC log to the end of this email because I am
not sure if you're using IRC.

I typically don't test with flow offloading enabled (I am testing with
OpenWrt's "default" network configuration, where flow offloading is
disabled by default). Also I am not familiar with the flow offloading
code at all and reading the xt_FLOWOFFLOAD code just raised more
questions for me.

Maybe you can share some info whether your workaround from [0] "fixes"
this issue. I am aware that it will probably break other devices. But
maybe it helps Pablo to confirm whether it's really an xt_FLOWOFFLOAD
bug or rather some generic flow offload issue (as Felix suggested on
IRC).


Best regards,
Martin


[0] https://github.com/abajk/openwrt/commit/ee4d790574c0edd170e1710d7cd4819727b23721


<rsalvaterra> nbd: I saw your flow offloading updates. Just to make sure, this issue hasn't been addressed yet, has it? https://lore.kernel.org/netdev/20210614215351.GA734@salvia/
<nbd> i don't think so
<nbd> can you reproduce it?
<rsalvaterra> nbd: Not really, I don't have the hardware.
<rsalvaterra> It's lantiq, I think (bthh5a).
<rsalvaterra> However, I believe dwmw2_gone has one, iirc.
<xdarklight> nbd: I also have a HH5A. if you have any patch ready you can also send it as RFC on the mailing list and Cc Aleksander
<rsalvaterra> xdarklight: Have you been able to reproduce the flow offloading regression?
<xdarklight> I can try (typically I test with a "default" network configuration, where flow offloading is disabled)
<rsalvaterra> xdarklight: I don't have a lot of details, only this thread: https://github.com/openwrt/openwrt/pull/4225#issuecomment-855454607
<xdarklight> rsalvaterra: this is the workaround that Aleksander has in his tree: https://github.com/abajk/openwrt/commit/ee4d790574c0edd170e1710d7cd4819727b23721
<rsalvaterra> xdarklight: Well, but that basically breaks hw flow offloading everywhere else, if I'm reading correctly. :)
<xdarklight> rsalvaterra: I am not arguing with that :). I wanted to point out that Pablo's finding on the netfilter mailing list seems to be correct
<rsalvaterra> xdarklight: Sure, which is why I pinged nbd, since he's the original author of the xt_FLOWOFFLOAD target.
<rsalvaterra> What it seems is that it isn't such trivial fix. :)
<xdarklight> I looked at the xt_FLOWOFFLOAD code myself and it raised more questions than I had before looking at the code. so I decided to wait for someone with better knowledge to look into that issue :)
<rsalvaterra> Same here. I just went "oh, this requires divine intervention" and set it aside. :P
<nbd> xdarklight: which finding did you mean?
<xdarklight> nbd: "The xt_flowoffload module is inconditionally setting on the hardware offload flag" [...] flowtable[1].ft.flags = NF_FLOWTABLE_HW_OFFLOAD;
<xdarklight> nbd: from this email: https://lore.kernel.org/netdev/20210614215351.GA734@salvia/
<nbd> i actually think that finding is wrong
<nbd> xt_FLOWOFFLOAD registers two flowtables
<nbd> one with hw offload, one without
<nbd> the target code does this:
<nbd> table = &flowtable[!!(info->flags & XT_FLOWOFFLOAD_HW)];
<nbd> so it selects flowtable[1] only if info->flags has XT_FLOWOFFLOAD_HW set
<rsalvaterra> nbd: That's between you and Pablo, I mustn't interfere. :)
<nbd> i did reply to pablo, but never heard back from him
<rsalvaterra> nbd: The merge window is still open, he's probably busy at the moment… maybe ping him on Monday?
<xdarklight> nbd: it seems that your mail also didn't make it to the netdev mailing list (at least I can't find it)
<rsalvaterra> xdarklight: Now that you mention it, neither do I.
<nbd> he wrote to me in private for some reason
<xdarklight> oh okay. so for me to summarize: you're saying that the xt_FLOWOFFLOAD code should be fine. in that case Aleksander's workaround (https://github.com/abajk/openwrt/commit/ee4d790574c0edd170e1710d7cd4819727b23721) is also a no-op and the original problem would still be seen
<rsalvaterra> xdarklight: I don't think it's a no-op, otherwise he wouldn't carry it in his tree… maybe something's wrong in the table selection? table = &flowtable[!!(info->flags & XT_FLOWOFFLOAD_HW)]; does look correct, though.
<nbd> xdarklight: well, it's not a no-op if the issue was reproduced with option flow_offloading_hw 1 in the config
<rsalvaterra> nbd: Uh… If he has option flow_offloading_hw '1' on a system which doesn't support hw flow offloading… he gets to keep the pieces, right?
<nbd> it shouldn't break
<nbd> and normally i'd expect the generic flow offload api to be able to deal with it without attempting to re-enable hw offload over and over again

Powered by blists - more mailing lists