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

Chris Mi cmi at nvidia.com
Wed Nov 3 08:44:04 UTC 2021


On 10/21/2021 4:27 PM, Eelco Chaudron wrote:
>
> On 21 Oct 2021, at 10:00, Chris Mi wrote:
>
>> On 10/15/2021 9:42 PM, Eelco Chaudron wrote:
>>> Small comments inline, and Ilya please take a look at the first comment/request.
>>>
>>> //Eelco
>>>
>>>
>>> On 12 Oct 2021, at 10:19, 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>
>>>> ---
>>> <SNIP>
>>>
>>>> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
>>>> index 7e11b9697..ce20cdeb1 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
>>>> @@ -635,6 +636,11 @@ struct dpif_class {
>>>>         * sufficient to store BOND_BUCKETS number of elements. */
>>>>        int (*bond_stats_get)(struct dpif *dpif, uint32_t bond_id,
>>>>                              uint64_t *n_bytes);
>>>> +
>>>> +    /* Some offload actions require functionality that is not netdev based,
>>>> +     * but dpif. Add dpif-offload-provider layer API to support such
>>>> +     * offload actions. */
>>>> +    const struct dpif_offload_api *offload_api;
>>>   From previous revisions:
>>>
>>> | 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.
>>>
>>>   From my side, this looks wrong as there is a direct relation between dpif and dpif-offload. I would assume you should be able to pick a specific one, or what else would have stopped us from adding the dpif_offload_sflow_recv_wait()/dpif_offload_sflow_recv() directly in the dpif.
>> Done.
> Thanks, I’m on PTO for a week+ starting tomorrow, I’ll try to review once I get back.
>
>>> 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?
>> I assume this question is for Ilya. If it is for me, I don't have the answer now. 😅
>> Maybe we can open another thread to discuss it.
> 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.
>
> 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.
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.

All other offload stuff can be moved to dpif-offload layer. But that's 
not the main aim for dpif-offload.
I think the plan is out of scope of this patchset.
>
>>>>    };
>>>>
>>>>    extern const struct dpif_class dpif_netlink_class;
>>>> diff --git a/lib/dpif.c b/lib/dpif.c
>>>> index 8c4aed47b..51cf5d666 100644
>>>> --- a/lib/dpif.c
>>>> +++ b/lib/dpif.c
>>>> @@ -153,6 +153,15 @@ dp_register_provider__(const struct dpif_class *new_class)
>>>>            return error;
>>>>        }
>>>>
>>>> +    if (new_class->offload_api && new_class->offload_api->init) {
>>>> +        error = new_class->offload_api->init();
>>>> +        if (error) {
>>>> +            VLOG_WARN("failed to initialize %s datapath class for offload: %s",
>>> Please use a capital F for Failed.
>> Done.
>>
>> Eelco, thanks for your review on this series. I'll send v17. If you still have comments, please let me know.
>>
>> Thanks,
>> Chris



More information about the dev mailing list