From: "Horia Geantă" <horia.geanta@nxp.com>
To: Sebastian Andrzej Siewior <bigeasy@linutronix.de>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Priyanka Jain <priyanka.jain@nxp.com>,
Vakul Garg <vakul.garg@nxp.com>
Cc: Aymen Sghaier <aymen.sghaier@nxp.com>,
Herbert Xu <herbert@gondor.apana.org.au>,
Madalin Bucur <madalin.bucur@nxp.com>,
Leo Li <leoyang.li@nxp.com>,
"linux-crypto@vger.kernel.org" <linux-crypto@vger.kernel.org>,
Jakub Kicinski <kuba@kernel.org>,
Thomas Gleixner <tglx@linutronix.de>,
"linuxppc-dev@lists.ozlabs.org" <linuxppc-dev@lists.ozlabs.org>,
"David S. Miller" <davem@davemloft.net>,
"linux-arm-kernel@lists.infradead.org"
<linux-arm-kernel@lists.infradead.org>
Subject: Re: [PATCH v2 net-next 1/3] soc/fsl/qbman: Add an argument to signal if NAPI processing is required.
Date: Mon, 2 Nov 2020 23:10:18 +0200 [thread overview]
Message-ID: <8e25b5d2-0c59-e756-8c04-479d1ad5a775@nxp.com> (raw)
In-Reply-To: <20201101232257.3028508-2-bigeasy@linutronix.de>
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@linutronix.de>
> Cc: "Horia Geantă" <horia.geanta@nxp.com>
> Cc: Aymen Sghaier <aymen.sghaier@nxp.com>
> Cc: Herbert XS <herbert@gondor.apana.org.au>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: Madalin Bucur <madalin.bucur@nxp.com>
> Cc: Jakub Kicinski <kuba@kernel.org>
> Cc: Li Yang <leoyang.li@nxp.com>
> Cc: linux-crypto@vger.kernel.org
> Cc: netdev@vger.kernel.org
> Cc: linuxppc-dev@lists.ozlabs.org
> Cc: linux-arm-kernel@lists.infradead.org
Reviewed-by: Horia Geantă <horia.geanta@nxp.com>
Thanks,
Horia
next prev parent reply other threads:[~2020-11-02 21:12 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20201101232257.3028508-1-bigeasy@linutronix.de>
2020-11-01 23:22 ` [PATCH v2 net-next 1/3] soc/fsl/qbman: Add an argument to signal if NAPI processing is required Sebastian Andrzej Siewior
2020-11-02 21:10 ` Horia Geantă [this message]
2020-11-01 23:22 ` [PATCH v2 net-next 2/3] net: dpaa: Replace in_irq() usage Sebastian Andrzej Siewior
2020-11-01 23:22 ` [PATCH v2 net-next 3/3] crypto: caam: " Sebastian Andrzej Siewior
2020-11-02 21:13 ` Horia Geantă
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=8e25b5d2-0c59-e756-8c04-479d1ad5a775@nxp.com \
--to=horia.geanta@nxp.com \
--cc=aymen.sghaier@nxp.com \
--cc=bigeasy@linutronix.de \
--cc=davem@davemloft.net \
--cc=herbert@gondor.apana.org.au \
--cc=kuba@kernel.org \
--cc=leoyang.li@nxp.com \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-crypto@vger.kernel.org \
--cc=linuxppc-dev@lists.ozlabs.org \
--cc=madalin.bucur@nxp.com \
--cc=netdev@vger.kernel.org \
--cc=priyanka.jain@nxp.com \
--cc=tglx@linutronix.de \
--cc=vakul.garg@nxp.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).