netdev - Re: [PATCH net-next v2 2/3] netconsole: pr_err() when netpoll

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: <ZscMdc6wmUGlusM4@gmail.com>
Date: Thu, 22 Aug 2024 03:01:25 -0700
From: Breno Leitao <leitao@...ian.org>
To: Jakub Kicinski <kuba@...nel.org>
Cc: davem@...emloft.net, edumazet@...gle.com, pabeni@...hat.com,
	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH net-next v2 2/3] netconsole: pr_err() when netpoll_setup
 fails

Hello Jakub,

On Wed, Aug 21, 2024 at 03:54:04PM -0700, Jakub Kicinski wrote:
> On Wed, 21 Aug 2024 01:41:55 -0700 Breno Leitao wrote:

> > Do you think this is useless?
> 
> I think it's better to push up more precise message into the fail sites.

Makese sense, I will remove it, and add the failing message once we
refactor ndo_netpoll_setup() callbacks.

> > Would it be better if the hot path just get one of the skbs from the
> > pool, and refill it in a workqueue? If the skb_poll() is empty, then
> > alloc_skb(len, GFP_ATOMIC) !?
> 
> Yeah, that seems a bit odd. If you can't find anything in the history
> that would explain this design - refactoring SG.

Thanks. I will add it to my todo list.

> > 2) Report statistic back from netpoll_send_udp(). netpoll_send_skb()
> > return values are being discarded, so, it is hard to know if the packet
> > was transmitted or got something as NET_XMIT_DROP, NETDEV_TX_BUSY,
> > NETDEV_TX_OK.
> > 
> > It is unclear where this should be reported two. Maybe a configfs entry?
> 
> Also sounds good. We don't use configfs much in networking so IDK if
> it's okay to use it for stats. But no other obviously better place
> comes to mind for me.

Exactly, configfs seems a bit weird, but, at the same time, I don't have
a better idea. Let me send a patch for this one, and we can continue the
discussion over there.

Thanks
--breno