> -----Original Message-----
> From: Amber, Kumar <[email protected]>
> Sent: Tuesday, May 24, 2022 12:39 PM
> To: [email protected]
> Cc: [email protected]; [email protected]; [email protected]; Van Haaren,
> Harry <[email protected]>; Ferriter, Cian <[email protected]>;
> Stokes, Ian <[email protected]>; Amber, Kumar <[email protected]>
> Subject: [PATCH v2 4/5] dpif-netdev: Add function pointer for dpif
> re-circulate.
>
> The patch adds and re-uses the dpif set command to set the
> function pointers to be used to switch between different inner
> dpifs.
>
> Signed-off-by: Kumar Amber <[email protected]>
> Signed-off-by: Cian Ferriter <[email protected]>
> Co-authored-by: Cian Ferriter <[email protected]>
> ---
> lib/dpif-netdev-private-dpif.c | 53 +++++++++++++++++++++++++++-----
> lib/dpif-netdev-private-dpif.h | 14 +++++++++
> lib/dpif-netdev-private-thread.h | 3 ++
> lib/dpif-netdev.c | 22 +++++++++++--
> 4 files changed, 83 insertions(+), 9 deletions(-)
>
> diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
> index 2dc51270a..49e719bde 100644
> --- a/lib/dpif-netdev-private-dpif.c
> +++ b/lib/dpif-netdev-private-dpif.c
> @@ -39,18 +39,21 @@ enum dpif_netdev_impl_info_idx {
> 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 (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD &&
> __SSE4_2__)
See comment below, can improve this multi VALUE && VALUE #ifdef.
<snip>
> +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;
> +
> +/* Configure-time overriding to run test suite on all implementations. */
> +#if (__x86_64__ && HAVE_AVX512F && HAVE_LD_AVX512_GOOD &&
> __SSE4_2__)
Sunil has posted a refactored/cleaner way of handling this; please update to
the latest best method;
https://patchwork.ozlabs.org/project/openvswitch/patch/[email protected]/
<snip>
> diff --git a/lib/dpif-netdev-private-dpif.h b/lib/dpif-netdev-private-dpif.h
> index 37908de9a..2ba032364 100644
> --- a/lib/dpif-netdev-private-dpif.h
> +++ b/lib/dpif-netdev-private-dpif.h
> @@ -36,6 +36,12 @@ 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.
> + * Returns whether all packets were processed successfully.
> + */
> +typedef int32_t (*dp_netdev_recirc_func)(struct dp_netdev_pmd_thread
> *pmd,
> + struct dp_packet_batch *packets);
The comment here is ambiguous, (refer back to 3/5 on return values comment).
Returns 1 on good, or 0 on good? Doc strings matter, as the typedef is the
official
"one true source" of how the implementing functions should behave.
It should be documented that an implementation must either pass or fail,
and either the whole batch of packets must be unchanged and failure returned,
or success and the whole batch of packets handled. This is as on error return,
the scalar implementation is called and expects the packets to require
processing.
(See snippet below for detail on error just passes the batch of packets_ to
scalar)
> @@ -8804,7 +8816,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_);
> + if (ret) {
> + dp_netdev_recirculate(pmd, packets_);
> + }
Note above about how an error return is handled by scalar impl.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
