From: Bartosz Golaszewski <brgl@bgdev.pl>
To: Arnd Bergmann <arnd@arndb.de>
Cc: Jonathan Corbet <corbet@lwn.net>,
Rob Herring <robh+dt@kernel.org>,
"David S . Miller" <davem@davemloft.net>,
Matthias Brugger <matthias.bgg@gmail.com>,
John Crispin <john@phrozen.org>,
Sean Wang <sean.wang@mediatek.com>,
Mark Lee <Mark-MC.Lee@mediatek.com>,
Jakub Kicinski <kuba@kernel.org>,
Fabien Parent <fparent@baylibre.com>,
Heiner Kallweit <hkallweit1@gmail.com>,
Edwin Peer <edwin.peer@broadcom.com>,
DTML <devicetree@vger.kernel.org>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
Networking <netdev@vger.kernel.org>,
Linux ARM <linux-arm-kernel@lists.infradead.org>,
"moderated list:ARM/Mediatek SoC..."
<linux-mediatek@lists.infradead.org>,
Stephane Le Provost <stephane.leprovost@mediatek.com>,
Pedro Tsai <pedro.tsai@mediatek.com>,
Andrew Perepech <andrew.perepech@mediatek.com>,
Bartosz Golaszewski <bgolaszewski@baylibre.com>
Subject: Re: [PATCH v3 10/15] net: ethernet: mtk-eth-mac: new driver
Date: Fri, 15 May 2020 14:56:09 +0200 [thread overview]
Message-ID: <CAMRc=Mf_vYt1J-cc6aZ2-Qv_YDEymVoC7ZiwuG9BrXoGMsXepw@mail.gmail.com> (raw)
In-Reply-To: <CAK8P3a0u53rHSW=72CnnbhrY28Z+9f=Yv2K-bbj5OD+2Ds4unA@mail.gmail.com>
pt., 15 maj 2020 o 14:04 Arnd Bergmann <arnd@arndb.de> napisał(a):
>
> On Fri, May 15, 2020 at 9:11 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> >
> > czw., 14 maj 2020 o 18:19 Arnd Bergmann <arnd@arndb.de> napisał(a):
> > >
> > > On Thu, May 14, 2020 at 10:00 AM Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> > > > +static unsigned int mtk_mac_intr_read_and_clear(struct mtk_mac_priv *priv)
> > > > +{
> > > > + unsigned int val;
> > > > +
> > > > + regmap_read(priv->regs, MTK_MAC_REG_INT_STS, &val);
> > > > + regmap_write(priv->regs, MTK_MAC_REG_INT_STS, val);
> > > > +
> > > > + return val;
> > > > +}
> > >
> > > Do you actually need to read the register? That is usually a relatively
> > > expensive operation, so if possible try to use clear the bits when
> > > you don't care which bits were set.
> > >
> >
> > I do care, I'm afraid. The returned value is being used in the napi
> > poll callback to see which ring to process.
>
> I suppose the other callers are not performance critical.
>
> For the rx and tx processing, it should be better to just always look at
> the queue directly and ignore the irq status, in particular when you
> are already in polling mode: suppose you receive ten frames at once
> and only process five but clear the irq flag.
>
> When the poll function is called again, you still need to process the
> others, but I would assume that the status tells you that nothing
> new has arrived so you don't process them until the next interrupt.
>
> For the statistics, I assume you do need to look at the irq status,
> but this doesn't have to be done in the poll function. How about
> something like:
>
> - in hardirq context, read the irq status word
> - irq rx or tx irq pending, call napi_schedule
> - if stats irq pending, schedule a work function
> - in napi poll, process both queues until empty or
> budget exhausted
> - if packet processing completed in poll function
> ack the irq and check again, call napi_complete
> - in work function, handle stats irq, then ack it
>
I see your point. I'll try to come up with something and send a new
version on Monday.
> > > > +static void mtk_mac_tx_complete_all(struct mtk_mac_priv *priv)
> > > > +{
> > > > + struct mtk_mac_ring *ring = &priv->tx_ring;
> > > > + struct net_device *ndev = priv->ndev;
> > > > + int ret;
> > > > +
> > > > + for (;;) {
> > > > + mtk_mac_lock(priv);
> > > > +
> > > > + if (!mtk_mac_ring_descs_available(ring)) {
> > > > + mtk_mac_unlock(priv);
> > > > + break;
> > > > + }
> > > > +
> > > > + ret = mtk_mac_tx_complete_one(priv);
> > > > + if (ret) {
> > > > + mtk_mac_unlock(priv);
> > > > + break;
> > > > + }
> > > > +
> > > > + if (netif_queue_stopped(ndev))
> > > > + netif_wake_queue(ndev);
> > > > +
> > > > + mtk_mac_unlock(priv);
> > > > + }
> > > > +}
> > >
> > > It looks like most of the stuff inside of the loop can be pulled out
> > > and only done once here.
> > >
> >
> > I did that in one of the previous submissions but it was pointed out
> > to me that a parallel TX path may fill up the queue before I wake it.
>
> Right, I see you plugged that hole, however the way you hold the
> spinlock across the expensive DMA management but then give it
> up in each loop iteration feels like this is not the most efficient
> way.
>
Maybe my thinking is wrong here, but I assumed that with a spinlock
it's better to give other threads the chance to run in between each
iteration. I didn't benchmark it though.
> The easy way would be to just hold the lock across the entire
> loop and then be sure you do it right. Alternatively you could
> minimize the locking and only do the wakeup after up do the final
> update to the tail pointer, at which point you know the queue is not
> full because you have just freed up at least one entry.
>
Makes sense, I'll see what I can do.
Bartosz
next prev parent reply other threads:[~2020-05-15 12:56 UTC|newest]
Thread overview: 26+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-05-14 7:59 [PATCH v3 00/15] mediatek: add support for MediaTek Ethernet MAC Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 01/15] dt-bindings: convert the binding document for mediatek PERICFG to yaml Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 02/15] dt-bindings: add new compatible to mediatek,pericfg Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 03/15] dt-bindings: net: add a binding document for MediaTek Ethernet MAC Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 04/15] net: ethernet: mediatek: rename Kconfig prompt Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 05/15] net: ethernet: mediatek: remove unnecessary spaces from Makefile Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 06/15] Documentation: devres: add a missing section for networking helpers Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 07/15] net: move devres helpers into a separate source file Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 08/15] net: devres: define a separate devres structure for devm_alloc_etherdev() Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 09/15] net: devres: provide devm_register_netdev() Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 10/15] net: ethernet: mtk-eth-mac: new driver Bartosz Golaszewski
2020-05-14 16:19 ` Arnd Bergmann
2020-05-15 7:11 ` Bartosz Golaszewski
2020-05-15 12:04 ` Arnd Bergmann
2020-05-15 12:56 ` Bartosz Golaszewski [this message]
2020-05-15 13:11 ` Arnd Bergmann
2020-05-15 13:14 ` Andrew Lunn
2020-05-15 13:32 ` Arnd Bergmann
2020-05-18 14:07 ` Bartosz Golaszewski
2020-05-18 14:38 ` Arnd Bergmann
2020-05-14 7:59 ` [PATCH v3 11/15] ARM64: dts: mediatek: add pericfg syscon to mt8516.dtsi Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 12/15] ARM64: dts: mediatek: add the ethernet node " Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 13/15] ARM64: dts: mediatek: add an alias for ethernet0 for pumpkin boards Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 14/15] ARM64: dts: mediatek: add ethernet pins " Bartosz Golaszewski
2020-05-14 7:59 ` [PATCH v3 15/15] ARM64: dts: mediatek: enable ethernet on " Bartosz Golaszewski
2020-05-14 19:56 ` [PATCH v3 00/15] mediatek: add support for MediaTek Ethernet MAC David Miller
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='CAMRc=Mf_vYt1J-cc6aZ2-Qv_YDEymVoC7ZiwuG9BrXoGMsXepw@mail.gmail.com' \
--to=brgl@bgdev.pl \
--cc=Mark-MC.Lee@mediatek.com \
--cc=andrew.perepech@mediatek.com \
--cc=arnd@arndb.de \
--cc=bgolaszewski@baylibre.com \
--cc=corbet@lwn.net \
--cc=davem@davemloft.net \
--cc=devicetree@vger.kernel.org \
--cc=edwin.peer@broadcom.com \
--cc=fparent@baylibre.com \
--cc=hkallweit1@gmail.com \
--cc=john@phrozen.org \
--cc=kuba@kernel.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=matthias.bgg@gmail.com \
--cc=netdev@vger.kernel.org \
--cc=pedro.tsai@mediatek.com \
--cc=robh+dt@kernel.org \
--cc=sean.wang@mediatek.com \
--cc=stephane.leprovost@mediatek.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).