All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism
@ 2018-12-12 17:40 Xue Chaojing
  2018-12-13 14:31 ` Neil Horman
  0 siblings, 1 reply; 7+ messages in thread
From: Xue Chaojing @ 2018-12-12 17:40 UTC (permalink / raw)
  To: davem
  Cc: linux-kernel, xuechaojing, netdev, wulike1, chiqijun, fy.wang,
	tony.qu, luoshaokai, nhorman

There is no need to schedule a different tasklet for refill,
This patch remove it.

Suggested-by: Neil Horman <nhorman@redhat.com>
Signed-off-by: Xue Chaojing <xuechaojing@huawei.com>
---
 drivers/net/ethernet/huawei/hinic/hinic_rx.c | 23 +++++---------------
 drivers/net/ethernet/huawei/hinic/hinic_rx.h |  2 --
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.c b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
index f86f2e693224..0098b206e7e9 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_rx.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
@@ -43,6 +43,7 @@
 #define RX_IRQ_NO_LLI_TIMER             0
 #define RX_IRQ_NO_CREDIT                0
 #define RX_IRQ_NO_RESEND_TIMER          0
+#define HINIC_RX_BUFFER_WRITE           16
 
 /**
  * hinic_rxq_clean_stats - Clean the statistics of specific queue
@@ -229,7 +230,6 @@ static int rx_alloc_pkts(struct hinic_rxq *rxq)
 		wmb();  /* write all the wqes before update PI */
 
 		hinic_rq_update(rxq->rq, prod_idx);
-		tasklet_schedule(&rxq->rx_task);
 	}
 
 	return i;
@@ -258,17 +258,6 @@ static void free_all_rx_skbs(struct hinic_rxq *rxq)
 	}
 }
 
-/**
- * rx_alloc_task - tasklet for queue allocation
- * @data: rx queue
- **/
-static void rx_alloc_task(unsigned long data)
-{
-	struct hinic_rxq *rxq = (struct hinic_rxq *)data;
-
-	(void)rx_alloc_pkts(rxq);
-}
-
 /**
  * rx_recv_jumbo_pkt - Rx handler for jumbo pkt
  * @rxq: rx queue
@@ -333,6 +322,7 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
 	struct hinic_qp *qp = container_of(rxq->rq, struct hinic_qp, rq);
 	u64 pkt_len = 0, rx_bytes = 0;
 	struct hinic_rq_wqe *rq_wqe;
+	unsigned int free_wqebbs;
 	int num_wqes, pkts = 0;
 	struct hinic_sge sge;
 	struct sk_buff *skb;
@@ -376,8 +366,9 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
 		rx_bytes += pkt_len;
 	}
 
-	if (pkts)
-		tasklet_schedule(&rxq->rx_task); /* rx_alloc_pkts */
+	free_wqebbs = hinic_get_rq_free_wqebbs(rxq->rq);
+	if (free_wqebbs > HINIC_RX_BUFFER_WRITE)
+		rx_alloc_pkts(rxq);
 
 	u64_stats_update_begin(&rxq->rxq_stats.syncp);
 	rxq->rxq_stats.pkts += pkts;
@@ -494,8 +485,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
 
 	sprintf(rxq->irq_name, "hinic_rxq%d", qp->q_id);
 
-	tasklet_init(&rxq->rx_task, rx_alloc_task, (unsigned long)rxq);
-
 	pkts = rx_alloc_pkts(rxq);
 	if (!pkts) {
 		err = -ENOMEM;
@@ -512,7 +501,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
 
 err_req_rx_irq:
 err_rx_pkts:
-	tasklet_kill(&rxq->rx_task);
 	free_all_rx_skbs(rxq);
 	devm_kfree(&netdev->dev, rxq->irq_name);
 	return err;
@@ -528,7 +516,6 @@ void hinic_clean_rxq(struct hinic_rxq *rxq)
 
 	rx_free_irq(rxq);
 
-	tasklet_kill(&rxq->rx_task);
 	free_all_rx_skbs(rxq);
 	devm_kfree(&netdev->dev, rxq->irq_name);
 }
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.h b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
index ab3fabab91b2..f8ed3fa6c8ee 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_rx.h
+++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
@@ -42,8 +42,6 @@ struct hinic_rxq {
 
 	char                    *irq_name;
 
-	struct tasklet_struct   rx_task;
-
 	struct napi_struct      napi;
 };
 
-- 
2.17.1


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

* Re: [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism
  2018-12-12 17:40 [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism Xue Chaojing
@ 2018-12-13 14:31 ` Neil Horman
  0 siblings, 0 replies; 7+ messages in thread
