All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] forcedeth: recv cache support
@ 2019-07-05  6:19 Zhu Yanjun
  2019-07-05  6:19 ` [PATCH 1/2] forcedeth: add recv cache make nic work steadily Zhu Yanjun
                   ` (2 more replies)
  0 siblings, 3 replies; 8+ messages in thread
From: Zhu Yanjun @ 2019-07-05  6:19 UTC (permalink / raw)
  To: yanjun.zhu, netdev, davem

This recv cache is to make NIC work steadily when the system memory is
not enough.

From long time testing, the NIC worked very well when the system memory
is not enough. And the NIC performance is better from about 920M to
about 940M.

Some simple tests are made:

ip link set forcedeth_nic down/up
modprobe/rmmod forcedeth
ip link set mtu 1500 dev forcedeth_nic
ethtool -G forcedeth_nic tx 512 rx 1024
And other tests, the NIC with the recv cache can work well.

Since the recv cache will reserve 125M memory for NIC, normally this recv
cache is disabled by default.

Zhu Yanjun (2):
  forcedeth: add recv cache make nic work steadily
  forcedeth: disable recv cache by default

 drivers/net/ethernet/nvidia/Kconfig     |  11 +++
 drivers/net/ethernet/nvidia/Makefile    |   1 +
 drivers/net/ethernet/nvidia/forcedeth.c | 128 +++++++++++++++++++++++++++++++-
 3 files changed, 138 insertions(+), 2 deletions(-)

-- 
2.7.4


^ permalink raw reply	[flat|nested] 8+ messages in thread

* [PATCH 1/2] forcedeth: add recv cache make nic work steadily
  2019-07-05  6:19 [PATCH 0/2] forcedeth: recv cache support Zhu Yanjun
@ 2019-07-05  6:19 ` Zhu Yanjun
  2019-07-06  4:14   ` Zhu Yanjun
  2019-07-08 20:52   ` Jakub Kicinski
  2019-07-05  6:19 ` [PATCH 2/2] forcedeth: disable recv cache by default Zhu Yanjun
  2019-07-08 22:23 ` [PATCH 0/2] forcedeth: recv cache support David Miller
  2 siblings, 2 replies; 8+ messages in thread
From: Zhu Yanjun @ 2019-07-05  6:19 UTC (permalink / raw)
  To: yanjun.zhu, netdev, davem

A recv cache is added. The size of recv cache is 1000Mb / skb_length.
When the system memory is not enough, this recv cache can make nic work
steadily.
When nic is up, this recv cache and work queue are created. When nic
is down, this recv cache will be destroyed and delayed workqueue is
canceled.
When nic is polled or rx interrupt is triggerred, rx handler will
get a skb from recv cache. Then the state of recv cache is checked.
If recv cache is not in filling up state, a work is queued to fill
up recv cache.
When skb size is changed, the old recv cache is destroyed and new recv
cache is created.
When the system memory is not enough, the allocation of skb failed.
recv cache will continue allocate skb until the recv cache is filled up.
When the system memory is not enough, this can make nic work steadily.
Becase of recv cache, the performance of nic is enhanced.

CC: Joe Jin <joe.jin@oracle.com>
CC: Junxiao Bi <junxiao.bi@oracle.com>
Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
---
 drivers/net/ethernet/nvidia/forcedeth.c | 100 +++++++++++++++++++++++++++++++-
 1 file changed, 98 insertions(+), 2 deletions(-)

diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index b327b29..a673005 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -674,6 +674,11 @@ struct nv_ethtool_stats {
 	u64 tx_broadcast;
 };
 
+/* 1000Mb is 125M bytes, 125 * 1024 * 1024 bytes
+ * The length of recv cache is 125M / skb_length
+ */
+#define RECV_CACHE_LIST_LENGTH		(125 * 1024 * 1024 / np->rx_buf_sz)
+
 #define NV_DEV_STATISTICS_V3_COUNT (sizeof(struct nv_ethtool_stats)/sizeof(u64))
 #define NV_DEV_STATISTICS_V2_COUNT (NV_DEV_STATISTICS_V3_COUNT - 3)
 #define NV_DEV_STATISTICS_V1_COUNT (NV_DEV_STATISTICS_V2_COUNT - 6)
@@ -844,8 +849,18 @@ struct fe_priv {
 	char name_rx[IFNAMSIZ + 3];       /* -rx    */
 	char name_tx[IFNAMSIZ + 3];       /* -tx    */
 	char name_other[IFNAMSIZ + 6];    /* -other */
+
+	/* This is to schedule work */
+	struct delayed_work     recv_cache_work;
+	/* This list is to store skb queue for recv */
+	struct sk_buff_head recv_list;
+	unsigned long nv_recv_list_state;
 };
 
+/* This is recv list state to fill up recv cache */
+enum recv_list_state {
+	RECV_LIST_ALLOCATE
+};
 /*
  * Maximum number of loops until we assume that a bit in the irq mask
  * is stuck. Overridable with module param.
@@ -1804,7 +1819,11 @@ static int nv_alloc_rx(struct net_device *dev)
 		less_rx = np->last_rx.orig;
 
 	while (np->put_rx.orig != less_rx) {
-		struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
+		struct sk_buff *skb = skb_dequeue(&np->recv_list);
+
+		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
+			schedule_delayed_work(&np->recv_cache_work, 0);
+
 		if (likely(skb)) {
 			np->put_rx_ctx->skb = skb;
 			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
@@ -1845,7 +1864,11 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
 		less_rx = np->last_rx.ex;
 
 	while (np->put_rx.ex != less_rx) {
-		struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
+		struct sk_buff *skb = skb_dequeue(&np->recv_list);
+
+		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
+			schedule_delayed_work(&np->recv_cache_work, 0);
+
 		if (likely(skb)) {
 			np->put_rx_ctx->skb = skb;
 			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
@@ -1957,6 +1980,40 @@ static void nv_init_tx(struct net_device *dev)
 	}
 }
 
+static void nv_init_recv_cache(struct net_device *dev)
+{
+	struct fe_priv *np = netdev_priv(dev);
+
+	skb_queue_head_init(&np->recv_list);
+	while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
+		struct sk_buff *skb = netdev_alloc_skb(dev,
+				 np->rx_buf_sz + NV_RX_ALLOC_PAD);
+		/* skb is null. This indicates that memory is not
+		 * enough.
+		 */
+		if (unlikely(!skb)) {
+			ndelay(3);
+			continue;
+		}
+
+		skb_queue_tail(&np->recv_list, skb);
+	}
+}
+
+static void nv_destroy_recv_cache(struct net_device *dev)
+{
+	struct sk_buff *skb;
+	struct fe_priv *np = netdev_priv(dev);
+
+	cancel_delayed_work_sync(&np->recv_cache_work);
+	WARN_ON(delayed_work_pending(&np->recv_cache_work));
+
+	while ((skb = skb_dequeue(&np->recv_list)))
+		kfree_skb(skb);
+
+	WARN_ON(skb_queue_len(&np->recv_list));
+}
+
 static int nv_init_ring(struct net_device *dev)
 {
 	struct fe_priv *np = netdev_priv(dev);
@@ -3047,6 +3104,8 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
 		nv_drain_rxtx(dev);
 		/* reinit driver view of the rx queue */
 		set_bufsize(dev);
+		nv_destroy_recv_cache(dev);
+		nv_init_recv_cache(dev);
 		if (nv_init_ring(dev)) {
 			if (!np->in_shutdown)
 				mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
@@ -4074,6 +4133,32 @@ static void nv_free_irq(struct net_device *dev)
 	}
 }
 
+static void nv_recv_cache_worker(struct work_struct *work)
+{
+	struct fe_priv *np = container_of(work, struct fe_priv,
+					  recv_cache_work.work);
+
+	set_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
+	while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
+		struct sk_buff *skb = netdev_alloc_skb(np->dev,
+				np->rx_buf_sz + NV_RX_ALLOC_PAD);
+		/* skb is null. This indicates that memory is not
+		 * enough.
+		 * When the system memory is not enough, the kernel
+		 * will compact memory or drop caches. At that time,
+		 * if memory allocation fails, it had better wait some
+		 * time for memory.
+		 */
+		if (unlikely(!skb)) {
+			ndelay(3);
+			continue;
+		}
+
+		skb_queue_tail(&np->recv_list, skb);
+	}
+	clear_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
+}
+
 static void nv_do_nic_poll(struct timer_list *t)
 {
 	struct fe_priv *np = from_timer(np, t, nic_poll);
@@ -4129,6 +4214,8 @@ static void nv_do_nic_poll(struct timer_list *t)
 			nv_drain_rxtx(dev);
 			/* reinit driver view of the rx queue */
 			set_bufsize(dev);
+			nv_destroy_recv_cache(dev);
+			nv_init_recv_cache(dev);
 			if (nv_init_ring(dev)) {
 				if (!np->in_shutdown)
 					mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
@@ -4681,6 +4768,8 @@ static int nv_set_ringparam(struct net_device *dev, struct ethtool_ringparam* ri
 	if (netif_running(dev)) {
 		/* reinit driver view of the queues */
 		set_bufsize(dev);
+		nv_destroy_recv_cache(dev);
+		nv_init_recv_cache(dev);
 		if (nv_init_ring(dev)) {
 			if (!np->in_shutdown)
 				mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
@@ -5402,6 +5491,10 @@ static int nv_open(struct net_device *dev)
 
 	/* initialize descriptor rings */
 	set_bufsize(dev);
+	nv_init_recv_cache(dev);
+
+	INIT_DELAYED_WORK(&np->recv_cache_work, nv_recv_cache_worker);
+	clear_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
 	oom = nv_init_ring(dev);
 
 	writel(0, base + NvRegLinkSpeed);
@@ -5583,6 +5676,9 @@ static int nv_close(struct net_device *dev)
 		nv_txrx_gate(dev, true);
 	}
 
+	/* free all SKBs in recv cache */
+	nv_destroy_recv_cache(dev);
+
 	/* FIXME: power down nic */
 
 	return 0;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* [PATCH 2/2] forcedeth: disable recv cache by default
  2019-07-05  6:19 [PATCH 0/2] forcedeth: recv cache support Zhu Yanjun
  2019-07-05  6:19 ` [PATCH 1/2] forcedeth: add recv cache make nic work steadily Zhu Yanjun
