| Message ID | 20240528150339.6791-5-paul.barker.ct@bp.renesas.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Improve GbEth performance on Renesas RZ/G2L and related SoCs | expand |
On 5/28/24 6:03 PM, Paul Barker wrote: > We can reduce code duplication in ravb_rx_gbeth(). > > Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> [...] > diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c > index 7df7d2e93a3a..c9c5cc641589 100644 > --- a/drivers/net/ethernet/renesas/ravb_main.c > +++ b/drivers/net/ethernet/renesas/ravb_main.c > @@ -817,47 +817,54 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q) > stats->rx_missed_errors++; > } else { > die_dt = desc->die_dt & 0xF0; > + skb = ravb_get_skb_gbeth(ndev, entry, desc); > switch (die_dt) { Why not do instead (as I've asked you alraedy): case DT_FSTART: priv->rx_1st_skb = skb; fallthrough; > case DT_FSINGLE: > - skb = ravb_get_skb_gbeth(ndev, entry, desc); > + case DT_FSTART: > + /* Start of packet: > + * Set initial data length. > + */ Please consider turning that block comment into one-liner... > skb_put(skb, desc_len); > + > + /* Save this SKB if the packet spans multiple > + * descriptors. > + */ This one too... (The current line length limit is 100 columns.) > + if (die_dt == DT_FSTART) > + priv->rx_1st_skb = skb; This needs to be done under *case* DT_FSTART above instead... > + break; > + > + case DT_FMID: > + case DT_FEND: > + /* 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 Please call it consistently, either SKB or skb (I prefer this one). > + * we only need one code path for finishing a > + * packet. > + */ > + skb = priv->rx_1st_skb; > + } > + > + switch (die_dt) { > + case DT_FSINGLE: > + case 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++; Otherwise, this is very good patch! Sorry for letting in the duplcate code earlier! :-) [...] MBR, Sergey
On 29/05/2024 19:30, Sergey Shtylyov wrote: > On 5/28/24 6:03 PM, Paul Barker wrote: > >> We can reduce code duplication in ravb_rx_gbeth(). >> >> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> > [...] > >> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >> index 7df7d2e93a3a..c9c5cc641589 100644 >> --- a/drivers/net/ethernet/renesas/ravb_main.c >> +++ b/drivers/net/ethernet/renesas/ravb_main.c >> @@ -817,47 +817,54 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q) >> stats->rx_missed_errors++; >> } else { >> die_dt = desc->die_dt & 0xF0; >> + skb = ravb_get_skb_gbeth(ndev, entry, desc); >> switch (die_dt) { > > Why not do instead (as I've asked you alraedy): > > case DT_FSTART: > priv->rx_1st_skb = skb; > fallthrough; I've avoided that change to keep patch 7/7 simpler (as we have to move the assignment of skb later in that patch). I can change this if you want though. > >> case DT_FSINGLE: >> - skb = ravb_get_skb_gbeth(ndev, entry, desc); > > >> + case DT_FSTART: >> + /* Start of packet: >> + * Set initial data length. >> + */ > > Please consider turning that block comment into one-liner... Ack. > >> skb_put(skb, desc_len); >> + >> + /* Save this SKB if the packet spans multiple >> + * descriptors. >> + */ > > This one too... > (The current line length limit is 100 columns.) Ack. I'll re-flow some other lines with a 100 col limit as well - I'm immediately thinking of the skb_copy_to_linear_data_offset call below. > >> + if (die_dt == DT_FSTART) >> + priv->rx_1st_skb = skb; > > This needs to be done under *case* DT_FSTART above instead... See above comment. We can do this under DT_FSTART in this patch if you want, but this if condition will then come back in a revised patch 7/7. > >> + break; >> + >> + case DT_FMID: >> + case DT_FEND: >> + /* 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 > > Please call it consistently, either SKB or skb (I prefer this one). Ack. > >> + * we only need one code path for finishing a >> + * packet. >> + */ >> + skb = priv->rx_1st_skb; >> + } >> + >> + switch (die_dt) { >> + case DT_FSINGLE: >> + case 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++; > > Otherwise, this is very good patch! Sorry for letting in the duplcate > code earlier! :-) > > [...] > > MBR, Sergey Thanks for the review!
On 5/29/24 10:07 PM, Paul Barker wrote: [...] >>> We can reduce code duplication in ravb_rx_gbeth(). >>> >>> Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> >> [...] >> >>> diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c >>> index 7df7d2e93a3a..c9c5cc641589 100644 >>> --- a/drivers/net/ethernet/renesas/ravb_main.c >>> +++ b/drivers/net/ethernet/renesas/ravb_main.c >>> @@ -817,47 +817,54 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q) >>> stats->rx_missed_errors++; >>> } else { >>> die_dt = desc->die_dt & 0xF0; >>> + skb = ravb_get_skb_gbeth(ndev, entry, desc); >>> switch (die_dt) { >> >> Why not do instead (as I've asked you alraedy): Already. :-) >> >> case DT_FSTART: >> priv->rx_1st_skb = skb; >> fallthrough; > > I've avoided that change to keep patch 7/7 simpler (as we have to move > the assignment of skb later in that patch). I can change this if you > want though. Oh, then please keep it as is! :-) [...] > Thanks for the review! MBR, Sergey
diff --git a/drivers/net/ethernet/renesas/ravb_main.c b/drivers/net/ethernet/renesas/ravb_main.c index 7df7d2e93a3a..c9c5cc641589 100644 --- a/drivers/net/ethernet/renesas/ravb_main.c +++ b/drivers/net/ethernet/renesas/ravb_main.c @@ -817,47 +817,54 @@ static int ravb_rx_gbeth(struct net_device *ndev, int budget, int q) stats->rx_missed_errors++; } else { die_dt = desc->die_dt & 0xF0; + skb = ravb_get_skb_gbeth(ndev, entry, desc); switch (die_dt) { case DT_FSINGLE: - skb = ravb_get_skb_gbeth(ndev, entry, desc); + case 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; + break; + + case DT_FMID: + case DT_FEND: + /* 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; + } + + switch (die_dt) { + case DT_FSINGLE: + case 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; } } }
We can reduce code duplication in ravb_rx_gbeth(). Signed-off-by: Paul Barker <paul.barker.ct@bp.renesas.com> --- Changes v3->v4: * Used switch statements instead of if statements in ravb_rx_gbeth(). drivers/net/ethernet/renesas/ravb_main.c | 73 +++++++++++++----------- 1 file changed, 40 insertions(+), 33 deletions(-)