From: Neil Horman @ 2018-12-13 14:31 UTC (permalink / raw)
  To: Xue Chaojing
  Cc: davem, linux-kernel, netdev, wulike1, chiqijun, fy.wang, tony.qu,
	luoshaokai

On Wed, Dec 12, 2018 at 05:40:23PM +0000, Xue Chaojing wrote:
> There is no need to schedule a different tasklet for refill,
> This patch remove it.
> 
> Suggested-by: Neil Horman <nhorman@redhat.com>
> Signed-off-by: Xue Chaojing <xuechaojing@huawei.com>
> ---
>  drivers/net/ethernet/huawei/hinic/hinic_rx.c | 23 +++++---------------
>  drivers/net/ethernet/huawei/hinic/hinic_rx.h |  2 --
>  2 files changed, 5 insertions(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.c b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
> index f86f2e693224..0098b206e7e9 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_rx.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
> @@ -43,6 +43,7 @@
>  #define RX_IRQ_NO_LLI_TIMER             0
>  #define RX_IRQ_NO_CREDIT                0
>  #define RX_IRQ_NO_RESEND_TIMER          0
> +#define HINIC_RX_BUFFER_WRITE           16
>  
>  /**
>   * hinic_rxq_clean_stats - Clean the statistics of specific queue
> @@ -229,7 +230,6 @@ static int rx_alloc_pkts(struct hinic_rxq *rxq)
>  		wmb();  /* write all the wqes before update PI */
>  
>  		hinic_rq_update(rxq->rq, prod_idx);
> -		tasklet_schedule(&rxq->rx_task);
>  	}
>  
>  	return i;
> @@ -258,17 +258,6 @@ static void free_all_rx_skbs(struct hinic_rxq *rxq)
>  	}
>  }
>  
> -/**
> - * rx_alloc_task - tasklet for queue allocation
> - * @data: rx queue
> - **/
> -static void rx_alloc_task(unsigned long data)
> -{
> -	struct hinic_rxq *rxq = (struct hinic_rxq *)data;
> -
> -	(void)rx_alloc_pkts(rxq);
> -}
> -
>  /**
>   * rx_recv_jumbo_pkt - Rx handler for jumbo pkt
>   * @rxq: rx queue
> @@ -333,6 +322,7 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
>  	struct hinic_qp *qp = container_of(rxq->rq, struct hinic_qp, rq);
>  	u64 pkt_len = 0, rx_bytes = 0;
>  	struct hinic_rq_wqe *rq_wqe;
> +	unsigned int free_wqebbs;
>  	int num_wqes, pkts = 0;
>  	struct hinic_sge sge;
>  	struct sk_buff *skb;
> @@ -376,8 +366,9 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
>  		rx_bytes += pkt_len;
>  	}
>  
> -	if (pkts)
> -		tasklet_schedule(&rxq->rx_task); /* rx_alloc_pkts */
> +	free_wqebbs = hinic_get_rq_free_wqebbs(rxq->rq);
> +	if (free_wqebbs > HINIC_RX_BUFFER_WRITE)
> +		rx_alloc_pkts(rxq);
>  
>  	u64_stats_update_begin(&rxq->rxq_stats.syncp);
>  	rxq->rxq_stats.pkts += pkts;
> @@ -494,8 +485,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
>  
>  	sprintf(rxq->irq_name, "hinic_rxq%d", qp->q_id);
>  
> -	tasklet_init(&rxq->rx_task, rx_alloc_task, (unsigned long)rxq);
> -
>  	pkts = rx_alloc_pkts(rxq);
>  	if (!pkts) {
>  		err = -ENOMEM;
> @@ -512,7 +501,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
>  
>  err_req_rx_irq:
>  err_rx_pkts:
> -	tasklet_kill(&rxq->rx_task);
>  	free_all_rx_skbs(rxq);
>  	devm_kfree(&netdev->dev, rxq->irq_name);
>  	return err;
> @@ -528,7 +516,6 @@ void hinic_clean_rxq(struct hinic_rxq *rxq)
>  
>  	rx_free_irq(rxq);
>  
> -	tasklet_kill(&rxq->rx_task);
>  	free_all_rx_skbs(rxq);
>  	devm_kfree(&netdev->dev, rxq->irq_name);
>  }
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.h b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
> index ab3fabab91b2..f8ed3fa6c8ee 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_rx.h
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
> @@ -42,8 +42,6 @@ struct hinic_rxq {
>  
>  	char                    *irq_name;
>  
> -	struct tasklet_struct   rx_task;
> -
>  	struct napi_struct      napi;
>  };
>  
> -- 
> 2.17.1
> 
I like that you're getting rid of the extra tasklet, but the other part of this
is properly refilling your rx ring.  The way you have this coded, you always
blindly just receive the incomming frame, even if your refill operation fails.
If you get a long enough period in which you are memory constrained, you will
wind up with an empty rx ring, which isn't good.  With this patch, if your ring
becomes empty, then you will stop receiving frames (no buffers to put them in),
which in turn will prevent further attempts to refill the ring.  The result is
effectively a hang in traffic reception that is only solveable by a NIC reset.

