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 <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.
> 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.
>>> };
>>>
>>> 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
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
