[ovs-dev] [PATCH v17 3/8] dpif-offload-provider: Introduce dpif-offload-provider layer

Chris Mi cmi at nvidia.com
Sat Nov 6 05:51:39 UTC 2021


Hi Eelco,

Thanks for reviewing this patchset again. I will address them next week.

Thanks,
Chris

On 11/5/2021 8:18 PM, Eelco Chaudron wrote:
>
> On 21 Oct 2021, at 10:00, Chris Mi wrote:
>
>> Some offload actions require functionality that is not netdev
>> based, but dpif. For example, sFlow action requires to create
>> a psample netlink socket to receive the sampled packets from
>> TC or kernel driver.
>>
>> Create dpif-offload-provider layer to support such actions.
>>
>> Signed-off-by: Chris Mi <cmi at nvidia.com>
>> Reviewed-by: Eli Britstein <elibr at nvidia.com>
>> ---
>>   lib/automake.mk             |  2 ++
>>   lib/dpif-offload-provider.h | 40 ++++++++++++++++++++++++++++++++++
>>   lib/dpif-offload.c          | 43 +++++++++++++++++++++++++++++++++++++
>>   lib/dpif-provider.h         |  4 +++-
>>   4 files changed, 88 insertions(+), 1 deletion(-)
>>   create mode 100644 lib/dpif-offload-provider.h
>>   create mode 100644 lib/dpif-offload.c
>>
>> diff --git a/lib/automake.mk b/lib/automake.mk
>> index 46f869a33..9259f57de 100644
>> --- a/lib/automake.mk
>> +++ b/lib/automake.mk
>> @@ -127,6 +127,8 @@ lib_libopenvswitch_la_SOURCES = \
>>   	lib/dpif-netdev-private.h \
>>   	lib/dpif-netdev-perf.c \
>>   	lib/dpif-netdev-perf.h \
>> +	lib/dpif-offload.c \
>> +	lib/dpif-offload-provider.h \
>>   	lib/dpif-provider.h \
>>   	lib/dpif.c \
>>   	lib/dpif.h \
>> diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h
>> new file mode 100644
>> index 000000000..b5eb4ab04
>> --- /dev/null
>> +++ b/lib/dpif-offload-provider.h
>> @@ -0,0 +1,40 @@
>> +/*
>> + * Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#ifndef DPIF_OFFLOAD_PROVIDER_H
>> +#define DPIF_OFFLOAD_PROVIDER_H
>> +
>> +struct dpif;
>> +struct dpif_offload_sflow;
>> +
>> +/* Datapath interface offload structure, to be defined by each implementation
>> + * of a datapath interface.
>> + */
>> +struct dpif_offload_api {
>> +    /* Arranges for the poll loop for an upcall handler to wake up when psample
>> +     * has a message queued to be received. */
>> +    void (*sflow_recv_wait)(void);
>> +
>> +    /* Polls for an upcall from psample for an upcall handler.
>> +     * Return 0 for success. */
>> +    int (*sflow_recv)(struct dpif_offload_sflow *sflow);
>> +};
>>
> Not sure why you removed the init and destroy functions, as the offload API should be like a class (where multiple could exist), and init/destroy should be called when you register the offload class.
> Guess more comments will be in patch 5.
>
>> +void dpif_offload_sflow_recv_wait(const struct dpif *dpif);
>> +int dpif_offload_sflow_recv(const struct dpif *dpif,
>> +                            struct dpif_offload_sflow *sflow);
>> +
>> +#endif /* DPIF_OFFLOAD_PROVIDER_H */
>> diff --git a/lib/dpif-offload.c b/lib/dpif-offload.c
>> new file mode 100644
>> index 000000000..44e79882f
>> --- /dev/null
>> +++ b/lib/dpif-offload.c
>> @@ -0,0 +1,43 @@
>> +/*
>> + * Copyright (c) 2021, NVIDIA CORPORATION & AFFILIATES. All rights reserved.
>> + *
>> + * Licensed under the Apache License, Version 2.0 (the "License");
>> + * you may not use this file except in compliance with the License.
>> + * You may obtain a copy of the License at:
>> + *
>> + *     http://www.apache.org/licenses/LICENSE-2.0
>> + *
>> + * Unless required by applicable law or agreed to in writing, software
>> + * distributed under the License is distributed on an "AS IS" BASIS,
>> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
>> + * See the License for the specific language governing permissions and
>> + * limitations under the License.
>> + */
>> +
>> +#include <config.h>
>> +#include <errno.h>
>> +
>> +#include "dpif-provider.h"
>> +
>> +void
>> +dpif_offload_sflow_recv_wait(const struct dpif *dpif)
>> +{
>> +    const struct dpif_offload_api *offload_api = dpif->offload_api;
>> +
>> +    if (offload_api && offload_api->sflow_recv_wait) {
>> +        offload_api->sflow_recv_wait();
>> +    }
>> +}
>> +
>> +int
>> +dpif_offload_sflow_recv(const struct dpif *dpif,
>> +                        struct dpif_offload_sflow *sflow)
>> +{
>> +    const struct dpif_offload_api *offload_api = dpif->offload_api;
>> +
>> +    if (offload_api && offload_api->sflow_recv) {
>> +        return offload_api->sflow_recv(sflow);
>> +    }
>> +
>> +    return EOPNOTSUPP;
>> +}
>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>> index 7e11b9697..15a0d2b63 100644
>> --- a/lib/dpif-provider.h
>> +++ b/lib/dpif-provider.h
>> @@ -22,8 +22,9 @@
>>    * exposed over OpenFlow as a single switch.  Datapaths and the collections of
>>    * ports that they contain may be fixed or dynamic. */
>>
>> -#include "openflow/openflow.h"
>>   #include "dpif.h"
>> +#include "dpif-offload-provider.h"
>> +#include "openflow/openflow.h"
>>   #include "util.h"
>>
>>   #ifdef  __cplusplus
>> @@ -35,6 +36,7 @@ extern "C" {
>>    * This structure should be treated as opaque by dpif implementations. */
>>   struct dpif {
>>       const struct dpif_class *dpif_class;
>> +    const struct dpif_offload_api *offload_api;
>>       char *base_name;
>>       char *full_name;
>>       uint8_t netflow_engine_type;
>> -- 
>> 2.30.2
> Adding the previous conversation about the API in general, so Ilya or who ever reviews this can follow up:
>
>
> EC> Here you add the provider directly into the dpif class. Not sure if this is what Ilya had in mind. As in general, these get integrated into the dpif/netdev, not the class. Ilya can you comment on/review this?
> CM> OK.
>
> EC> To me, it's also not clear how we would continue from here, are there any plans to move all offload stuff to the offload provider? If so, in what time frame?
>
> CM> I assume this question is for Ilya. If it is for me, I don't have the answer now. 😅
> CM> Maybe we can open another thread to discuss it.
>
> EC> Guess it’s for both you and Ilya. In some of the conversations with the CCed people, this new offload provider was discussed, and I remember it was mentioned that it only makes sense if there was a plan to follow through with the migration.
>
> EC> I’m afraid if we add the framework in this patch and it will not be followed up, it will cause confusion in the future.
>
> CM> OK, I see. We'll also discuss the plan internally.
>
> ====
>
> CM> The main idea of dpif-offload layer is for some offload actions that are not netdev based, but dpif based, like sFlow. And metering is another example that can leverage on it.
> CM> All other offload stuff can be moved to dpif-offload layer. But that's not the main aim for dpif-offload.
> CM> I think the plan is out of scope of this patchset.
>



More information about the dev mailing list