Common practice is to, for each skb that you intend to receive:

1) Allocate a replacement buffer/skb
2a) If allocation succedes, receive the buffer currently on the ring, and
replace it with the buffer from (1)
2b) If allocation fails, record a frame drop, mark the existing buffer as clean,
and move on

This process ensures that the ring never has any gaps in it, preventing the
above hang condition.

Neil
 



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

* Re: [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism
  2018-12-16 22:32 Xue Chaojing
  2018-12-17 17:02 ` Neil Horman
@ 2018-12-17 19:13 ` David Miller
  1 sibling, 0 replies; 7+ messages in thread
From: David Miller @ 2018-12-17 19:13 UTC (permalink / raw)
  To: xuechaojing
  Cc: linux-kernel, netdev, wulike1, chiqijun, fy.wang, tony.qu,
	luoshaokai, nhorman

From: Xue Chaojing <xuechaojing@huawei.com>
Date: Sun, 16 Dec 2018 22:32:34 +0000

> In rx_alloc_pkts(), there is no need to schedule a different tasklet for
> refill and it will cause some extra overhead. this patch remove it.
> 
> Suggested-by: Neil Horman <nhorman@redhat.com>
> Signed-off-by: Xue Chaojing <xuechaojing@huawei.com>

In future submissions please formation your Subject line properly.

First, there should always be a space character after the ":" following
the subsystem prefix.

Next, "net-next" belongs inside of the '[]' so "[PATCH net-next N/M] ".

However, when you have a single patch, there is no reason to give the
"N/M" part, so you can just drop it in this situation.

Please also capitalize the text after the subsystem prefix string.

Therefore this Subject must be formatted exactly like this:

	Subject: "[PATCH net-next] hinic: Optimize RX refill buffer mechanism"

But meanwhile you must address the feedback given to you by Neil and myself.

You absolutely MUST NOT receive a RX packet if you cannot successfully
allocate a replacement buffer for the RX ring in that slot.  It allows
the RX ring to become empty, which many cards respond to by hanging.

It would really serve you poorly if you continue to ignore our
feedback because addressing it is a requirement before we will be able
to consider integrating your work.

Thank you.


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

* Re: [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism
  2018-12-16 22:32 Xue Chaojing
@ 2018-12-17 17:02 ` Neil Horman
  2018-12-17 19:13 ` David Miller
  1 sibling, 0 replies; 7+ messages in thread
From: Neil Horman @ 2018-12-17 17:02 UTC (permalink / raw)
  To: Xue Chaojing
  Cc: davem, linux-kernel, netdev, wulike1, chiqijun, fy.wang, tony.qu,
	luoshaokai

On Sun, Dec 16, 2018 at 10:32:34PM +0000, Xue Chaojing wrote:
> In rx_alloc_pkts(), there is no need to schedule a different tasklet for
> refill and it will cause some extra overhead. this patch remove it.
> 
> Suggested-by: Neil Horman <nhorman@redhat.com>
> Signed-off-by: Xue Chaojing <xuechaojing@huawei.com>
> ---
>  drivers/net/ethernet/huawei/hinic/hinic_rx.c | 23 +++++---------------
>  drivers/net/ethernet/huawei/hinic/hinic_rx.h |  2 --
>  2 files changed, 5 insertions(+), 20 deletions(-)
> 
I thought I had responded to this already, but this still looks like the same
patch.  While I'm supportive of the removal of the second tasklet (as its not
needed), this patch still doesn't address the holes that can result in your rx
ring buffer.  By always receiving a frame , and then just filling as many of the
remaining buffers as possible, you open yourself to the possibility that, in low
memory conditions, you will wind up with an empty rx ring that will result in
hanging your NIC.  You need to, for every recevied frame, pre-allocate a
replacement buffer, and, should that allocation fail, return the received frame,
to the ring, recording a drop in the process.  That way your ring buffer will
never have holes in it.

Neil
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.c b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
> index f86f2e693224..0098b206e7e9 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_rx.c
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
> @@ -43,6 +43,7 @@
>  #define RX_IRQ_NO_LLI_TIMER             0
>  #define RX_IRQ_NO_CREDIT                0
>  #define RX_IRQ_NO_RESEND_TIMER          0
> +#define HINIC_RX_BUFFER_WRITE           16
>  
>  /**
>   * hinic_rxq_clean_stats - Clean the statistics of specific queue
> @@ -229,7 +230,6 @@ static int rx_alloc_pkts(struct hinic_rxq *rxq)
>  		wmb();  /* write all the wqes before update PI */
>  
>  		hinic_rq_update(rxq->rq, prod_idx);
> -		tasklet_schedule(&rxq->rx_task);
>  	}
>  
>  	return i;
> @@ -258,17 +258,6 @@ static void free_all_rx_skbs(struct hinic_rxq *rxq)
>  	}
>  }
>  
> -/**
> - * rx_alloc_task - tasklet for queue allocation
> - * @data: rx queue
> - **/
> -static void rx_alloc_task(unsigned long data)
> -{
> -	struct hinic_rxq *rxq = (struct hinic_rxq *)data;
> -
> -	(void)rx_alloc_pkts(rxq);
> -}
> -
>  /**
>   * rx_recv_jumbo_pkt - Rx handler for jumbo pkt
>   * @rxq: rx queue
> @@ -333,6 +322,7 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
>  	struct hinic_qp *qp = container_of(rxq->rq, struct hinic_qp, rq);
>  	u64 pkt_len = 0, rx_bytes = 0;
>  	struct hinic_rq_wqe *rq_wqe;
> +	unsigned int free_wqebbs;
>  	int num_wqes, pkts = 0;
>  	struct hinic_sge sge;
>  	struct sk_buff *skb;
> @@ -376,8 +366,9 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
>  		rx_bytes += pkt_len;
>  	}
>  
> -	if (pkts)
> -		tasklet_schedule(&rxq->rx_task); /* rx_alloc_pkts */
> +	free_wqebbs = hinic_get_rq_free_wqebbs(rxq->rq);
> +	if (free_wqebbs > HINIC_RX_BUFFER_WRITE)
> +		rx_alloc_pkts(rxq);
>  
>  	u64_stats_update_begin(&rxq->rxq_stats.syncp);
>  	rxq->rxq_stats.pkts += pkts;
> @@ -494,8 +485,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
>  
>  	sprintf(rxq->irq_name, "hinic_rxq%d", qp->q_id);
>  
> -	tasklet_init(&rxq->rx_task, rx_alloc_task, (unsigned long)rxq);
> -
>  	pkts = rx_alloc_pkts(rxq);
>  	if (!pkts) {
>  		err = -ENOMEM;
> @@ -512,7 +501,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
>  
>  err_req_rx_irq:
>  err_rx_pkts:
> -	tasklet_kill(&rxq->rx_task);
>  	free_all_rx_skbs(rxq);
>  	devm_kfree(&netdev->dev, rxq->irq_name);
>  	return err;
> @@ -528,7 +516,6 @@ void hinic_clean_rxq(struct hinic_rxq *rxq)
>  
>  	rx_free_irq(rxq);
>  
> -	tasklet_kill(&rxq->rx_task);
>  	free_all_rx_skbs(rxq);
>  	devm_kfree(&netdev->dev, rxq->irq_name);
>  }
> diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.h b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
> index ab3fabab91b2..f8ed3fa6c8ee 100644
> --- a/drivers/net/ethernet/huawei/hinic/hinic_rx.h
> +++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
> @@ -42,8 +42,6 @@ struct hinic_rxq {
>  
>  	char                    *irq_name;
>  
> -	struct tasklet_struct   rx_task;
> -
>  	struct napi_struct      napi;
>  };
>  
> -- 
> 2.17.1
> 

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

