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

Eelco Chaudron echaudro at redhat.com
Fri Oct 15 13:42:25 UTC 2021


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. 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



More information about the dev mailing list