* [PATCH 0/1] can: gs_usb: remove dma allocations @ 2022-09-13 20:41 Rhett Aultman 2022-09-13 20:41 ` [PATCH 1/1] " Rhett Aultman 2022-09-20 15:47 ` [PATCH 0/1] " Rhett Aultman 0 siblings, 2 replies; 8+ messages in thread From: Rhett Aultman @ 2022-09-13 20:41 UTC (permalink / raw) To: linux-can Dear USB CAN mailing list members, In a previous thread reviewing my changes to stop a DMA memory leak in the gs_usb driver, a discussion ensued about why it (and other USB CAN drivers were using DMA in the first place. At the time, it was suggested that we perform an exploration to determine if there is any performance benefit to DMA or if the DMA allocations could be replaced with normal kmalloc( )s instead. A method for gathering this information was proposed here: https://lore.kernel.org/linux-can/CAMZ6Rq+FSzy5ijQZhYyVJrbe86U9faD5aPFO4cezNkN9G-USzQ@mail.gmail.com/ A member of my team found some time and resources to work on it, and so I'm communicating his work to the list as an RFC on his behalf. We are seeking input from the mailing list regarding his testing methods as well as his patch. The short summary is that he couldn't identify any meaningful change in performance on his x86-64 hardware. We should be able to also repeat these tests on our ARM products under and older kernel and perhaps also on more modern ARM hardware such as a Raspberry Pi. This might give us the confidence we need to consider removing DMA allocations from the gs_usb driver and perhaps others. His detailed notes may be found here: https://www.dropbox.com/s/c3gm3oomihdze1n/USB%20CAN%20Driver%20DMA%20Removal%20Testing%20Doc.pdf?dl=0 Feedback is very much welcome. Best, Rhett Aultman ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] can: gs_usb: remove dma allocations 2022-09-13 20:41 [PATCH 0/1] can: gs_usb: remove dma allocations Rhett Aultman @ 2022-09-13 20:41 ` Rhett Aultman 2022-09-18 21:31 ` Marc Kleine-Budde 2022-09-20 15:47 ` [PATCH 0/1] " Rhett Aultman 1 sibling, 1 reply; 8+ messages in thread From: Rhett Aultman @ 2022-09-13 20:41 UTC (permalink / raw) To: linux-can; +Cc: Rhett Aultman, Vasanth Sadhasivan From: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> DMA allocated buffers are a precious resource. If there is no need for DMA allocations, then it might be worth to use non-dma allocated buffers. After testing the gs_usb driver with and without DMA allocation, there does not seem to be a significant change in latency or cpu utilization either way. Therefore, DMA allocation is not necessary and removed. Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com> Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com> Signed-off-by: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> --- drivers/net/can/usb/gs_usb.c | 38 ++++++++++-------------------------- 1 file changed, 10 insertions(+), 28 deletions(-) diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c index baf749c8cda3..1bfc775c62c5 100644 --- a/drivers/net/can/usb/gs_usb.c +++ b/drivers/net/can/usb/gs_usb.c @@ -269,7 +269,6 @@ struct gs_can { struct usb_anchor tx_submitted; atomic_t active_tx_urbs; void *rxbuf[GS_MAX_RX_URBS]; - dma_addr_t rxbuf_dma[GS_MAX_RX_URBS]; }; /* usb interface struct */ @@ -587,9 +586,7 @@ static void gs_usb_xmit_callback(struct urb *urb) if (urb->status) netdev_info(netdev, "usb xmit fail %u\n", txc->echo_id); - - usb_free_coherent(urb->dev, urb->transfer_buffer_length, - urb->transfer_buffer, urb->transfer_dma); + devm_kfree(&urb->dev->dev, urb->transfer_buffer); } static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, @@ -618,8 +615,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, if (!urb) goto nomem_urb; - hf = usb_alloc_coherent(dev->udev, dev->hf_size_tx, GFP_ATOMIC, - &urb->transfer_dma); + hf = devm_kmalloc(&dev->udev->dev, dev->hf_size_tx, GFP_ATOMIC); if (!hf) { netdev_err(netdev, "No memory left for USB buffer\n"); goto nomem_hf; @@ -663,7 +659,6 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, hf, dev->hf_size_tx, gs_usb_xmit_callback, txc); - urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; usb_anchor_urb(urb, &dev->tx_submitted); can_put_echo_skb(skb, netdev, idx, 0); @@ -678,8 +673,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, gs_free_tx_context(txc); usb_unanchor_urb(urb); - usb_free_coherent(dev->udev, urb->transfer_buffer_length, - urb->transfer_buffer, urb->transfer_dma); + devm_kfree(&dev->udev->dev, hf); if (rc == -ENODEV) { netif_device_detach(netdev); @@ -699,8 +693,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, return NETDEV_TX_OK; badidx: - usb_free_coherent(dev->udev, urb->transfer_buffer_length, - urb->transfer_buffer, urb->transfer_dma); + devm_kfree(&dev->udev->dev, hf); nomem_hf: usb_free_urb(urb); @@ -744,7 +737,6 @@ static int gs_can_open(struct net_device *netdev) for (i = 0; i < GS_MAX_RX_URBS; i++) { struct urb *urb; u8 *buf; - dma_addr_t buf_dma; /* alloc rx urb */ urb = usb_alloc_urb(0, GFP_KERNEL); @@ -752,10 +744,9 @@ static int gs_can_open(struct net_device *netdev) return -ENOMEM; /* alloc rx buffer */ - buf = usb_alloc_coherent(dev->udev, - dev->parent->hf_size_rx, - GFP_KERNEL, - &buf_dma); + buf = devm_kmalloc(&dev->udev->dev, + dev->parent->hf_size_rx, + GFP_KERNEL); if (!buf) { netdev_err(netdev, "No memory left for USB buffer\n"); @@ -763,8 +754,6 @@ static int gs_can_open(struct net_device *netdev) return -ENOMEM; } - urb->transfer_dma = buf_dma; - /* fill, anchor, and submit rx urb */ usb_fill_bulk_urb(urb, dev->udev, @@ -773,7 +762,6 @@ static int gs_can_open(struct net_device *netdev) buf, dev->parent->hf_size_rx, gs_usb_receive_bulk_callback, parent); - urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; usb_anchor_urb(urb, &parent->rx_submitted); @@ -786,16 +774,12 @@ static int gs_can_open(struct net_device *netdev) "usb_submit failed (err=%d)\n", rc); usb_unanchor_urb(urb); - usb_free_coherent(dev->udev, - sizeof(struct gs_host_frame), - buf, - buf_dma); + devm_kfree(&dev->udev->dev, buf); usb_free_urb(urb); break; } dev->rxbuf[i] = buf; - dev->rxbuf_dma[i] = buf_dma; /* Drop reference, * USB core will take care of freeing it @@ -863,10 +847,8 @@ static int gs_can_close(struct net_device *netdev) if (!parent->active_channels) { usb_kill_anchored_urbs(&parent->rx_submitted); for (i = 0; i < GS_MAX_RX_URBS; i++) - usb_free_coherent(dev->udev, - sizeof(struct gs_host_frame), - dev->rxbuf[i], - dev->rxbuf_dma[i]); + devm_kfree(&dev->udev->dev, + dev->rxbuf[i]); } /* Stop sending URBs */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] can: gs_usb: remove dma allocations 2022-09-13 20:41 ` [PATCH 1/1] " Rhett Aultman @ 2022-09-18 21:31 ` Marc Kleine-Budde 0 siblings, 0 replies; 8+ messages in thread From: Marc Kleine-Budde @ 2022-09-18 21:31 UTC (permalink / raw) To: Rhett Aultman; +Cc: linux-can, Vasanth Sadhasivan [-- Attachment #1: Type: text/plain, Size: 2118 bytes --] On 13.09.2022 16:41:10, Rhett Aultman wrote: > From: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> > > DMA allocated buffers are a precious resource. If there is no need for DMA > allocations, then it might be worth to use non-dma allocated buffers. > After testing the gs_usb driver with and without DMA allocation, there > does not seem to be a significant change in latency or cpu utilization > either way. Therefore, DMA allocation is not necessary and removed. > > Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com> > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com> > Signed-off-by: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> > --- > drivers/net/can/usb/gs_usb.c | 38 ++++++++++-------------------------- > 1 file changed, 10 insertions(+), 28 deletions(-) > > diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c > index baf749c8cda3..1bfc775c62c5 100644 > --- a/drivers/net/can/usb/gs_usb.c > +++ b/drivers/net/can/usb/gs_usb.c > @@ -269,7 +269,6 @@ struct gs_can { > struct usb_anchor tx_submitted; > atomic_t active_tx_urbs; > void *rxbuf[GS_MAX_RX_URBS]; > - dma_addr_t rxbuf_dma[GS_MAX_RX_URBS]; > }; > > /* usb interface struct */ > @@ -587,9 +586,7 @@ static void gs_usb_xmit_callback(struct urb *urb) > > if (urb->status) > netdev_info(netdev, "usb xmit fail %u\n", txc->echo_id); > - > - usb_free_coherent(urb->dev, urb->transfer_buffer_length, > - urb->transfer_buffer, urb->transfer_dma); > + devm_kfree(&urb->dev->dev, urb->transfer_buffer); Consider using the URB_FREE_BUFFER flag: | https://elixir.bootlin.com/linux/v5.19/source/include/linux/usb.h#L1330 and standard kmalloc() (i.e. not devm_kmalloc()). The USB stack will take care of kfree()ing the buffer associated with each URB. regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 0/1] can: gs_usb: remove dma allocations 2022-09-13 20:41 [PATCH 0/1] can: gs_usb: remove dma allocations Rhett Aultman 2022-09-13 20:41 ` [PATCH 1/1] " Rhett Aultman @ 2022-09-20 15:47 ` Rhett Aultman 2022-09-20 15:47 ` [PATCH v2] " Rhett Aultman 2022-09-20 15:50 ` [PATCH 0/1] " Rhett Aultman 1 sibling, 2 replies; 8+ messages in thread From: Rhett Aultman @ 2022-09-20 15:47 UTC (permalink / raw) To: linux-can Hi all, Please find here an updated patch from Vasanth incorporating Marc's feedback. Vasanth performed his benchmarking tests again and found no change in the results, so the remarks in our PDF are still current and valid. If there aren't any concerns with how we're checking performance without DMA, then perhaps we should double-check this on an ARM platform and then proceed? Best, Rhett Aultman ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH v2] can: gs_usb: remove dma allocations 2022-09-20 15:47 ` [PATCH 0/1] " Rhett Aultman @ 2022-09-20 15:47 ` Rhett Aultman 2022-09-23 12:11 ` Marc Kleine-Budde 2022-09-20 15:50 ` [PATCH 0/1] " Rhett Aultman 1 sibling, 1 reply; 8+ messages in thread From: Rhett Aultman @ 2022-09-20 15:47 UTC (permalink / raw) To: linux-can; +Cc: Rhett Aultman, Vasanth Sadhasivan From: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> DMA allocated buffers are a precious resource. If there is no need for DMA allocations, then it might be worth to use non-dma allocated buffers. After testing the gs_usb driver with and without DMA allocation, there does not seem to be a significant change in latency or cpu utilization either way. Therefore, DMA allocation is not necessary and removed. Internal buffers used within urbs were managed and freed manually. These buffers are no longer needed to be managed by the driver. The URB_FREE_BUFFER flag, allows for the buffers in question to be automatically freed. Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com> Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com> Signed-off-by: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> --- drivers/net/can/usb/gs_usb.c | 39 ++++++------------------------------ 1 file changed, 6 insertions(+), 33 deletions(-) diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c index baf749c8cda3..8562553ea6d0 100644 --- a/drivers/net/can/usb/gs_usb.c +++ b/drivers/net/can/usb/gs_usb.c @@ -268,8 +268,6 @@ struct gs_can { struct usb_anchor tx_submitted; atomic_t active_tx_urbs; - void *rxbuf[GS_MAX_RX_URBS]; - dma_addr_t rxbuf_dma[GS_MAX_RX_URBS]; }; /* usb interface struct */ @@ -587,9 +585,6 @@ static void gs_usb_xmit_callback(struct urb *urb) if (urb->status) netdev_info(netdev, "usb xmit fail %u\n", txc->echo_id); - - usb_free_coherent(urb->dev, urb->transfer_buffer_length, - urb->transfer_buffer, urb->transfer_dma); } static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, @@ -618,8 +613,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, if (!urb) goto nomem_urb; - hf = usb_alloc_coherent(dev->udev, dev->hf_size_tx, GFP_ATOMIC, - &urb->transfer_dma); + hf = kmalloc(dev->hf_size_tx, GFP_ATOMIC); if (!hf) { netdev_err(netdev, "No memory left for USB buffer\n"); goto nomem_hf; @@ -663,7 +657,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, hf, dev->hf_size_tx, gs_usb_xmit_callback, txc); - urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; + urb->transfer_flags |= URB_FREE_BUFFER; usb_anchor_urb(urb, &dev->tx_submitted); can_put_echo_skb(skb, netdev, idx, 0); @@ -678,8 +672,6 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, gs_free_tx_context(txc); usb_unanchor_urb(urb); - usb_free_coherent(dev->udev, urb->transfer_buffer_length, - urb->transfer_buffer, urb->transfer_dma); if (rc == -ENODEV) { netif_device_detach(netdev); @@ -699,8 +691,7 @@ static netdev_tx_t gs_can_start_xmit(struct sk_buff *skb, return NETDEV_TX_OK; badidx: - usb_free_coherent(dev->udev, urb->transfer_buffer_length, - urb->transfer_buffer, urb->transfer_dma); + kfree(hf); nomem_hf: usb_free_urb(urb); @@ -744,7 +735,6 @@ static int gs_can_open(struct net_device *netdev) for (i = 0; i < GS_MAX_RX_URBS; i++) { struct urb *urb; u8 *buf; - dma_addr_t buf_dma; /* alloc rx urb */ urb = usb_alloc_urb(0, GFP_KERNEL); @@ -752,10 +742,8 @@ static int gs_can_open(struct net_device *netdev) return -ENOMEM; /* alloc rx buffer */ - buf = usb_alloc_coherent(dev->udev, - dev->parent->hf_size_rx, - GFP_KERNEL, - &buf_dma); + buf = kmalloc(dev->parent->hf_size_rx, + GFP_KERNEL); if (!buf) { netdev_err(netdev, "No memory left for USB buffer\n"); @@ -763,8 +751,6 @@ static int gs_can_open(struct net_device *netdev) return -ENOMEM; } - urb->transfer_dma = buf_dma; - /* fill, anchor, and submit rx urb */ usb_fill_bulk_urb(urb, dev->udev, @@ -773,7 +759,7 @@ static int gs_can_open(struct net_device *netdev) buf, dev->parent->hf_size_rx, gs_usb_receive_bulk_callback, parent); - urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; + urb->transfer_flags |= URB_FREE_BUFFER; usb_anchor_urb(urb, &parent->rx_submitted); @@ -786,17 +772,10 @@ static int gs_can_open(struct net_device *netdev) "usb_submit failed (err=%d)\n", rc); usb_unanchor_urb(urb); - usb_free_coherent(dev->udev, - sizeof(struct gs_host_frame), - buf, - buf_dma); usb_free_urb(urb); break; } - dev->rxbuf[i] = buf; - dev->rxbuf_dma[i] = buf_dma; - /* Drop reference, * USB core will take care of freeing it */ @@ -854,7 +833,6 @@ static int gs_can_close(struct net_device *netdev) int rc; struct gs_can *dev = netdev_priv(netdev); struct gs_usb *parent = dev->parent; - unsigned int i; netif_stop_queue(netdev); @@ -862,11 +840,6 @@ static int gs_can_close(struct net_device *netdev) parent->active_channels--; if (!parent->active_channels) { usb_kill_anchored_urbs(&parent->rx_submitted); - for (i = 0; i < GS_MAX_RX_URBS; i++) - usb_free_coherent(dev->udev, - sizeof(struct gs_host_frame), - dev->rxbuf[i], - dev->rxbuf_dma[i]); } /* Stop sending URBs */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH v2] can: gs_usb: remove dma allocations 2022-09-20 15:47 ` [PATCH v2] " Rhett Aultman @ 2022-09-23 12:11 ` Marc Kleine-Budde 2022-09-23 12:12 ` Marc Kleine-Budde 0 siblings, 1 reply; 8+ messages in thread From: Marc Kleine-Budde @ 2022-09-23 12:11 UTC (permalink / raw) To: Rhett Aultman; +Cc: linux-can, Vasanth Sadhasivan [-- Attachment #1: Type: text/plain, Size: 1229 bytes --] On 20.09.2022 11:47:24, Rhett Aultman wrote: > From: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> > > DMA allocated buffers are a precious resource. If there is no need for DMA > allocations, then it might be worth to use non-dma allocated buffers. > After testing the gs_usb driver with and without DMA allocation, there > does not seem to be a significant change in latency or cpu utilization > either way. Therefore, DMA allocation is not necessary and removed. > Internal buffers used within urbs were managed and freed manually. These > buffers are no longer needed to be managed by the driver. The > URB_FREE_BUFFER flag, allows for the buffers in question to > be automatically freed. > > Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com> > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com> > Signed-off-by: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> Applied to linux-can. Thanks, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH v2] can: gs_usb: remove dma allocations 2022-09-23 12:11 ` Marc Kleine-Budde @ 2022-09-23 12:12 ` Marc Kleine-Budde 0 siblings, 0 replies; 8+ messages in thread From: Marc Kleine-Budde @ 2022-09-23 12:12 UTC (permalink / raw) To: Rhett Aultman; +Cc: linux-can, Vasanth Sadhasivan [-- Attachment #1: Type: text/plain, Size: 1333 bytes --] On 23.09.2022 14:11:11, Marc Kleine-Budde wrote: > On 20.09.2022 11:47:24, Rhett Aultman wrote: > > From: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> > > > > DMA allocated buffers are a precious resource. If there is no need for DMA > > allocations, then it might be worth to use non-dma allocated buffers. > > After testing the gs_usb driver with and without DMA allocation, there > > does not seem to be a significant change in latency or cpu utilization > > either way. Therefore, DMA allocation is not necessary and removed. > > Internal buffers used within urbs were managed and freed manually. These > > buffers are no longer needed to be managed by the driver. The > > URB_FREE_BUFFER flag, allows for the buffers in question to > > be automatically freed. > > > > Co-developed-by: Rhett Aultman <rhett.aultman@samsara.com> > > Signed-off-by: Rhett Aultman <rhett.aultman@samsara.com> > > Signed-off-by: Vasanth Sadhasivan <vasanth.sadhasivan@samsara.com> > > Applied to linux-can. linux-can-next that is. Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 | [-- Attachment #2: signature.asc --] [-- Type: application/pgp-signature, Size: 488 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 0/1] can: gs_usb: remove dma allocations 2022-09-20 15:47 ` [PATCH 0/1] " Rhett Aultman 2022-09-20 15:47 ` [PATCH v2] " Rhett Aultman @ 2022-09-20 15:50 ` Rhett Aultman 1 sibling, 0 replies; 8+ messages in thread From: Rhett Aultman @ 2022-09-20 15:50 UTC (permalink / raw) To: linux-can Apologies for the mixed-up subject lines. That's my mistake. --Rhett On Tue, 20 Sep 2022, Rhett Aultman wrote: > Hi all, > > Please find here an updated patch from Vasanth incorporating Marc's feedback. > > Vasanth performed his benchmarking tests again and found no change in the > results, so the remarks in our PDF are still current and valid. > > If there aren't any concerns with how we're checking performance without DMA, > then perhaps we should double-check this on an ARM platform and then proceed? > > > Best, > Rhett Aultman > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2022-09-23 12:22 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-13 20:41 [PATCH 0/1] can: gs_usb: remove dma allocations Rhett Aultman 2022-09-13 20:41 ` [PATCH 1/1] " Rhett Aultman 2022-09-18 21:31 ` Marc Kleine-Budde 2022-09-20 15:47 ` [PATCH 0/1] " Rhett Aultman 2022-09-20 15:47 ` [PATCH v2] " Rhett Aultman 2022-09-23 12:11 ` Marc Kleine-Budde 2022-09-23 12:12 ` Marc Kleine-Budde 2022-09-20 15:50 ` [PATCH 0/1] " Rhett Aultman
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.