On 12 Oct 2022, at 13:55, Cian Ferriter wrote:
> From: Kumar Amber <[email protected]>
>
> This patch adds support for selecting the recirculation
> implementation based on the DPIF implementation.
>
> Signed-off-by: Kumar Amber <[email protected]>
> Signed-off-by: Cian Ferriter <[email protected]>
> Co-authored-by: Cian Ferriter <[email protected]>
> Acked-by: Sunil Pai G <[email protected]>
> ---
> lib/dpif-netdev-private-dpif.c | 73 +++++++++++++++++++++++---------
> lib/dpif-netdev-private-dpif.h | 18 ++++++++
> lib/dpif-netdev-private-thread.h | 3 ++
> lib/dpif-netdev.c | 14 +++++-
> 4 files changed, 88 insertions(+), 20 deletions(-)
>
> diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
> index 07039b1c2..3c7218ca0 100644
> --- a/lib/dpif-netdev-private-dpif.c
> +++ b/lib/dpif-netdev-private-dpif.c
> @@ -55,18 +55,39 @@ dp_netdev_input_avx512_probe(void)
> static struct dpif_netdev_impl_info_t dpif_impls[] = {
> /* The default scalar C code implementation. */
> [DPIF_NETDEV_IMPL_SCALAR] = { .input_func = dp_netdev_input,
> + .recirc_func = dp_netdev_recirculate,
> .probe = NULL,
> .name = "dpif_scalar", },
>
> #if DPIF_NETDEV_IMPL_AVX512_CHECK
> /* Only available on x86_64 bit builds with SSE 4.2 used for OVS core. */
> [DPIF_NETDEV_IMPL_AVX512] = { .input_func = dp_netdev_input_avx512,
> + .recirc_func = dp_netdev_input_avx512_recirc,
> .probe = dp_netdev_input_avx512_probe,
> .name = "dpif_avx512", },
> #endif
> };
>
> static dp_netdev_input_func default_dpif_func;
Now that we have more dpif functions, this should be called
default_dpif_input_func.
> +static dp_netdev_recirc_func default_dpif_recirc_func;
> +
> +static inline int
> +dp_netdev_dpif_probe(int dpif_idx)
This name sound odd to me, it should be somehting like:
dp_netdev_dpif_get_default_impl_idx()
Also I would change it to not have any input, just return
DPIF_NETDEV_IMPL_SCALAR.
So the code in the individual function can just be;
int dpif_idx = dp_netdev_dpif_probe();
> +{
> +/* Configure-time overriding to run test suite on all implementations. */
> +#if DPIF_NETDEV_IMPL_AVX512_CHECK
> +#ifdef DPIF_AVX512_DEFAULT
> + dp_netdev_input_func_probe probe;
> +
> + /* Check if the compiled default is compatible. */
> + probe = dpif_impls[DPIF_NETDEV_IMPL_AVX512].probe;
> + if (!probe || !probe()) {
> + dpif_idx = DPIF_NETDEV_IMPL_AVX512;
> + }
> +#endif
> +#endif
> + return dpif_idx;
> +}
>
> dp_netdev_input_func
> dp_netdev_impl_get_default(void)
> @@ -76,18 +97,7 @@ dp_netdev_impl_get_default(void)
> if (!default_dpif_func) {
> int dpif_idx = DPIF_NETDEV_IMPL_SCALAR;
>
> -/* Configure-time overriding to run test suite on all implementations. */
> -#if DPIF_NETDEV_IMPL_AVX512_CHECK
> -#ifdef DPIF_AVX512_DEFAULT
> - dp_netdev_input_func_probe probe;
> -
> - /* Check if the compiled default is compatible. */
> - probe = dpif_impls[DPIF_NETDEV_IMPL_AVX512].probe;
> - if (!probe || !probe()) {
> - dpif_idx = DPIF_NETDEV_IMPL_AVX512;
> - }
> -#endif
> -#endif
> + dpif_idx = dp_netdev_dpif_probe(dpif_idx);
>
> VLOG_INFO("Default DPIF implementation is %s.\n",
Vlog does not need a terminating \n.
> dpif_impls[dpif_idx].name);
> @@ -97,6 +107,24 @@ dp_netdev_impl_get_default(void)
> return default_dpif_func;
> }
>
> +dp_netdev_recirc_func
> +dp_netdev_recirc_impl_get_default(void)
> +{
> + /* For the first call, this will be NULL. Compute the compile time
> default.
> + */
> + if (!default_dpif_recirc_func) {
> + int dpif_idx = DPIF_NETDEV_IMPL_SCALAR;
> +
> + dpif_idx = dp_netdev_dpif_probe(dpif_idx);
> +
> + VLOG_INFO("Default re-circulate DPIF implementation is %s.\n",
Vlog does not need a terminating \n.
> + dpif_impls[dpif_idx].name);
> + default_dpif_recirc_func = dpif_impls[dpif_idx].recirc_func;
> + }
> +
> + return default_dpif_recirc_func;
> +}
> +
> void
> dp_netdev_impl_get(struct ds *reply, struct dp_netdev_pmd_thread **pmd_list,
> size_t n)
> @@ -132,10 +160,12 @@ dp_netdev_impl_get(struct ds *reply, struct
> dp_netdev_pmd_thread **pmd_list,
> * returns the function pointer to the one requested by "name".
> */
> static int32_t
> -dp_netdev_impl_get_by_name(const char *name, dp_netdev_input_func *out_func)
> +dp_netdev_impl_get_by_name(const char *name, dp_netdev_input_func *dpif_func,
This should be dpif_input_func
> + dp_netdev_recirc_func *dpif_recirc_func)
> {
> ovs_assert(name);
> - ovs_assert(out_func);
> + ovs_assert(dpif_func);
> + ovs_assert(dpif_recirc_func);
>
> uint32_t i;
>
> @@ -145,11 +175,13 @@ dp_netdev_impl_get_by_name(const char *name,
> dp_netdev_input_func *out_func)
> if (dpif_impls[i].probe) {
> int probe_err = dpif_impls[i].probe();
> if (probe_err) {
> - *out_func = NULL;
> + *dpif_func = NULL;
> + *dpif_recirc_func = NULL;
> return probe_err;
> }
> }
> - *out_func = dpif_impls[i].input_func;
> + *dpif_func = dpif_impls[i].input_func;
> + *dpif_recirc_func = dpif_impls[i].recirc_func;
> return 0;
I do have an objection to how this is implemented. This way, we assume every
implementation supports all implementation functions. We should rework this so
that it will support implementations that will only support one of the
functions. If the function is not supported, it should fall back to the scaler
implementation.
As a solution for dp_netdev_recirc_func, we could just always add a function
and return EOPNOTSUPP, however, this will not work for an implementation with a
missing dp_netdev_input_func. So something more general should be implemented
like was done for the action implementation.
> }
> }
> @@ -160,12 +192,15 @@ dp_netdev_impl_get_by_name(const char *name,
> dp_netdev_input_func *out_func)
> int32_t
> dp_netdev_impl_set_default_by_name(const char *name)
> {
> - dp_netdev_input_func new_default;
> + dp_netdev_input_func new_dpif_default;
I guess the new_default should be new_dpif_input_default so we know what this
function is about.
> + dp_netdev_recirc_func new_dpif_recirc_default;
>
> - int32_t err = dp_netdev_impl_get_by_name(name, &new_default);
> + int32_t err = dp_netdev_impl_get_by_name(name, &new_dpif_default,
> + &new_dpif_recirc_default);
>
> if (!err) {
> - default_dpif_func = new_default;
> + default_dpif_func = new_dpif_default;
> + default_dpif_recirc_func = new_dpif_recirc_default;
> }
>
> return err;
> diff --git a/lib/dpif-netdev-private-dpif.h b/lib/dpif-netdev-private-dpif.h
> index 4465d034f..dd4fedec3 100644
> --- a/lib/dpif-netdev-private-dpif.h
> +++ b/lib/dpif-netdev-private-dpif.h
We should probably also update the help text above,
--- a/lib/dpif-netdev-private-dpif.h
+++ b/lib/dpif-netdev-private-dpif.h
@@ -29,7 +29,7 @@ struct dp_netdev_pmd_thread;
struct dp_packet_batch;
struct ds;
-/* Typedef for DPIF functions.
+/* Typedef for DPIF input functions.
* Returns whether all packets were processed successfully.
> @@ -36,6 +36,16 @@ typedef int32_t (*dp_netdev_input_func)(struct
> dp_netdev_pmd_thread *pmd,
> struct dp_packet_batch *packets,
> odp_port_t port_no);
>
> +/* Typedef for DPIF re-circulate functions.
> + * The implementation on success must handle the whole batch of packets.
> + * The implementation on failure must return the error code and also
> + * the batch of packets must be unchnaged. After failure, scalar dpif
> + * is called and is expected to handle packet processing thereafter.
> + * Returns 0 on sucess else returns error code on failure.
> + */
> +typedef int32_t (*dp_netdev_recirc_func)(struct dp_netdev_pmd_thread *pmd,
type should be int, not int32_t
> + struct dp_packet_batch *packets);
> +
> /* Probe a DPIF implementation. This allows the implementation to validate
> CPU
> * ISA availability. Returns -ENOTSUP if not available, returns 0 if valid to
> * use.
> @@ -46,6 +56,10 @@ typedef int32_t (*dp_netdev_input_func_probe)(void);
> struct dpif_netdev_impl_info_t {
> /* Function pointer to execute to have this DPIF implementation run. */
> dp_netdev_input_func input_func;
> +
> + /* Function pointer to execute recirc DPIF implementation. */
> + dp_netdev_recirc_func recirc_func;
> +
> /* Function pointer to execute to check the CPU ISA is available to run.
> If
> * not necessary, it must be set to NULL which implies that it is always
> * valid to use. */
> @@ -63,6 +77,10 @@ dp_netdev_impl_get(struct ds *reply, struct
> dp_netdev_pmd_thread **pmd_list,
> * overridden at runtime. */
> dp_netdev_input_func dp_netdev_impl_get_default(void);
>
> +/* Returns the default recirculate DPIF which is first ./configure selected,
> + * but can be overridden at runtime. */
> +dp_netdev_recirc_func dp_netdev_recirc_impl_get_default(void);
> +
> /* Overrides the default DPIF with the user set DPIF. */
> int32_t dp_netdev_impl_set_default_by_name(const char *name);
>
> diff --git a/lib/dpif-netdev-private-thread.h
> b/lib/dpif-netdev-private-thread.h
> index 4472b199d..bce91358b 100644
> --- a/lib/dpif-netdev-private-thread.h
> +++ b/lib/dpif-netdev-private-thread.h
> @@ -124,6 +124,9 @@ struct dp_netdev_pmd_thread {
> /* Function pointer to call for dp_netdev_input() functionality. */
> ATOMIC(dp_netdev_input_func) netdev_input_func;
>
> + /* Function pointer to call for dp_netdev_recirculate() functionality. */
> + ATOMIC(dp_netdev_recirc_func) netdev_input_recirc_func;
Should this not be called netdev_recirc_func to be consistent?
> +
> /* Pointer for per-DPIF implementation scratch space. */
> void *netdev_input_func_userdata;
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index a50571dc8..99102c5a0 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -1148,6 +1148,10 @@ dpif_netdev_impl_set(struct unixctl_conn *conn, int
> argc OVS_UNUSED,
> * default. */
> atomic_store_relaxed(&pmd->netdev_input_func,
> dp_netdev_impl_get_default());
> + /* Initialize recirc DPIF function pointer to the newly
> configured
> + * default. */
> + atomic_store_relaxed(&pmd->netdev_input_recirc_func,
> + dp_netdev_recirc_impl_get_default());
> };
>
> free(pmd_list);
> @@ -7473,6 +7477,11 @@ dp_netdev_configure_pmd(struct dp_netdev_pmd_thread
> *pmd, struct dp_netdev *dp,
> /* Initialize DPIF function pointer to the default configured version. */
> atomic_init(&pmd->netdev_input_func, dp_netdev_impl_get_default());
>
> + /* Initialize recirculate DPIF function pointer to the default configured
> + * version. */
> + atomic_init(&pmd->netdev_input_recirc_func,
> + dp_netdev_recirc_impl_get_default());
> +
> /* Init default miniflow_extract function */
> atomic_init(&pmd->miniflow_extract_opt, dp_mfex_impl_get_default());
>
> @@ -8818,7 +8827,10 @@ dp_execute_cb(void *aux_, struct dp_packet_batch
> *packets_,
> }
>
> (*depth)++;
> - dp_netdev_recirculate(pmd, packets_);
> + int ret = pmd->netdev_input_recirc_func(pmd, packets_);
The function name should change to netdev_recirc_func.
Any reason why we do not add this code to the dp_netdev_recirculate() function?
This will also take care of the other use case of recirculating, i.e.
OVS_ACTION_ATTR_RECIRC, or is there a reason why this is not done?
In addition do we want a coverage counter for this?
> + if (ret != 0) {
We do not use the ret value, so maybe just all combine it in a single if?
if (pmd->netdev_input_recirc_func(pmd, packets_) != 0) {
dp_netdev_recirculate(pmd, packets_);
}
> + dp_netdev_recirculate(pmd, packets_);
> + }
> (*depth)--;
> return;
> }
> --
> 2.25.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
