Hello! 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. >>>> 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? > [1] > https://wiki.linuxfoundation.org/networking/napi > > Best regards, > Yoshihiro Shimoda MBR, Sergei
