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 <[email protected]> > Reviewed-by: Eli Britstein <[email protected]> > --- <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 _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
