[PATCH 0/1] can: gs_usb: remove dma allocations

[PATCH 0/1] can: gs_usb: remove dma allocations

[Rhett Aultman]

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

[PATCH 1/1] can: gs_usb: remove dma allocations

[Rhett Aultman]

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

Re: [PATCH 1/1] can: gs_usb: remove dma allocations

[Marc Kleine-Budde]

[-- 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 --]

[PATCH 0/1] can: gs_usb: remove dma allocations

[Rhett Aultman]

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

[PATCH v2] can: gs_usb: remove dma allocations

[Rhett Aultman]

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

Re: [PATCH 0/1] can: gs_usb: remove dma allocations

[Rhett Aultman]

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
>

Re: [PATCH v2] can: gs_usb: remove dma allocations

[Marc Kleine-Budde]

[-- 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 --]

Re: [PATCH v2] can: gs_usb: remove dma allocations

[Marc Kleine-Budde]

[-- 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 --]