diff mbox series

[net-next,RFC,v3,1/7] net: ravb: Simplify poll & receive functions

Message ID 20240415094804.8016-2-paul.barker.ct@bp.renesas.com (mailing list archive)
State RFC
Delegated to: Netdev Maintainers
Headers show
Series Improve GbEth performance on Renesas RZ/G2L and related SoCs | expand

Checks

Context Check Description
netdev/tree_selection success Clearly marked for net-next
netdev/apply fail Patch does not apply to net-next-1

Commit Message

Paul Barker April 15, 2024, 9:47 a.m. UTC 
We don't need to pass the work budget to ravb_rx() by reference, it's
cleaner to pass this by value and return the amount of work done. This
allows us to simplify the ravb_poll() function and use the common
`work_done` variable name seen in other network drivers for consistency
and ease of understanding.

This is a pure refactor and should not affect behaviour.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb.h      |  2 +-
 drivers/net/ethernet/renesas/ravb_main.c | 29 ++++++++++--------------
 2 files changed, 13 insertions(+), 18 deletions(-)

Comments

Niklas Söderlund April 15, 2024, 11:23 a.m. UTC | #1 
Hi Paul,

I really like this patch!

One nit below.

On 2024-04-15 10:47:58 +0100, Paul Barker wrote:
> We don't need to pass the work budget to ravb_rx() by reference, it's
> cleaner to pass this by value and return the amount of work done. This
> allows us to simplify the ravb_poll() function and use the common
> `work_done` variable name seen in other network drivers for consistency
> and ease of understanding.
> 
> This is a pure refactor and should not affect behaviour.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb.h      |  2 +-
>  drivers/net/ethernet/renesas/ravb_main.c | 29 ++++++++++--------------
>  2 files changed, 13 insertions(+), 18 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
> index b48935ec7e28..71de2a7aa27c 100644
> --- a/drivers/net/ethernet/renesas/ravb.h
> +++ b/drivers/net/ethernet/renesas/ravb.h
> @@ -1039,7 +1039,7 @@ struct ravb_ptp {
>  };
>  
>  struct ravb_hw_info {
> -	bool (*receive)(struct net_device *ndev, int *quota, int q);
> +	int (*receive)(struct net_device *ndev, int budget, int q);
>  	void (*set_rate)(struct net_device *ndev);
>  	int (*set_feature)(struct net_device *ndev, netdev_features_t features);
>  	int (*dmac_init)(struct net_device *ndev);
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index fd2679ce4e3d..33f8043143c1 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -759,7 +759,7 @@ static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
>  }
>  
>  /* Packet receive function for Gigabit Ethernet */
> -static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
> +static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *info = priv->info;
> @@ -778,7 +778,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>  	stats = &priv->stats[q];
>  
> -	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> +	for (i = 0; i < limit && rx_packets < budget; i++, priv->cur_rx[q]++) {
>  		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>  		desc = &priv->rx_ring[q].desc[entry];
>  		if (desc->die_dt == DT_FEMPTY)
> @@ -881,13 +881,11 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>  		desc->die_dt = DT_FEMPTY;
>  	}
>  
> -	stats->rx_packets += rx_packets;
> -	*quota -= rx_packets;
> -	return *quota == 0;
> +	return rx_packets;
>  }
>  
>  /* Packet receive function for Ethernet AVB */
> -static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
> +static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *info = priv->info;
> @@ -904,7 +902,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  	int i;
>  
>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
> -	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
> +	for (i = 0; i < limit && rx_packets < budget; i++, priv->cur_rx[q]++) {
>  		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>  		desc = &priv->rx_ring[q].ex_desc[entry];
>  		if (desc->die_dt == DT_FEMPTY)
> @@ -992,18 +990,16 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>  		desc->die_dt = DT_FEMPTY;
>  	}
>  
> -	stats->rx_packets += rx_packets;

Don't we still need to update the statistics? Same for ravb_rx_gbeth().

