From: Yoshihiro Shimoda <yoshihiro.shimoda.uh@renesas.com>
To: Sergei Shtylyov <sergei.shtylyov@gmail.com>
Cc: "netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-renesas-soc@vger.kernel.org"
<linux-renesas-soc@vger.kernel.org>,
"davem@davemloft.net" <davem@davemloft.net>,
"kuba@kernel.org" <kuba@kernel.org>
Subject: RE: [PATCH] net: renesas: ravb: Fix a stuck issue when a lot of frames are received
Date: Tue, 18 May 2021 02:20:12 +0000 [thread overview]
Message-ID: <TY2PR01MB369233B20CC5A48072BA474CD82C9@TY2PR01MB3692.jpnprd01.prod.outlook.com> (raw)
In-Reply-To: <0ce3a5e2-8e42-3648-83c3-fea7b1147b5a@gmail.com>
Hello!
> From: Sergei Shtylyov, Sent: Tuesday, May 18, 2021 4:36 AM
>
> On 5/10/21 1:29 PM, Yoshihiro Shimoda wrote:
>
> >>> Posting a review of the already commited (over my head) patch. It would have
> >>> been appropriate if the patch looked OK but it's not. :-/
> >>>
> >>>> When a lot of frames were received in the short term, the driver
> >>>> caused a stuck of receiving until a new frame was received. For example,
> >>>> the following command from other device could cause this issue.
> >>>>
> >>>> $ sudo ping -f -l 1000 -c 1000 <this driver's ipaddress>
> >>>
> >>> -l is essential here, right?
> >
> > Yes.
> >
> >>> Have you tried testing sh_eth sriver like that, BTW?
> >>
> >> It's driver! :-)
> >
> > I have not tried testing sh_eth driver yet. I'll test it after I got an actual board.
>
> Now you've got it, let's not rush forth with the fix this time.
I sent a report yesterday:
https://patchwork.kernel.org/project/linux-renesas-soc/patch/20210421045246.215779-1-yoshihiro.shimoda.uh@renesas.com/#24181167
> >>>> The previous code always cleared the interrupt flag of RX but checks
> >>>> the interrupt flags in ravb_poll(). So, ravb_poll() could not call
> >>>> ravb_rx() in the next time until a new RX frame was received if
> >>>> ravb_rx() returned true. To fix the issue, always calls ravb_rx()
> >>>> regardless the interrupt flags condition.
> >>>
> >>> That bacially defeats the purpose of IIUC...
> >> ^ NAPI,
> >>
> >> I was sure I typed NAPI here, yet it got lost in the edits. :-)
> >
> > I could not understand "that" (calling ravb_rx() regardless the interrupt
> > flags condition) defeats the purpose of NAPI. According to an article on
> > the Linux Foundation wiki [1], one of the purpose of NAPI is "Interrupt mitigation".
>
> Thank you for the pointer, BTW! Would have helped me with enabling NAPI in sh_eth
> (and ravb) drivers...
>
> > In poll(), the interrupts are already disabled, and ravb_rx() will check the
> > descriptor's status. So, this patch keeps the "Interrupt mitigation" IIUC.
>
> I think we'll still have the short race window, described in section 5.1
> of this doc. So perhaps what we should do is changing the order of the code in
> the poll() method, not eliminating the loops totally. Thoughts?
The ravb hardware acts as "non-level sensitive IRQs". However, fortunately,
the hardware can set an interrupt flag even if the interrupt is masked.
So, I don't think this patch have any race window.
Best regards,
Yoshihiro Shimoda
> > [1]
> >
> https://jpn01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fwiki.linuxfoundation.org%2Fnetworking%2Fnapi&d
> ata=04%7C01%7Cyoshihiro.shimoda.uh%40renesas.com%7C0102c1f2995947bcca1608d9196af978%7C53d82571da1947e49cb4625a166a4a
> 2a%7C0%7C0%7C637568769530134169%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6
> Mn0%3D%7C1000&sdata=47kgAmI3d%2Fz%2BHunT0a8bzHRRQk1VdnxRETSExLkTrdI%3D&reserved=0
> >
> > Best regards,
> > Yoshihiro Shimoda
>
> MBR, Sergei
prev parent reply other threads:[~2021-05-18 2:20 UTC|newest]
Thread overview: 12+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-21 4:52 [PATCH] net: renesas: ravb: Fix a stuck issue when a lot of frames are received Yoshihiro Shimoda
2021-04-21 18:00 ` patchwork-bot+netdevbpf
2021-04-21 18:14 ` Sergei Shtylyov
2021-04-21 18:20 ` David Miller
2021-05-05 19:00 ` Sergei Shtylyov
2021-05-05 20:09 ` Sergei Shtylyov
2021-05-08 20:47 ` Sergei Shtylyov
2021-05-09 10:21 ` Sergei Shtylyov
2021-05-10 10:29 ` Yoshihiro Shimoda
2021-05-17 8:23 ` Yoshihiro Shimoda
2021-05-17 19:35 ` Sergei Shtylyov
2021-05-18 2:20 ` Yoshihiro Shimoda [this message]
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=TY2PR01MB369233B20CC5A48072BA474CD82C9@TY2PR01MB3692.jpnprd01.prod.outlook.com \
--to=yoshihiro.shimoda.uh@renesas.com \
--cc=davem@davemloft.net \
--cc=kuba@kernel.org \
--cc=linux-renesas-soc@vger.kernel.org \
--cc=netdev@vger.kernel.org \
--cc=sergei.shtylyov@gmail.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).