@ 2019-07-05  6:19 ` Zhu Yanjun
  2019-07-08 22:23 ` [PATCH 0/2] forcedeth: recv cache support David Miller
  2 siblings, 0 replies; 8+ messages in thread
From: Zhu Yanjun @ 2019-07-05  6:19 UTC (permalink / raw)
  To: yanjun.zhu, netdev, davem

The recv cache is to allocate 125MiB memory to reserve for NIC.
In the past time, this recv cache works very well. When the memory
is not enough, this recv cache reserves memory for NIC.
And the communications through this NIC is not affected by the
memory shortage. And the performance of NIC is better because of
this recv cache.
But this recv cache reserves 125MiB memory for one NIC port. Normally
there are 2 NIC ports in one card. So in a host, there are about 250
MiB memory reserved for NIC ports. To a host on which communications
are not mandatory, it is not necessary to reserve memory.
So this recv cache is disabled by default.

CC: Joe Jin <joe.jin@oracle.com>
CC: Junxiao Bi <junxiao.bi@oracle.com>
Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
---
 drivers/net/ethernet/nvidia/Kconfig     | 11 +++++++++++
 drivers/net/ethernet/nvidia/Makefile    |  1 +
 drivers/net/ethernet/nvidia/forcedeth.c | 34 ++++++++++++++++++++++++++++++---
 3 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/nvidia/Kconfig b/drivers/net/ethernet/nvidia/Kconfig
index faacbd1..9a9f42a 100644
--- a/drivers/net/ethernet/nvidia/Kconfig
+++ b/drivers/net/ethernet/nvidia/Kconfig
@@ -26,4 +26,15 @@ config FORCEDETH
 	  To compile this driver as a module, choose M here. The module
 	  will be called forcedeth.
 
+config	FORCEDETH_RECV_CACHE
+	bool "nForce Ethernet recv cache support"
+	depends on FORCEDETH
+	default n
+	---help---
+	  The recv cache can make nic work steadily when the system memory is
+	  not enough. And it can also enhance nic performance. But to a host
+	  on which the communications are not mandatory, it is not necessary
+	  to reserve 125MiB memory for NIC.
+	  So recv cache is disabled by default.
+
 endif # NET_VENDOR_NVIDIA