> -	*quota -= rx_packets;
> -	return *quota == 0;
> +	return rx_packets;
>  }
>  
>  /* Packet receive function for Ethernet AVB */
> -static bool ravb_rx(struct net_device *ndev, int *quota, int q)
> +static int ravb_rx(struct net_device *ndev, int budget, int q)
>  {
>  	struct ravb_private *priv = netdev_priv(ndev);
>  	const struct ravb_hw_info *info = priv->info;
>  
> -	return info->receive(ndev, quota, q);
> +	return info->receive(ndev, budget, q);
>  }
>  
>  static void ravb_rcv_snd_disable(struct net_device *ndev)
> @@ -1320,13 +1316,12 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	unsigned long flags;
>  	int q = napi - priv->napi;
>  	int mask = BIT(q);
> -	int quota = budget;
> -	bool unmask;
> +	int work_done;
>  
>  	/* Processing RX Descriptor Ring */
>  	/* Clear RX interrupt */
>  	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
> -	unmask = !ravb_rx(ndev, &quota, q);
> +	work_done = ravb_rx(ndev, budget, q);
>  
>  	/* Processing TX Descriptor Ring */
>  	spin_lock_irqsave(&priv->lock, flags);
> @@ -1345,7 +1340,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors)
>  		ndev->stats.rx_fifo_errors = priv->rx_fifo_errors;
>  
> -	if (!unmask)
> +	if (work_done == budget)
>  		goto out;
>  
>  	napi_complete(napi);
> @@ -1362,7 +1357,7 @@ static int ravb_poll(struct napi_struct *napi, int budget)
>  	spin_unlock_irqrestore(&priv->lock, flags);
>  
>  out:
> -	return budget - quota;
> +	return work_done;
>  }
>  
>  static void ravb_set_duplex_gbeth(struct net_device *ndev)
> -- 
> 2.39.2
>
Paul Barker April 15, 2024, 11:31 a.m. UTC | #2 
On 15/04/2024 12:23, Niklas Söderlund wrote:
> Hi Paul,
> 
> I really like this patch!
> 
> One nit below.
> 
> On 2024-04-15 10:47:58 +0100, Paul Barker wrote:
>> We don't need to pass the work budget to ravb_rx() by reference, it's
>> cleaner to pass this by value and return the amount of work done. This
>> allows us to simplify the ravb_poll() function and use the common
>> `work_done` variable name seen in other network drivers for consistency
>> and ease of understanding.
>>
>> This is a pure refactor and should not affect behaviour.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb.h      |  2 +-
>>  drivers/net/ethernet/renesas/ravb_main.c | 29 ++++++++++--------------
>>  2 files changed, 13 insertions(+), 18 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
>> index b48935ec7e28..71de2a7aa27c 100644
>> --- a/drivers/net/ethernet/renesas/ravb.h
>> +++ b/drivers/net/ethernet/renesas/ravb.h
>> @@ -1039,7 +1039,7 @@ struct ravb_ptp {
>>  };
>>  
>>  struct ravb_hw_info {
>> -	bool (*receive)(struct net_device *ndev, int *quota, int q);
>> +	int (*receive)(struct net_device *ndev, int budget, int q);
>>  	void (*set_rate)(struct net_device *ndev);
>>  	int (*set_feature)(struct net_device *ndev, netdev_features_t features);
>>  	int (*dmac_init)(struct net_device *ndev);
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index fd2679ce4e3d..33f8043143c1 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -759,7 +759,7 @@ static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
>>  }
>>  
>>  /* Packet receive function for Gigabit Ethernet */
>> -static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>> +static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	const struct ravb_hw_info *info = priv->info;
>> @@ -778,7 +778,7 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>>  	stats = &priv->stats[q];
>>  
>> -	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
>> +	for (i = 0; i < limit && rx_packets < budget; i++, priv->cur_rx[q]++) {
>>  		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>>  		desc = &priv->rx_ring[q].desc[entry];
>>  		if (desc->die_dt == DT_FEMPTY)
>> @@ -881,13 +881,11 @@ static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
>>  		desc->die_dt = DT_FEMPTY;
>>  	}
>>  
>> -	stats->rx_packets += rx_packets;
>> -	*quota -= rx_packets;
>> -	return *quota == 0;
>> +	return rx_packets;
>>  }
>>  
>>  /* Packet receive function for Ethernet AVB */
>> -static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>> +static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
>>  {
>>  	struct ravb_private *priv = netdev_priv(ndev);
>>  	const struct ravb_hw_info *info = priv->info;
>> @@ -904,7 +902,7 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>>  	int i;
>>  
>>  	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
>> -	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
>> +	for (i = 0; i < limit && rx_packets < budget; i++, priv->cur_rx[q]++) {
>>  		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
>>  		desc = &priv->rx_ring[q].ex_desc[entry];
>>  		if (desc->die_dt == DT_FEMPTY)
>> @@ -992,18 +990,16 @@ static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
>>  		desc->die_dt = DT_FEMPTY;
>>  	}
>>  
>> -	stats->rx_packets += rx_packets;
> 
> Don't we still need to update the statistics? Same for ravb_rx_gbeth().

Good catch! This was present in the previous version of the series [1],
but I accidentally dropped it while splitting out the bugfix patches.
I'll add it back in for the next version.

[1]: https://lore.kernel.org/all/20240206091909.3191-3-paul.barker.ct@bp.renesas.com/

Thanks,
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb.h b/drivers/net/ethernet/renesas/ravb.h
index b48935ec7e28..71de2a7aa27c 100644
--- a/drivers/net/ethernet/renesas/ravb.h
+++ b/drivers/net/ethernet/renesas/ravb.h
@@ -1039,7 +1039,7 @@  struct ravb_ptp {
 };
 
 struct ravb_hw_info {
-	bool (*receive)(struct net_device *ndev, int *quota, int q);
+	int (*receive)(struct net_device *ndev, int budget, int q);
 	void (*set_rate)(struct net_device *ndev);
 	int (*set_feature)(struct net_device *ndev, netdev_features_t features);
 	int (*dmac_init)(struct net_device *ndev);
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index fd2679ce4e3d..33f8043143c1 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -759,7 +759,7 @@  static struct sk_buff *ravb_get_skb_gbeth(struct net_device *ndev, int entry,
 }
 
 /* Packet receive function for Gigabit Ethernet */
-static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
+static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
@@ -778,7 +778,7 @@  static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
 	stats = &priv->stats[q];
 
-	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
+	for (i = 0; i < limit && rx_packets < budget; i++, priv->cur_rx[q]++) {
 		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
 		desc = &priv->rx_ring[q].desc[entry];
 		if (desc->die_dt == DT_FEMPTY)
@@ -881,13 +881,11 @@  static bool ravb_rx_gbeth(struct net_device *ndev, int *quota, int q)
 		desc->die_dt = DT_FEMPTY;
 	}
 
-	stats->rx_packets += rx_packets;
-	*quota -= rx_packets;
-	return *quota == 0;
+	return rx_packets;
 }
 
 /* Packet receive function for Ethernet AVB */
-static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
+static int ravb_rx_rcar(struct net_device *ndev, int budget, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
@@ -904,7 +902,7 @@  static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 	int i;
 
 	limit = priv->dirty_rx[q] + priv->num_rx_ring[q] - priv->cur_rx[q];
-	for (i = 0; i < limit && rx_packets < *quota; i++, priv->cur_rx[q]++) {
+	for (i = 0; i < limit && rx_packets < budget; i++, priv->cur_rx[q]++) {
 		entry = priv->cur_rx[q] % priv->num_rx_ring[q];
 		desc = &priv->rx_ring[q].ex_desc[entry];
 		if (desc->die_dt == DT_FEMPTY)
@@ -992,18 +990,16 @@  static bool ravb_rx_rcar(struct net_device *ndev, int *quota, int q)
 		desc->die_dt = DT_FEMPTY;
 	}
 
-	stats->rx_packets += rx_packets;
-	*quota -= rx_packets;
-	return *quota == 0;
+	return rx_packets;
 }
 
 /* Packet receive function for Ethernet AVB */
-static bool ravb_rx(struct net_device *ndev, int *quota, int q)
+static int ravb_rx(struct net_device *ndev, int budget, int q)
 {
 	struct ravb_private *priv = netdev_priv(ndev);
 	const struct ravb_hw_info *info = priv->info;
 
-	return info->receive(ndev, quota, q);
+	return info->receive(ndev, budget, q);
 }
 
 static void ravb_rcv_snd_disable(struct net_device *ndev)
@@ -1320,13 +1316,12 @@  static int ravb_poll(struct napi_struct *napi, int budget)
 	unsigned long flags;
 	int q = napi - priv->napi;
 	int mask = BIT(q);
-	int quota = budget;
-	bool unmask;
+	int work_done;
 
 	/* Processing RX Descriptor Ring */
 	/* Clear RX interrupt */
 	ravb_write(ndev, ~(mask | RIS0_RESERVED), RIS0);
-	unmask = !ravb_rx(ndev, &quota, q);
+	work_done = ravb_rx(ndev, budget, q);
 
 	/* Processing TX Descriptor Ring */
 	spin_lock_irqsave(&priv->lock, flags);
@@ -1345,7 +1340,7 @@  static int ravb_poll(struct napi_struct *napi, int budget)
 	if (priv->rx_fifo_errors != ndev->stats.rx_fifo_errors)
 		ndev->stats.rx_fifo_errors = priv->rx_fifo_errors;
 
-	if (!unmask)
+	if (work_done == budget)
 		goto out;
 
 	napi_complete(napi);
@@ -1362,7 +1357,7 @@  static int ravb_poll(struct napi_struct *napi, int budget)
 	spin_unlock_irqrestore(&priv->lock, flags);
 
 out:
-	return budget - quota;
+	return work_done;
 }
 
 static void ravb_set_duplex_gbeth(struct net_device *ndev)