* [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism
@ 2018-12-16 22:32 Xue Chaojing
  2018-12-17 17:02 ` Neil Horman
  2018-12-17 19:13 ` David Miller
  0 siblings, 2 replies; 7+ messages in thread
From: Xue Chaojing @ 2018-12-16 22:32 UTC (permalink / raw)
  To: davem
  Cc: linux-kernel, xuechaojing, netdev, wulike1, chiqijun, fy.wang,
	tony.qu, luoshaokai, nhorman

In rx_alloc_pkts(), there is no need to schedule a different tasklet for
refill and it will cause some extra overhead. this patch remove it.

Suggested-by: Neil Horman <nhorman@redhat.com>
Signed-off-by: Xue Chaojing <xuechaojing@huawei.com>
---
 drivers/net/ethernet/huawei/hinic/hinic_rx.c | 23 +++++---------------
 drivers/net/ethernet/huawei/hinic/hinic_rx.h |  2 --
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.c b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
index f86f2e693224..0098b206e7e9 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_rx.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
@@ -43,6 +43,7 @@
 #define RX_IRQ_NO_LLI_TIMER             0
 #define RX_IRQ_NO_CREDIT                0
 #define RX_IRQ_NO_RESEND_TIMER          0
+#define HINIC_RX_BUFFER_WRITE           16
 
 /**
  * hinic_rxq_clean_stats - Clean the statistics of specific queue
@@ -229,7 +230,6 @@ static int rx_alloc_pkts(struct hinic_rxq *rxq)
 		wmb();  /* write all the wqes before update PI */
 
 		hinic_rq_update(rxq->rq, prod_idx);
-		tasklet_schedule(&rxq->rx_task);
 	}
 
 	return i;