diff --git a/drivers/net/ethernet/nvidia/Makefile b/drivers/net/ethernet/nvidia/Makefile
index 8935699..40c055e 100644
--- a/drivers/net/ethernet/nvidia/Makefile
+++ b/drivers/net/ethernet/nvidia/Makefile
@@ -4,3 +4,4 @@
 #
 
 obj-$(CONFIG_FORCEDETH) += forcedeth.o
+ccflags-$(CONFIG_FORCEDETH_RECV_CACHE)	:=	-DFORCEDETH_RECV_CACHE
diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
index a673005..59f813b 100644
--- a/drivers/net/ethernet/nvidia/forcedeth.c
+++ b/drivers/net/ethernet/nvidia/forcedeth.c
@@ -674,10 +674,12 @@ struct nv_ethtool_stats {
 	u64 tx_broadcast;
 };
 
+#ifdef FORCEDETH_RECV_CACHE
 /* 1000Mb is 125M bytes, 125 * 1024 * 1024 bytes
  * The length of recv cache is 125M / skb_length
  */
 #define RECV_CACHE_LIST_LENGTH		(125 * 1024 * 1024 / np->rx_buf_sz)
+#endif
 
 #define NV_DEV_STATISTICS_V3_COUNT (sizeof(struct nv_ethtool_stats)/sizeof(u64))
 #define NV_DEV_STATISTICS_V2_COUNT (NV_DEV_STATISTICS_V3_COUNT - 3)
@@ -850,17 +852,22 @@ struct fe_priv {
 	char name_tx[IFNAMSIZ + 3];       /* -tx    */
 	char name_other[IFNAMSIZ + 6];    /* -other */
 
+#ifdef FORCEDETH_RECV_CACHE
 	/* This is to schedule work */
 	struct delayed_work     recv_cache_work;
 	/* This list is to store skb queue for recv */
 	struct sk_buff_head recv_list;
 	unsigned long nv_recv_list_state;
+#endif
 };
 
+#ifdef FORCEDETH_RECV_CACHE
 /* This is recv list state to fill up recv cache */
 enum recv_list_state {
 	RECV_LIST_ALLOCATE
 };
+#endif
+
 /*
  * Maximum number of loops until we assume that a bit in the irq mask
  * is stuck. Overridable with module param.
@@ -1819,11 +1826,15 @@ static int nv_alloc_rx(struct net_device *dev)
 		less_rx = np->last_rx.orig;
 
 	while (np->put_rx.orig != less_rx) {
+#ifdef FORCEDETH_RECV_CACHE
 		struct sk_buff *skb = skb_dequeue(&np->recv_list);
 
 		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
 			schedule_delayed_work(&np->recv_cache_work, 0);
-
+#else
+		struct sk_buff *skb = netdev_alloc_skb(np->dev,
+					 np->rx_buf_sz + NV_RX_ALLOC_PAD);
+#endif
 		if (likely(skb)) {
 			np->put_rx_ctx->skb = skb;
 			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
@@ -1864,11 +1875,15 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
 		less_rx = np->last_rx.ex;
 
 	while (np->put_rx.ex != less_rx) {
+#ifdef FORCEDETH_RECV_CACHE
 		struct sk_buff *skb = skb_dequeue(&np->recv_list);
 
 		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
 			schedule_delayed_work(&np->recv_cache_work, 0);
-
+#else
+		struct sk_buff *skb = netdev_alloc_skb(np->dev,
+					np->rx_buf_sz + NV_RX_ALLOC_PAD);
+#endif
 		if (likely(skb)) {
 			np->put_rx_ctx->skb = skb;
 			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
@@ -1980,6 +1995,7 @@ static void nv_init_tx(struct net_device *dev)
 	}
 }
 
+#ifdef FORCEDETH_RECV_CACHE
 static void nv_init_recv_cache(struct net_device *dev)
 {
 	struct fe_priv *np = netdev_priv(dev);
@@ -2013,6 +2029,7 @@ static void nv_destroy_recv_cache(struct net_device *dev)
 
 	WARN_ON(skb_queue_len(&np->recv_list));
 }
+#endif
 
 static int nv_init_ring(struct net_device *dev)
 {
@@ -3104,8 +3121,10 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
 		nv_drain_rxtx(dev);
 		/* reinit driver view of the rx queue */
 		set_bufsize(dev);
+#ifdef FORCEDETH_RECV_CACHE
 		nv_destroy_recv_cache(dev);
 		nv_init_recv_cache(dev);
+#endif
 		if (nv_init_ring(dev)) {
 			if (!np->in_shutdown)
 				mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
@@ -4133,6 +4152,7 @@ static void nv_free_irq(struct net_device *dev)
 	}
 }
 
+#ifdef FORCEDETH_RECV_CACHE
 static void nv_recv_cache_worker(struct work_struct *work)
 {
 	struct fe_priv *np = container_of(work, struct fe_priv,
@@ -4158,6 +4178,7 @@ static void nv_recv_cache_worker(struct work_struct *work)
 	}
 	clear_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
 }
