linux-usb.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH net-next 0/5] RX improve
@ 2019-08-06 11:17 Hayes Wang
  2019-08-06 11:18 ` [PATCH net-next 1/5] r8152: separate the rx buffer size Hayes Wang
                   ` (4 more replies)
  0 siblings, 5 replies; 22+ messages in thread
From: Hayes Wang @ 2019-08-06 11:17 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang

The different chips use different rx buffer size.

Use skb_add_rx_frag() to reduce memory copy for RX.

Hayes Wang (5):
  r8152: separate the rx buffer size
  r8152: replace array with linking list for rx information
  r8152: use alloc_pages for rx buffer
  r8152: support skb_add_rx_frag
  r8152: change rx_frag_head_sz and rx_max_agg_num dynamically

 drivers/net/usb/r8152.c | 415 +++++++++++++++++++++++++++++++++-------
 1 file changed, 346 insertions(+), 69 deletions(-)

-- 
2.21.0


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

* [PATCH net-next 1/5] r8152: separate the rx buffer size
  2019-08-06 11:17 [PATCH net-next 0/5] RX improve Hayes Wang
@ 2019-08-06 11:18 ` Hayes Wang
  2019-08-06 11:18 ` [PATCH net-next 2/5] r8152: replace array with linking list for rx information Hayes Wang
                   ` (3 subsequent siblings)
  4 siblings, 0 replies; 22+ messages in thread
From: Hayes Wang @ 2019-08-06 11:18 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang

The different chips may accept different rx buffer sizes. The RTL8152
supports 16K bytes, and RTL8153 support 32K bytes.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 39e0768d734d..0f07ed05ab34 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -749,6 +749,7 @@ struct r8152 {
 	u32 msg_enable;
 	u32 tx_qlen;
 	u32 coalesce;
+	u32 rx_buf_sz;
 	u16 ocp_base;
 	u16 speed;
 	u8 *intr_buff;
@@ -1516,13 +1517,13 @@ static int alloc_all_mem(struct r8152 *tp)
 	skb_queue_head_init(&tp->rx_queue);
 
 	for (i = 0; i < RTL8152_MAX_RX; i++) {
-		buf = kmalloc_node(agg_buf_sz, GFP_KERNEL, node);
+		buf = kmalloc_node(tp->rx_buf_sz, GFP_KERNEL, node);
 		if (!buf)
 			goto err1;
 
 		if (buf != rx_agg_align(buf)) {
 			kfree(buf);
-			buf = kmalloc_node(agg_buf_sz + RX_ALIGN, GFP_KERNEL,
+			buf = kmalloc_node(tp->rx_buf_sz + RX_ALIGN, GFP_KERNEL,
 					   node);
 			if (!buf)
 				goto err1;
@@ -2113,7 +2114,7 @@ int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
 		return 0;
 
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
-			  agg->head, agg_buf_sz,
+			  agg->head, tp->rx_buf_sz,
 			  (usb_complete_t)read_bulk_callback, agg);
 
 	ret = usb_submit_urb(agg->urb, mem_flags);
@@ -2447,7 +2448,7 @@ static void r8153_set_rx_early_timeout(struct r8152 *tp)
 
 static void r8153_set_rx_early_size(struct r8152 *tp)
 {
-	u32 ocp_data = agg_buf_sz - rx_reserved_size(tp->netdev->mtu);
+	u32 ocp_data = tp->rx_buf_sz - rx_reserved_size(tp->netdev->mtu);
 
 	switch (tp->version) {
 	case RTL_VER_03:
@@ -5115,6 +5116,7 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->in_nway		= rtl8152_in_nway;
 		ops->hw_phy_cfg		= r8152b_hw_phy_cfg;
 		ops->autosuspend_en	= rtl_runtime_suspend_enable;
+		tp->rx_buf_sz		= 16 * 1024;
 		break;
 
 	case RTL_VER_03:
@@ -5132,6 +5134,7 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->in_nway		= rtl8153_in_nway;
 		ops->hw_phy_cfg		= r8153_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8153_runtime_enable;
+		tp->rx_buf_sz		= 32 * 1024;
 		break;
 
 	case RTL_VER_08:
@@ -5147,6 +5150,7 @@ static int rtl_ops_init(struct r8152 *tp)
 		ops->in_nway		= rtl8153_in_nway;
 		ops->hw_phy_cfg		= r8153b_hw_phy_cfg;
 		ops->autosuspend_en	= rtl8153b_runtime_enable;
+		tp->rx_buf_sz		= 32 * 1024;
 		break;
 
 	default:
-- 
2.21.0


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

* [PATCH net-next 2/5] r8152: replace array with linking list for rx information
  2019-08-06 11:17 [PATCH net-next 0/5] RX improve Hayes Wang
  2019-08-06 11:18 ` [PATCH net-next 1/5] r8152: separate the rx buffer size Hayes Wang
@ 2019-08-06 11:18 ` Hayes Wang
  2019-08-06 19:53   ` Jakub Kicinski
  2019-08-06 11:18 ` [PATCH net-next 3/5] r8152: use alloc_pages for rx buffer Hayes Wang
                   ` (2 subsequent siblings)
  4 siblings, 1 reply; 22+ messages in thread
From: Hayes Wang @ 2019-08-06 11:18 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang

The original method uses an array to store the rx information. The
new one uses a list to link each rx structure. Then, it is possible
to increase/decrease the number of rx structure dynamically.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 187 ++++++++++++++++++++++++++++------------
 1 file changed, 132 insertions(+), 55 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 0f07ed05ab34..44d832ceb516 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -22,6 +22,7 @@
 #include <linux/mdio.h>
 #include <linux/usb/cdc.h>
 #include <linux/suspend.h>
+#include <linux/atomic.h>
 #include <linux/acpi.h>
 
 /* Information for net-next */