@@ -258,17 +258,6 @@ static void free_all_rx_skbs(struct hinic_rxq *rxq)
 	}
 }
 
-/**
- * rx_alloc_task - tasklet for queue allocation
- * @data: rx queue
- **/
-static void rx_alloc_task(unsigned long data)
-{
-	struct hinic_rxq *rxq = (struct hinic_rxq *)data;
-
-	(void)rx_alloc_pkts(rxq);
-}
-
 /**
  * rx_recv_jumbo_pkt - Rx handler for jumbo pkt
  * @rxq: rx queue
@@ -333,6 +322,7 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
 	struct hinic_qp *qp = container_of(rxq->rq, struct hinic_qp, rq);
 	u64 pkt_len = 0, rx_bytes = 0;
 	struct hinic_rq_wqe *rq_wqe;
+	unsigned int free_wqebbs;
 	int num_wqes, pkts = 0;
 	struct hinic_sge sge;
 	struct sk_buff *skb;
@@ -376,8 +366,9 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
 		rx_bytes += pkt_len;
 	}
 
-	if (pkts)
-		tasklet_schedule(&rxq->rx_task); /* rx_alloc_pkts */
+	free_wqebbs = hinic_get_rq_free_wqebbs(rxq->rq);
+	if (free_wqebbs > HINIC_RX_BUFFER_WRITE)
+		rx_alloc_pkts(rxq);
 
 	u64_stats_update_begin(&rxq->rxq_stats.syncp);
 	rxq->rxq_stats.pkts += pkts;
