| Message ID | 20211012081937.440411-4-cmi@nvidia.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Add offload support for sFlow | expand |
| Context | Check | Description |
|---|---|---|
| ovsrobot/apply-robot | success | apply and check: success |
| ovsrobot/github-robot-_Build_and_Test | success | github build: passed |
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@nvidia.com> > Reviewed-by: Eli Britstein <elibr@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. 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
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 <cmi@nvidia.com> >> Reviewed-by: Eli Britstein <elibr@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; ... 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
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 <cmi@nvidia.com> >>> Reviewed-by: Eli Britstein <elibr@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
On 10/18/2021 11:14 PM, Eelco Chaudron wrote: > > 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 <cmi@nvidia.com> >>>> Reviewed-by: Eli Britstein <elibr@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. OK, got it. Thanks, Chris
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@nvidia.com> >> Reviewed-by: Eli Britstein <elibr@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. > 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. > >> }; >> >> 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
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@nvidia.com> >>> Reviewed-by: Eli Britstein <elibr@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
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@nvidia.com> >>>> Reviewed-by: Eli Britstein <elibr@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. Enjoy your vacation! > >>> 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. OK, I see. We'll also discuss the plan internally. > >>>> }; >>>> >>>> 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
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@nvidia.com> >>>> Reviewed-by: Eli Britstein <elibr@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
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-netdev.c b/lib/dpif-netdev.c index 75f381ec1..52e1bb40b 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -8801,6 +8801,7 @@ const struct dpif_class dpif_netdev_class = { dpif_netdev_bond_add, dpif_netdev_bond_del, dpif_netdev_bond_stats_get, + NULL, /* dpif_offload_api */ }; static void diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 34fc04237..f546b48e5 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -34,6 +34,7 @@ #include "bitmap.h" #include "dpif-netlink-rtnl.h" +#include "dpif-offload-provider.h" #include "dpif-provider.h" #include "fat-rwlock.h" #include "flow.h" @@ -4340,6 +4341,7 @@ const struct dpif_class dpif_netlink_class = { NULL, /* bond_add */ NULL, /* bond_del */ NULL, /* bond_stats_get */ + NULL, /* dpif_offload_api */ }; static int diff --git a/lib/dpif-offload-provider.h b/lib/dpif-offload-provider.h new file mode 100644 index 000000000..44cc98aa1 --- /dev/null +++ b/lib/dpif-offload-provider.h @@ -0,0 +1,47 @@ +/* + * 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 { + /* Called when the dpif provider is registered and right after dpif + * provider init function. */ + int (*init)(void); + + /* Free all dpif offload resources. */ + void (*destroy)(void); + + /* 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); +}; + +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..842e05798 --- /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->dpif_class->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->dpif_class->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..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; }; 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", + 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);