diff mbox series

[ovs-dev,v4,12/12] dpif/dpcls: limit count subtable search info logs

Message ID 20210617162754.2028048-13-kumar.amber@intel.com
State Superseded
Headers show
Series MFEX Infrastructure + Optimizations | expand

Commit Message

Kumar Amber June 17, 2021, 4:27 p.m. UTC 
From: Harry van Haaren <harry.van.haaren@intel.com>

This commit avoids many instances of "using subtable X for miniflow (x,y)"
in the ovs-vswitchd log when using the DPCLS Autovalidator. This occurs
when no specialized subtable is found, and the generic "_any" version of
the avx512 subtable search implementation was used. This change logs the
subtable usage once, avoiding duplicates.

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 lib/dpif-netdev-lookup-avx512-gather.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Stokes, Ian June 29, 2021, 4:40 p.m. UTC | #1 
> From: Harry van Haaren <harry.van.haaren@intel.com>
> 
> This commit avoids many instances of "using subtable X for miniflow (x,y)"
> in the ovs-vswitchd log when using the DPCLS Autovalidator. This occurs
> when no specialized subtable is found, and the generic "_any" version of
> the avx512 subtable search implementation was used. This change logs the
> subtable usage once, avoiding duplicates.
> 

Good point here, I think people forget there is a cost to logs and no need to flood them.

Just to confirm, I think this log is already upstream? What I mean is that it is not added by either the DPIF or MFEX patch series so this is the earliest we can make the change on it?