@@ -694,7 +695,7 @@ struct tx_desc {
 struct r8152;
 
 struct rx_agg {
-	struct list_head list;
+	struct list_head list, info_list;
 	struct urb *urb;
 	struct r8152 *context;
 	void *buffer;
@@ -719,7 +720,7 @@ struct r8152 {
 	struct net_device *netdev;
 	struct urb *intr_urb;
 	struct tx_agg tx_info[RTL8152_MAX_TX];
-	struct rx_agg rx_info[RTL8152_MAX_RX];
+	struct list_head rx_info;
 	struct list_head rx_done, tx_free;
 	struct sk_buff_head tx_queue, rx_queue;
 	spinlock_t rx_lock, tx_lock;
@@ -744,6 +745,8 @@ struct r8152 {
 		void (*autosuspend_en)(struct r8152 *tp, bool enable);
 	} rtl_ops;
 
+	atomic_t rx_count;
+
 	int intr_interval;
 	u32 saved_wolopts;
 	u32 msg_enable;
@@ -1468,19 +1471,86 @@ static inline void *tx_agg_align(void *data)
 	return (void *)ALIGN((uintptr_t)data, TX_ALIGN);
 }
 
+static void free_rx_agg(struct r8152 *tp, struct rx_agg *agg)
+{
+	list_del(&agg->info_list);
+
+	usb_free_urb(agg->urb);
+	kfree(agg->buffer);
+	kfree(agg);
+
+	atomic_dec(&tp->rx_count);
+}
+
+static struct rx_agg *alloc_rx_agg(struct r8152 *tp, gfp_t mflags)
+{
+	struct net_device *netdev = tp->netdev;
+	int node = netdev->dev.parent ? dev_to_node(netdev->dev.parent) : -1;
+	struct rx_agg *rx_agg;
+
+	rx_agg = kmalloc_node(sizeof(*rx_agg), mflags, node);
+	if (rx_agg) {
+		unsigned long flags;
+		u8 *buf;
+
+		buf = kmalloc_node(tp->rx_buf_sz, mflags, node);
+		if (!buf)
+			goto free_rx;
+
+		if (buf != rx_agg_align(buf)) {
+			kfree(buf);
+			buf = kmalloc_node(tp->rx_buf_sz + RX_ALIGN, mflags,
+					   node);
+			if (!buf)
+				goto free_rx;
+		}
+
+		rx_agg->buffer = buf;
+		rx_agg->head = rx_agg_align(buf);
+
+		rx_agg->urb = usb_alloc_urb(0, mflags);
+		if (!rx_agg->urb)
+			goto free_buf;
+
+		rx_agg->context = tp;
+
+		INIT_LIST_HEAD(&rx_agg->list);
+		INIT_LIST_HEAD(&rx_agg->info_list);
+		spin_lock_irqsave(&tp->rx_lock, flags);
+		list_add_tail(&rx_agg->info_list, &tp->rx_info);
+		spin_unlock_irqrestore(&tp->rx_lock, flags);
+
+		atomic_inc(&tp->rx_count);
+	}
+
+	return rx_agg;
+
+free_buf:
+	kfree(rx_agg->buffer);
+free_rx:
+	kfree(rx_agg);
+	return NULL;
+}
+
 static void free_all_mem(struct r8152 *tp)
 {
+	struct list_head *cursor, *next;
+	unsigned long flags;
 	int i;
 
-	for (i = 0; i < RTL8152_MAX_RX; i++) {
-		usb_free_urb(tp->rx_info[i].urb);
-		tp->rx_info[i].urb = NULL;
+	spin_lock_irqsave(&tp->rx_lock, flags);
 
-		kfree(tp->rx_info[i].buffer);
-		tp->rx_info[i].buffer = NULL;
-		tp->rx_info[i].head = NULL;
+	list_for_each_safe(cursor, next, &tp->rx_info) {
+		struct rx_agg *agg;
+
+		agg = list_entry(cursor, struct rx_agg, info_list);
+		free_rx_agg(tp, agg);
 	}
 
+	spin_unlock_irqrestore(&tp->rx_lock, flags);
+
+	WARN_ON(unlikely(atomic_read(&tp->rx_count)));
+
 	for (i = 0; i < RTL8152_MAX_TX; i++) {
 		usb_free_urb(tp->tx_info[i].urb);
 		tp->tx_info[i].urb = NULL;
@@ -1503,46 +1573,28 @@ static int alloc_all_mem(struct r8152 *tp)
 	struct usb_interface *intf = tp->intf;
 	struct usb_host_interface *alt = intf->cur_altsetting;
 	struct usb_host_endpoint *ep_intr = alt->endpoint + 2;
-	struct urb *urb;
 	int node, i;
-	u8 *buf;
 
 	node = netdev->dev.parent ? dev_to_node(netdev->dev.parent) : -1;
 
 	spin_lock_init(&tp->rx_lock);
 	spin_lock_init(&tp->tx_lock);
+	INIT_LIST_HEAD(&tp->rx_info);
 	INIT_LIST_HEAD(&tp->tx_free);
 	INIT_LIST_HEAD(&tp->rx_done);
 	skb_queue_head_init(&tp->tx_queue);
 	skb_queue_head_init(&tp->rx_queue);
+	atomic_set(&tp->rx_count, 0);
 
 	for (i = 0; i < RTL8152_MAX_RX; i++) {
-		buf = kmalloc_node(tp->rx_buf_sz, GFP_KERNEL, node);
-		if (!buf)
-			goto err1;
-
-		if (buf != rx_agg_align(buf)) {
-			kfree(buf);
-			buf = kmalloc_node(tp->rx_buf_sz + RX_ALIGN, GFP_KERNEL,
-					   node);
-			if (!buf)
-				goto err1;
-		}
-
-		urb = usb_alloc_urb(0, GFP_KERNEL);
-		if (!urb) {
-			kfree(buf);
+		if (!alloc_rx_agg(tp, GFP_KERNEL))
 			goto err1;
-		}
-
-		INIT_LIST_HEAD(&tp->rx_info[i].list);
-		tp->rx_info[i].context = tp;
-		tp->rx_info[i].urb = urb;
-		tp->rx_info[i].buffer = buf;
-		tp->rx_info[i].head = rx_agg_align(buf);
 	}
 
 	for (i = 0; i < RTL8152_MAX_TX; i++) {
+		struct urb *urb;
+		u8 *buf;
+
 		buf = kmalloc_node(agg_buf_sz, GFP_KERNEL, node);
 		if (!buf)
 			goto err1;
@@ -2331,44 +2383,69 @@ static void rxdy_gated_en(struct r8152 *tp, bool enable)
 
 static int rtl_start_rx(struct r8152 *tp)
 {
-	int i, ret = 0;
+	struct list_head *cursor, *next, tmp_list;
+	unsigned long flags;
+	int ret = 0;
+
+	INIT_LIST_HEAD(&tmp_list);
+
+	spin_lock_irqsave(&tp->rx_lock, flags);
 
 	INIT_LIST_HEAD(&tp->rx_done);
-	for (i = 0; i < RTL8152_MAX_RX; i++) {
-		INIT_LIST_HEAD(&tp->rx_info[i].list);
-		ret = r8152_submit_rx(tp, &tp->rx_info[i], GFP_KERNEL);
-		if (ret)
-			break;
-	}
 
-	if (ret && ++i < RTL8152_MAX_RX) {
-		struct list_head rx_queue;
-		unsigned long flags;
+	list_splice_init(&tp->rx_info, &tmp_list);
 
-		INIT_LIST_HEAD(&rx_queue);
+	spin_unlock_irqrestore(&tp->rx_lock, flags);
 
-		do {
-			struct rx_agg *agg = &tp->rx_info[i++];
-			struct urb *urb = agg->urb;
+	list_for_each_safe(cursor, next, &tmp_list) {
+		struct rx_agg *agg;
 
-			urb->actual_length = 0;
-			list_add_tail(&agg->list, &rx_queue);
-		} while (i < RTL8152_MAX_RX);
+		agg = list_entry(cursor, struct rx_agg, info_list);
+		INIT_LIST_HEAD(&agg->list);
 
-		spin_lock_irqsave(&tp->rx_lock, flags);
-		list_splice_tail(&rx_queue, &tp->rx_done);
-		spin_unlock_irqrestore(&tp->rx_lock, flags);
+		if (ret < 0)
+			list_add_tail(&agg->list, &tp->rx_done);
+		else
+			ret = r8152_submit_rx(tp, agg, GFP_KERNEL);
 	}
 
+	spin_lock_irqsave(&tp->rx_lock, flags);
+	WARN_ON(unlikely(!list_empty(&tp->rx_info)));
+	list_splice(&tmp_list, &tp->rx_info);
+	spin_unlock_irqrestore(&tp->rx_lock, flags);
+
 	return ret;
 }
 
 static int rtl_stop_rx(struct r8152 *tp)
 {
-	int i;
+	struct list_head *cursor, *next, tmp_list;
+	unsigned long flags;
+
+	INIT_LIST_HEAD(&tmp_list);
 
-	for (i = 0; i < RTL8152_MAX_RX; i++)
-		usb_kill_urb(tp->rx_info[i].urb);
+	/* The usb_kill_urb() couldn't be used in atomic.
+	 * Therefore, move the list of rx_info to a tmp one.
+	 * Then, list_for_each_safe could be used without
+	 * spin lock.
+	 */
+
+	spin_lock_irqsave(&tp->rx_lock, flags);
+	list_splice_init(&tp->rx_info, &tmp_list);
+	spin_unlock_irqrestore(&tp->rx_lock, flags);
+
+	list_for_each_safe(cursor, next, &tmp_list) {
+		struct rx_agg *agg;
+
+		agg = list_entry(cursor, struct rx_agg, info_list);
+		usb_kill_urb(agg->urb);
+	}
+
+	/* Move back the list of temp to the rx_info */
+	spin_lock_irqsave(&tp->rx_lock, flags);
+	WARN_ON(unlikely(!list_empty(&tp->rx_info)));
+	list_splice(&tmp_list, &tp->rx_info);
+	spin_unlock_irqrestore(&tp->rx_lock, flags);
 
 	while (!skb_queue_empty(&tp->rx_queue))
 		dev_kfree_skb(__skb_dequeue(&tp->rx_queue));
-- 
2.21.0


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

* [PATCH net-next 3/5] r8152: use alloc_pages for rx buffer
  2019-08-06 11:17 [PATCH net-next 0/5] RX improve Hayes Wang
  2019-08-06 11:18 ` [PATCH net-next 1/5] r8152: separate the rx buffer size Hayes Wang
  2019-08-06 11:18 ` [PATCH net-next 2/5] r8152: replace array with linking list for rx information Hayes Wang
@ 2019-08-06 11:18 ` Hayes Wang
  2019-08-06 11:18 ` [PATCH net-next 4/5] r8152: support skb_add_rx_frag Hayes Wang
  2019-08-06 11:18 ` [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically Hayes Wang
  4 siblings, 0 replies; 22+ messages in thread
From: Hayes Wang @ 2019-08-06 11:18 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang

Replace kmalloc_node() with alloc_pages() for rx buffer.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 31 +++++++++++--------------------
 1 file changed, 11 insertions(+), 20 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 44d832ceb516..401e56112365 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -698,8 +698,8 @@ struct rx_agg {
 	struct list_head list, info_list;
 	struct urb *urb;
 	struct r8152 *context;
+	struct page *page;
 	void *buffer;
-	void *head;
 };
 
 struct tx_agg {
@@ -1476,7 +1476,7 @@ static void free_rx_agg(struct r8152 *tp, struct rx_agg *agg)
 	list_del(&agg->info_list);
 
 	usb_free_urb(agg->urb);
-	kfree(agg->buffer);
+	__free_pages(agg->page, get_order(tp->rx_buf_sz));
 	kfree(agg);
 
 	atomic_dec(&tp->rx_count);
@@ -1487,26 +1487,17 @@ static struct rx_agg *alloc_rx_agg(struct r8152 *tp, gfp_t mflags)
 	struct net_device *netdev = tp->netdev;
 	int node = netdev->dev.parent ? dev_to_node(netdev->dev.parent) : -1;
 	struct rx_agg *rx_agg;
+	unsigned int order = get_order(tp->rx_buf_sz);
 
 	rx_agg = kmalloc_node(sizeof(*rx_agg), mflags, node);
 	if (rx_agg) {
 		unsigned long flags;
-		u8 *buf;
 
-		buf = kmalloc_node(tp->rx_buf_sz, mflags, node);
-		if (!buf)
+		rx_agg->page = alloc_pages(mflags, order);
+		if (!rx_agg->page)
 			goto free_rx;
 
-		if (buf != rx_agg_align(buf)) {
-			kfree(buf);
-			buf = kmalloc_node(tp->rx_buf_sz + RX_ALIGN, mflags,
-					   node);
-			if (!buf)
-				goto free_rx;
-		}
-
-		rx_agg->buffer = buf;
-		rx_agg->head = rx_agg_align(buf);
+		rx_agg->buffer = page_address(rx_agg->page);
 
 		rx_agg->urb = usb_alloc_urb(0, mflags);
 		if (!rx_agg->urb)
@@ -1526,7 +1517,7 @@ static struct rx_agg *alloc_rx_agg(struct r8152 *tp, gfp_t mflags)
 	return rx_agg;
 
 free_buf:
-	kfree(rx_agg->buffer);
+	__free_pages(rx_agg->page, order);
 free_rx:
 	kfree(rx_agg);
 	return NULL;
@@ -2007,8 +1998,8 @@ static int rx_bottom(struct r8152 *tp, int budget)
 		if (urb->actual_length < ETH_ZLEN)
 			goto submit;
 
-		rx_desc = agg->head;
-		rx_data = agg->head;
+		rx_desc = agg->buffer;
+		rx_data = agg->buffer;
 		len_used += sizeof(struct rx_desc);
 
 		while (urb->actual_length > len_used) {
@@ -2055,7 +2046,7 @@ static int rx_bottom(struct r8152 *tp, int budget)
 find_next_rx:
 			rx_data = rx_agg_align(rx_data + pkt_len + ETH_FCS_LEN);
 			rx_desc = (struct rx_desc *)rx_data;
-			len_used = (int)(rx_data - (u8 *)agg->head);
+			len_used = (int)(rx_data - (u8 *)agg->buffer);
 			len_used += sizeof(struct rx_desc);
 		}
 
@@ -2166,7 +2157,7 @@ int r8152_submit_rx(struct r8152 *tp, struct rx_agg *agg, gfp_t mem_flags)
 		return 0;
 
 	usb_fill_bulk_urb(agg->urb, tp->udev, usb_rcvbulkpipe(tp->udev, 1),
-			  agg->head, tp->rx_buf_sz,
+			  agg->buffer, tp->rx_buf_sz,
 			  (usb_complete_t)read_bulk_callback, agg);
 
 	ret = usb_submit_urb(agg->urb, mem_flags);
-- 
2.21.0


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

* [PATCH net-next 4/5] r8152: support skb_add_rx_frag
  2019-08-06 11:17 [PATCH net-next 0/5] RX improve Hayes Wang
                   ` (2 preceding siblings ...)
  2019-08-06 11:18 ` [PATCH net-next 3/5] r8152: use alloc_pages for rx buffer Hayes Wang
@ 2019-08-06 11:18 ` Hayes Wang
  2019-08-06 22:08   ` Jakub Kicinski
  2019-08-06 11:18 ` [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically Hayes Wang
  4 siblings, 1 reply; 22+ messages in thread
From: Hayes Wang @ 2019-08-06 11:18 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang

Use skb_add_rx_frag() to reduce the memory copy for rx data.

Use a new list of rx_used to store the rx buffer which couldn't be
reused yet.

Besides, the total number of rx buffer may be increased or decreased
dynamically. And it is limited by RTL8152_MAX_RX_AGG.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 122 +++++++++++++++++++++++++++++++++++-----
 1 file changed, 108 insertions(+), 14 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 401e56112365..1615900c8592 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -584,6 +584,9 @@ enum rtl_register_content {
 #define TX_ALIGN		4
 #define RX_ALIGN		8
 
+#define RTL8152_MAX_RX_AGG	(10 * RTL8152_MAX_RX)
+#define RTL8152_RXFG_HEADSZ	256
+
 #define INTR_LINK		0x0004
 
 #define RTL8152_REQT_READ	0xc0
@@ -720,7 +723,7 @@ struct r8152 {
 	struct net_device *netdev;
 	struct urb *intr_urb;
 	struct tx_agg tx_info[RTL8152_MAX_TX];
-	struct list_head rx_info;
+	struct list_head rx_info, rx_used;
 	struct list_head rx_done, tx_free;
 	struct sk_buff_head tx_queue, rx_queue;
 	spinlock_t rx_lock, tx_lock;
@@ -1476,7 +1479,7 @@ static void free_rx_agg(struct r8152 *tp, struct rx_agg *agg)
 	list_del(&agg->info_list);
 
 	usb_free_urb(agg->urb);
-	__free_pages(agg->page, get_order(tp->rx_buf_sz));
+	put_page(agg->page);
 	kfree(agg);
 
 	atomic_dec(&tp->rx_count);
@@ -1493,7 +1496,7 @@ static struct rx_agg *alloc_rx_agg(struct r8152 *tp, gfp_t mflags)
 	if (rx_agg) {
 		unsigned long flags;
 
-		rx_agg->page = alloc_pages(mflags, order);
+		rx_agg->page = alloc_pages(mflags | __GFP_COMP, order);
 		if (!rx_agg->page)
 			goto free_rx;
 
@@ -1951,6 +1954,50 @@ static u8 r8152_rx_csum(struct r8152 *tp, struct rx_desc *rx_desc)
 	return checksum;
 }
 
+static inline bool rx_count_exceed(struct r8152 *tp)
+{
+	return atomic_read(&tp->rx_count) > RTL8152_MAX_RX;
+}
+
+static inline int agg_offset(struct rx_agg *agg, void *addr)
+{
+	return (int)(addr - agg->buffer);
+}
+
+static struct rx_agg *rtl_get_free_rx(struct r8152 *tp, gfp_t mflags)
+{
+	struct list_head *cursor, *next;
+	struct rx_agg *agg_free = NULL;
+	unsigned long flags;
+
+	spin_lock_irqsave(&tp->rx_lock, flags);
+
+	list_for_each_safe(cursor, next, &tp->rx_used) {
+		struct rx_agg *agg;
+
+		agg = list_entry(cursor, struct rx_agg, list);
+
+		if (page_count(agg->page) == 1) {
+			if (!agg_free) {
+				list_del_init(cursor);
+				agg_free = agg;
+				continue;
+			} else if (rx_count_exceed(tp)) {
+				list_del_init(cursor);
+				free_rx_agg(tp, agg);
+			}
+			break;
+		}
+	}
+
+	spin_unlock_irqrestore(&tp->rx_lock, flags);
+
+	if (!agg_free && atomic_read(&tp->rx_count) < RTL8152_MAX_RX_AGG)
+		agg_free = alloc_rx_agg(tp, mflags);
+
+	return agg_free;
+}
+
 static int rx_bottom(struct r8152 *tp, int budget)
 {
 	unsigned long flags;
@@ -1986,7 +2033,7 @@ static int rx_bottom(struct r8152 *tp, int budget)
 
 	list_for_each_safe(cursor, next, &rx_queue) {
 		struct rx_desc *rx_desc;
-		struct rx_agg *agg;
+		struct rx_agg *agg, *agg_next;
 		int len_used = 0;
 		struct urb *urb;
 		u8 *rx_data;
@@ -1998,6 +2045,8 @@ static int rx_bottom(struct r8152 *tp, int budget)
 		if (urb->actual_length < ETH_ZLEN)
 			goto submit;
 
+		agg_next = rtl_get_free_rx(tp, GFP_ATOMIC);
+
 		rx_desc = agg->buffer;
 		rx_data = agg->buffer;
 		len_used += sizeof(struct rx_desc);
@@ -2005,7 +2054,7 @@ static int rx_bottom(struct r8152 *tp, int budget)
 		while (urb->actual_length > len_used) {
 			struct net_device *netdev = tp->netdev;
 			struct net_device_stats *stats = &netdev->stats;
-			unsigned int pkt_len;
+			unsigned int pkt_len, rx_frag_head_sz;
 			struct sk_buff *skb;
 
 			/* limite the skb numbers for rx_queue */
@@ -2023,22 +2072,37 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			pkt_len -= ETH_FCS_LEN;
 			rx_data += sizeof(struct rx_desc);
 
-			skb = napi_alloc_skb(napi, pkt_len);
+			if (!agg_next || RTL8152_RXFG_HEADSZ > pkt_len)
+				rx_frag_head_sz = pkt_len;
+			else
+				rx_frag_head_sz = RTL8152_RXFG_HEADSZ;
+
+			skb = napi_alloc_skb(napi, rx_frag_head_sz);
 			if (!skb) {
 				stats->rx_dropped++;
 				goto find_next_rx;
 			}
 
 			skb->ip_summed = r8152_rx_csum(tp, rx_desc);
-			memcpy(skb->data, rx_data, pkt_len);
-			skb_put(skb, pkt_len);
+			memcpy(skb->data, rx_data, rx_frag_head_sz);
+			skb_put(skb, rx_frag_head_sz);
+			pkt_len -= rx_frag_head_sz;
+			rx_data += rx_frag_head_sz;
+			if (pkt_len) {
+				skb_add_rx_frag(skb, 0, agg->page,
+						agg_offset(agg, rx_data),
+						pkt_len,
+						SKB_DATA_ALIGN(pkt_len));
+				get_page(agg->page);
+			}
+
 			skb->protocol = eth_type_trans(skb, netdev);
 			rtl_rx_vlan_tag(rx_desc, skb);
 			if (work_done < budget) {
 				napi_gro_receive(napi, skb);
 				work_done++;
 				stats->rx_packets++;
-				stats->rx_bytes += pkt_len;
+				stats->rx_bytes += skb->len;
 			} else {
 				__skb_queue_tail(&tp->rx_queue, skb);
 			}
@@ -2046,10 +2110,24 @@ static int rx_bottom(struct r8152 *tp, int budget)
 find_next_rx:
 			rx_data = rx_agg_align(rx_data + pkt_len + ETH_FCS_LEN);
 			rx_desc = (struct rx_desc *)rx_data;
-			len_used = (int)(rx_data - (u8 *)agg->buffer);
+			len_used = agg_offset(agg, rx_data);
 			len_used += sizeof(struct rx_desc);
 		}
 
+		WARN_ON(!agg_next && page_count(agg->page) > 1);
+
+		if (agg_next) {
+			spin_lock_irqsave(&tp->rx_lock, flags);
+			if (page_count(agg->page) == 1) {
+				list_add(&agg_next->list, &tp->rx_used);
+			} else {
+				list_add_tail(&agg->list, &tp->rx_used);
+				agg = agg_next;
+				urb = agg->urb;
+			}
+			spin_unlock_irqrestore(&tp->rx_lock, flags);
+		}
+
 submit:
 		if (!ret) {
 			ret = r8152_submit_rx(tp, agg, GFP_ATOMIC);
@@ -2376,13 +2454,14 @@ static int rtl_start_rx(struct r8152 *tp)
 {
 	struct list_head *cursor, *next, tmp_list;
 	unsigned long flags;
-	int ret = 0;
+	int ret = 0, i = 0;
 
 	INIT_LIST_HEAD(&tmp_list);
 
 	spin_lock_irqsave(&tp->rx_lock, flags);
 
 	INIT_LIST_HEAD(&tp->rx_done);
+	INIT_LIST_HEAD(&tp->rx_used);
 
 	list_splice_init(&tp->rx_info, &tmp_list);
 
@@ -2394,10 +2473,18 @@ static int rtl_start_rx(struct r8152 *tp)
 		agg = list_entry(cursor, struct rx_agg, info_list);
 		INIT_LIST_HEAD(&agg->list);
 
-		if (ret < 0)
+		/* Only RTL8152_MAX_RX rx_agg need to be submitted. */
+		if (++i > RTL8152_MAX_RX) {
+			spin_lock_irqsave(&tp->rx_lock, flags);
+			list_add_tail(&agg->list, &tp->rx_used);
+			spin_unlock_irqrestore(&tp->rx_lock, flags);
+		} else if (unlikely(ret < 0)) {
+			spin_lock_irqsave(&tp->rx_lock, flags);
 			list_add_tail(&agg->list, &tp->rx_done);
-		else
+			spin_unlock_irqrestore(&tp->rx_lock, flags);
+		} else {
 			ret = r8152_submit_rx(tp, agg, GFP_KERNEL);
+		}
 	}
 
 	spin_lock_irqsave(&tp->rx_lock, flags);
@@ -2429,7 +2516,14 @@ static int rtl_stop_rx(struct r8152 *tp)
 		struct rx_agg *agg;
 
 		agg = list_entry(cursor, struct rx_agg, info_list);
-		usb_kill_urb(agg->urb);
+
+		/* At least RTL8152_MAX_RX rx_agg have the page_count being
+		 * equal to 1, so the other ones could be freed safely.
+		 */
+		if (page_count(agg->page) > 1)
+			free_rx_agg(tp, agg);
+		else
+			usb_kill_urb(agg->urb);
 	}
 
 	/* Move back the list of temp to the rx_info */
-- 
2.21.0


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

* [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-06 11:17 [PATCH net-next 0/5] RX improve Hayes Wang
                   ` (3 preceding siblings ...)
  2019-08-06 11:18 ` [PATCH net-next 4/5] r8152: support skb_add_rx_frag Hayes Wang
@ 2019-08-06 11:18 ` Hayes Wang
  2019-08-06 22:10   ` Jakub Kicinski
  4 siblings, 1 reply; 22+ messages in thread
From: Hayes Wang @ 2019-08-06 11:18 UTC (permalink / raw)
  To: netdev; +Cc: nic_swsd, linux-kernel, linux-usb, Hayes Wang

Let rx_frag_head_sz and rx_max_agg_num could be modified dynamically
through the sysfs.

Signed-off-by: Hayes Wang <hayeswang@realtek.com>
---
 drivers/net/usb/r8152.c | 119 ++++++++++++++++++++++++++++++++++++++--
 1 file changed, 115 insertions(+), 4 deletions(-)

diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
index 1615900c8592..0b4d439f603a 100644
--- a/drivers/net/usb/r8152.c
+++ b/drivers/net/usb/r8152.c
@@ -26,7 +26,7 @@
 #include <linux/acpi.h>
 
 /* Information for net-next */
-#define NETNEXT_VERSION		"09"
+#define NETNEXT_VERSION		"10"
 
 /* Information for net */
 #define NET_VERSION		"10"
@@ -756,6 +756,9 @@ struct r8152 {
 	u32 tx_qlen;
 	u32 coalesce;
 	u32 rx_buf_sz;
+	u32 rx_frag_head_sz;
+	u32 rx_max_agg_num;
+
 	u16 ocp_base;
 	u16 speed;
 	u8 *intr_buff;
@@ -1992,7 +1995,7 @@ static struct rx_agg *rtl_get_free_rx(struct r8152 *tp, gfp_t mflags)
 
 	spin_unlock_irqrestore(&tp->rx_lock, flags);
 
-	if (!agg_free && atomic_read(&tp->rx_count) < RTL8152_MAX_RX_AGG)
+	if (!agg_free && atomic_read(&tp->rx_count) < tp->rx_max_agg_num)
 		agg_free = alloc_rx_agg(tp, mflags);
 
 	return agg_free;
@@ -2072,10 +2075,10 @@ static int rx_bottom(struct r8152 *tp, int budget)
 			pkt_len -= ETH_FCS_LEN;
 			rx_data += sizeof(struct rx_desc);
 
-			if (!agg_next || RTL8152_RXFG_HEADSZ > pkt_len)
+			if (!agg_next || tp->rx_frag_head_sz > pkt_len)
 				rx_frag_head_sz = pkt_len;
 			else
-				rx_frag_head_sz = RTL8152_RXFG_HEADSZ;
+				rx_frag_head_sz = tp->rx_frag_head_sz;
 
 			skb = napi_alloc_skb(napi, rx_frag_head_sz);
 			if (!skb) {
@@ -5383,6 +5386,101 @@ static u8 rtl_get_version(struct usb_interface *intf)
 	return version;
 }
 
+static ssize_t rx_frag_head_sz_show(struct device *dev,
+				    struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct r8152 *tp = usb_get_intfdata(intf);
+
+	sprintf(buf, "%u\n", tp->rx_frag_head_sz);
+
+	return strlen(buf);
+}
+
+static ssize_t rx_frag_head_sz_store(struct device *dev,
+				     struct device_attribute *attr,
+				     const char *buf, size_t count)
+{
+	struct usb_interface *intf;
+	u32 rx_frag_head_sz;
+	struct r8152 *tp;
+
+	intf = to_usb_interface(dev);
+	tp = usb_get_intfdata(intf);
+
+	if (sscanf(buf, "%u\n", &rx_frag_head_sz) != 1)
+		return -EINVAL;
+
+	if (rx_frag_head_sz < ETH_ZLEN)
+		return -EINVAL;
+
+	if (test_bit(RTL8152_UNPLUG, &tp->flags))
+		return -ENODEV;
+
+	if (tp->rx_frag_head_sz != rx_frag_head_sz) {
+		napi_disable(&tp->napi);
+		tp->rx_frag_head_sz = rx_frag_head_sz;
+		napi_enable(&tp->napi);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(rx_frag_head_sz);
+
+static ssize_t rx_max_agg_num_show(struct device *dev,
+				   struct device_attribute *attr, char *buf)
+{
+	struct usb_interface *intf = to_usb_interface(dev);
+	struct r8152 *tp = usb_get_intfdata(intf);
+
+	sprintf(buf, "%u\n", tp->rx_max_agg_num);
+
+	return strlen(buf);
+}
+
+static ssize_t rx_max_agg_num_store(struct device *dev,
+				    struct device_attribute *attr,
+				    const char *buf, size_t count)
+{
+	struct usb_interface *intf;
+	u32 rx_max_agg_num;
+	struct r8152 *tp;
+
+	intf = to_usb_interface(dev);
+	tp = usb_get_intfdata(intf);
+
+	if (sscanf(buf, "%u\n", &rx_max_agg_num) != 1)
+		return -EINVAL;
+
+	if (rx_max_agg_num < (RTL8152_MAX_RX * 2))
+		return -EINVAL;
+
+	if (test_bit(RTL8152_UNPLUG, &tp->flags))
+		return -ENODEV;
+
+	if (tp->rx_max_agg_num != rx_max_agg_num) {
+		napi_disable(&tp->napi);
+		tp->rx_max_agg_num = rx_max_agg_num;
+		napi_enable(&tp->napi);
+	}
+
+	return count;
+}
+
+static DEVICE_ATTR_RW(rx_max_agg_num);
+
+static struct attribute *rtk_adv_attrs[] = {
+	&dev_attr_rx_frag_head_sz.attr,
+	&dev_attr_rx_max_agg_num.attr,
+	NULL
+};
+
+static struct attribute_group rtk_adv_grp = {
+	.name = "rtl_adv",
+	.attrs = rtk_adv_attrs,
+};
+
 static int rtl8152_probe(struct usb_interface *intf,
 			 const struct usb_device_id *id)
 {
@@ -5487,6 +5585,9 @@ static int rtl8152_probe(struct usb_interface *intf,
 	tp->speed = tp->mii.supports_gmii ? SPEED_1000 : SPEED_100;
 	tp->duplex = DUPLEX_FULL;
 
+	tp->rx_frag_head_sz = RTL8152_RXFG_HEADSZ;
+	tp->rx_max_agg_num = RTL8152_MAX_RX_AGG;
+
 	intf->needs_remote_wakeup = 1;
 
 	tp->rtl_ops.init(tp);
@@ -5513,8 +5614,16 @@ static int rtl8152_probe(struct usb_interface *intf,
 
 	netif_info(tp, probe, netdev, "%s\n", DRIVER_VERSION);
 
+	ret = sysfs_create_group(&intf->dev.kobj, &rtk_adv_grp);
+	if (ret < 0) {
+		netif_err(tp, probe, netdev, "creat rtk_adv_grp fail\n");
+		goto out2;
+	}
+
 	return 0;
 
+out2:
+	unregister_netdev(netdev);
 out1:
 	netif_napi_del(&tp->napi);
 	usb_set_intfdata(intf, NULL);
@@ -5527,6 +5636,8 @@ static void rtl8152_disconnect(struct usb_interface *intf)
 {
 	struct r8152 *tp = usb_get_intfdata(intf);
 
+	sysfs_remove_group(&intf->dev.kobj, &rtk_adv_grp);
+
 	usb_set_intfdata(intf, NULL);
 	if (tp) {
 		rtl_set_unplug(tp);
-- 
2.21.0


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

* Re: [PATCH net-next 2/5] r8152: replace array with linking list for rx information
  2019-08-06 11:18 ` [PATCH net-next 2/5] r8152: replace array with linking list for rx information Hayes Wang
@ 2019-08-06 19:53   ` Jakub Kicinski
  2019-08-06 21:40     ` Jakub Kicinski
  2019-08-07  4:34     ` Hayes Wang
  0 siblings, 2 replies; 22+ messages in thread
From: Jakub Kicinski @ 2019-08-06 19:53 UTC (permalink / raw)
  To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb

On Tue, 6 Aug 2019 19:18:01 +0800, Hayes Wang wrote:
> The original method uses an array to store the rx information. The
> new one uses a list to link each rx structure. Then, it is possible
> to increase/decrease the number of rx structure dynamically.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ---
>  drivers/net/usb/r8152.c | 187 ++++++++++++++++++++++++++++------------
>  1 file changed, 132 insertions(+), 55 deletions(-)
> 
> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 0f07ed05ab34..44d832ceb516 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -22,6 +22,7 @@
>  #include <linux/mdio.h>
>  #include <linux/usb/cdc.h>
>  #include <linux/suspend.h>
> +#include <linux/atomic.h>
>  #include <linux/acpi.h>
>  
>  /* Information for net-next */
> @@ -694,7 +695,7 @@ struct tx_desc {
>  struct r8152;
>  
>  struct rx_agg {
> -	struct list_head list;
> +	struct list_head list, info_list;
>  	struct urb *urb;
>  	struct r8152 *context;
>  	void *buffer;
> @@ -719,7 +720,7 @@ struct r8152 {
>  	struct net_device *netdev;
>  	struct urb *intr_urb;
>  	struct tx_agg tx_info[RTL8152_MAX_TX];
> -	struct rx_agg rx_info[RTL8152_MAX_RX];
> +	struct list_head rx_info;
>  	struct list_head rx_done, tx_free;
>  	struct sk_buff_head tx_queue, rx_queue;
>  	spinlock_t rx_lock, tx_lock;
> @@ -744,6 +745,8 @@ struct r8152 {
>  		void (*autosuspend_en)(struct r8152 *tp, bool enable);
>  	} rtl_ops;
>  
> +	atomic_t rx_count;

I wonder what the advantage of rx_count being atomic is, perhpas it
could be protected by the same lock as the list head?

>  	int intr_interval;
>  	u32 saved_wolopts;
>  	u32 msg_enable;
> @@ -1468,19 +1471,86 @@ static inline void *tx_agg_align(void *data)
>  	return (void *)ALIGN((uintptr_t)data, TX_ALIGN);
>  }
>  
> +static void free_rx_agg(struct r8152 *tp, struct rx_agg *agg)
> +{
> +	list_del(&agg->info_list);
> +
> +	usb_free_urb(agg->urb);
> +	kfree(agg->buffer);
> +	kfree(agg);
> +
> +	atomic_dec(&tp->rx_count);
> +}
> +
> +static struct rx_agg *alloc_rx_agg(struct r8152 *tp, gfp_t mflags)
> +{
> +	struct net_device *netdev = tp->netdev;
> +	int node = netdev->dev.parent ? dev_to_node(netdev->dev.parent) : -1;
> +	struct rx_agg *rx_agg;
> +
> +	rx_agg = kmalloc_node(sizeof(*rx_agg), mflags, node);
> +	if (rx_agg) {

nit: you could possibly save the indentation by returning early here

> +		unsigned long flags;
> +		u8 *buf;
> +
> +		buf = kmalloc_node(tp->rx_buf_sz, mflags, node);
> +		if (!buf)
> +			goto free_rx;
> +
> +		if (buf != rx_agg_align(buf)) {
> +			kfree(buf);
> +			buf = kmalloc_node(tp->rx_buf_sz + RX_ALIGN, mflags,
> +					   node);
> +			if (!buf)
> +				goto free_rx;
> +		}
> +
> +		rx_agg->buffer = buf;
> +		rx_agg->head = rx_agg_align(buf);
> +
> +		rx_agg->urb = usb_alloc_urb(0, mflags);
> +		if (!rx_agg->urb)
> +			goto free_buf;
> +
> +		rx_agg->context = tp;
> +
> +		INIT_LIST_HEAD(&rx_agg->list);
> +		INIT_LIST_HEAD(&rx_agg->info_list);
> +		spin_lock_irqsave(&tp->rx_lock, flags);
> +		list_add_tail(&rx_agg->info_list, &tp->rx_info);
> +		spin_unlock_irqrestore(&tp->rx_lock, flags);
> +
> +		atomic_inc(&tp->rx_count);
> +	}
> +
> +	return rx_agg;
> +
> +free_buf:
> +	kfree(rx_agg->buffer);
> +free_rx:
> +	kfree(rx_agg);
> +	return NULL;
> +}
> +
>  static void free_all_mem(struct r8152 *tp)
>  {
> +	struct list_head *cursor, *next;
> +	unsigned long flags;
>  	int i;
>  
> -	for (i = 0; i < RTL8152_MAX_RX; i++) {
> -		usb_free_urb(tp->rx_info[i].urb);
> -		tp->rx_info[i].urb = NULL;
> +	spin_lock_irqsave(&tp->rx_lock, flags);
>  
> -		kfree(tp->rx_info[i].buffer);
> -		tp->rx_info[i].buffer = NULL;
> -		tp->rx_info[i].head = NULL;
> +	list_for_each_safe(cursor, next, &tp->rx_info) {

nit: list_for_each_entry_safe, perhaps?

> +		struct rx_agg *agg;
> +
> +		agg = list_entry(cursor, struct rx_agg, info_list);
> +		free_rx_agg(tp, agg);
>  	}
>  
> +	spin_unlock_irqrestore(&tp->rx_lock, flags);
> +
> +	WARN_ON(unlikely(atomic_read(&tp->rx_count)));

nit: WARN_ON() has an unlikely() already built in

>  	for (i = 0; i < RTL8152_MAX_TX; i++) {
>  		usb_free_urb(tp->tx_info[i].urb);
>  		tp->tx_info[i].urb = NULL;
> @@ -1503,46 +1573,28 @@ static int alloc_all_mem(struct r8152 *tp)
>  	struct usb_interface *intf = tp->intf;
>  	struct usb_host_interface *alt = intf->cur_altsetting;
>  	struct usb_host_endpoint *ep_intr = alt->endpoint + 2;
> -	struct urb *urb;
>  	int node, i;
> -	u8 *buf;
>  
>  	node = netdev->dev.parent ? dev_to_node(netdev->dev.parent) : -1;
>  
>  	spin_lock_init(&tp->rx_lock);
>  	spin_lock_init(&tp->tx_lock);
> +	INIT_LIST_HEAD(&tp->rx_info);
>  	INIT_LIST_HEAD(&tp->tx_free);
>  	INIT_LIST_HEAD(&tp->rx_done);
>  	skb_queue_head_init(&tp->tx_queue);
>  	skb_queue_head_init(&tp->rx_queue);
> +	atomic_set(&tp->rx_count, 0);
>  
>  	for (i = 0; i < RTL8152_MAX_RX; i++) {
> -		buf = kmalloc_node(tp->rx_buf_sz, GFP_KERNEL, node);
> -		if (!buf)
> -			goto err1;
> -
> -		if (buf != rx_agg_align(buf)) {
> -			kfree(buf);
> -			buf = kmalloc_node(tp->rx_buf_sz + RX_ALIGN, GFP_KERNEL,
> -					   node);
> -			if (!buf)
> -				goto err1;
> -		}
> -
> -		urb = usb_alloc_urb(0, GFP_KERNEL);
> -		if (!urb) {
> -			kfree(buf);
> +		if (!alloc_rx_agg(tp, GFP_KERNEL))
>  			goto err1;
> -		}
> -
> -		INIT_LIST_HEAD(&tp->rx_info[i].list);
> -		tp->rx_info[i].context = tp;
> -		tp->rx_info[i].urb = urb;
> -		tp->rx_info[i].buffer = buf;
> -		tp->rx_info[i].head = rx_agg_align(buf);
>  	}
>  
>  	for (i = 0; i < RTL8152_MAX_TX; i++) {
> +		struct urb *urb;
> +		u8 *buf;
> +
>  		buf = kmalloc_node(agg_buf_sz, GFP_KERNEL, node);
>  		if (!buf)
>  			goto err1;
> @@ -2331,44 +2383,69 @@ static void rxdy_gated_en(struct r8152 *tp, bool enable)
>  
>  static int rtl_start_rx(struct r8152 *tp)
>  {
> -	int i, ret = 0;
> +	struct list_head *cursor, *next, tmp_list;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	INIT_LIST_HEAD(&tmp_list);
> +
> +	spin_lock_irqsave(&tp->rx_lock, flags);
>  
>  	INIT_LIST_HEAD(&tp->rx_done);
> -	for (i = 0; i < RTL8152_MAX_RX; i++) {
> -		INIT_LIST_HEAD(&tp->rx_info[i].list);
> -		ret = r8152_submit_rx(tp, &tp->rx_info[i], GFP_KERNEL);
> -		if (ret)
> -			break;
> -	}
>  
> -	if (ret && ++i < RTL8152_MAX_RX) {
> -		struct list_head rx_queue;
> -		unsigned long flags;
> +	list_splice_init(&tp->rx_info, &tmp_list);
>  
> -		INIT_LIST_HEAD(&rx_queue);
> +	spin_unlock_irqrestore(&tp->rx_lock, flags);
>  
> -		do {
> -			struct rx_agg *agg = &tp->rx_info[i++];
> -			struct urb *urb = agg->urb;
> +	list_for_each_safe(cursor, next, &tmp_list) {
> +		struct rx_agg *agg;
>  
> -			urb->actual_length = 0;
> -			list_add_tail(&agg->list, &rx_queue);
> -		} while (i < RTL8152_MAX_RX);
> +		agg = list_entry(cursor, struct rx_agg, info_list);
> +		INIT_LIST_HEAD(&agg->list);
>  
> -		spin_lock_irqsave(&tp->rx_lock, flags);
> -		list_splice_tail(&rx_queue, &tp->rx_done);
> -		spin_unlock_irqrestore(&tp->rx_lock, flags);
> +		if (ret < 0)
> +			list_add_tail(&agg->list, &tp->rx_done);
> +		else
> +			ret = r8152_submit_rx(tp, agg, GFP_KERNEL);
>  	}
>  
> +	spin_lock_irqsave(&tp->rx_lock, flags);
> +	WARN_ON(unlikely(!list_empty(&tp->rx_info)));
> +	list_splice(&tmp_list, &tp->rx_info);
> +	spin_unlock_irqrestore(&tp->rx_lock, flags);
> +
>  	return ret;
>  }
>  
>  static int rtl_stop_rx(struct r8152 *tp)
>  {
> -	int i;
> +	struct list_head *cursor, *next, tmp_list;
> +	unsigned long flags;
> +
> +	INIT_LIST_HEAD(&tmp_list);
>  
> -	for (i = 0; i < RTL8152_MAX_RX; i++)
> -		usb_kill_urb(tp->rx_info[i].urb);
> +	/* The usb_kill_urb() couldn't be used in atomic.
> +	 * Therefore, move the list of rx_info to a tmp one.
> +	 * Then, list_for_each_safe could be used without
> +	 * spin lock.
> +	 */

Would you mind explaining in a little more detail why taking the
entries from the list for a brief period of time is safe? 

> +	spin_lock_irqsave(&tp->rx_lock, flags);
> +	list_splice_init(&tp->rx_info, &tmp_list);
> +	spin_unlock_irqrestore(&tp->rx_lock, flags);
> +
> +	list_for_each_safe(cursor, next, &tmp_list) {
> +		struct rx_agg *agg;
> +
> +		agg = list_entry(cursor, struct rx_agg, info_list);
> +		usb_kill_urb(agg->urb);
> +	}
> +
> +	/* Move back the list of temp to the rx_info */
> +	spin_lock_irqsave(&tp->rx_lock, flags);
> +	WARN_ON(unlikely(!list_empty(&tp->rx_info)));
> +	list_splice(&tmp_list, &tp->rx_info);
> +	spin_unlock_irqrestore(&tp->rx_lock, flags);
>  
>  	while (!skb_queue_empty(&tp->rx_queue))
>  		dev_kfree_skb(__skb_dequeue(&tp->rx_queue));


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

* Re: [PATCH net-next 2/5] r8152: replace array with linking list for rx information
  2019-08-06 19:53   ` Jakub Kicinski
@ 2019-08-06 21:40     ` Jakub Kicinski
  2019-08-07  4:34     ` Hayes Wang
  1 sibling, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2019-08-06 21:40 UTC (permalink / raw)
  To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb

On Tue, 6 Aug 2019 12:53:42 -0700, Jakub Kicinski wrote:
> > @@ -744,6 +745,8 @@ struct r8152 {
> >  		void (*autosuspend_en)(struct r8152 *tp, bool enable);
> >  	} rtl_ops;
> >  
> > +	atomic_t rx_count;  
> 
> I wonder what the advantage of rx_count being atomic is, perhpas it
> could be protected by the same lock as the list head?

Okay, I see you're using it to test the queue length without the lock in
a later patch.

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

* Re: [PATCH net-next 4/5] r8152: support skb_add_rx_frag
  2019-08-06 11:18 ` [PATCH net-next 4/5] r8152: support skb_add_rx_frag Hayes Wang
@ 2019-08-06 22:08   ` Jakub Kicinski
  2019-08-07  4:34     ` Hayes Wang
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2019-08-06 22:08 UTC (permalink / raw)
  To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb

On Tue, 6 Aug 2019 19:18:03 +0800, Hayes Wang wrote:
> Use skb_add_rx_frag() to reduce the memory copy for rx data.
> 
> Use a new list of rx_used to store the rx buffer which couldn't be
> reused yet.
> 
> Besides, the total number of rx buffer may be increased or decreased
> dynamically. And it is limited by RTL8152_MAX_RX_AGG.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>

> diff --git a/drivers/net/usb/r8152.c b/drivers/net/usb/r8152.c
> index 401e56112365..1615900c8592 100644
> --- a/drivers/net/usb/r8152.c
> +++ b/drivers/net/usb/r8152.c
> @@ -584,6 +584,9 @@ enum rtl_register_content {
>  #define TX_ALIGN		4
>  #define RX_ALIGN		8
>  
> +#define RTL8152_MAX_RX_AGG	(10 * RTL8152_MAX_RX)
> +#define RTL8152_RXFG_HEADSZ	256
> +
>  #define INTR_LINK		0x0004
>  
>  #define RTL8152_REQT_READ	0xc0
> @@ -720,7 +723,7 @@ struct r8152 {
>  	struct net_device *netdev;
>  	struct urb *intr_urb;
>  	struct tx_agg tx_info[RTL8152_MAX_TX];
> -	struct list_head rx_info;
> +	struct list_head rx_info, rx_used;

I don't see where entries on the rx_used list get freed when driver is
unloaded, could you explain how that's taken care of?

>  	struct list_head rx_done, tx_free;
>  	struct sk_buff_head tx_queue, rx_queue;
>  	spinlock_t rx_lock, tx_lock;
> @@ -1476,7 +1479,7 @@ static void free_rx_agg(struct r8152 *tp, struct rx_agg *agg)
>  	list_del(&agg->info_list);
>  
>  	usb_free_urb(agg->urb);
> -	__free_pages(agg->page, get_order(tp->rx_buf_sz));
> +	put_page(agg->page);
>  	kfree(agg);
>  
>  	atomic_dec(&tp->rx_count);
> @@ -1493,7 +1496,7 @@ static struct rx_agg *alloc_rx_agg(struct r8152 *tp, gfp_t mflags)
>  	if (rx_agg) {
>  		unsigned long flags;
>  
> -		rx_agg->page = alloc_pages(mflags, order);
> +		rx_agg->page = alloc_pages(mflags | __GFP_COMP, order);
>  		if (!rx_agg->page)
>  			goto free_rx;
>  
> @@ -1951,6 +1954,50 @@ static u8 r8152_rx_csum(struct r8152 *tp, struct rx_desc *rx_desc)
>  	return checksum;
>  }
>  
> +static inline bool rx_count_exceed(struct r8152 *tp)
> +{
> +	return atomic_read(&tp->rx_count) > RTL8152_MAX_RX;
> +}
> +
> +static inline int agg_offset(struct rx_agg *agg, void *addr)
> +{
> +	return (int)(addr - agg->buffer);
> +}
> +
> +static struct rx_agg *rtl_get_free_rx(struct r8152 *tp, gfp_t mflags)
> +{
> +	struct list_head *cursor, *next;
> +	struct rx_agg *agg_free = NULL;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&tp->rx_lock, flags);
> +
> +	list_for_each_safe(cursor, next, &tp->rx_used) {
> +		struct rx_agg *agg;
> +
> +		agg = list_entry(cursor, struct rx_agg, list);
> +
> +		if (page_count(agg->page) == 1) {
> +			if (!agg_free) {
> +				list_del_init(cursor);
> +				agg_free = agg;
> +				continue;
> +			} else if (rx_count_exceed(tp)) {

nit: else unnecessary after continue

> +				list_del_init(cursor);
> +				free_rx_agg(tp, agg);
> +			}
> +			break;
> +		}
> +	}
> +
> +	spin_unlock_irqrestore(&tp->rx_lock, flags);
> +
> +	if (!agg_free && atomic_read(&tp->rx_count) < RTL8152_MAX_RX_AGG)
> +		agg_free = alloc_rx_agg(tp, mflags);
> +
> +	return agg_free;
> +}
> +
>  static int rx_bottom(struct r8152 *tp, int budget)
>  {
>  	unsigned long flags;

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

* Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-06 11:18 ` [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically Hayes Wang
@ 2019-08-06 22:10   ` Jakub Kicinski
  2019-08-07  7:12     ` Hayes Wang
  2019-08-08  8:52     ` Hayes Wang
  0 siblings, 2 replies; 22+ messages in thread
From: Jakub Kicinski @ 2019-08-06 22:10 UTC (permalink / raw)
  To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb

On Tue, 6 Aug 2019 19:18:04 +0800, Hayes Wang wrote:
> Let rx_frag_head_sz and rx_max_agg_num could be modified dynamically
> through the sysfs.
> 
> Signed-off-by: Hayes Wang <hayeswang@realtek.com>

Please don't expose those via sysfs. Ethtool's copybreak and descriptor
count should be applicable here, I think.

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

* RE: [PATCH net-next 2/5] r8152: replace array with linking list for rx information
  2019-08-06 19:53   ` Jakub Kicinski
  2019-08-06 21:40     ` Jakub Kicinski
@ 2019-08-07  4:34     ` Hayes Wang
  2019-08-07 18:21       ` Jakub Kicinski
  1 sibling, 1 reply; 22+ messages in thread
From: Hayes Wang @ 2019-08-07  4:34 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, nic_swsd, linux-kernel, linux-usb

Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
[...]
> >  static int rtl_stop_rx(struct r8152 *tp)
> >  {
> > -	int i;
> > +	struct list_head *cursor, *next, tmp_list;
> > +	unsigned long flags;
> > +
> > +	INIT_LIST_HEAD(&tmp_list);
> >
> > -	for (i = 0; i < RTL8152_MAX_RX; i++)
> > -		usb_kill_urb(tp->rx_info[i].urb);
> > +	/* The usb_kill_urb() couldn't be used in atomic.
> > +	 * Therefore, move the list of rx_info to a tmp one.
> > +	 * Then, list_for_each_safe could be used without
> > +	 * spin lock.
> > +	 */
> 
> Would you mind explaining in a little more detail why taking the
> entries from the list for a brief period of time is safe?

Usually, it needs the spin lock before accessing the entry
of the list "tp->rx_info". However, for some reasons,
if we want to access the entry without spin lock, we
cloud move all entries to a local list temporally. Then,
we could make sure no other one could access the entries
included in the temporal local list.

For this case, when I move all entries to a temporal 
local list, no other one could access them. Therefore,
I could access the entries included in the temporal local
list without spin lock.


Best Regards,
Hayes



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

* RE: [PATCH net-next 4/5] r8152: support skb_add_rx_frag
  2019-08-06 22:08   ` Jakub Kicinski
@ 2019-08-07  4:34     ` Hayes Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Hayes Wang @ 2019-08-07  4:34 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, nic_swsd, linux-kernel, linux-usb

Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> Sent: Wednesday, August 07, 2019 6:08 AM
[...]
> >  #define RTL8152_REQT_READ	0xc0
> > @@ -720,7 +723,7 @@ struct r8152 {
> >  	struct net_device *netdev;
> >  	struct urb *intr_urb;
> >  	struct tx_agg tx_info[RTL8152_MAX_TX];
> > -	struct list_head rx_info;
> > +	struct list_head rx_info, rx_used;
> 
> I don't see where entries on the rx_used list get freed when driver is
> unloaded, could you explain how that's taken care of?

When the driver is unloaded, all rx_agg would be freed from
info_list list.

The info_list includes all rx_agg buffers which may be idle
or be busy. The rx_done and rx_use are used to determine
the status of rx_agg buffer included in info_list.

info_list: the rx_agg buffer would be inserted in this list
	   when it is allocated.
rx_done: the rx_agg buffer is ready (contains rx data). Or
	 it needs to be resubmitted.
rx_use: the rx_agg buffer is busy and couldn't be submitted
	yet.


Best Regards,
Hayes



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

* RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-06 22:10   ` Jakub Kicinski
@ 2019-08-07  7:12     ` Hayes Wang
  2019-08-07 12:43       ` Maciej Fijalkowski
  2019-08-08  8:52     ` Hayes Wang
  1 sibling, 1 reply; 22+ messages in thread
From: Hayes Wang @ 2019-08-07  7:12 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, nic_swsd, linux-kernel, linux-usb

Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> Sent: Wednesday, August 07, 2019 6:10 AM
[...]
> Please don't expose those via sysfs. Ethtool's copybreak and descriptor
> count should be applicable here, I think.

Excuse me.
I find struct ethtool_tunable for ETHTOOL_RX_COPYBREAK.
How about the descriptor count?


Best Regards,
Hayes



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

* Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-07  7:12     ` Hayes Wang
@ 2019-08-07 12:43       ` Maciej Fijalkowski
  2019-08-08  1:40         ` Hayes Wang
  0 siblings, 1 reply; 22+ messages in thread
From: Maciej Fijalkowski @ 2019-08-07 12:43 UTC (permalink / raw)
  To: Hayes Wang; +Cc: Jakub Kicinski, netdev, nic_swsd, linux-kernel, linux-usb

On Wed, 7 Aug 2019 07:12:32 +0000
Hayes Wang <hayeswang@realtek.com> wrote:

> Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> > Sent: Wednesday, August 07, 2019 6:10 AM  
> [...]
> > Please don't expose those via sysfs. Ethtool's copybreak and descriptor
> > count should be applicable here, I think.  
> 
> Excuse me.
> I find struct ethtool_tunable for ETHTOOL_RX_COPYBREAK.
> How about the descriptor count?

Look how drivers implement ethtool's set_ringparam ops.

Thanks,
Maciej

> 
> 
> Best Regards,
> Hayes
> 
> 


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

* Re: [PATCH net-next 2/5] r8152: replace array with linking list for rx information
  2019-08-07  4:34     ` Hayes Wang
@ 2019-08-07 18:21       ` Jakub Kicinski
  0 siblings, 0 replies; 22+ messages in thread
From: Jakub Kicinski @ 2019-08-07 18:21 UTC (permalink / raw)
  To: Hayes Wang; +Cc: netdev, nic_swsd, linux-kernel, linux-usb

On Wed, 7 Aug 2019 04:34:24 +0000, Hayes Wang wrote:
> Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> > >  static int rtl_stop_rx(struct r8152 *tp)
> > >  {
> > > -	int i;
> > > +	struct list_head *cursor, *next, tmp_list;
> > > +	unsigned long flags;
> > > +
> > > +	INIT_LIST_HEAD(&tmp_list);
> > >
> > > -	for (i = 0; i < RTL8152_MAX_RX; i++)
> > > -		usb_kill_urb(tp->rx_info[i].urb);
> > > +	/* The usb_kill_urb() couldn't be used in atomic.
> > > +	 * Therefore, move the list of rx_info to a tmp one.
> > > +	 * Then, list_for_each_safe could be used without
> > > +	 * spin lock.
> > > +	 */  
> > 
> > Would you mind explaining in a little more detail why taking the
> > entries from the list for a brief period of time is safe?  
> 
> Usually, it needs the spin lock before accessing the entry
> of the list "tp->rx_info". However, for some reasons,
> if we want to access the entry without spin lock, we
> cloud move all entries to a local list temporally. Then,
> we could make sure no other one could access the entries
> included in the temporal local list.
> 
> For this case, when I move all entries to a temporal 
> local list, no other one could access them. Therefore,
> I could access the entries included in the temporal local
> list without spin lock.

I see, thanks for the explanation.

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

* RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-07 12:43       ` Maciej Fijalkowski
@ 2019-08-08  1:40         ` Hayes Wang
  0 siblings, 0 replies; 22+ messages in thread
From: Hayes Wang @ 2019-08-08  1:40 UTC (permalink / raw)
  To: Maciej Fijalkowski
  Cc: Jakub Kicinski, netdev, nic_swsd, linux-kernel, linux-usb

Maciej Fijalkowski [mailto:maciejromanfijalkowski@gmail.com]
> Sent: Wednesday, August 07, 2019 8:44 PM
[...]
> > Excuse me.
> > I find struct ethtool_tunable for ETHTOOL_RX_COPYBREAK.
> > How about the descriptor count?
> 
> Look how drivers implement ethtool's set_ringparam ops.

I would check it. Thanks.


Best Regards,
Hayes



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

* RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-06 22:10   ` Jakub Kicinski
  2019-08-07  7:12     ` Hayes Wang
@ 2019-08-08  8:52     ` Hayes Wang
  2019-08-08 11:49       ` Maciej Fijalkowski
  1 sibling, 1 reply; 22+ messages in thread
From: Hayes Wang @ 2019-08-08  8:52 UTC (permalink / raw)
  To: Jakub Kicinski; +Cc: netdev, nic_swsd, linux-kernel, linux-usb

Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> Sent: Wednesday, August 07, 2019 6:10 AM
[...]
> On Tue, 6 Aug 2019 19:18:04 +0800, Hayes Wang wrote:
> > Let rx_frag_head_sz and rx_max_agg_num could be modified dynamically
> > through the sysfs.
> >
> > Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> 
> Please don't expose those via sysfs. Ethtool's copybreak and descriptor
> count should be applicable here, I think.

Excuse me again.
I find the kernel supports the copybreak of Ethtool.
However, I couldn't find a command of Ethtool to use it.
Do I miss something?

Best Regards,
Hayes


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

* Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-08  8:52     ` Hayes Wang
@ 2019-08-08 11:49       ` Maciej Fijalkowski
  2019-08-08 12:16         ` Hayes Wang
  0 siblings, 1 reply; 22+ messages in thread
From: Maciej Fijalkowski @ 2019-08-08 11:49 UTC (permalink / raw)
  To: Hayes Wang; +Cc: Jakub Kicinski, netdev, nic_swsd, linux-kernel, linux-usb

On Thu, 8 Aug 2019 08:52:51 +0000
Hayes Wang <hayeswang@realtek.com> wrote:

> Jakub Kicinski [mailto:jakub.kicinski@netronome.com]
> > Sent: Wednesday, August 07, 2019 6:10 AM  
> [...]
> > On Tue, 6 Aug 2019 19:18:04 +0800, Hayes Wang wrote:  
> > > Let rx_frag_head_sz and rx_max_agg_num could be modified dynamically
> > > through the sysfs.
> > >
> > > Signed-off-by: Hayes Wang <hayeswang@realtek.com>  
> > 
> > Please don't expose those via sysfs. Ethtool's copybreak and descriptor
> > count should be applicable here, I think.  
> 
> Excuse me again.
> I find the kernel supports the copybreak of Ethtool.
> However, I couldn't find a command of Ethtool to use it.

Ummm there's set_tunable ops. Amazon's ena driver is making use of it from what
I see. Look at ena_set_tunable() in
drivers/net/ethernet/amazon/ena/ena_ethtool.c.

Maciej

> Do I miss something?
> 
> Best Regards,
> Hayes
> 


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

* RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-08 11:49       ` Maciej Fijalkowski
@ 2019-08-08 12:16         ` Hayes Wang
  2019-08-08 18:43           ` Jakub Kicinski
  0 siblings, 1 reply; 22+ messages in thread
From: Hayes Wang @ 2019-08-08 12:16 UTC (permalink / raw)
  To: Maciej Fijalkowski
  Cc: Jakub Kicinski, netdev, nic_swsd, linux-kernel, linux-usb

Maciej Fijalkowski [mailto:maciejromanfijalkowski@gmail.com]
> Sent: Thursday, August 08, 2019 7:50 PM
[...]
> > Excuse me again.
> > I find the kernel supports the copybreak of Ethtool.
> > However, I couldn't find a command of Ethtool to use it.
> 
> Ummm there's set_tunable ops. Amazon's ena driver is making use of it from
> what
> I see. Look at ena_set_tunable() in
> drivers/net/ethernet/amazon/ena/ena_ethtool.c.

The kernel could support it. And I has finished it.
However, when I want to test it by ethtool, I couldn't find suitable command.
I couldn't find relative feature in the source code of ethtool, either.


Best Regards,
Hayes



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

* Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-08 12:16         ` Hayes Wang
@ 2019-08-08 18:43           ` Jakub Kicinski
  2019-08-09  3:38             ` Hayes Wang
  0 siblings, 1 reply; 22+ messages in thread
From: Jakub Kicinski @ 2019-08-08 18:43 UTC (permalink / raw)
  To: Hayes Wang; +Cc: Maciej Fijalkowski, netdev, nic_swsd, linux-kernel, linux-usb

On Thu, 8 Aug 2019 12:16:50 +0000, Hayes Wang wrote:
> Maciej Fijalkowski [mailto:maciejromanfijalkowski@gmail.com]
> > Sent: Thursday, August 08, 2019 7:50 PM  
> > > Excuse me again.
> > > I find the kernel supports the copybreak of Ethtool.
> > > However, I couldn't find a command of Ethtool to use it.  
> > 
> > Ummm there's set_tunable ops. Amazon's ena driver is making use of it from
> > what
> > I see. Look at ena_set_tunable() in
> > drivers/net/ethernet/amazon/ena/ena_ethtool.c.  
> 
> The kernel could support it. And I has finished it.
> However, when I want to test it by ethtool, I couldn't find suitable command.
> I couldn't find relative feature in the source code of ethtool, either.

It's possible it's not implemented in the user space tool 🤔

Looks like it got posted here:

https://www.spinics.net/lists/netdev/msg299877.html

But perhaps never finished? 

It should be fairly straightforward to implement by looking at how
phy-tunables are handled.

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

* RE: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-08 18:43           ` Jakub Kicinski
@ 2019-08-09  3:38             ` Hayes Wang
  2019-08-09  4:51               ` David Miller
  0 siblings, 1 reply; 22+ messages in thread
From: Hayes Wang @ 2019-08-09  3:38 UTC (permalink / raw)
  To: Jakub Kicinski
  Cc: Maciej Fijalkowski, netdev, nic_swsd, linux-kernel, linux-usb

Jakub Kicinski [jakub.kicinski@netronome.com]
[..]> The kernel could support it. And I has finished it.
> > However, when I want to test it by ethtool, I couldn't find suitable command.
> > I couldn't find relative feature in the source code of ethtool, either.

> It's possible it's not implemented in the user space tool 🤔
>
> Looks like it got posted here:
>
> https://www.spinics.net/lists/netdev/msg299877.html
>
> But perhaps never finished?

May I implement both sysfs and set_tunalbe for copybreak first
before the user space tool is ready? Otherwise, the user couldn't
change the copybreak now.

Best Regards,
Hayes

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

* Re: [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically
  2019-08-09  3:38             ` Hayes Wang
@ 2019-08-09  4:51               ` David Miller
  0 siblings, 0 replies; 22+ messages in thread
From: David Miller @ 2019-08-09  4:51 UTC (permalink / raw)
  To: hayeswang
  Cc: jakub.kicinski, maciejromanfijalkowski, netdev, nic_swsd,
	linux-kernel, linux-usb

From: Hayes Wang <hayeswang@realtek.com>
Date: Fri, 9 Aug 2019 03:38:53 +0000

> Jakub Kicinski [jakub.kicinski@netronome.com]
> [..]> The kernel could support it. And I has finished it.
>> > However, when I want to test it by ethtool, I couldn't find suitable command.
>> > I couldn't find relative feature in the source code of ethtool, either.
> 
>> It's possible it's not implemented in the user space tool 🤔
>>
>> Looks like it got posted here:
>>
>> https://www.spinics.net/lists/netdev/msg299877.html
>>
>> But perhaps never finished?
> 
> May I implement both sysfs and set_tunalbe for copybreak first
> before the user space tool is ready? Otherwise, the user couldn't
> change the copybreak now.

No, fix the tool please.

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

end of thread, other threads:[~2019-08-09  4:51 UTC | newest]

Thread overview: 22+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-06 11:17 [PATCH net-next 0/5] RX improve Hayes Wang
2019-08-06 11:18 ` [PATCH net-next 1/5] r8152: separate the rx buffer size Hayes Wang
2019-08-06 11:18 ` [PATCH net-next 2/5] r8152: replace array with linking list for rx information Hayes Wang
2019-08-06 19:53   ` Jakub Kicinski
2019-08-06 21:40     ` Jakub Kicinski
2019-08-07  4:34     ` Hayes Wang
2019-08-07 18:21       ` Jakub Kicinski
2019-08-06 11:18 ` [PATCH net-next 3/5] r8152: use alloc_pages for rx buffer Hayes Wang
2019-08-06 11:18 ` [PATCH net-next 4/5] r8152: support skb_add_rx_frag Hayes Wang
2019-08-06 22:08   ` Jakub Kicinski
2019-08-07  4:34     ` Hayes Wang
2019-08-06 11:18 ` [PATCH net-next 5/5] r8152: change rx_frag_head_sz and rx_max_agg_num dynamically Hayes Wang
2019-08-06 22:10   ` Jakub Kicinski
2019-08-07  7:12     ` Hayes Wang
2019-08-07 12:43       ` Maciej Fijalkowski
2019-08-08  1:40         ` Hayes Wang
2019-08-08  8:52     ` Hayes Wang
2019-08-08 11:49       ` Maciej Fijalkowski
2019-08-08 12:16         ` Hayes Wang
2019-08-08 18:43           ` Jakub Kicinski
2019-08-09  3:38             ` Hayes Wang
2019-08-09  4:51               ` David Miller

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).