From: Vincent MAILHOL <mailhol.vincent@wanadoo.fr>
To: Oliver Hartkopp <socketcan@hartkopp.net>
Cc: Jakub Kicinski <kuba@kernel.org>,
Marc Kleine-Budde <mkl@pengutronix.de>,
Eric Dumazet <edumazet@google.com>,
netdev <netdev@vger.kernel.org>,
David Miller <davem@davemloft.net>,
linux-can <linux-can@vger.kernel.org>,
kernel@pengutronix.de
Subject: Re: [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard IRQ context
Date: Thu, 5 Nov 2020 13:47:53 +0900 [thread overview]
Message-ID: <CAMZ6RqK=oEn3pgHb2byMC_SOVdG3Bbsfzssu9Fd-jDpSzEbrwQ@mail.gmail.com> (raw)
In-Reply-To: <550bf8d4-bf4c-b1ef-cd41-78c2b71514e3@hartkopp.net>
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@mail.gmail.com/
>>
>> 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
next prev parent reply other threads:[~2020-11-05 4:48 UTC|newest]
Thread overview: 41+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-03 22:06 pull-request: can 2020-11-03 Marc Kleine-Budde
2020-11-03 22:06 ` [net 01/27] dt-bindings: can: add can-controller.yaml Marc Kleine-Budde
2020-11-03 22:06 ` [net 02/27] dt-bindings: can: flexcan: convert fsl,*flexcan bindings to yaml Marc Kleine-Budde
2020-11-03 22:06 ` [net 03/27] can: proc: can_remove_proc(): silence remove_proc_entry warning Marc Kleine-Budde
2020-11-03 22:06 ` [net 04/27] can: rx-offload: don't call kfree_skb() from IRQ context Marc Kleine-Budde
2020-11-03 22:06 ` [net 05/27] can: dev: can_get_echo_skb(): prevent call to kfree_skb() in hard " Marc Kleine-Budde
2020-11-04 1:21 ` Jakub Kicinski
2020-11-04 4:29 ` Vincent MAILHOL
2020-11-04 8:16 ` Eric Dumazet
2020-11-04 14:59 ` Oliver Hartkopp
2020-11-04 16:02 ` Jakub Kicinski
2020-11-04 17:46 ` Oliver Hartkopp
2020-11-05 4:47 ` Vincent MAILHOL [this message]
2020-11-05 7:44 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 06/27] can: dev: __can_get_echo_skb(): fix real payload length return value for RTR frames Marc Kleine-Budde
2020-11-03 22:06 ` [net 07/27] can: can_create_echo_skb(): fix echo skb generation: always use skb_clone() Marc Kleine-Budde
2020-11-03 22:06 ` [net 08/27] can: j1939: rename jacd tool Marc Kleine-Budde
2020-11-03 22:06 ` [net 09/27] can: j1939: fix syntax and spelling Marc Kleine-Budde
2020-11-03 22:06 ` [net 10/27] can: j1939: swap addr and pgn in the send example Marc Kleine-Budde
2020-11-03 22:06 ` [net 11/27] can: j1939: use backquotes for code samples Marc Kleine-Budde
2020-11-03 22:06 ` [net 12/27] can: j1939: j1939_sk_bind(): return failure if netdev is down Marc Kleine-Budde
2020-11-03 22:06 ` [net 13/27] can: isotp: Explain PDU in CAN_ISOTP help text Marc Kleine-Budde
2020-11-03 22:06 ` [net 14/27] can: isotp: isotp_rcv_cf(): enable RX timeout handling in listen-only mode Marc Kleine-Budde
2020-11-03 22:06 ` [net 15/27] can: isotp: padlen(): make const array static, makes object smaller Marc Kleine-Budde
2020-11-03 22:06 ` [net 16/27] can: ti_hecc: ti_hecc_probe(): add missed clk_disable_unprepare() in error path Marc Kleine-Budde
2020-11-03 22:06 ` [net 17/27] can: xilinx_can: handle failure cases of pm_runtime_get_sync Marc Kleine-Budde
2020-11-03 22:06 ` [net 18/27] can: peak_usb: add range checking in decode operations Marc Kleine-Budde
2020-11-03 22:06 ` [net 19/27] can: peak_usb: peak_usb_get_ts_time(): fix timestamp wrapping Marc Kleine-Budde
2020-11-03 22:06 ` [net 20/27] can: peak_canfd: pucan_handle_can_rx(): fix echo management when loopback is on Marc Kleine-Budde
2020-11-03 22:06 ` [net 21/27] can: mcp251xfd: mcp251xfd_regmap_crc_read(): increase severity of CRC read error messages Marc Kleine-Budde
2020-11-05 16:24 ` Manivannan Sadhasivam
2020-11-05 16:39 ` Marc Kleine-Budde
2020-11-05 18:14 ` Manivannan Sadhasivam
2020-11-06 8:35 ` Marc Kleine-Budde
2020-11-03 22:06 ` [net 22/27] can: mcp251xfd: mcp251xfd_regmap_nocrc_read(): fix semicolon.cocci warnings Marc Kleine-Budde
2020-11-03 22:06 ` [net 23/27] can: mcp251xfd: remove unneeded break Marc Kleine-Budde
2020-11-03 22:06 ` [net 24/27] can: flexcan: remove FLEXCAN_QUIRK_DISABLE_MECR quirk for LS1021A Marc Kleine-Budde
2020-11-03 22:06 ` [net 25/27] can: flexcan: add ECC initialization for LX2160A Marc Kleine-Budde
2020-11-03 22:06 ` [net 26/27] can: flexcan: add ECC initialization for VF610 Marc Kleine-Budde
2020-11-03 22:06 ` [net 27/27] can: flexcan: flexcan_remove(): disable wakeup completely Marc Kleine-Budde
2020-11-04 18:43 ` pull-request: can 2020-11-03 Jakub Kicinski
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='CAMZ6RqK=oEn3pgHb2byMC_SOVdG3Bbsfzssu9Fd-jDpSzEbrwQ@mail.gmail.com' \
--to=mailhol.vincent@wanadoo.fr \
--cc=davem@davemloft.net \
--cc=edumazet@google.com \
--cc=kernel@pengutronix.de \
--cc=kuba@kernel.org \
--cc=linux-can@vger.kernel.org \
--cc=mkl@pengutronix.de \
--cc=netdev@vger.kernel.org \
--cc=socketcan@hartkopp.net \
/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).