Regards
Ian
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  lib/dpif-netdev-lookup-avx512-gather.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/lib/dpif-netdev-lookup-avx512-gather.c b/lib/dpif-netdev-lookup-
> avx512-gather.c
> index 2e754c89f..deed527b0 100644
> --- a/lib/dpif-netdev-lookup-avx512-gather.c
> +++ b/lib/dpif-netdev-lookup-avx512-gather.c
> @@ -411,7 +411,7 @@ dpcls_subtable_avx512_gather_probe(uint32_t u0_bits,
> uint32_t u1_bits)
>       */
>      if (!f && (u0_bits + u1_bits) < (NUM_U64_IN_ZMM_REG * 2)) {
>          f = dpcls_avx512_gather_mf_any;
> -        VLOG_INFO("Using avx512_gather_mf_any for subtable (%d,%d)\n",
> +        VLOG_INFO_ONCE("Using avx512_gather_mf_any for subtable
> (%d,%d)\n",
>                    u0_bits, u1_bits);
>      }
> 
> --
> 2.25.1
> 
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Van Haaren, Harry June 29, 2021, 4:46 p.m. UTC | #2 
> -----Original Message-----
> From: Stokes, Ian <ian.stokes@intel.com>
> Sent: Tuesday, June 29, 2021 5:40 PM
> To: Amber, Kumar <kumar.amber@intel.com>; dev@openvswitch.org
> Cc: i.maximets@ovn.org; Ferriter, Cian <cian.ferriter@intel.com>; Van Haaren,
> Harry <harry.van.haaren@intel.com>
> Subject: RE: [ovs-dev] [v4 12/12] dpif/dpcls: limit count subtable search info logs
> 
> > From: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > This commit avoids many instances of "using subtable X for miniflow (x,y)"
> > in the ovs-vswitchd log when using the DPCLS Autovalidator. This occurs
> > when no specialized subtable is found, and the generic "_any" version of
> > the avx512 subtable search implementation was used. This change logs the
> > subtable usage once, avoiding duplicates.
> >
> 
> Good point here, I think people forget there is a cost to logs and no need to flood
> them.
> 
> Just to confirm, I think this log is already upstream? What I mean is that it is not
> added by either the DPIF or MFEX patch series so this is the earliest we can make the
> change on it?

This change can be made earlier. The logs spam gets worse if we use the autovalidator, so it was
identified as an issue to fix when testing with MFEX autovalidator && DPCLS autovalidator,
hence why here in the patchset.

Can submit separately if preferred.


> Regards
> Ian

Thanks for review, -Harry

<snip patch contents>
Kumar Amber June 29, 2021, 5:03 p.m. UTC | #3 
Hi Ian,

Pls find the separated patch for DPCLS at :
http://patchwork.ozlabs.org/project/openvswitch/patch/20210629164941.156352-1-kumar.amber@intel.com/

Regards
Amber

> -----Original Message-----
> From: Van Haaren, Harry <harry.van.haaren@intel.com>
> Sent: Tuesday, June 29, 2021 10:16 PM
> To: Stokes, Ian <ian.stokes@intel.com>; Amber, Kumar
> <kumar.amber@intel.com>; dev@openvswitch.org
> Cc: i.maximets@ovn.org; Ferriter, Cian <cian.ferriter@intel.com>
> Subject: RE: [ovs-dev] [v4 12/12] dpif/dpcls: limit count subtable search info
> logs
> 
> > -----Original Message-----
> > From: Stokes, Ian <ian.stokes@intel.com>
> > Sent: Tuesday, June 29, 2021 5:40 PM
> > To: Amber, Kumar <kumar.amber@intel.com>; dev@openvswitch.org
> > Cc: i.maximets@ovn.org; Ferriter, Cian <cian.ferriter@intel.com>; Van
> > Haaren, Harry <harry.van.haaren@intel.com>
> > Subject: RE: [ovs-dev] [v4 12/12] dpif/dpcls: limit count subtable
> > search info logs
> >
> > > From: Harry van Haaren <harry.van.haaren@intel.com>
> > >
> > > This commit avoids many instances of "using subtable X for miniflow
> (x,y)"
> > > in the ovs-vswitchd log when using the DPCLS Autovalidator. This
> > > occurs when no specialized subtable is found, and the generic "_any"
> > > version of the avx512 subtable search implementation was used. This
> > > change logs the subtable usage once, avoiding duplicates.
> > >
> >
> > Good point here, I think people forget there is a cost to logs and no
> > need to flood them.
> >
> > Just to confirm, I think this log is already upstream? What I mean is
> > that it is not added by either the DPIF or MFEX patch series so this
> > is the earliest we can make the change on it?
> 
> This change can be made earlier. The logs spam gets worse if we use the
> autovalidator, so it was identified as an issue to fix when testing with MFEX
> autovalidator && DPCLS autovalidator, hence why here in the patchset.
> 
> Can submit separately if preferred.
> 
> 
> > Regards
> > Ian
> 
> Thanks for review, -Harry
> 
> <snip patch contents>
Stokes, Ian June 29, 2021, 5:07 p.m. UTC | #4 
> Hi Ian,
> 
> Pls find the separated patch for DPCLS at :
> http://patchwork.ozlabs.org/project/openvswitch/patch/20210629164941.1563
> 52-1-kumar.amber@intel.com/
> 
> Regards
> Amber

Just spotted it, thanks.

Regards
Ian
> 
> > -----Original Message-----
> > From: Van Haaren, Harry <harry.van.haaren@intel.com>
> > Sent: Tuesday, June 29, 2021 10:16 PM
> > To: Stokes, Ian <ian.stokes@intel.com>; Amber, Kumar
> > <kumar.amber@intel.com>; dev@openvswitch.org
> > Cc: i.maximets@ovn.org; Ferriter, Cian <cian.ferriter@intel.com>
> > Subject: RE: [ovs-dev] [v4 12/12] dpif/dpcls: limit count subtable search info
> > logs
> >
> > > -----Original Message-----
> > > From: Stokes, Ian <ian.stokes@intel.com>
> > > Sent: Tuesday, June 29, 2021 5:40 PM
> > > To: Amber, Kumar <kumar.amber@intel.com>; dev@openvswitch.org
> > > Cc: i.maximets@ovn.org; Ferriter, Cian <cian.ferriter@intel.com>; Van
> > > Haaren, Harry <harry.van.haaren@intel.com>
> > > Subject: RE: [ovs-dev] [v4 12/12] dpif/dpcls: limit count subtable
> > > search info logs
> > >
> > > > From: Harry van Haaren <harry.van.haaren@intel.com>
> > > >
> > > > This commit avoids many instances of "using subtable X for miniflow
> > (x,y)"
> > > > in the ovs-vswitchd log when using the DPCLS Autovalidator. This
> > > > occurs when no specialized subtable is found, and the generic "_any"
> > > > version of the avx512 subtable search implementation was used. This
> > > > change logs the subtable usage once, avoiding duplicates.
> > > >
> > >
> > > Good point here, I think people forget there is a cost to logs and no
> > > need to flood them.
> > >
> > > Just to confirm, I think this log is already upstream? What I mean is
> > > that it is not added by either the DPIF or MFEX patch series so this
> > > is the earliest we can make the change on it?
> >
> > This change can be made earlier. The logs spam gets worse if we use the
> > autovalidator, so it was identified as an issue to fix when testing with MFEX
> > autovalidator && DPCLS autovalidator, hence why here in the patchset.
> >
> > Can submit separately if preferred.
> >
> >
> > > Regards
> > > Ian
> >
> > Thanks for review, -Harry
> >
> > <snip patch contents>
Eelco Chaudron June 30, 2021, 2:57 p.m. UTC | #5 
No additional comments on this patch! This concludes my review of v4, looking forward to v5.

I will now do some additional tests on my non AVX512 machine. Guess I need to update my Intel NUC to an AVX512 supported one :)

//Eelco


On 17 Jun 2021, at 18:27, Kumar Amber wrote:

