From: "Niklas Söderlund" <niklas.soderlund+renesas@ragnatech.se>
To: Paul Barker <paul.barker.ct@bp.renesas.com>
Cc: Sergey Shtylyov <s.shtylyov@omp.ru>,
"David S. Miller" <davem@davemloft.net>,
Eric Dumazet <edumazet@google.com>,
Jakub Kicinski <kuba@kernel.org>, Paolo Abeni <pabeni@redhat.com>,
Geert Uytterhoeven <geert+renesas@glider.be>,
netdev@vger.kernel.org, linux-renesas-soc@vger.kernel.org,
linux-kernel@vger.kernel.org
Subject: Re: [PATCH net 2/4] net: ravb: Allow RX loop to move past DMA mapping errors
Date: Mon, 15 Apr 2024 11:52:06 +0200 [thread overview]
Message-ID: <20240415095206.GC3156415@ragnatech.se> (raw)
In-Reply-To: <d1ec92db-eefe-465a-9eb5-3a57c22b86c3@bp.renesas.com>
Hello Paul,
On 2024-04-15 08:12:06 +0100, Paul Barker wrote:
> On 14/04/2024 13:17, Niklas Söderlund wrote:
> > Hi Paul,
> >
> > Thanks for your patch.
> >
> > On 2024-04-11 12:44:31 +0100, Paul Barker wrote:
> >> The RX loops in ravb_rx_gbeth() and ravb_rx_rcar() skip to the next loop
> >> interation if a zero-length descriptor is seen (indicating a DMA mapping
> >> error). However, the current rx descriptor index `priv->cur_rx[q]` was
> >> incremented at the end of the loop and so would not be incremented when
> >> we skip to the next loop iteration. This would cause the loop to keep
> >> seeing the same zero-length descriptor instead of moving on to the next
> >> descriptor.
> >>
> >> As the loop counter `i` still increments, the loop would eventually
> >> terminate so there is no risk of being stuck here forever - but we
> >> should still fix this to avoid wasting cycles.
> >>
> >> To fix this, the rx descriptor index is incremented at the top of the
> >> loop, in the for statement itself. The assignments of `entry` and `desc`
> >> are brought into the loop to avoid the need for duplication.
> >>
> >> Fixes: d8b48911fd24 ("ravb: fix ring memory allocation")
> >> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> >> ---
> >> drivers/net/ethernet/renesas/ravb_main.c | 25 ++++++++++++------------
> >> 1 file changed, 13 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> >> index 70f2900648d4..028ab5c6aaf7 100644
> >> --- a/drivers/net/ethernet/renesas/ravb_main.c
> >> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> >> @@ -775,12 +775,15 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> >> int limit;
> >> int i;
> >>
> >> - entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> >> stats = &priv->stats[q];
> >>
> >> - desc = &priv->rx_ring[q].desc[entry];
> >> - for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> >> + for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> >> + entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> + desc = &priv->rx_ring[q].desc[entry];
> >> + if (desc->die_dt == DT_FEMPTY)
> >> + break;
> >> +
> >> /* Descriptor type must be checked before all other reads */
> >> dma_rmb();
> >> desc_status = desc->msc;
> >> @@ -848,9 +851,6 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> >> break;
> >> }
> >> }
> >> -
> >> - entry = (++priv->cur_rx[q]) % priv->num_rx_ring[q];
> >> - desc = &priv->rx_ring[q].desc[entry];
> >> }
> >>
> >> /* Refill the RX ring buffers. */
> >> @@ -891,7 +891,6 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> >> {
> >> struct ravb_private *priv = netdev_priv(ndev);
> >> const struct ravb_hw_info *info = priv->info;
> >> - int entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> struct net_device_stats *stats = &priv->stats[q];
> >> struct ravb_ex_rx_desc *desc;
> >> struct sk_buff *skb;
> >> @@ -900,12 +899,17 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> >> int rx_packets = 0;
> >> u8 desc_status;
> >> u16 pkt_len;
> >> + int entry;
> >> int limit;
> >> int i;
> >>
> >> limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> >> - desc = &priv->rx_ring[q].ex_desc[entry];
> >> - for (i = 0; i < limit && rx_packets < *quota && desc->die_dt != DT_FEMPTY; i++) {
> >> + for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> >> + entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> >> + desc = &priv->rx_ring[q].ex_desc[entry];
> >> + if (desc->die_dt == DT_FEMPTY)
> >> + break;
> >
> > I really like moving the assignment of entry and desc to the top of the
> > loop. But I don't like the loop limits as it's hard, at least for me, to
> > immediately see what's going on. How about,
> >
> > limit = min(priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q], *quota);
> >
> > for (i = 0; i < limit; i++) {
> > entry = priv->cur_rx[q] % priv->num_rx_ring[q];
> > desc = &priv->rx_ring[q].ex_desc[entry];
> >
> > /* There are no more valid descriptors after an empty one. */
> > if (desc->die_dt == DT_FEMPTY)
> > break;
> >
> > ...
> > }
>
> We need to count received packets separately from the number of
> descriptors processed, as done in the previous commit in this series,
> so we can't just have a single check against limit.
As noted in 1/4 I was only considering the R-Car code path where split
descriptors are not supported. I agree it's good to keep the two code
paths in sync and with that in mind I'm OK with this approach.
>
> We also need to increment priv->cur_rx[q]. If we put `priv->cur_rx[q]++`
> at the end of the loop then we're back to having to worry about it when
> we have a continue statement.
>
> We could move the `rx_packets < *quota` check inside the loop itself,
> but I don't see that as any clearer myself.
I do think this is a good idea however. As this would split the logic in
two distinct sets. The loop would only deal with descriptors and the
stop conditions based on number of packets / work done would be a stop
condition inside the loop.
Thinking a head a bit I think it would be nice if in future the private
data variable rx_1st_skb could be reworked as this will not play nice
with multiple queues. And with a split of the loop to only consider
descriptors we could try and look a head and only process a packet if
all descriptors for it are available to us. Lets cross that bridge when
we get to it. But I think having the loop only consider descriptors
would make this easier.
>
> Thanks,
>
> --
> Paul Barker
--
Kind Regards,
Niklas Söderlund
next prev parent reply other threads:[~2024-04-15 9:52 UTC|newest]
Thread overview: 15+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-11 11:44 [PATCH net 0/4] ravb Ethernet driver bugfixes Paul Barker
2024-04-11 11:44 ` [PATCH net 1/4] net: ravb: Count packets instead of descriptors in R-Car RX path Paul Barker
2024-04-11 16:23 ` Sergey Shtylyov
2024-04-14 12:08 ` Niklas Söderlund
2024-04-15 7:04 ` Paul Barker
2024-04-15 9:44 ` Niklas Söderlund
2024-04-11 11:44 ` [PATCH net 2/4] net: ravb: Allow RX loop to move past DMA mapping errors Paul Barker
2024-04-11 16:37 ` Sergey Shtylyov
2024-04-14 12:17 ` Niklas Söderlund
2024-04-15 7:12 ` Paul Barker
2024-04-15 9:52 ` Niklas Söderlund [this message]
2024-04-11 11:44 ` [PATCH net 3/4] net: ravb: Fix GbEth jumbo packet RX checksum handling Paul Barker
2024-04-11 16:50 ` Sergey Shtylyov
2024-04-11 11:44 ` [PATCH net 4/4] net: ravb: Fix RX byte accounting for jumbo packets Paul Barker
2024-04-11 17:56 ` Sergey Shtylyov
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=20240415095206.GC3156415@ragnatech.se \
--to=niklas.soderlund+renesas@ragnatech.se \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=geert+renesas@glider.be \
--cc=kuba@kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=pabeni@redhat.com \
--cc=paul.barker.ct@bp.renesas.com \
--cc=s.shtylyov@omp.ru \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).