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>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAO6EAnX0gqnDOxw5OZ7xT=3FMYoh0ELU5CTnsa6JtUxn0jX51Q@mail.gmail.com>
Date: Mon, 26 Aug 2024 19:55:36 -0400
From: Maksym Kutsevol <max@...sevol.com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: "David S. Miller" <davem@...emloft.net>, Eric Dumazet <edumazet@...gle.com>, 
	Paolo Abeni <pabeni@...hat.com>, Breno Leitao <leitao@...ian.org>, netdev@...r.kernel.org, 
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/2] netcons: Add udp send fail statistics to netconsole

Hey Jakub,
thank you for your time looking into this.

PS. Sorry for the html message, noob mistake.

On Mon, Aug 26, 2024 at 5:35 PM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Sat, 24 Aug 2024 14:50:24 -0700 Maksym Kutsevol wrote:
> > Enhance observability of netconsole. UDP sends can fail. Start tracking at
>
> nit: "UDP sends" sounds a bit too much like it's using sockets
> maybe "packet sends"?

Sure, it makes sense, I will update it.

>
> > least two failure possibilities: ENOMEM and NET_XMIT_DROP for every target.
> > Stats are exposed via an additional attribute in CONFIGFS.
>
> Please provide a reference to another configfs user in the kernel which
> exposes stats. To help reviewers validate it's a legit use case.
>
doc/Documentation/block/stat.txt describes what stats block devices expose.
The idea is the same there - a single read gives a coherent snapshot of stats.
Did you mean that the commit message needs updating or info provided
here is enough?

> > +#ifdef CONFIG_NETCONSOLE_DYNAMIC
> > +struct netconsole_target_stats  {
> > +     size_t xmit_drop_count;
> > +     size_t enomem_count;
> > +};
> > +#endif
>
> Don't hide types under ifdefs
> In fact I'm not sure if hiding stats if DYNAMIC isn't enabled makes
> sense. They don't take up much space.
>
I'll remove the ifdef.

Without _DYNAMIC it will not create the sysfs config group, so there
will be no place to expose the stats from,
hence the attachment to dynamic config.
The reason to hide the type comes from the same idea. It's not about
saving space, but about the fact that
it can't exist without it the way it's currently implemented. There's
no way to expose those metrics if _DYNAMIC
is not enabled.

> > +static ssize_t stats_show(struct config_item *item, char *buf)
> > +{
> > +     struct netconsole_target *nt = to_target(item);
> > +
> > +     return
> > +             nt->stats.xmit_drop_count, nt->stats.enomem_count);
>
> does configfs require value per file like sysfs or this is okay?

Docs say (Documentation/filesystems/sysfs.txt):

Attributes should be ASCII text files, preferably with only one value
per file. It is noted that it may not be efficient to contain only one
value per file, so it is socially acceptable to express an array of
values of the same type.

Given those are of the same type, I thought it's ok. To make it less
"fancy" maybe move to
just values separated by whitespace + a block in
Documentation/networking/netconsole.rst describing the format?
E.g. sysfs_emit(buf, "%lu %lu\n", .....) ? I really don't want to have
multiple files for it.
What do you think?

>
>
> >  /**
> >   * send_ext_msg_udp - send extended log message to target
> >   * @nt: target to send message to
> > @@ -1063,7 +1102,9 @@ static void send_ext_msg_udp(struct netconsole_target *nt, const char *msg,
> >                                            "%s", userdata);
> >
> >               msg_ready = buf;
> > -             netpoll_send_udp(&nt->np, msg_ready, msg_len);
> > +             count_udp_send_stats(nt, netpoll_send_udp(&nt->np,
> > +                                                       msg_ready,
> > +                                                       msg_len));
>
> Please add a wrapper which calls netpoll_send_udp() and counts the
> stats. This sort of nested function calls are unlikely to meet kernel
> coding requirements.

I see, will do. Noob question -  I couldn't find any written guidance
regarding this, if you have it handy - I'd appreciate
a link to some guidance regarding passing the result of a function to
a classifier vs wrapper function.

> --
> pw-bot: cr