> From: Harry van Haaren <harry.van.haaren@intel.com>
>
> This commit avoids many instances of "using subtable X for miniflow (x,y)"
> in the ovs-vswitchd log when using the DPCLS Autovalidator. This occurs
> when no specialized subtable is found, and the generic "_any" version of
> the avx512 subtable search implementation was used. This change logs the
> subtable usage once, avoiding duplicates.
>
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> ---
>  lib/dpif-netdev-lookup-avx512-gather.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/lib/dpif-netdev-lookup-avx512-gather.c b/lib/dpif-netdev-lookup-avx512-gather.c
> index 2e754c89f..deed527b0 100644
> --- a/lib/dpif-netdev-lookup-avx512-gather.c
> +++ b/lib/dpif-netdev-lookup-avx512-gather.c
> @@ -411,7 +411,7 @@ dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits)
>       */
>      if (!f && (u0_bits + u1_bits) < (NUM_U64_IN_ZMM_REG * 2)) {
>          f = dpcls_avx512_gather_mf_any;
> -        VLOG_INFO("Using avx512_gather_mf_any for subtable (%d,%d)\n",
> +        VLOG_INFO_ONCE("Using avx512_gather_mf_any for subtable (%d,%d)\n",
>                    u0_bits, u1_bits);
>      }
>
> -- 
> 2.25.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Kumar Amber June 30, 2021, 3:11 p.m. UTC | #6 
Thanks Eelco sure go ahead :) upgrade will surely raise the pps! 

> -----Original Message-----
> From: Eelco Chaudron <echaudro@redhat.com>
> Sent: Wednesday, June 30, 2021 8:27 PM
> To: Amber, Kumar <kumar.amber@intel.com>; Van Haaren, Harry
> <harry.van.haaren@intel.com>
> Cc: dev@openvswitch.org; i.maximets@ovn.org; Stokes, Ian
> <ian.stokes@intel.com>; Flavio Leitner <fbl@sysclose.org>
> Subject: Re: [ovs-dev] [v4 12/12] dpif/dpcls: limit count subtable search info logs
> 
> No additional comments on this patch! This concludes my review of v4, looking
> forward to v5.
> 
> I will now do some additional tests on my non AVX512 machine. Guess I need to
> update my Intel NUC to an AVX512 supported one :)
> 
> //Eelco
> 
> 
> On 17 Jun 2021, at 18:27, Kumar Amber wrote:
> 
> > From: Harry van Haaren <harry.van.haaren@intel.com>
> >
> > This commit avoids many instances of "using subtable X for miniflow (x,y)"
> > in the ovs-vswitchd log when using the DPCLS Autovalidator. This
> > occurs when no specialized subtable is found, and the generic "_any"
> > version of the avx512 subtable search implementation was used. This
> > change logs the subtable usage once, avoiding duplicates.
> >
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> > ---
> >  lib/dpif-netdev-lookup-avx512-gather.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/lib/dpif-netdev-lookup-avx512-gather.c
> > b/lib/dpif-netdev-lookup-avx512-gather.c
> > index 2e754c89f..deed527b0 100644
> > --- a/lib/dpif-netdev-lookup-avx512-gather.c
> > +++ b/lib/dpif-netdev-lookup-avx512-gather.c
> > @@ -411,7 +411,7 @@ dpcls_subtable_avx512_gather_probe(uint32_t
> u0_bits, uint32_t u1_bits)
> >       */
> >      if (!f && (u0_bits + u1_bits) < (NUM_U64_IN_ZMM_REG * 2)) {
> >          f = dpcls_avx512_gather_mf_any;
> > -        VLOG_INFO("Using avx512_gather_mf_any for subtable (%d,%d)\n",
> > +        VLOG_INFO_ONCE("Using avx512_gather_mf_any for subtable
> > + (%d,%d)\n",
> >                    u0_bits, u1_bits);
> >      }
> >
> > --
> > 2.25.1
> >
> > _______________________________________________
> > dev mailing list
> > dev@openvswitch.org
> > https://mail.openvswitch.org/mailman/listinfo/ovs-dev
diff mbox series

Patch

diff --git a/lib/dpif-netdev-lookup-avx512-gather.c b/lib/dpif-netdev-lookup-avx512-gather.c
index 2e754c89f..deed527b0 100644
--- a/lib/dpif-netdev-lookup-avx512-gather.c
+++ b/lib/dpif-netdev-lookup-avx512-gather.c
@@ -411,7 +411,7 @@  dpcls_subtable_avx512_gather_probe(uint32_t u0_bits, uint32_t u1_bits)
      */
     if (!f && (u0_bits + u1_bits) < (NUM_U64_IN_ZMM_REG * 2)) {
         f = dpcls_avx512_gather_mf_any;
-        VLOG_INFO("Using avx512_gather_mf_any for subtable (%d,%d)\n",
+        VLOG_INFO_ONCE("Using avx512_gather_mf_any for subtable (%d,%d)\n",
                   u0_bits, u1_bits);
     }