diff mbox series

[net-next,RFC,v3,4/7] net: ravb: Refactor GbEth RX code path

Message ID 20240415094804.8016-5-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:48 a.m. UTC 
We can reduce code duplication in ravb_rx_gbeth() and add comments to
make the code flow easier to understand.

Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
---
 drivers/net/ethernet/renesas/ravb_main.c | 70 ++++++++++++------------
 1 file changed, 35 insertions(+), 35 deletions(-)

Comments

Sergey Shtylyov April 19, 2024, 8:03 p.m. UTC | #1 
On 4/15/24 12:48 PM, Paul Barker wrote:

> We can reduce code duplication in ravb_rx_gbeth() and add comments to
> make the code flow easier to understand.
> 
> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
> ---
>  drivers/net/ethernet/renesas/ravb_main.c | 70 ++++++++++++------------
>  1 file changed, 35 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
> index baa01bd81f2d..12618171f6d5 100644
> --- a/drivers/net/ethernet/renesas/ravb_main.c
> +++ b/drivers/net/ethernet/renesas/ravb_main.c
> @@ -818,47 +818,47 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
>  				stats->rx_missed_errors++;
>  		} else {
>  			die_dt = desc->die_dt & 0xF0;
> -			switch (die_dt) {
> -			case DT_FSINGLE:
> -				skb = ravb_get_skb_gbeth(ndev, entry, desc);
> +			skb = ravb_get_skb_gbeth(ndev, entry, desc);
> +			if (die_dt == DT_FSINGLE || die_dt == DT_FSTART) {

   No, please keep using *switch* statement here...

> +				/* Start of packet:
> +				 * Set initial data length.
> +				 */
>  				skb_put(skb, desc_len);
> +
> +				/* Save this SKB if the packet spans multiple
> +				 * descriptors.
> +				 */
> +				if (die_dt == DT_FSTART)
> +					priv->rx_1st_skb = skb;

   Hm, looks like we can do that under *case* DT_FSTART: and do
a fallthru to *case* DT_FSINGLE: after that...

> +			} else {
> +				/* Continuing a packet:
> +				 * Move data into the saved SKB.
> +				 */
> +				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
> +							       priv->rx_1st_skb->len,
> +							       skb->data,
> +							       desc_len);
> +				skb_put(priv->rx_1st_skb, desc_len);
> +				dev_kfree_skb(skb);
> +
> +				/* Set skb to point at the whole packet so that
> +				 * we only need one code path for finishing a
> +				 * packet.
> +				 */
> +				skb = priv->rx_1st_skb;
> +			}
> +
> +			if (die_dt == DT_FSINGLE || die_dt == DT_FEND) {

   Again, keep using *switch*, please...

> +				/* Finishing a packet:
> +				 * Determine protocol & checksum, hand off to
> +				 * NAPI and update our stats.
> +				 */
>  				skb->protocol = eth_type_trans(skb, ndev);
>  				if (ndev->features & NETIF_F_RXCSUM)
>  					ravb_rx_csum_gbeth(skb);
> +				stats->rx_bytes += skb->len;
>  				napi_gro_receive(&priv->napi[q], skb);
>  				rx_packets++;
[...]

MBR, Sergey
Paul Barker April 21, 2024, 3:49 p.m. UTC | #2 
On 19/04/2024 21:03, Sergey Shtylyov wrote:
> On 4/15/24 12:48 PM, Paul Barker wrote:
> 
>> We can reduce code duplication in ravb_rx_gbeth() and add comments to
>> make the code flow easier to understand.
>>
>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
>> ---
>>  drivers/net/ethernet/renesas/ravb_main.c | 70 ++++++++++++------------
>>  1 file changed, 35 insertions(+), 35 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>> index baa01bd81f2d..12618171f6d5 100644
>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>> @@ -818,47 +818,47 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
>>  				stats->rx_missed_errors++;
>>  		} else {
>>  			die_dt = desc->die_dt & 0xF0;
>> -			switch (die_dt) {
>> -			case DT_FSINGLE:
>> -				skb = ravb_get_skb_gbeth(ndev, entry, desc);
>> +			skb = ravb_get_skb_gbeth(ndev, entry, desc);
>> +			if (die_dt == DT_FSINGLE || die_dt == DT_FSTART) {
> 
>    No, please keep using *switch* statement here...

That's a shame - I much prefer this version with reduced code
duplication, especially when we add page pool support. Duplication with
subtle differences leads to bugs, see [1] for an example.

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

An alternative would be to drop this refactor patch, then when we add
page pool support we instead use separate functions to avoid code
duplication. At the end of the series, the switch would then look
something like this:

	switch (die_dt) {
	case DT_FSINGLE:
		skb = ravb_rx_build_skb(ndev, q, rx_buff, desc_len);
		if (!skb)
			break;
		ravb_rx_finish_skb(ndev, q, skb);
		rx_packets++;
		break;
	case DT_FSTART:
		priv->rx_1st_skb = ravb_rx_build_skb(ndev, q, rx_buff, desc_len);
		break;
	case DT_FMID:
		ravb_rx_add_frag(ndev, q, rx_buff, desc_len);
		break;
	case DT_FEND:
		if (ravb_rx_add_frag(ndev, q, rx_buff, desc_len))
			break;
		ravb_rx_finish_skb(ndev, q, priv->rx_1st_skb);
		rx_packets++;
		priv->rx_1st_skb = NULL;
	}

Would you prefer that approach?

> 
>> +				/* Start of packet:
>> +				 * Set initial data length.
>> +				 */
>>  				skb_put(skb, desc_len);
>> +
>> +				/* Save this SKB if the packet spans multiple
>> +				 * descriptors.
>> +				 */
>> +				if (die_dt == DT_FSTART)
>> +					priv->rx_1st_skb = skb;
> 
>    Hm, looks like we can do that under *case* DT_FSTART: and do
> a fallthru to *case* DT_FSINGLE: after that...

A fallthrough would just have to be removed again when page pool support
is added in a later patch in this series, since we will need to call
napi_build_skb() before the skb is valid.

> 
>> +			} else {
>> +				/* Continuing a packet:
>> +				 * Move data into the saved SKB.
>> +				 */
>> +				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
>> +							       priv->rx_1st_skb->len,
>> +							       skb->data,
>> +							       desc_len);
>> +				skb_put(priv->rx_1st_skb, desc_len);
>> +				dev_kfree_skb(skb);
>> +
>> +				/* Set skb to point at the whole packet so that
>> +				 * we only need one code path for finishing a
>> +				 * packet.
>> +				 */
>> +				skb = priv->rx_1st_skb;
>> +			}
>> +
>> +			if (die_dt == DT_FSINGLE || die_dt == DT_FEND) {
> 
>    Again, keep using *switch*, please...
> 
>> +				/* Finishing a packet:
>> +				 * Determine protocol & checksum, hand off to
>> +				 * NAPI and update our stats.
>> +				 */
>>  				skb->protocol = eth_type_trans(skb, ndev);
>>  				if (ndev->features & NETIF_F_RXCSUM)
>>  					ravb_rx_csum_gbeth(skb);
>> +				stats->rx_bytes += skb->len;
>>  				napi_gro_receive(&priv->napi[q], skb);
>>  				rx_packets++;
> [...]
> 
> MBR, Sergey

Thanks,
Sergey Shtylyov April 21, 2024, 8:23 p.m. UTC | #3 
On 4/21/24 6:49 PM, Paul Barker wrote:
[...]

>>> We can reduce code duplication in ravb_rx_gbeth() and add comments to
>>> make the code flow easier to understand.
>>>
>>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com>
>>> ---
>>>  drivers/net/ethernet/renesas/ravb_main.c | 70 ++++++++++++------------
>>>  1 file changed, 35 insertions(+), 35 deletions(-)
>>>
>>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
>>> index baa01bd81f2d..12618171f6d5 100644
>>> --- a/drivers/net/ethernet/renesas/ravb_main.c
>>> +++ b/drivers/net/ethernet/renesas/ravb_main.c
>>> @@ -818,47 +818,47 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
>>>  				stats->rx_missed_errors++;
>>>  		} else {
>>>  			die_dt = desc->die_dt & 0xF0;
>>> -			switch (die_dt) {
>>> -			case DT_FSINGLE:
>>> -				skb = ravb_get_skb_gbeth(ndev, entry, desc);
>>> +			skb = ravb_get_skb_gbeth(ndev, entry, desc);
>>> +			if (die_dt == DT_FSINGLE || die_dt == DT_FSTART) {
>>
>>    No, please keep using *switch* statement here...
> 
> That's a shame - I much prefer this version with reduced code
> duplication, especially when we add page pool support. Duplication with
> subtle differences leads to bugs, see [1] for an example.
> 
> [1]: https://lore.kernel.org/all/20240416120254.2620-4-paul.barker.ct@bp.renesas.com/

   I wasn't clear enough probably, sorry about that.
   What I meant to say was that your use of the *if* statement
wasn't  actually justified. Please use the *switch* statement
instead.

[...]

> Thanks,

MBR, Sergey
diff mbox series

Patch

diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c
index baa01bd81f2d..12618171f6d5 100644
--- a/drivers/net/ethernet/renesas/ravb_main.c
+++ b/drivers/net/ethernet/renesas/ravb_main.c
@@ -818,47 +818,47 @@  static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q)
 				stats->rx_missed_errors++;
 		} else {
 			die_dt = desc->die_dt & 0xF0;
-			switch (die_dt) {
-			case DT_FSINGLE:
-				skb = ravb_get_skb_gbeth(ndev, entry, desc);
+			skb = ravb_get_skb_gbeth(ndev, entry, desc);
+			if (die_dt == DT_FSINGLE || die_dt == DT_FSTART) {
+				/* Start of packet:
+				 * Set initial data length.
+				 */
 				skb_put(skb, desc_len);
+
+				/* Save this SKB if the packet spans multiple
+				 * descriptors.
+				 */
+				if (die_dt == DT_FSTART)
+					priv->rx_1st_skb = skb;
+			} else {
+				/* Continuing a packet:
+				 * Move data into the saved SKB.
+				 */
+				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
+							       priv->rx_1st_skb->len,
+							       skb->data,
+							       desc_len);
+				skb_put(priv->rx_1st_skb, desc_len);
+				dev_kfree_skb(skb);
+
+				/* Set skb to point at the whole packet so that
+				 * we only need one code path for finishing a
+				 * packet.
+				 */
+				skb = priv->rx_1st_skb;
+			}
+
+			if (die_dt == DT_FSINGLE || die_dt == DT_FEND) {
+				/* Finishing a packet:
+				 * Determine protocol & checksum, hand off to
+				 * NAPI and update our stats.
+				 */
 				skb->protocol = eth_type_trans(skb, ndev);
 				if (ndev->features & NETIF_F_RXCSUM)
 					ravb_rx_csum_gbeth(skb);
+				stats->rx_bytes += skb->len;
 				napi_gro_receive(&priv->napi[q], skb);
 				rx_packets++;
-				stats->rx_bytes += desc_len;
-				break;
-			case DT_FSTART:
-				priv->rx_1st_skb = ravb_get_skb_gbeth(ndev, entry, desc);
-				skb_put(priv->rx_1st_skb, desc_len);
-				break;
-			case DT_FMID:
-				skb = ravb_get_skb_gbeth(ndev, entry, desc);
-				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
-							       priv->rx_1st_skb->len,
-							       skb->data,
-							       desc_len);
-				skb_put(priv->rx_1st_skb, desc_len);
-				dev_kfree_skb(skb);
-				break;
-			case DT_FEND:
-				skb = ravb_get_skb_gbeth(ndev, entry, desc);
-				skb_copy_to_linear_data_offset(priv->rx_1st_skb,
-							       priv->rx_1st_skb->len,
-							       skb->data,
-							       desc_len);
-				skb_put(priv->rx_1st_skb, desc_len);
-				dev_kfree_skb(skb);
-				priv->rx_1st_skb->protocol =
-					eth_type_trans(priv->rx_1st_skb, ndev);
-				if (ndev->features & NETIF_F_RXCSUM)
-					ravb_rx_csum_gbeth(priv->rx_1st_skb);
-				stats->rx_bytes += priv->rx_1st_skb->len;
-				napi_gro_receive(&priv->napi[q],
-						 priv->rx_1st_skb);
-				rx_packets++;
-				break;
 			}
 		}
 	}