On 18 Oct 2021, at 14:03, Chris Mi wrote:
> Hi Eelco,
>
> 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 <c...@nvidia.com>
>>> Reviewed-by: Eli Britstein <el...@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.
> I'll think about it. If I understand correctly, it should be something like
> this, right?
>
> struct dpif {
> const struct dpif_class *dpif_class;
> const struct dpif_offload_api *dpif_offload_api;
> ...
Yes, this is what I was referring to.
> Thanks,
> Chris
>> 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?
>>
>>> };
>>>
>>> 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.
>>
>>> + new_class->type, ovs_strerror(error));
>>> + return error;
>>> + }
>>> + }
>>> +
>>> registered_class = xmalloc(sizeof *registered_class);
>>> registered_class->dpif_class = new_class;
>>> registered_class->refcount = 0;
>>> @@ -183,6 +192,7 @@ static int
>>> dp_unregister_provider__(const char *type)
>>> {
>>> struct shash_node *node;
>>> + const struct dpif_class *dpif_class;
>>> struct registered_dpif_class *registered_class;
>>>
>>> node = shash_find(&dpif_classes, type);
>>> @@ -196,6 +206,11 @@ dp_unregister_provider__(const char *type)
>>> return EBUSY;
>>> }
>>>
>>> + dpif_class = registered_class->dpif_class;
>>> + if (dpif_class->offload_api && dpif_class->offload_api->destroy) {
>>> + dpif_class->offload_api->destroy();
>>> + }
>>> +
>>> shash_delete(&dpif_classes, node);
>>> free(registered_class);
>>>
>>> --
>>> 2.30.2
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
