netdev - Re: [PATCH net-next 2/2] rtase: Link queues to NAPI instances

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]

Message-ID: <aEmM6D9DKxxiarSP@MacBook-Air.local>
Date: Wed, 11 Jun 2025 17:04:24 +0300
From: Joe Damato <joe@...a.to>
To: Justin Lai <justinlai0215@...ltek.com>
Cc: kuba@...nel.org, davem@...emloft.net, edumazet@...gle.com,
	pabeni@...hat.com, andrew+netdev@...n.ch,
	linux-kernel@...r.kernel.org, netdev@...r.kernel.org,
	horms@...nel.org, jdamato@...tly.com, pkshih@...ltek.com,
	larry.chiu@...ltek.com
Subject: Re: [PATCH net-next 2/2] rtase: Link queues to NAPI instances

On Tue, Jun 10, 2025 at 06:33:34PM +0800, Justin Lai wrote:
> Link queues to NAPI instances with netif_queue_set_napi. This
> information can be queried with the netdev-genl API.
> 
> Signed-off-by: Justin Lai <justinlai0215@...ltek.com>
> ---
>  drivers/net/ethernet/realtek/rtase/rtase.h    |  4 +++
>  .../net/ethernet/realtek/rtase/rtase_main.c   | 33 +++++++++++++++++--
>  2 files changed, 35 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/ethernet/realtek/rtase/rtase.h b/drivers/net/ethernet/realtek/rtase/rtase.h
> index 498cfe4d0cac..be98f4de46c4 100644
> --- a/drivers/net/ethernet/realtek/rtase/rtase.h
> +++ b/drivers/net/ethernet/realtek/rtase/rtase.h
> @@ -39,6 +39,9 @@
>  #define RTASE_FUNC_RXQ_NUM  1
>  #define RTASE_INTERRUPT_NUM 1
>  
> +#define RTASE_TX_RING 0
> +#define RTASE_RX_RING 1
> +
>  #define RTASE_MITI_TIME_COUNT_MASK    GENMASK(3, 0)
>  #define RTASE_MITI_TIME_UNIT_MASK     GENMASK(7, 4)
>  #define RTASE_MITI_DEFAULT_TIME       128
> @@ -288,6 +291,7 @@ struct rtase_ring {
>  	u32 cur_idx;
>  	u32 dirty_idx;
>  	u16 index;
> +	u8 ring_type;

Two questions:

1. why not use enum netdev_queue_type instead of making driver specific
values that are the opposite of the existing enum values ?

If you used the existing enum, you could omit the if below (see below), as
well?

2. is "ring" in the name redundant? maybe just "type"? asking because below
the code becomes "ring->ring_type" and maybe "ring->type" is better?

>  	struct sk_buff *skbuff[RTASE_NUM_DESC];
>  	void *data_buf[RTASE_NUM_DESC];
> diff --git a/drivers/net/ethernet/realtek/rtase/rtase_main.c b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> index a88af868da8c..ef3ada91d555 100644
> --- a/drivers/net/ethernet/realtek/rtase/rtase_main.c
> +++ b/drivers/net/ethernet/realtek/rtase/rtase_main.c
> @@ -326,6 +326,7 @@ static void rtase_tx_desc_init(struct rtase_private *tp, u16 idx)
>  	ring->cur_idx = 0;
>  	ring->dirty_idx = 0;
>  	ring->index = idx;
> +	ring->ring_type = RTASE_TX_RING;
>  	ring->alloc_fail = 0;
>  
>  	for (i = 0; i < RTASE_NUM_DESC; i++) {
> @@ -345,6 +346,10 @@ static void rtase_tx_desc_init(struct rtase_private *tp, u16 idx)
>  		ring->ivec = &tp->int_vector[0];
>  		list_add_tail(&ring->ring_entry, &tp->int_vector[0].ring_list);
>  	}
> +
> +	netif_queue_set_napi(tp->dev, ring->index,
> +			     NETDEV_QUEUE_TYPE_TX,
> +			     &ring->ivec->napi);
>  }
>  
>  static void rtase_map_to_asic(union rtase_rx_desc *desc, dma_addr_t mapping,
> @@ -590,6 +595,7 @@ static void rtase_rx_desc_init(struct rtase_private *tp, u16 idx)
>  	ring->cur_idx = 0;
>  	ring->dirty_idx = 0;
>  	ring->index = idx;
> +	ring->ring_type = RTASE_RX_RING;
>  	ring->alloc_fail = 0;
>  
>  	for (i = 0; i < RTASE_NUM_DESC; i++)
> @@ -597,6 +603,9 @@ static void rtase_rx_desc_init(struct rtase_private *tp, u16 idx)
>  
>  	ring->ring_handler = rx_handler;
>  	ring->ivec = &tp->int_vector[idx];
> +	netif_queue_set_napi(tp->dev, ring->index,
> +			     NETDEV_QUEUE_TYPE_RX,
> +			     &ring->ivec->napi);
>  	list_add_tail(&ring->ring_entry, &tp->int_vector[idx].ring_list);
>  }
>  
> @@ -1161,8 +1170,18 @@ static void rtase_down(struct net_device *dev)
>  		ivec = &tp->int_vector[i];
>  		napi_disable(&ivec->napi);
>  		list_for_each_entry_safe(ring, tmp, &ivec->ring_list,
> -					 ring_entry)
> +					 ring_entry) {
> +			if (ring->ring_type == RTASE_TX_RING)
> +				netif_queue_set_napi(tp->dev, ring->index,
> +						     NETDEV_QUEUE_TYPE_TX,
> +						     NULL);
> +			else
> +				netif_queue_set_napi(tp->dev, ring->index,
> +						     NETDEV_QUEUE_TYPE_RX,
> +						     NULL);
> +

Using the existing enum would simplify this block?

  netif_queue_set_napi(tp->dev, ring->index, ring->type, NULL);

>  			list_del(&ring->ring_entry);
> +		}
>  	}
>  
>  	netif_tx_disable(dev);
> @@ -1518,8 +1537,18 @@ static void rtase_sw_reset(struct net_device *dev)
>  	for (i = 0; i < tp->int_nums; i++) {
>  		ivec = &tp->int_vector[i];
>  		list_for_each_entry_safe(ring, tmp, &ivec->ring_list,
> -					 ring_entry)
> +					 ring_entry) {
> +			if (ring->ring_type == RTASE_TX_RING)
> +				netif_queue_set_napi(tp->dev, ring->index,
> +						     NETDEV_QUEUE_TYPE_TX,
> +						     NULL);
> +			else
> +				netif_queue_set_napi(tp->dev, ring->index,
> +						     NETDEV_QUEUE_TYPE_RX,
> +						     NULL);
> +

Same as above?