This is due to error in over budget processing.
When dealing with high throughput, the used buffers
that exceeds the budget is not cleaned up. In addition,
it takes a lot of cycles to clean up the used buffer,
and then the buffer where the valid data is located can take effect.
Signed-off-by: Jiangfeng Xiao <[email protected]>
---
drivers/net/ethernet/hisilicon/hip04_eth.c | 14 ++++++++------
1 file changed, 8 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
index ad6d912..78f338a 100644
--- a/drivers/net/ethernet/hisilicon/hip04_eth.c
+++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
@@ -575,7 +575,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
struct net_device *ndev = priv->ndev;
struct net_device_stats *stats = &ndev->stats;
- unsigned int cnt = hip04_recv_cnt(priv);
+ static unsigned int cnt_remaining;
struct rx_desc *desc;
struct sk_buff *skb;
unsigned char *buf;
@@ -588,8 +588,8 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
/* clean up tx descriptors */
tx_remaining = hip04_tx_reclaim(ndev, false);
-
- while (cnt && !last) {
+ cnt_remaining += hip04_recv_cnt(priv);
+ while (cnt_remaining && !last) {
buf = priv->rx_buf[priv->rx_head];
skb = build_skb(buf, priv->rx_buf_size);
if (unlikely(!skb)) {
@@ -635,11 +635,13 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
hip04_set_recv_desc(priv, phys);
priv->rx_head = RX_NEXT(priv->rx_head);
- if (rx >= budget)
+ if (rx >= budget) {
+ --cnt_remaining;
goto done;
+ }
- if (--cnt == 0)
- cnt = hip04_recv_cnt(priv);
+ if (--cnt_remaining == 0)
+ cnt_remaining += hip04_recv_cnt(priv);
}
if (!(priv->reg_inten & RCV_INT)) {
--
1.8.5.6
On Sat, 2019-10-26 at 16:49 +0800, Jiangfeng Xiao wrote:
> This is due to error in over budget processing.
> When dealing with high throughput, the used buffers
> that exceeds the budget is not cleaned up. In addition,
> it takes a lot of cycles to clean up the used buffer,
> and then the buffer where the valid data is located can take effect.
[]
> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
[]
> @@ -575,7 +575,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
> struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
> struct net_device *ndev = priv->ndev;
> struct net_device_stats *stats = &ndev->stats;
> - unsigned int cnt = hip04_recv_cnt(priv);
> + static unsigned int cnt_remaining;
static doesn't seem a great idea here as it's for just a single
driver instance. Maybe make this part of struct hip04_priv?
From: Jiangfeng Xiao <[email protected]>
Date: Sat, 26 Oct 2019 16:49:39 +0800
> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> index ad6d912..78f338a 100644
> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
> @@ -575,7 +575,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
> struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
> struct net_device *ndev = priv->ndev;
> struct net_device_stats *stats = &ndev->stats;
> - unsigned int cnt = hip04_recv_cnt(priv);
> + static unsigned int cnt_remaining;
There is no way a piece of software state should be system wide, this is
a per device instance value.
On 2019/10/26 23:44, Joe Perches wrote:
> On Sat, 2019-10-26 at 16:49 +0800, Jiangfeng Xiao wrote:
>> This is due to error in over budget processing.
>> When dealing with high throughput, the used buffers
>> that exceeds the budget is not cleaned up. In addition,
>> it takes a lot of cycles to clean up the used buffer,
>> and then the buffer where the valid data is located can take effect.
> []
>> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
> []
>> @@ -575,7 +575,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>> struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
>> struct net_device *ndev = priv->ndev;
>> struct net_device_stats *stats = &ndev->stats;
>> - unsigned int cnt = hip04_recv_cnt(priv);
>> + static unsigned int cnt_remaining;
>
> static doesn't seem a great idea here as it's for just a single
> driver instance. Maybe make this part of struct hip04_priv?
>
Thank you for your review. Your suggestion is very good.
On 2019/10/27 2:22, David Miller wrote:
> From: Jiangfeng Xiao <[email protected]>
> Date: Sat, 26 Oct 2019 16:49:39 +0800
>
>> diff --git a/drivers/net/ethernet/hisilicon/hip04_eth.c b/drivers/net/ethernet/hisilicon/hip04_eth.c
>> index ad6d912..78f338a 100644
>> --- a/drivers/net/ethernet/hisilicon/hip04_eth.c
>> +++ b/drivers/net/ethernet/hisilicon/hip04_eth.c
>> @@ -575,7 +575,7 @@ static int hip04_rx_poll(struct napi_struct *napi, int budget)
>> struct hip04_priv *priv = container_of(napi, struct hip04_priv, napi);
>> struct net_device *ndev = priv->ndev;
>> struct net_device_stats *stats = &ndev->stats;
>> - unsigned int cnt = hip04_recv_cnt(priv);
>> + static unsigned int cnt_remaining;
>
> There is no way a piece of software state should be system wide, this is
> a per device instance value.
>
> .
>
Thank you for your guidance, I will fix it in v2.