diff mbox

[(net-next.git),16/18] stmmac: do not perform zero-copy for rx frames

Message ID 1449650274-14896-17-git-send-email-peppe.cavallaro@st.com
State Changes Requested, archived
Delegated to: David Miller
Headers show

Commit Message

Giuseppe CAVALLARO Dec. 9, 2015, 8:37 a.m. UTC 
This patch is to allow this driver to copy tiny frames during the reception
process. This is giving more stability while stressing the driver on STi
embedded systems.

Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c |   67 ++++++++++++++++-----
 1 files changed, 52 insertions(+), 15 deletions(-)

Comments

David Miller Dec. 12, 2015, 1:09 a.m. UTC | #1 
From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Date: Wed, 9 Dec 2015 09:37:52 +0100

> @@ -98,6 +98,10 @@ static int buf_sz = DEFAULT_BUFSIZE;
>  module_param(buf_sz, int, S_IRUGO | S_IWUSR);
>  MODULE_PARM_DESC(buf_sz, "DMA buffer size");
>  
> +static int minrx = 256;
> +module_param(minrx, int, S_IRUGO | S_IWUSR);
> +MODULE_PARM_DESC(minrx, "Copy only tiny-frames");
> +

When you type module_param() in a network driver, you ought to receive
an electric shock in order to remind you that except in the most extreme
cases module parameters are absolutely not appropriate.

In this case we have an ethtool tunable people can use to control copy
break values like this, so use that instead.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Giuseppe CAVALLARO Dec. 16, 2015, 11:33 a.m. UTC | #2 
On 12/12/2015 2:09 AM, David Miller wrote:
> From: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Date: Wed, 9 Dec 2015 09:37:52 +0100
>
>> @@ -98,6 +98,10 @@ static int buf_sz = DEFAULT_BUFSIZE;
>>   module_param(buf_sz, int, S_IRUGO | S_IWUSR);
>>   MODULE_PARM_DESC(buf_sz, "DMA buffer size");
>>
>> +static int minrx = 256;
>> +module_param(minrx, int, S_IRUGO | S_IWUSR);
>> +MODULE_PARM_DESC(minrx, "Copy only tiny-frames");
>> +
>
> When you type module_param() in a network driver, you ought to receive
> an electric shock in order to remind you that except in the most extreme
> cases module parameters are absolutely not appropriate.
>
> In this case we have an ethtool tunable people can use to control copy
> break values like this, so use that instead.

ok, v2 will have this change. Let me know if there is other to fix.

Peppe

--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 6b83213..d68775a 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -98,6 +98,10 @@  static int buf_sz = DEFAULT_BUFSIZE;
 module_param(buf_sz, int, S_IRUGO | S_IWUSR);
 MODULE_PARM_DESC(buf_sz, "DMA buffer size");
 
+static int minrx = 256;
+module_param(minrx, int, S_IRUGO | S_IWUSR);
+MODULE_PARM_DESC(minrx, "Copy only tiny-frames");
+
 static const u32 default_msg_level = (NETIF_MSG_DRV | NETIF_MSG_PROBE |
 				      NETIF_MSG_LINK | NETIF_MSG_IFUP |
 				      NETIF_MSG_IFDOWN | NETIF_MSG_TIMER);
@@ -147,6 +151,8 @@  static void stmmac_verify_args(void)
 		pause = PAUSE_TIME;
 	if (eee_timer < 0)
 		eee_timer = STMMAC_DEFAULT_LPI_TIMER;
+	if (minrx < 0)
+		minrx = ETH_FRAME_LEN;
 }
 
 /**
@@ -2198,8 +2204,7 @@  static inline void stmmac_rx_refill(struct stmmac_priv *priv)
 			struct sk_buff *skb;
 
 			skb = netdev_alloc_skb_ip_align(priv->dev, bfsize);
-
-			if (unlikely(skb == NULL))
+			if (unlikely(!skb))
 				break;
 
 			priv->rx_skbuff[entry] = skb;
@@ -2322,23 +2327,52 @@  static int stmmac_rx(struct stmmac_priv *priv, int limit)
 					pr_debug("\tframe size %d, COE: %d\n",
 						 frame_len, status);
 			}
-			skb = priv->rx_skbuff[entry];
-			if (unlikely(!skb)) {
-				pr_err("%s: Inconsistent Rx descriptor chain\n",
-				       priv->dev->name);
-				priv->dev->stats.rx_dropped++;
-				break;
+
+			if (unlikely(frame_len < minrx)) {
+				skb = netdev_alloc_skb_ip_align(priv->dev,
+								frame_len);
+				if (unlikely(!skb)) {
+					if (net_ratelimit())
+						dev_warn(priv->device,
+							 "packet dropped\n");
+					priv->dev->stats.rx_dropped++;
+					break;
+				}
+
+				dma_sync_single_for_cpu(priv->device,
+							priv->rx_skbuff_dma
+							[entry], frame_len,
+							DMA_FROM_DEVICE);
+				skb_copy_to_linear_data(skb,
+							priv->
+							rx_skbuff[entry]->data,
+							frame_len);
+
+				skb_put(skb, frame_len);
+				dma_sync_single_for_device(priv->device,
+							   priv->rx_skbuff_dma
+							   [entry], frame_len,
+							   DMA_FROM_DEVICE);
+			} else {
+				skb = priv->rx_skbuff[entry];
+				if (unlikely(!skb)) {
+					pr_err("%s: Inconsistent Rx chain\n",
+					       priv->dev->name);
+					priv->dev->stats.rx_dropped++;
+					break;
+				}
+				prefetch(skb->data - NET_IP_ALIGN);
+				priv->rx_skbuff[entry] = NULL;
+
+				skb_put(skb, frame_len);
+				dma_unmap_single(priv->device,
+						 priv->rx_skbuff_dma[entry],
+						 priv->dma_buf_sz,
+						 DMA_FROM_DEVICE);
 			}
-			prefetch(skb->data - NET_IP_ALIGN);
-			priv->rx_skbuff[entry] = NULL;
 
 			stmmac_get_rx_hwtstamp(priv, entry, skb);
 
-			skb_put(skb, frame_len);
-			dma_unmap_single(priv->device,
-					 priv->rx_skbuff_dma[entry],
-					 priv->dma_buf_sz, DMA_FROM_DEVICE);
-
 			if (netif_msg_pktdata(priv)) {
 				pr_debug("frame received (%dbytes)", frame_len);
 				print_pkt(skb->data, frame_len);
@@ -3235,6 +3269,9 @@  static int __init stmmac_cmdline_opt(char *str)
 		} else if (!strncmp(opt, "chain_mode:", 11)) {
 			if (kstrtoint(opt + 11, 0, &chain_mode))
 				goto err;
+		} else if (!strncmp(opt, "minrx:", 6)) {
+			if (kstrtoint(opt + 6, 0, &minrx))
+				goto err;
 		}
 	}
 	return 0;