Received-SPF: pass (google.com: domain of linux-kernel+bounces-135669-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:40f1:3f00::1 as permitted sender) client-ip=2604:1380:40f1:3f00::1;
Date: Mon, 8 Apr 2024 09:25:39 -0700
From: Breno Leitao <leitao@debian.org>
To: Eric Dumazet <edumazet@google.com>
Cc: kuba@kernel.org, davem@davemloft.net, pabeni@redhat.com,
	linux-kernel@vger.kernel.org, netdev@vger.kernel.org
Subject: Re: [PATCH net-next 3/3] net: dql: Optimize stall information
 population
Message-ID: <ZhQagwjRHBNsQbIw@gmail.com>
References: <20240404145939.3601097-1-leitao@debian.org>
 <20240404145939.3601097-4-leitao@debian.org>
 <CANn89iLhi365XA2-5vQwf+ELRYEep=NFSHxVAE=v0cXjpANh4w@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CANn89iLhi365XA2-5vQwf+ELRYEep=NFSHxVAE=v0cXjpANh4w@mail.gmail.com>

On Thu, Apr 04, 2024 at 06:36:00PM +0200, Eric Dumazet wrote:
> On Thu, Apr 4, 2024 at 5:00 PM Breno Leitao <leitao@debian.org> wrote:
> >
> > When Dynamic Queue Limit (DQL) is set, it always populate stall
> > information through dql_queue_stall().  However, this information is
> > only necessary if a stall threshold is set, stored in struct
> > dql->stall_thrs.
> >
> > dql_queue_stall() is cheap, but not free, since it does have memory
> > barriers and so forth.
> >
> > Do not call dql_queue_stall() if there is no stall threshold set, and
> > save some CPU cycles.
> >
> > Signed-off-by: Breno Leitao <leitao@debian.org>
> > ---
> >  include/linux/dynamic_queue_limits.h | 4 +++-
> >  1 file changed, 3 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/dynamic_queue_limits.h b/include/linux/dynamic_queue_limits.h
> > index 9980df0b7247..869afb800ea1 100644
> > --- a/include/linux/dynamic_queue_limits.h
> > +++ b/include/linux/dynamic_queue_limits.h
> > @@ -137,7 +137,9 @@ static inline void dql_queued(struct dql *dql, unsigned int count)
> >
> >         dql->num_queued += count;
> >
> > -       dql_queue_stall(dql);
> > +       /* Only populate stall information if the threshold is set */
> > +       if (READ_ONCE(dql->stall_thrs))
> > +               dql_queue_stall(dql);
> 
> Note that this field is not in the first cache line of 'struct dql',
> we will have some false sharing.
> 
> Perhaps we could copy it in a hole of the first cache line (used by producers).

That is a good point. Let me move stall_thrs to the first hole.

Thanks