[ovs-dev] [PATCH v15 3/7] dpif-offload-provider: Introduce dpif-offload-provider layer

Eelco Chaudron echaudro at redhat.com
Fri Oct 1 09:52:44 UTC 2021


See some small comments inline.

On 15 Sep 2021, at 14:43, 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>
> ---
>  lib/automake.mk             |  2 ++
>  lib/dpif-netdev.c           |  1 +
>  lib/dpif-netlink.c          |  2 ++
>  lib/dpif-offload-provider.h | 47 +++++++++++++++++++++++++++++++++++++
>  lib/dpif-offload.c          | 43 +++++++++++++++++++++++++++++++++
>  lib/dpif-provider.h         |  8 ++++++-
>  lib/dpif.c                  | 10 ++++++++
>  7 files changed, 112 insertions(+), 1 deletion(-)
>  create mode 100644 lib/dpif-offload-provider.h
>  create mode 100644 lib/dpif-offload.c
>
> 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 4cd0e9ebd..1645b44b0 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..75238c4cb
> --- /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. */
> +    void (*init)(void);

| EC> Do we want to think ahead and allow init to fail? Other init callbacks return int.
| CM> We don't want to fail the whole offload if dpif offload fails to init. So we don't check the return value.

Why? In theory, if dpif offload fails, all offload would fill as we could miss vital functions? I think we should add it and check for it (even in the current partial implementation can’t fail).

> +
> +    /* 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;

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


Guess we are still waiting for Ilya’s feedback on this…

>  };
>
>  extern const struct dpif_class dpif_netlink_class;
> diff --git a/lib/dpif.c b/lib/dpif.c
> index 8c4aed47b..13a3cd0d4 100644
> --- a/lib/dpif.c
> +++ b/lib/dpif.c
> @@ -153,6 +153,10 @@ dp_register_provider__(const struct dpif_class *new_class)
>          return error;
>      }
>
> +    if (new_class->offload_api && new_class->offload_api->init) {
> +        new_class->offload_api->init();
> +    }
> +
>      registered_class = xmalloc(sizeof *registered_class);
>      registered_class->dpif_class = new_class;
>      registered_class->refcount = 0;
> @@ -183,6 +187,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 +201,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.27.0



More information about the dev mailing list