netdev - Re: [PATCH RFC] r8152: Ensure that napi

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: <20210514144130.7287af8e@kicinski-fedora-PC1C0HJN>
Date:   Fri, 14 May 2021 14:41:30 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Thomas Gleixner <tglx@...utronix.de>
Cc:     linux-usb@...r.kernel.org, netdev@...r.kernel.org,
        Michal Svec <msvec@...e.com>,
        "David S. Miller" <davem@...emloft.net>,
        Hayes Wang <hayeswang@...ltek.com>,
        Thierry Reding <treding@...dia.com>,
        Lee Jones <lee.jones@...aro.org>,
        Borislav Petkov <bp@...en8.de>
Subject: Re: [PATCH RFC] r8152: Ensure that napi_schedule() is handled

On Fri, 14 May 2021 23:10:43 +0200 Thomas Gleixner wrote:
> On Fri, May 14 2021 at 13:46, Jakub Kicinski wrote:
> > On Fri, 14 May 2021 22:25:50 +0200 Thomas Gleixner wrote:  
> >> Except that some instruction cycle beancounters might complain about
> >> the extra conditional for the sane cases.
> >> 
> >> But yes, I'm fine with that as well. That's why this patch is marked RFC :)  
> >
> > When we're in the right context (irq/bh disabled etc.) the cost is just
> > read of preempt_count() and jump, right? And presumably preempt_count()
> > is in the cache already, because those sections aren't very long. Let me
> > make this change locally and see if it is in any way perceivable.  
> 
> Right. Just wanted to mention it :)
> 
> > Obviously if anyone sees a way to solve the problem without much
> > ifdefinery and force_irqthreads checks that'd be great - I don't.  
> 
> This is not related to force_irqthreads at all. This very driver invokes
> it from plain thread context.

I see, but a driver calling __napi_schedule_irqoff() from its IRQ
handler _would_ be an issue, right? Or do irq threads trigger softirq
processing on exit?

> > I'd rather avoid pushing this kind of stuff out to the drivers.  
> 
> You could have napi_schedule_intask() or something like that which would
> do the local_bh_disable()/enable() dance around the invocation of
> napi_schedule(). That would also document it clearly in the drivers. A
> quick grep shows a bunch of instances which could be replaced:
> 
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c-5704-		local_bh_disable();
> drivers/net/ethernet/mellanox/mlx4/en_netdev.c-1830-		local_bh_disable();
> drivers/net/usb/r8152.c-1552-	local_bh_disable();
> drivers/net/virtio_net.c-1355-	local_bh_disable();
> drivers/net/wireless/intel/iwlwifi/pcie/rx.c-1650-	local_bh_disable();
> drivers/net/wireless/intel/iwlwifi/pcie/rx.c-2015-		local_bh_disable();
> drivers/net/wireless/intel/iwlwifi/pcie/rx.c-2225-		local_bh_disable();
> drivers/net/wireless/intel/iwlwifi/pcie/rx.c-2235-		local_bh_disable();
> drivers/s390/net/qeth_core_main.c-3515-	local_bh_disable();

Very well aware, I've just sent a patch for mlx5 last week :)

My initial reaction was the same as yours - we should add lockdep
check, and napi_schedule_intask(). But then I started wondering
if it's all for nothing on rt or with force_irqthreads, and therefore
we should just eat the extra check.