+#endif
 
 static void nv_do_nic_poll(struct timer_list *t)
 {
@@ -4214,8 +4235,10 @@ static void nv_do_nic_poll(struct timer_list *t)
 			nv_drain_rxtx(dev);
 			/* reinit driver view of the rx queue */
 			set_bufsize(dev);
+#ifdef FORCEDETH_RECV_CACHE
 			nv_destroy_recv_cache(dev);
 			nv_init_recv_cache(dev);
+#endif
 			if (nv_init_ring(dev)) {
 				if (!np->in_shutdown)
 					mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
@@ -4768,8 +4791,10 @@ static int nv_set_ringparam(struct net_device *dev, struct ethtool_ringparam* ri
 	if (netif_running(dev)) {
 		/* reinit driver view of the queues */
 		set_bufsize(dev);
+#ifdef FORCEDETH_RECV_CACHE
 		nv_destroy_recv_cache(dev);
 		nv_init_recv_cache(dev);
+#endif
 		if (nv_init_ring(dev)) {
 			if (!np->in_shutdown)
 				mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
@@ -5491,10 +5516,12 @@ static int nv_open(struct net_device *dev)
 
 	/* initialize descriptor rings */
 	set_bufsize(dev);
+#ifdef FORCEDETH_RECV_CACHE
 	nv_init_recv_cache(dev);
 
 	INIT_DELAYED_WORK(&np->recv_cache_work, nv_recv_cache_worker);
 	clear_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
+#endif
 	oom = nv_init_ring(dev);
 
 	writel(0, base + NvRegLinkSpeed);
@@ -5676,9 +5703,10 @@ static int nv_close(struct net_device *dev)
 		nv_txrx_gate(dev, true);
 	}
 
+#ifdef FORCEDETH_RECV_CACHE
 	/* free all SKBs in recv cache */
 	nv_destroy_recv_cache(dev);
-
+#endif
 	/* FIXME: power down nic */
 
 	return 0;
-- 
2.7.4


^ permalink raw reply related	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] forcedeth: add recv cache make nic work steadily
  2019-07-05  6:19 ` [PATCH 1/2] forcedeth: add recv cache make nic work steadily Zhu Yanjun
@ 2019-07-06  4:14   ` Zhu Yanjun
  2019-07-08 20:52   ` Jakub Kicinski
  1 sibling, 0 replies; 8+ messages in thread
From: Zhu Yanjun @ 2019-07-06  4:14 UTC (permalink / raw)
  To: netdev, davem, nan.1986san

Add Nan

He is interested this commit.

在 2019/7/5 14:19, Zhu Yanjun 写道:
> A recv cache is added. The size of recv cache is 1000Mb / skb_length.
> When the system memory is not enough, this recv cache can make nic work
> steadily.
> When nic is up, this recv cache and work queue are created. When nic
> is down, this recv cache will be destroyed and delayed workqueue is
> canceled.
> When nic is polled or rx interrupt is triggerred, rx handler will
> get a skb from recv cache. Then the state of recv cache is checked.
> If recv cache is not in filling up state, a work is queued to fill
> up recv cache.
> When skb size is changed, the old recv cache is destroyed and new recv
> cache is created.
> When the system memory is not enough, the allocation of skb failed.
> recv cache will continue allocate skb until the recv cache is filled up.
> When the system memory is not enough, this can make nic work steadily.
> Becase of recv cache, the performance of nic is enhanced.
>
> CC: Joe Jin <joe.jin@oracle.com>
> CC: Junxiao Bi <junxiao.bi@oracle.com>
> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> ---
>   drivers/net/ethernet/nvidia/forcedeth.c | 100 +++++++++++++++++++++++++++++++-
>   1 file changed, 98 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index b327b29..a673005 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -674,6 +674,11 @@ struct nv_ethtool_stats {
>   	u64 tx_broadcast;
>   };
>   
> +/* 1000Mb is 125M bytes, 125 * 1024 * 1024 bytes
> + * The length of recv cache is 125M / skb_length
> + */
> +#define RECV_CACHE_LIST_LENGTH		(125 * 1024 * 1024 / np->rx_buf_sz)
> +
>   #define NV_DEV_STATISTICS_V3_COUNT (sizeof(struct nv_ethtool_stats)/sizeof(u64))
>   #define NV_DEV_STATISTICS_V2_COUNT (NV_DEV_STATISTICS_V3_COUNT - 3)
>   #define NV_DEV_STATISTICS_V1_COUNT (NV_DEV_STATISTICS_V2_COUNT - 6)
> @@ -844,8 +849,18 @@ struct fe_priv {
>   	char name_rx[IFNAMSIZ + 3];       /* -rx    */
>   	char name_tx[IFNAMSIZ + 3];       /* -tx    */
>   	char name_other[IFNAMSIZ + 6];    /* -other */
> +
> +	/* This is to schedule work */
> +	struct delayed_work     recv_cache_work;
> +	/* This list is to store skb queue for recv */
> +	struct sk_buff_head recv_list;
> +	unsigned long nv_recv_list_state;
>   };
>   
> +/* This is recv list state to fill up recv cache */
> +enum recv_list_state {
> +	RECV_LIST_ALLOCATE
> +};
>   /*
>    * Maximum number of loops until we assume that a bit in the irq mask
>    * is stuck. Overridable with module param.
> @@ -1804,7 +1819,11 @@ static int nv_alloc_rx(struct net_device *dev)
>   		less_rx = np->last_rx.orig;
>   
>   	while (np->put_rx.orig != less_rx) {
> -		struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		struct sk_buff *skb = skb_dequeue(&np->recv_list);
> +
> +		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
> +			schedule_delayed_work(&np->recv_cache_work, 0);
> +
>   		if (likely(skb)) {
>   			np->put_rx_ctx->skb = skb;
>   			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
> @@ -1845,7 +1864,11 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
>   		less_rx = np->last_rx.ex;
>   
>   	while (np->put_rx.ex != less_rx) {
> -		struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		struct sk_buff *skb = skb_dequeue(&np->recv_list);
> +
> +		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
> +			schedule_delayed_work(&np->recv_cache_work, 0);
> +
>   		if (likely(skb)) {
>   			np->put_rx_ctx->skb = skb;
>   			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
> @@ -1957,6 +1980,40 @@ static void nv_init_tx(struct net_device *dev)
>   	}
>   }
>   
> +static void nv_init_recv_cache(struct net_device *dev)
> +{
> +	struct fe_priv *np = netdev_priv(dev);
> +
> +	skb_queue_head_init(&np->recv_list);
> +	while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
> +		struct sk_buff *skb = netdev_alloc_skb(dev,
> +				 np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		/* skb is null. This indicates that memory is not
> +		 * enough.
> +		 */
> +		if (unlikely(!skb)) {
> +			ndelay(3);
> +			continue;
> +		}
> +
> +		skb_queue_tail(&np->recv_list, skb);
> +	}
> +}
> +
> +static void nv_destroy_recv_cache(struct net_device *dev)
> +{
> +	struct sk_buff *skb;
> +	struct fe_priv *np = netdev_priv(dev);
> +
> +	cancel_delayed_work_sync(&np->recv_cache_work);
> +	WARN_ON(delayed_work_pending(&np->recv_cache_work));
> +
> +	while ((skb = skb_dequeue(&np->recv_list)))
> +		kfree_skb(skb);
> +
> +	WARN_ON(skb_queue_len(&np->recv_list));
> +}
> +
>   static int nv_init_ring(struct net_device *dev)
>   {
>   	struct fe_priv *np = netdev_priv(dev);
> @@ -3047,6 +3104,8 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
>   		nv_drain_rxtx(dev);
>   		/* reinit driver view of the rx queue */
>   		set_bufsize(dev);
> +		nv_destroy_recv_cache(dev);
> +		nv_init_recv_cache(dev);
>   		if (nv_init_ring(dev)) {
>   			if (!np->in_shutdown)
>   				mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
> @@ -4074,6 +4133,32 @@ static void nv_free_irq(struct net_device *dev)
>   	}
>   }
>   
> +static void nv_recv_cache_worker(struct work_struct *work)
> +{
> +	struct fe_priv *np = container_of(work, struct fe_priv,
> +					  recv_cache_work.work);
> +
> +	set_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
> +	while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
> +		struct sk_buff *skb = netdev_alloc_skb(np->dev,
> +				np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		/* skb is null. This indicates that memory is not
> +		 * enough.
> +		 * When the system memory is not enough, the kernel
> +		 * will compact memory or drop caches. At that time,
> +		 * if memory allocation fails, it had better wait some
> +		 * time for memory.
> +		 */
> +		if (unlikely(!skb)) {
> +			ndelay(3);
> +			continue;
> +		}
> +
> +		skb_queue_tail(&np->recv_list, skb);
> +	}
> +	clear_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
> +}
> +
>   static void nv_do_nic_poll(struct timer_list *t)
>   {
>   	struct fe_priv *np = from_timer(np, t, nic_poll);
> @@ -4129,6 +4214,8 @@ static void nv_do_nic_poll(struct timer_list *t)
>   			nv_drain_rxtx(dev);
>   			/* reinit driver view of the rx queue */
>   			set_bufsize(dev);
> +			nv_destroy_recv_cache(dev);
> +			nv_init_recv_cache(dev);
>   			if (nv_init_ring(dev)) {
>   				if (!np->in_shutdown)
>   					mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
> @@ -4681,6 +4768,8 @@ static int nv_set_ringparam(struct net_device *dev, struct ethtool_ringparam* ri
>   	if (netif_running(dev)) {
>   		/* reinit driver view of the queues */
>   		set_bufsize(dev);
> +		nv_destroy_recv_cache(dev);
> +		nv_init_recv_cache(dev);
>   		if (nv_init_ring(dev)) {
>   			if (!np->in_shutdown)
>   				mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
> @@ -5402,6 +5491,10 @@ static int nv_open(struct net_device *dev)
>   
>   	/* initialize descriptor rings */
>   	set_bufsize(dev);
> +	nv_init_recv_cache(dev);
> +
> +	INIT_DELAYED_WORK(&np->recv_cache_work, nv_recv_cache_worker);
> +	clear_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
>   	oom = nv_init_ring(dev);
>   
>   	writel(0, base + NvRegLinkSpeed);
> @@ -5583,6 +5676,9 @@ static int nv_close(struct net_device *dev)
>   		nv_txrx_gate(dev, true);
>   	}
>   
> +	/* free all SKBs in recv cache */
> +	nv_destroy_recv_cache(dev);
> +
>   	/* FIXME: power down nic */
>   
>   	return 0;

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] forcedeth: add recv cache make nic work steadily
  2019-07-05  6:19 ` [PATCH 1/2] forcedeth: add recv cache make nic work steadily Zhu Yanjun
  2019-07-06  4:14   ` Zhu Yanjun
@ 2019-07-08 20:52   ` Jakub Kicinski
  2019-07-09  7:38     ` Yanjun Zhu
  1 sibling, 1 reply; 8+ messages in thread
From: Jakub Kicinski @ 2019-07-08 20:52 UTC (permalink / raw)
  To: Zhu Yanjun; +Cc: netdev, davem

On Fri,  5 Jul 2019 02:19:27 -0400, Zhu Yanjun wrote:
> A recv cache is added. The size of recv cache is 1000Mb / skb_length.
> When the system memory is not enough, this recv cache can make nic work
> steadily.
> When nic is up, this recv cache and work queue are created. When nic
> is down, this recv cache will be destroyed and delayed workqueue is
> canceled.
> When nic is polled or rx interrupt is triggerred, rx handler will
> get a skb from recv cache. Then the state of recv cache is checked.
> If recv cache is not in filling up state, a work is queued to fill
> up recv cache.
> When skb size is changed, the old recv cache is destroyed and new recv
> cache is created.
> When the system memory is not enough, the allocation of skb failed.
> recv cache will continue allocate skb until the recv cache is filled up.
> When the system memory is not enough, this can make nic work steadily.
> Becase of recv cache, the performance of nic is enhanced.
> 
> CC: Joe Jin <joe.jin@oracle.com>
> CC: Junxiao Bi <junxiao.bi@oracle.com>
> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>

Could you tell us a little more about the use case and the system
condition?

> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
> index b327b29..a673005 100644
> --- a/drivers/net/ethernet/nvidia/forcedeth.c
> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
> @@ -674,6 +674,11 @@ struct nv_ethtool_stats {
>  	u64 tx_broadcast;
>  };
>  
> +/* 1000Mb is 125M bytes, 125 * 1024 * 1024 bytes
> + * The length of recv cache is 125M / skb_length
> + */
> +#define RECV_CACHE_LIST_LENGTH		(125 * 1024 * 1024 / np->rx_buf_sz)
> +
>  #define NV_DEV_STATISTICS_V3_COUNT (sizeof(struct nv_ethtool_stats)/sizeof(u64))
>  #define NV_DEV_STATISTICS_V2_COUNT (NV_DEV_STATISTICS_V3_COUNT - 3)
>  #define NV_DEV_STATISTICS_V1_COUNT (NV_DEV_STATISTICS_V2_COUNT - 6)
> @@ -844,8 +849,18 @@ struct fe_priv {
>  	char name_rx[IFNAMSIZ + 3];       /* -rx    */
>  	char name_tx[IFNAMSIZ + 3];       /* -tx    */
>  	char name_other[IFNAMSIZ + 6];    /* -other */
> +
> +	/* This is to schedule work */
> +	struct delayed_work     recv_cache_work;
> +	/* This list is to store skb queue for recv */
> +	struct sk_buff_head recv_list;
> +	unsigned long nv_recv_list_state;
>  };
>  
> +/* This is recv list state to fill up recv cache */
> +enum recv_list_state {
> +	RECV_LIST_ALLOCATE
> +};
>  /*
>   * Maximum number of loops until we assume that a bit in the irq mask
>   * is stuck. Overridable with module param.
> @@ -1804,7 +1819,11 @@ static int nv_alloc_rx(struct net_device *dev)
>  		less_rx = np->last_rx.orig;
>  
>  	while (np->put_rx.orig != less_rx) {
> -		struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		struct sk_buff *skb = skb_dequeue(&np->recv_list);
> +
> +		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
> +			schedule_delayed_work(&np->recv_cache_work, 0);

Interesting, this seems to be coming up in multiple places recently..

Could you explain why you have your own RECV_LIST_ALLOCATE bit here?
Workqueue implementation itself uses an atomic bit to avoid scheduling
work mutliple times:

bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
			   struct delayed_work *dwork, unsigned long delay)
{
	struct work_struct *work = &dwork->work;
	bool ret = false;
	unsigned long flags;

	/* read the comment in __queue_work() */
	local_irq_save(flags);

	if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
		__queue_delayed_work(cpu, wq, dwork, delay);
		ret = true;
	}

	local_irq_restore(flags);
	return ret;
}
EXPORT_SYMBOL(queue_delayed_work_on);

>  		if (likely(skb)) {
>  			np->put_rx_ctx->skb = skb;
>  			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
> @@ -1845,7 +1864,11 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
>  		less_rx = np->last_rx.ex;
>  
>  	while (np->put_rx.ex != less_rx) {
> -		struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		struct sk_buff *skb = skb_dequeue(&np->recv_list);
> +
> +		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
> +			schedule_delayed_work(&np->recv_cache_work, 0);

It seems a little heavy to schedule this work on every packet, would it
make sense to add this in nv_napi_poll() instead?

>  		if (likely(skb)) {
>  			np->put_rx_ctx->skb = skb;
>  			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
> @@ -1957,6 +1980,40 @@ static void nv_init_tx(struct net_device *dev)
>  	}
>  }
>  
> +static void nv_init_recv_cache(struct net_device *dev)
> +{
> +	struct fe_priv *np = netdev_priv(dev);
> +
> +	skb_queue_head_init(&np->recv_list);
> +	while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
> +		struct sk_buff *skb = netdev_alloc_skb(dev,
> +				 np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		/* skb is null. This indicates that memory is not
> +		 * enough.
> +		 */
> +		if (unlikely(!skb)) {
> +			ndelay(3);
> +			continue;

Does this path ever hit?  Seems like doing an ndelay() and retrying
allocation is not the best idea for non-preempt kernel.  

Also perhaps you should consider using __netdev_alloc_skb() and passing
GFP_KERNEL, that way the system has a chance to go into memory reclaim
(I presume this function can sleep).

> +		}
> +
> +		skb_queue_tail(&np->recv_list, skb);
> +	}
> +}
> +
> +static void nv_destroy_recv_cache(struct net_device *dev)
> +{
> +	struct sk_buff *skb;
> +	struct fe_priv *np = netdev_priv(dev);
> +
> +	cancel_delayed_work_sync(&np->recv_cache_work);
> +	WARN_ON(delayed_work_pending(&np->recv_cache_work));
> +
> +	while ((skb = skb_dequeue(&np->recv_list)))
> +		kfree_skb(skb);

skb_queue_purge()

> +	WARN_ON(skb_queue_len(&np->recv_list));
> +}
> +
>  static int nv_init_ring(struct net_device *dev)
>  {
>  	struct fe_priv *np = netdev_priv(dev);
> @@ -3047,6 +3104,8 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
>  		nv_drain_rxtx(dev);
>  		/* reinit driver view of the rx queue */
>  		set_bufsize(dev);
> +		nv_destroy_recv_cache(dev);
> +		nv_init_recv_cache(dev);
>  		if (nv_init_ring(dev)) {
>  			if (!np->in_shutdown)
>  				mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
> @@ -4074,6 +4133,32 @@ static void nv_free_irq(struct net_device *dev)
>  	}
>  }
>  
> +static void nv_recv_cache_worker(struct work_struct *work)
> +{
> +	struct fe_priv *np = container_of(work, struct fe_priv,
> +					  recv_cache_work.work);
> +
> +	set_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
> +	while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
> +		struct sk_buff *skb = netdev_alloc_skb(np->dev,
> +				np->rx_buf_sz + NV_RX_ALLOC_PAD);
> +		/* skb is null. This indicates that memory is not
> +		 * enough.
> +		 * When the system memory is not enough, the kernel
> +		 * will compact memory or drop caches. At that time,
> +		 * if memory allocation fails, it had better wait some
> +		 * time for memory.
> +		 */
> +		if (unlikely(!skb)) {
> +			ndelay(3);
> +			continue;

Same comments as for the init function.

> +		}
> +
> +		skb_queue_tail(&np->recv_list, skb);
> +	}
> +	clear_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
> +}
> +
>  static void nv_do_nic_poll(struct timer_list *t)
>  {
>  	struct fe_priv *np = from_timer(np, t, nic_poll);
> @@ -4129,6 +4214,8 @@ static void nv_do_nic_poll(struct timer_list *t)
>  			nv_drain_rxtx(dev);
>  			/* reinit driver view of the rx queue */
>  			set_bufsize(dev);
> +			nv_destroy_recv_cache(dev);
> +			nv_init_recv_cache(dev);
>  			if (nv_init_ring(dev)) {
>  				if (!np->in_shutdown)
>  					mod_timer(&np->oom_kick, jiffies + OOM_REFILL);


^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] forcedeth: recv cache support
  2019-07-05  6:19 [PATCH 0/2] forcedeth: recv cache support Zhu Yanjun
  2019-07-05  6:19 ` [PATCH 1/2] forcedeth: add recv cache make nic work steadily Zhu Yanjun
  2019-07-05  6:19 ` [PATCH 2/2] forcedeth: disable recv cache by default Zhu Yanjun
@ 2019-07-08 22:23 ` David Miller
  2019-07-09  8:28   ` Yanjun Zhu
  2 siblings, 1 reply; 8+ messages in thread
From: David Miller @ 2019-07-08 22:23 UTC (permalink / raw)
  To: yanjun.zhu; +Cc: netdev

From: Zhu Yanjun <yanjun.zhu@oracle.com>
Date: Fri,  5 Jul 2019 02:19:26 -0400

> This recv cache is to make NIC work steadily when the system memory is
> not enough.

The system is supposed to hold onto enough atomic memory to absorb all
reasonable situations like this.

If anything a solution to this problem belongs generically somewhere,
not in a driver.  And furthermore looping over an allocation attempt
with a delay is strongly discouraged.

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 1/2] forcedeth: add recv cache make nic work steadily
  2019-07-08 20:52   ` Jakub Kicinski
@ 2019-07-09  7:38     ` Yanjun Zhu
  0 siblings, 0 replies; 8+ messages in thread
From: Yanjun Zhu @ 2019-07-09  7:38 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, davem


On 2019/7/9 4:52, Jakub Kicinski wrote:
> On Fri,  5 Jul 2019 02:19:27 -0400, Zhu Yanjun wrote:
>> A recv cache is added. The size of recv cache is 1000Mb / skb_length.
>> When the system memory is not enough, this recv cache can make nic work
>> steadily.
>> When nic is up, this recv cache and work queue are created. When nic
>> is down, this recv cache will be destroyed and delayed workqueue is
>> canceled.
>> When nic is polled or rx interrupt is triggerred, rx handler will
>> get a skb from recv cache. Then the state of recv cache is checked.
>> If recv cache is not in filling up state, a work is queued to fill
>> up recv cache.
>> When skb size is changed, the old recv cache is destroyed and new recv
>> cache is created.
>> When the system memory is not enough, the allocation of skb failed.
>> recv cache will continue allocate skb until the recv cache is filled up.
>> When the system memory is not enough, this can make nic work steadily.
>> Becase of recv cache, the performance of nic is enhanced.
>>
>> CC: Joe Jin <joe.jin@oracle.com>
>> CC: Junxiao Bi <junxiao.bi@oracle.com>
>> Signed-off-by: Zhu Yanjun <yanjun.zhu@oracle.com>
> Could you tell us a little more about the use case and the system
> condition?

When the host run for long time, there are a lot of memory fragments in 
hosts.

In this condition, this patch can help us a lot.

>
>> diff --git a/drivers/net/ethernet/nvidia/forcedeth.c b/drivers/net/ethernet/nvidia/forcedeth.c
>> index b327b29..a673005 100644
>> --- a/drivers/net/ethernet/nvidia/forcedeth.c
>> +++ b/drivers/net/ethernet/nvidia/forcedeth.c
>> @@ -674,6 +674,11 @@ struct nv_ethtool_stats {
>>   	u64 tx_broadcast;
>>   };
>>   
>> +/* 1000Mb is 125M bytes, 125 * 1024 * 1024 bytes
>> + * The length of recv cache is 125M / skb_length
>> + */
>> +#define RECV_CACHE_LIST_LENGTH		(125 * 1024 * 1024 / np->rx_buf_sz)
>> +
>>   #define NV_DEV_STATISTICS_V3_COUNT (sizeof(struct nv_ethtool_stats)/sizeof(u64))
>>   #define NV_DEV_STATISTICS_V2_COUNT (NV_DEV_STATISTICS_V3_COUNT - 3)
>>   #define NV_DEV_STATISTICS_V1_COUNT (NV_DEV_STATISTICS_V2_COUNT - 6)
>> @@ -844,8 +849,18 @@ struct fe_priv {
>>   	char name_rx[IFNAMSIZ + 3];       /* -rx    */
>>   	char name_tx[IFNAMSIZ + 3];       /* -tx    */
>>   	char name_other[IFNAMSIZ + 6];    /* -other */
>> +
>> +	/* This is to schedule work */
>> +	struct delayed_work     recv_cache_work;
>> +	/* This list is to store skb queue for recv */
>> +	struct sk_buff_head recv_list;
>> +	unsigned long nv_recv_list_state;
>>   };
>>   
>> +/* This is recv list state to fill up recv cache */
>> +enum recv_list_state {
>> +	RECV_LIST_ALLOCATE
>> +};
>>   /*
>>    * Maximum number of loops until we assume that a bit in the irq mask
>>    * is stuck. Overridable with module param.
>> @@ -1804,7 +1819,11 @@ static int nv_alloc_rx(struct net_device *dev)
>>   		less_rx = np->last_rx.orig;
>>   
>>   	while (np->put_rx.orig != less_rx) {
>> -		struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
>> +		struct sk_buff *skb = skb_dequeue(&np->recv_list);
>> +
>> +		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
>> +			schedule_delayed_work(&np->recv_cache_work, 0);
> Interesting, this seems to be coming up in multiple places recently..
>
> Could you explain why you have your own RECV_LIST_ALLOCATE bit here?
> Workqueue implementation itself uses an atomic bit to avoid scheduling
> work mutliple times:

This can avoid function call overhead to optimize the recv process.

Thanks

Zhu Yanjun

>
> bool queue_delayed_work_on(int cpu, struct workqueue_struct *wq,
> 			   struct delayed_work *dwork, unsigned long delay)
> {
> 	struct work_struct *work = &dwork->work;
> 	bool ret = false;
> 	unsigned long flags;
>
> 	/* read the comment in __queue_work() */
> 	local_irq_save(flags);
>
> 	if (!test_and_set_bit(WORK_STRUCT_PENDING_BIT, work_data_bits(work))) {
> 		__queue_delayed_work(cpu, wq, dwork, delay);
> 		ret = true;
> 	}
>
> 	local_irq_restore(flags);
> 	return ret;
> }
> EXPORT_SYMBOL(queue_delayed_work_on);
>
>>   		if (likely(skb)) {
>>   			np->put_rx_ctx->skb = skb;
>>   			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
>> @@ -1845,7 +1864,11 @@ static int nv_alloc_rx_optimized(struct net_device *dev)
>>   		less_rx = np->last_rx.ex;
>>   
>>   	while (np->put_rx.ex != less_rx) {
>> -		struct sk_buff *skb = netdev_alloc_skb(dev, np->rx_buf_sz + NV_RX_ALLOC_PAD);
>> +		struct sk_buff *skb = skb_dequeue(&np->recv_list);
>> +
>> +		if (!test_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state))
>> +			schedule_delayed_work(&np->recv_cache_work, 0);
> It seems a little heavy to schedule this work on every packet, would it
> make sense to add this in nv_napi_poll() instead?
>
>>   		if (likely(skb)) {
>>   			np->put_rx_ctx->skb = skb;
>>   			np->put_rx_ctx->dma = dma_map_single(&np->pci_dev->dev,
>> @@ -1957,6 +1980,40 @@ static void nv_init_tx(struct net_device *dev)
>>   	}
>>   }
>>   
>> +static void nv_init_recv_cache(struct net_device *dev)
>> +{
>> +	struct fe_priv *np = netdev_priv(dev);
>> +
>> +	skb_queue_head_init(&np->recv_list);
>> +	while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
>> +		struct sk_buff *skb = netdev_alloc_skb(dev,
>> +				 np->rx_buf_sz + NV_RX_ALLOC_PAD);
>> +		/* skb is null. This indicates that memory is not
>> +		 * enough.
>> +		 */
>> +		if (unlikely(!skb)) {
>> +			ndelay(3);
>> +			continue;
> Does this path ever hit?  Seems like doing an ndelay() and retrying
> allocation is not the best idea for non-preempt kernel.
>
> Also perhaps you should consider using __netdev_alloc_skb() and passing
> GFP_KERNEL, that way the system has a chance to go into memory reclaim
> (I presume this function can sleep).
>
>> +		}
>> +
>> +		skb_queue_tail(&np->recv_list, skb);
>> +	}
>> +}
>> +
>> +static void nv_destroy_recv_cache(struct net_device *dev)
>> +{
>> +	struct sk_buff *skb;
>> +	struct fe_priv *np = netdev_priv(dev);
>> +
>> +	cancel_delayed_work_sync(&np->recv_cache_work);
>> +	WARN_ON(delayed_work_pending(&np->recv_cache_work));
>> +
>> +	while ((skb = skb_dequeue(&np->recv_list)))
>> +		kfree_skb(skb);
> skb_queue_purge()
>
>> +	WARN_ON(skb_queue_len(&np->recv_list));
>> +}
>> +
>>   static int nv_init_ring(struct net_device *dev)
>>   {
>>   	struct fe_priv *np = netdev_priv(dev);
>> @@ -3047,6 +3104,8 @@ static int nv_change_mtu(struct net_device *dev, int new_mtu)
>>   		nv_drain_rxtx(dev);
>>   		/* reinit driver view of the rx queue */
>>   		set_bufsize(dev);
>> +		nv_destroy_recv_cache(dev);
>> +		nv_init_recv_cache(dev);
>>   		if (nv_init_ring(dev)) {
>>   			if (!np->in_shutdown)
>>   				mod_timer(&np->oom_kick, jiffies + OOM_REFILL);
>> @@ -4074,6 +4133,32 @@ static void nv_free_irq(struct net_device *dev)
>>   	}
>>   }
>>   
>> +static void nv_recv_cache_worker(struct work_struct *work)
>> +{
>> +	struct fe_priv *np = container_of(work, struct fe_priv,
>> +					  recv_cache_work.work);
>> +
>> +	set_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
>> +	while (skb_queue_len(&np->recv_list) < RECV_CACHE_LIST_LENGTH) {
>> +		struct sk_buff *skb = netdev_alloc_skb(np->dev,
>> +				np->rx_buf_sz + NV_RX_ALLOC_PAD);
>> +		/* skb is null. This indicates that memory is not
>> +		 * enough.
>> +		 * When the system memory is not enough, the kernel
>> +		 * will compact memory or drop caches. At that time,
>> +		 * if memory allocation fails, it had better wait some
>> +		 * time for memory.
>> +		 */
>> +		if (unlikely(!skb)) {
>> +			ndelay(3);
>> +			continue;
> Same comments as for the init function.
>
>> +		}
>> +
>> +		skb_queue_tail(&np->recv_list, skb);
>> +	}
>> +	clear_bit(RECV_LIST_ALLOCATE, &np->nv_recv_list_state);
>> +}
>> +
>>   static void nv_do_nic_poll(struct timer_list *t)
>>   {
>>   	struct fe_priv *np = from_timer(np, t, nic_poll);
>> @@ -4129,6 +4214,8 @@ static void nv_do_nic_poll(struct timer_list *t)
>>   			nv_drain_rxtx(dev);
>>   			/* reinit driver view of the rx queue */
>>   			set_bufsize(dev);
>> +			nv_destroy_recv_cache(dev);
>> +			nv_init_recv_cache(dev);
>>   			if (nv_init_ring(dev)) {
>>   				if (!np->in_shutdown)
>>   					mod_timer(&np->oom_kick, jiffies + OOM_REFILL);

^ permalink raw reply	[flat|nested] 8+ messages in thread

* Re: [PATCH 0/2] forcedeth: recv cache support
  2019-07-08 22:23 ` [PATCH 0/2] forcedeth: recv cache support David Miller
@ 2019-07-09  8:28   ` Yanjun Zhu
  0 siblings, 0 replies; 8+ messages in thread
From: Yanjun Zhu @ 2019-07-09  8:28 UTC (permalink / raw)
  To: David Miller; +Cc: netdev


On 2019/7/9 6:23, David Miller wrote:
> From: Zhu Yanjun <yanjun.zhu@oracle.com>
> Date: Fri,  5 Jul 2019 02:19:26 -0400
>
>> This recv cache is to make NIC work steadily when the system memory is
>> not enough.
> The system is supposed to hold onto enough atomic memory to absorb all
> reasonable situations like this.
>
> If anything a solution to this problem belongs generically somewhere,
> not in a driver.  And furthermore looping over an allocation attempt
> with a delay is strongly discouraged.

Thanks a lot for your suggestions. Now a user is testing this patch in 
LAB and production hosts.

After this patch can pass tests, I will send V2 based on your suggestions.

Thanks,

Zhu Yanjun


^ permalink raw reply	[flat|nested] 8+ messages in thread

end of thread, other threads:[~2019-07-09  8:27 UTC | newest]

Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-05  6:19 [PATCH 0/2] forcedeth: recv cache support Zhu Yanjun
2019-07-05  6:19 ` [PATCH 1/2] forcedeth: add recv cache make nic work steadily Zhu Yanjun
2019-07-06  4:14   ` Zhu Yanjun
2019-07-08 20:52   ` Jakub Kicinski
2019-07-09  7:38     ` Yanjun Zhu
2019-07-05  6:19 ` [PATCH 2/2] forcedeth: disable recv cache by default Zhu Yanjun
2019-07-08 22:23 ` [PATCH 0/2] forcedeth: recv cache support David Miller
2019-07-09  8:28   ` Yanjun Zhu

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.