On Wed, 5 Nov 2020 02:46, Oliver Hartkopp wrote: > On 04.11.20 17:02, Jakub Kicinski wrote: >> On Wed, 4 Nov 2020 15:59:25 +0100 Oliver Hartkopp wrote: >>> On 04.11.20 09:16, Eric Dumazet wrote: > >>>> So skb_orphan(skb) in CAN before calling netif_rx() is better IMO. >>>> >>> >>> Unfortunately you missed the answer from Vincent, why skb_orphan() does >>> not work here: Hope that my answer did not go to the spam box. >>> https://lore.kernel.org/linux-can/CAMZ6RqJyZTcqZcq6jEzm5LLM_MMe=dYDbwvv=Y+dBR0drWuFmw@xxxxxxxxxxxxxx/ >> >> Okay, we can take this as a quick fix but to me it seems a little >> strange to be dropping what is effectively locally generated frames. For those who are not familiar with SocketCAN and to make sure that we are all aligned here, let me give a bit of context of how the echo on CAN skbs is usually implement in the drivers: * In the xmit() branch, the driver would queue the skb using can_put_echo_skb(). * Whenever the driver gets notified of a successful TX, it will loopback the skb using can_get_echo_skb(). This is why the loopback is usually done in hardware IRQ context (but drivers are also free to skip the second step and directly loopback the skb in the xmit() branch). > Thanks! So this patch doesn't hinder Marc's PR :-) > >> Can we use a NAPI poll model here and back pressure TX if the echo >> is not keeping up? > > Some of the CAN network drivers already support NAPI. I had a quick look at NAPI in the past and my understanding is that it requires the ability to turn off hardware interrupts meaning that it can be only used on some NIC, not but not, for example, on USB devices. It would be nice to extend the NAPI with skb loopback for drivers which already supports it but I am not sure how to include the other drivers. > @Marc: Can we also use NAPI for echo'ing the skbs? Yours sincerely, Vincent Mailhol
