Date:   Thu, 17 Oct 2019 18:59:22 +0100
From:   Edward Cree <ecree@...arflare.com>
To:     Pablo Neira Ayuso <pablo@...filter.org>,
        Jakub Kicinski <jakub.kicinski@...ronome.com>
CC:     <netfilter-devel@...r.kernel.org>, <davem@...emloft.net>,
        <netdev@...r.kernel.org>, <jiri@...nulli.us>,
        <saeedm@...lanox.com>, <vishal@...lsio.com>, <vladbu@...lanox.com>
Subject: Re: [PATCH net-next,v5 3/4] net: flow_offload: mangle action at byte
 level

On 17/10/2019 17:22, Pablo Neira Ayuso wrote:
> You refer to single u32 word changing both sport and dport.
>
> What's the point with including this in the subset of the uAPI to be
> supported?
What's the point with removing this from the uAPI which implicitly the
 kernel internal layers supported (whether individual drivers did or
 not)?

> In software, it really makes sense to use this representation since it
> is speeding up packet processing.
>
> In hardware, supporting this uAPI really gets you nothing at all.
That depends how the hardware works.  Current hardware tends to do pedit
 offload based on protocol(-ossified) fields, but there's no guarantee
 that every future piece of hardware will be the same.  Someone might
 build HW whose packet-editing part is based on the pedit uAPI approach,
 explicitly designing their HW to support Linux semantics.  And you'd be
 telling them "sorry, we decided to remove part of that uAPI because,
 uhh, well we didn't actually have a reason but we did it anyway".

> We have to document what subset of the uAPI is support through
> hardware offloads. Pretending that we can support all uAPI is not
> true, we only support for tc pedit extended mode right now.
Who's "we"?  AIUI the kernel will pass any pedits to drivers, they don't
 have to be extended mode, and they certainly don't have to have come
 from the iproute2 'tc' binary, so they might not bear any relation to
 the protocol fields tc knows about.
Pedit is supposed to work even in the absence of protocol knowledge in
 the kernel (e.g. in combination with cls_u32, you can filter and mangle
 traffic in a completely new protocol), you're turning it into Yet
 Another Ossified TCP/IP Monoculture.  This is not the direction the
 networking offloads community is trying to move in.

+100 to everything Jakub said, and please remember that some of us have
 to maintain driver support for slightly more than just the latest
 kernel, and your pointless churn makes that much harder.  ("A slow sort
 of country; here, it takes all the running _you_ can do, just to stay
 in the same place.")  I'm not talking about drivers stretching back to
 2.6 era here; we _expect_ that to be painful.  But when code to build
 on as recent as 4.20 is a thicket of clustered ifdefs, without a single
 piece of user-visible functionality being added as a result (and some
 removed; not only are you chopping bits of the pedit API off, but TC
 action stats are *still* broken since 5.1), something is _very_ wrong.

Of course we know the real reason you're making all these API changes is
 for netfilter offload (I have my doubts whether anyone is actually
 using netfilter in the wild; crusties still use iptables and early-
 adopters have all jumped ship to eBPF solutions, but if you're so
 desperate for netfilter to remain relevant I suppose we have to humour
 you at least a little), but there's absolutely no technical reason I
 can see why netfilter couldn't translate its mangles into existing
 pedit language.  If patch 3 is truly and unavoidably a prerequisite of
 patch 4, you'll need to explain why if you want a sympathetic hearing.

-Ed

Powered by blists - more mailing lists