netdev - Re: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [day] [month] [year] [list]

Message-ID: <CAO6EAnVO3px9thFJWyV=_UfmMBbSwt-uy=FvM4xxdYxZWknv3w@mail.gmail.com>
Date: Wed, 28 Aug 2024 11:31:01 -0400
From: Maksym Kutsevol <max@...sevol.com>
To: Breno Leitao <leitao@...ian.org>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole

Hey Breno,
thanks for looking at it.


On Wed, Aug 28, 2024 at 11:19 AM Breno Leitao <leitao@...ian.org> wrote:
>
> Hello Maksym,
>
> On Wed, Aug 28, 2024 at 10:26:20AM -0400, Maksym Kutsevol wrote:
> > > > diff --git a/drivers/net/netconsole.c b/drivers/net/netconsole.c
> > > > index 9c09293b5258..45c07ec7842d 100644
> > > > --- a/drivers/net/netconsole.c
> > > > +++ b/drivers/net/netconsole.c
> > > > @@ -82,6 +82,13 @@ static DEFINE_SPINLOCK(target_list_lock);
> > > >   */
> > > >  static struct console netconsole_ext;
> > > >
> > > > +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> > > > +struct netconsole_target_stats  {
> > > > +     size_t xmit_drop_count;
> > > > +     size_t enomem_count;
> > >
> > > I am looking at other drivers, and they use a specific type for these
> > > counters, u64_stats_sync.
> > > if it is possible to use this format, then you can leverage the
> > > `__u64_stats_update` helpers, and not worry about locking/overflow!?
> > >
> > Do you think that these counters really need more than an int?
>
> An int can overflow and become negative, so, you will see negative
> values, right?
It's an unsigned int (maybe not an int on every platform, but still unsigned),
it will become 0, not negative. E.g.
https://www.programiz.com/online-compiler/3qbkayylX5Cmf

> > Switching them to unsigned int instead might be better?
>
> Why not `u64_stats_sync` ?
>
Only because it's smaller and does the job. Unless locking is needed.

> > I'd argue that at the point when an external system collection
> > interval is not short enough
> > to see the unsigned counter going to a lesser value (counters are
> > unsigned, they go to 0 at UINT_MAX+1).
> > I need advice/pointer on locking - I'm looking at it and it looks to
> > me as if there's no locking needed when
> > updating a member of nt there. Tell me if I'm wrong.
>
> well, they are updated while holding `target_list_lock` right? But I
> would not rely on it.
Then I'll update to use them instead.

> If you just convert the values to u64_stats_sync, you get the
> synchronization for free, basically doing the following:
>
>         u64_stats_update_begin()
>         u64_stats_inc()
>         u64_stats_update_end()
>
> Thanks for working on it,
> --breno
Absolutely, my pleasure :)