Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Aaron Conole <aconole@redhat.com>
To:     Adrian Moreno <amorenoz@redhat.com>
Cc:     Ilya Maximets <i.maximets@ovn.org>, netdev@vger.kernel.org,
        dev@openvswitch.org, linux-kernel@vger.kernel.org,
        Eric Dumazet <edumazet@google.com>,
        linux-kselftest@vger.kernel.org, Jakub Kicinski <kuba@kernel.org>,
        Paolo Abeni <pabeni@redhat.com>, Shuah Khan <shuah@kernel.org>,
        "David S. Miller" <davem@davemloft.net>
Subject: Re: [ovs-dev] [RFC net-next 1/6] openvswitch: exclude kernel flow
 key from upcalls
References: <20221122140307.705112-1-aconole@redhat.com>
        <20221122140307.705112-2-aconole@redhat.com>
        <c04242ee-f125-6d95-e263-65470222d3cf@ovn.org>
        <83a0b3e4-1327-c1c4-4eb4-9a25ff533d1d@redhat.com>
        <bf975714-7edc-efdd-de84-56194aa6eb60@ovn.org>
        <753d995d-d4f4-bf2e-994d-435a36414127@redhat.com>
Date:   Tue, 29 Nov 2022 09:26:22 -0500
In-Reply-To: <753d995d-d4f4-bf2e-994d-435a36414127@redhat.com> (Adrian
        Moreno's message of "Mon, 28 Nov 2022 10:12:21 +0100")
Message-ID: <f7tedtlsupd.fsf@redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Adrian Moreno <amorenoz@redhat.com> writes:

> On 11/25/22 16:51, Ilya Maximets wrote:
>> On 11/25/22 16:29, Adrian Moreno wrote:
>>>
>>>
>>> On 11/23/22 22:22, Ilya Maximets wrote:
>>>> On 11/22/22 15:03, Aaron Conole wrote:
>>>>> When processing upcall commands, two groups of data are available to
>>>>> userspace for processing: the actual packet data and the kernel
>>>>> sw flow key data.=C2=A0 The inclusion of the flow key allows the user=
space
>>>>> avoid running through the dissection again.
>>>>>
>>>>> However, the userspace can choose to ignore the flow key data, as is
>>>>> the case in some ovs-vswitchd upcall processing.=C2=A0 For these mess=
ages,
>>>>> having the flow key data merely adds additional data to the upcall
>>>>> pipeline without any actual gain.=C2=A0 Userspace simply throws the d=
ata
>>>>> away anyway.
>>>>
>>>> Hi, Aaron.=C2=A0 While it's true that OVS in userpsace is re-parsing t=
he
>>>> packet from scratch and using the newly parsed key for the OpenFlow
>>>> translation, the kernel-porvided key is still used in a few important
>>>> places.=C2=A0 Mainly for the compatibility checking.=C2=A0 The use is =
described
>>>> here in more details:
>>>>  =C2=A0=C2=A0 https://docs.kernel.org/networking/openvswitch.html#flow=
-key-compatibility
>>>>
>>>> We need to compare the key generated in userspace with the key
>>>> generated by the kernel to know if it's safe to install the new flow
>>>> to the kernel, i.e. if the kernel and OVS userpsace are parsing the
>>>> packet in the same way.
>>>>
>>>
>>> Hi Ilya,
>>>
>>> Do we need to do that for every packet?
>>> Could we send a bitmask of supported fields to userspace at feature
>>> negotiation and let OVS slowpath flows that it knows the kernel won't
>>> be able to handle properly?
>> It's not that simple, because supported fields in a packet depend
>> on previous fields in that same packet.  For example, parsing TCP
>> header is generally supported, but it won't be parsed for IPv6
>> fragments (even the first one), number of vlan headers will affect
>> the parsing as we do not parse deeper than 2 vlan headers, etc.
>> So, I'm afraid we have to have a per-packet information, unless we
>> can somehow probe all the possible valid combinations of packet
>> headers.
>>=20
>
> Surely. I understand that we'd need more than just a bit per
> field. Things like L4 on IPv6 frags would need another bit and the
> number of VLAN headers would need some more. But, are these a handful
> of exceptions or do we really need all the possible combinations of
> headers? If it's a matter of naming a handful of corner cases I think
> we could consider expressing them at initialization time and safe some
> buffer space plus computation time both in kernel and userspace.

I will take a bit more of a look here - there must surely be a way to
express this when pulling information via DP_GET command so that we
don't need to wait for a packet to come in to figure out whether we can
parse it.