netdev - Re: [PATCH v2 net-next 1/3] soc/fsl/qbman: Add an argument to signal if NAPI processing is required.

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: <8e25b5d2-0c59-e756-8c04-479d1ad5a775@nxp.com>
Date:   Mon, 2 Nov 2020 23:10:18 +0200
From:   Horia Geantă <horia.geanta@....com>
To:     Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        Priyanka Jain <priyanka.jain@....com>,
        Vakul Garg <vakul.garg@....com>
Cc:     Aymen Sghaier <aymen.sghaier@....com>,
        Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Madalin Bucur <madalin.bucur@....com>,
        Jakub Kicinski <kuba@...nel.org>, Leo Li <leoyang.li@....com>,
        Thomas Gleixner <tglx@...utronix.de>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linuxppc-dev@...ts.ozlabs.org" <linuxppc-dev@...ts.ozlabs.org>,
        "linux-arm-kernel@...ts.infradead.org" 
        <linux-arm-kernel@...ts.infradead.org>
Subject: Re: [PATCH v2 net-next 1/3] soc/fsl/qbman: Add an argument to signal
 if NAPI processing is required.

On 11/2/2020 1:23 AM, Sebastian Andrzej Siewior wrote:
> dpaa_eth_napi_schedule() and caam_qi_napi_schedule() schedule NAPI if
> invoked from:
> 
>  - Hard interrupt context
>  - Any context which is not serving soft interrupts
> 
> Any context which is not serving soft interrupts includes hard interrupts
> so the in_irq() check is redundant. caam_qi_napi_schedule() has a comment
> about this:
> 
>         /*
>          * In case of threaded ISR, for RT kernels in_irq() does not return
>          * appropriate value, so use in_serving_softirq to distinguish between
>          * softirq and irq contexts.
>          */
>          if (in_irq() || !in_serving_softirq())
> 
> This has nothing to do with RT. Even on a non RT kernel force threaded
> interrupts run obviously in thread context and therefore in_irq() returns
> false when invoked from the handler.
> 
> The extension of the in_irq() check with !in_serving_softirq() was there
> when the drivers were added, but in the out of tree FSL BSP the original
> condition was in_irq() which got extended due to failures on RT.
> 
Looks like the initial FSL BSP commit adding this check is:
edca0b7a448a ("dpaa_eth: Fix Rx-stall issue in threaded ISR")
https://source.codeaurora.org/external/qoriq/qoriq-yocto-sdk/linux/commit/?h=fsl-sdk-v1.2&id=edca0b7a448ac18ef0a9b1238209b7595d511e19

This was done for dpaa_eth and the same logic was reused in caam.
In the process of upstreaming the development history got lost and
the comment in dpaa_eth was removed.

This was back in 2012 on a v3.0.34 kernel.
Not sure if/how things changed in the meantime, i.e. whether in_irq()
behaviour when called from softirq changed on -rt kernels (assuming this was
the problem Priyanka tried solving).

> The usage of in_xxx() in drivers is phased out and Linus clearly requested
> that code which changes behaviour depending on context should either be
> separated or the context be conveyed in an argument passed by the caller,
> which usually knows the context. Right he is, the above construct is
> clearly showing why.
> 
> The following callchains have been analyzed to end up in
> dpaa_eth_napi_schedule():
> 
> qman_p_poll_dqrr()
>   __poll_portal_fast()
>     fq->cb.dqrr()
>        dpaa_eth_napi_schedule()
> 
> portal_isr()
>   __poll_portal_fast()
>     fq->cb.dqrr()
>        dpaa_eth_napi_schedule()
> 
> Both need to schedule NAPI.
Only the call from interrupt context.

> The crypto part has another code path leading up to this:
>   kill_fq()
>      empty_retired_fq()
>        qman_p_poll_dqrr()
>          __poll_portal_fast()
>             fq->cb.dqrr()
>                dpaa_eth_napi_schedule()
> 
> kill_fq() is called from task context and ends up scheduling NAPI, but
> that's pointless and an unintended side effect of the !in_serving_softirq()
> check.
> 
Correct.

> The code path:
>   caam_qi_poll() -> qman_p_poll_dqrr()
> 
> is invoked from NAPI and I *assume* from crypto's NAPI device and not
> from qbman's NAPI device. I *guess* it is okay to skip scheduling NAPI
> (because this is what happens now) but could be changed if it is wrong
> due to `budget' handling.
> 
Looks good to me.

> Add an argument to __poll_portal_fast() which is true if NAPI needs to be
> scheduled. This requires propagating the value to the caller including
> `qman_cb_dqrr' typedef which is used by the dpaa and the crypto driver.
> 
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@...utronix.de>
> Cc: "Horia Geantă" <horia.geanta@....com>
> Cc: Aymen Sghaier <aymen.sghaier@....com>
> Cc: Herbert XS <herbert@...dor.apana.org.au>
> Cc: "David S. Miller" <davem@...emloft.net>
> Cc: Madalin Bucur <madalin.bucur@....com>
> Cc: Jakub Kicinski <kuba@...nel.org>
> Cc: Li Yang <leoyang.li@....com>
> Cc: linux-crypto@...r.kernel.org
> Cc: netdev@...r.kernel.org
> Cc: linuxppc-dev@...ts.ozlabs.org
> Cc: linux-arm-kernel@...ts.infradead.org
Reviewed-by: Horia Geantă <horia.geanta@....com>

Thanks,
Horia