linux-kernel - Re: [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 17 Dec 2018 12:02:41 -0500
From:   Neil Horman <nhorman@...hat.com>
To:     Xue Chaojing <xuechaojing@...wei.com>
Cc:     davem@...emloft.net, linux-kernel@...r.kernel.org,
        netdev@...r.kernel.org, wulike1@...wei.com, chiqijun@...wei.com,
        fy.wang@...wei.com, tony.qu@...wei.com, luoshaokai@...wei.com
Subject: Re: [PATCH 1/1] net-next/hinic:optmize rx refill buffer mechanism

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@...hat.com>
> Signed-off-by: Xue Chaojing <xuechaojing@...wei.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
>