@@ -494,8 +485,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
 
 	sprintf(rxq->irq_name, "hinic_rxq%d", qp->q_id);
 
-	tasklet_init(&rxq->rx_task, rx_alloc_task, (unsigned long)rxq);
-
 	pkts = rx_alloc_pkts(rxq);
 	if (!pkts) {
 		err = -ENOMEM;
@@ -512,7 +501,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
 
 err_req_rx_irq:
 err_rx_pkts:
-	tasklet_kill(&rxq->rx_task);
 	free_all_rx_skbs(rxq);
 	devm_kfree(&netdev->dev, rxq->irq_name);
 	return err;
@@ -528,7 +516,6 @@ void hinic_clean_rxq(struct hinic_rxq *rxq)
 
 	rx_free_irq(rxq);
 
-	tasklet_kill(&rxq->rx_task);
 	free_all_rx_skbs(rxq);
 	devm_kfree(&netdev->dev, rxq->irq_name);
 }
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.h b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
index ab3fabab91b2..f8ed3fa6c8ee 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_rx.h
+++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
@@ -42,8 +42,6 @@ struct hinic_rxq {
 
 	char                    *irq_name;
 
-	struct tasklet_struct   rx_task;
-
 	struct napi_struct      napi;
 };
 
-- 
2.17.1


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

* Re: [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism
  2018-12-09 19:14 Xue Chaojing
@ 2018-12-15 19:31 ` David Miller
  0 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2018-12-15 19:31 UTC (permalink / raw)
  To: xuechaojing
  Cc: linux-kernel, netdev, wulike1, chiqijun, fy.wang, tony.qu, luoshaokai

From: Xue Chaojing <xuechaojing@huawei.com>
Date: Sun, 9 Dec 2018 19:14:19 +0000

> There is no need to schedule a different tasklet for refill,
> This patch remove it.
> 
> Suggested-by: Neil Horman <nhorman@redhat.com>
> Signed-off-by: Xue Chaojing <xuechaojing@huawei.com>

I completely agree with Neil's analysis.

You should never receive a packet if refilling the ring entry
resources fails.

It is so dangerous to allow the RX ring to reach an empty state,
not only does it mean that the device will stop receiving frames
but also this is probably one of the least tested conditions in
ring based networking designs and it proabably has bugs that hang
the chip and require a HW reset to recover from.


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

* [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism
@ 2018-12-09 19:14 Xue Chaojing
  2018-12-15 19:31 ` David Miller
  0 siblings, 1 reply; 7+ messages in thread
From: Xue Chaojing @ 2018-12-09 19:14 UTC (permalink / raw)
  To: davem
  Cc: linux-kernel, xuechaojing, netdev, wulike1, chiqijun, fy.wang,
	tony.qu, luoshaokai

There is no need to schedule a different tasklet for refill,
This patch remove it.

Suggested-by: Neil Horman <nhorman@redhat.com>
Signed-off-by: Xue Chaojing <xuechaojing@huawei.com>
---
 drivers/net/ethernet/huawei/hinic/hinic_rx.c | 23 +++++---------------
 drivers/net/ethernet/huawei/hinic/hinic_rx.h |  2 --
 2 files changed, 5 insertions(+), 20 deletions(-)

diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.c b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
index f86f2e693224..0098b206e7e9 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_rx.c
+++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.c
@@ -43,6 +43,7 @@
 #define RX_IRQ_NO_LLI_TIMER             0
 #define RX_IRQ_NO_CREDIT                0
 #define RX_IRQ_NO_RESEND_TIMER          0
+#define HINIC_RX_BUFFER_WRITE           16
 
 /**
  * hinic_rxq_clean_stats - Clean the statistics of specific queue
@@ -229,7 +230,6 @@ static int rx_alloc_pkts(struct hinic_rxq *rxq)
 		wmb();  /* write all the wqes before update PI */
 
 		hinic_rq_update(rxq->rq, prod_idx);
-		tasklet_schedule(&rxq->rx_task);
 	}
 
 	return i;
