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:     Ilya Maximets <i.maximets@ovn.org>
Cc:     netdev@vger.kernel.org, Pravin B Shelar <pshelar@ovn.org>,
        Jakub Kicinski <kuba@kernel.org>,
        "David S. Miller" <davem@davemloft.net>,
        Paolo Abeni <pabeni@redhat.com>,
        Eric Dumazet <edumazet@google.com>,
        Thomas Graf <tgraf@suug.ch>, dev@openvswitch.org,
        Eelco Chaudron <echaudro@redhat.com>,
        Shuah Khan <shuah@kernel.org>, linux-kernel@vger.kernel.org,
        linux-kselftest@vger.kernel.org
Subject: Re: [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>
Date:   Tue, 29 Nov 2022 09:30:38 -0500
In-Reply-To: <c04242ee-f125-6d95-e263-65470222d3cf@ovn.org> (Ilya Maximets's
        message of "Wed, 23 Nov 2022 22:22:23 +0100")
Message-ID: <f7ta649sui9.fsf@redhat.com>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/27.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Precedence: bulk

Ilya Maximets <i.maximets@ovn.org> writes:

> 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.  The inclusion of the flow key allows the userspace
>> 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.  For these messages,
>> having the flow key data merely adds additional data to the upcall
>> pipeline without any actual gain.  Userspace simply throws the data
>> away anyway.
>
> Hi, Aaron.  While it's true that OVS in userpsace is re-parsing the
> 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.  Mainly for the compatibility checking.  The use is described
> here in more details:
>   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.
>
> On the other hand, OVS today doesn't check the data, it only checks
> which fields are present.  So, if we can generate and pass the bitmap
> of fields present in the key or something similar without sending the
> full key, that might still save some CPU cycles and memory in the
> socket buffer while preserving the ability to check for forward and
> backward compatibility.  What do you think?

Maybe that can work.  I will try testing.  If so, then I would change
this semantic to send just the bitmap rather than omitting everything.

> The rest of the patch set seems useful even without patch #1 though.

I agree - but I didn't know if it made sense to submit the series
without adding something impactful (like a test).  I will work a bit
more on the flow area - maybe I can add enough actions and matches to
implement basic flow tests to submit while we think more about the feature.

> Nit: This patch #1 should probably be merged with the patch #6 and be
> at the end of a patch set, so the selftest and the main code are updated
> at the same time.

Okay - I can restructure them this way.

> Best regards, Ilya Maximets.