On 12 Oct 2022, at 13:55, Cian Ferriter wrote:
> From: Kumar Amber <[email protected]>
>
> Create new APIs for the avx512 DPIF, enabling one baseline
> common code to be specialized into DPIF implementations for
> "outer" processing, and "recirc" processing.
>
> 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]>
I have some comments on patches 3 till 5, however, the main question remains,
do we really need all of these changes?
Asking, as it would be way simpler to just add the md_is_valid to the existing
function as you do for the mfex change. As it looks like we do not really need
two different callbacks at al.
I understand that for the mfex part you would like to have two different
function for better optimizations.
> ---
> lib/dpif-netdev-avx512.c | 25 +++++++++++++++++++++----
> lib/dpif-netdev-private-dpif.c | 6 +++---
> lib/dpif-netdev-private-dpif.h | 14 +++++++++++---
> lib/dpif-netdev.c | 5 ++---
> 4 files changed, 37 insertions(+), 13 deletions(-)
>
> diff --git a/lib/dpif-netdev-avx512.c b/lib/dpif-netdev-avx512.c
> index 83e7a1394..a36f4f312 100644
> --- a/lib/dpif-netdev-avx512.c
> +++ b/lib/dpif-netdev-avx512.c
> @@ -58,10 +58,10 @@ struct dpif_userdata {
> struct pkt_flow_meta pkt_meta[NETDEV_MAX_BURST];
> };
>
> -int32_t
> -dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd,
> - struct dp_packet_batch *packets,
> - odp_port_t in_port)
> +static inline int32_t ALWAYS_INLINE
As discussed in earlier AVX patches we should use a general int as a return
code, so this might be the time to clean this up.
Guess this might require the "typedef int32_t
(*dp_netdev_input_func_probe)(void);" to change and some other code, but I
think this patch is the right place ;)
> +dp_netdev_input_avx512__(struct dp_netdev_pmd_thread *pmd,
> + struct dp_packet_batch *packets,
> + bool md_is_valid OVS_UNUSED, odp_port_t in_port)
> {
> /* Allocate DPIF userdata. */
> if (OVS_UNLIKELY(!pmd->netdev_input_func_userdata)) {
> @@ -413,5 +413,22 @@ action_stage:
> return 0;
> }
>
> +int32_t
int
> +dp_netdev_input_avx512(struct dp_netdev_pmd_thread *pmd,
> + struct dp_packet_batch *packets,
> + odp_port_t in_port)
> +{
> + int ret = dp_netdev_input_avx512__(pmd, packets, false, in_port);
> + return ret;
Guess this should just be; return dp_netdev_input_avx512__(pmd, packets, false,
in_port);
> +}
> +
> +int32_t
int
> +dp_netdev_input_avx512_recirc(struct dp_netdev_pmd_thread *pmd,
> + struct dp_packet_batch *packets)
> +{
> + int ret = dp_netdev_input_avx512__(pmd, packets, true, 0);
> + return ret;
Guess this should just be; return dp_netdev_input_avx512__(pmd, packets, true,
0);
> +}
> +
> #endif
> #endif
> diff --git a/lib/dpif-netdev-private-dpif.c b/lib/dpif-netdev-private-dpif.c
> index 7f16fa0dc..07039b1c2 100644
> --- a/lib/dpif-netdev-private-dpif.c
> +++ b/lib/dpif-netdev-private-dpif.c
> @@ -40,7 +40,7 @@ enum dpif_netdev_impl_info_idx {
>
> #if DPIF_NETDEV_IMPL_AVX512_CHECK
> static int32_t
> -dp_netdev_input_outer_avx512_probe(void)
> +dp_netdev_input_avx512_probe(void)
> {
> if (!cpu_has_isa(OVS_CPU_ISA_X86_AVX512F)
> || !cpu_has_isa(OVS_CPU_ISA_X86_BMI2)) {
> @@ -60,8 +60,8 @@ static struct dpif_netdev_impl_info_t dpif_impls[] = {
>
> #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_outer_avx512,
> - .probe = dp_netdev_input_outer_avx512_probe,
> + [DPIF_NETDEV_IMPL_AVX512] = { .input_func = dp_netdev_input_avx512,
> + .probe = dp_netdev_input_avx512_probe,
> .name = "dpif_avx512", },
> #endif
> };
> diff --git a/lib/dpif-netdev-private-dpif.h b/lib/dpif-netdev-private-dpif.h
> index b3e75b7a2..4465d034f 100644
> --- a/lib/dpif-netdev-private-dpif.h
> +++ b/lib/dpif-netdev-private-dpif.h
> @@ -84,10 +84,18 @@ dp_netdev_input(struct dp_netdev_pmd_thread *pmd,
> struct dp_packet_batch *packets,
> odp_port_t in_port);
>
> +int32_t
Change this to int, and others below.
> +dp_netdev_recirculate(struct dp_netdev_pmd_thread *,
> + struct dp_packet_batch *);
> +
It looks like in this include file all definitions have argument names, so you
might as well add them here for uniformity.
> /* AVX512 enabled DPIF implementation function. */
> int32_t
> -dp_netdev_input_outer_avx512(struct dp_netdev_pmd_thread *pmd,
> - struct dp_packet_batch *packets,
> - odp_port_t in_port);
> +dp_netdev_input_avx512(struct dp_netdev_pmd_thread *pmd,
> + struct dp_packet_batch *packets,
> + odp_port_t in_port);
> +
> +int32_t
> +dp_netdev_input_avx512_recirc(struct dp_netdev_pmd_thread *pmd,
> + struct dp_packet_batch *packets);
>
> #endif /* netdev-private.h */
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 92e63599e..a50571dc8 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -545,8 +545,6 @@ static void dp_netdev_execute_actions(struct
> dp_netdev_pmd_thread *pmd,
> const struct flow *flow,
> const struct nlattr *actions,
> size_t actions_len);
> -static void dp_netdev_recirculate(struct dp_netdev_pmd_thread *,
> - struct dp_packet_batch *);
>
> static void dp_netdev_disable_upcall(struct dp_netdev *);
> static void dp_netdev_pmd_reload_done(struct dp_netdev_pmd_thread *pmd);
> @@ -8492,11 +8490,12 @@ dp_netdev_input(struct dp_netdev_pmd_thread *pmd,
> return 0;
> }
>
> -static void
> +int32_t
please change to int.
> dp_netdev_recirculate(struct dp_netdev_pmd_thread *pmd,
> struct dp_packet_batch *packets)
> {
> dp_netdev_input__(pmd, packets, true, 0);
> + return 0;
> }
>
> struct dp_netdev_execute_aux {
> --
> 2.25.1
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