@@ -258,17 +258,6 @@ static void free_all_rx_skbs(struct hinic_rxq *rxq)
 	}
 }
 
-/**
- * rx_alloc_task - tasklet for queue allocation
- * @data: rx queue
- **/
-static void rx_alloc_task(unsigned long data)
-{
-	struct hinic_rxq *rxq = (struct hinic_rxq *)data;
-
-	(void)rx_alloc_pkts(rxq);
-}
-
 /**
  * rx_recv_jumbo_pkt - Rx handler for jumbo pkt
  * @rxq: rx queue
@@ -333,6 +322,7 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
 	struct hinic_qp *qp = container_of(rxq->rq, struct hinic_qp, rq);
 	u64 pkt_len = 0, rx_bytes = 0;
 	struct hinic_rq_wqe *rq_wqe;
+	unsigned int free_wqebbs;
 	int num_wqes, pkts = 0;
 	struct hinic_sge sge;
 	struct sk_buff *skb;
@@ -376,8 +366,9 @@ static int rxq_recv(struct hinic_rxq *rxq, int budget)
 		rx_bytes += pkt_len;
 	}
 
-	if (pkts)
-		tasklet_schedule(&rxq->rx_task); /* rx_alloc_pkts */
+	free_wqebbs = hinic_get_rq_free_wqebbs(rxq->rq);
+	if (free_wqebbs > HINIC_RX_BUFFER_WRITE)
+		rx_alloc_pkts(rxq);
 
 	u64_stats_update_begin(&rxq->rxq_stats.syncp);
 	rxq->rxq_stats.pkts += pkts;
@@ -494,8 +485,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
 
 	sprintf(rxq->irq_name, "hinic_rxq%d", qp->q_id);
 
-	tasklet_init(&rxq->rx_task, rx_alloc_task, (unsigned long)rxq);
-
 	pkts = rx_alloc_pkts(rxq);
 	if (!pkts) {
 		err = -ENOMEM;
@@ -512,7 +501,6 @@ int hinic_init_rxq(struct hinic_rxq *rxq, struct hinic_rq *rq,
 
 err_req_rx_irq:
 err_rx_pkts:
-	tasklet_kill(&rxq->rx_task);
 	free_all_rx_skbs(rxq);
 	devm_kfree(&netdev->dev, rxq->irq_name);
 	return err;
@@ -528,7 +516,6 @@ void hinic_clean_rxq(struct hinic_rxq *rxq)
 
 	rx_free_irq(rxq);
 
-	tasklet_kill(&rxq->rx_task);
 	free_all_rx_skbs(rxq);
 	devm_kfree(&netdev->dev, rxq->irq_name);
 }
diff --git a/drivers/net/ethernet/huawei/hinic/hinic_rx.h b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
index ab3fabab91b2..f8ed3fa6c8ee 100644
--- a/drivers/net/ethernet/huawei/hinic/hinic_rx.h
+++ b/drivers/net/ethernet/huawei/hinic/hinic_rx.h
@@ -42,8 +42,6 @@ struct hinic_rxq {
 
 	char                    *irq_name;
 
-	struct tasklet_struct   rx_task;
-
 	struct napi_struct      napi;
 };
 
-- 
2.17.1


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

end of thread, other threads:[~2018-12-17 19:13 UTC | newest]

Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-12 17:40 [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism Xue Chaojing
2018-12-13 14:31 ` Neil Horman
  -- strict thread matches above, loose matches on Subject: below --
2018-12-16 22:32 Xue Chaojing
2018-12-17 17:02 ` Neil Horman
2018-12-17 19:13 ` David Miller
2018-12-09 19:14 Xue Chaojing
2018-12-15 19:31 ` David Miller

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.