| Message ID | 20250219054247.733243-3-wei.fang@nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | net: enetc: fix some known issues | expand |
On Wed, Feb 19, 2025 at 01:42:40PM +0800, Wei Fang wrote: > When creating a TSO header, if the skb is VLAN tagged, the extended BD > will be used and the 'count' should be increased by 2 instead of 1. > Otherwise, when an error occurs, less tx_swbd will be freed than the > actual number. > > Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO") > Cc: stable@vger.kernel.org > Signed-off-by: Wei Fang <wei.fang@nxp.com> > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> Reviewed-by: Ioana Ciornei <ioana.ciornei@nxp.com>
On Wed, Feb 19, 2025 at 01:42:40PM +0800, Wei Fang wrote: > When creating a TSO header, if the skb is VLAN tagged, the extended BD > will be used and the 'count' should be increased by 2 instead of 1. > Otherwise, when an error occurs, less tx_swbd will be freed than the > actual number. > > Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO") > Cc: stable@vger.kernel.org > Signed-off-by: Wei Fang <wei.fang@nxp.com> > Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> > --- I'm not sure "correct the statistics" is the best way to describe this change. Maybe "keep track of correct TXBD count in enetc_map_tx_tso_buffs()"? The bug is that not all TX buffers are freed on error, not that some statistics are wrong. > drivers/net/ethernet/freescale/enetc/enetc.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c > index 01c09fd26f9f..0658c06a23c1 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -759,6 +759,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb) > static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) > { > struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev); > + bool ext_bd = skb_vlan_tag_present(skb); > int hdr_len, total_len, data_len; > struct enetc_tx_swbd *tx_swbd; > union enetc_tx_bd *txbd; > @@ -792,7 +793,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb > csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos); > enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len, data_len); > bd_data_num = 0; > - count++; > + count += ext_bd ? 2 : 1; > > while (data_len > 0) { > int size; > -- > 2.34.1 > stylistic nitpick: I think this implementation choice obscures the fact, to an unfamiliar reader, that the requirement for an extended TXBD comes from enetc_map_tx_tso_hdr(). This is because you repeat the condition for skb_vlan_tag_present(), but it's not obvious it's correlated to the other one. Something like the change below is more expressive in this regard, in my opinion: diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index fe3967268a19..6178157611db 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -410,14 +410,15 @@ static int enetc_map_tx_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) return 0; } -static void enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb, - struct enetc_tx_swbd *tx_swbd, - union enetc_tx_bd *txbd, int *i, int hdr_len, - int data_len) +static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb, + struct enetc_tx_swbd *tx_swbd, + union enetc_tx_bd *txbd, int *i, int hdr_len, + int data_len) { union enetc_tx_bd txbd_tmp; u8 flags = 0, e_flags = 0; dma_addr_t addr; + int count = 1; enetc_clear_tx_bd(&txbd_tmp); addr = tx_ring->tso_headers_dma + *i * TSO_HEADER_SIZE; @@ -460,7 +461,10 @@ static void enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff *skb, /* Write the BD */ txbd_tmp.ext.e_flags = e_flags; *txbd = txbd_tmp; + count++; } + + return count; } static int enetc_map_tx_tso_data(struct enetc_bdr *tx_ring, struct sk_buff *skb, @@ -786,7 +790,6 @@ static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb) static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) { struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev); - bool ext_bd = skb_vlan_tag_present(skb); int hdr_len, total_len, data_len; struct enetc_tx_swbd *tx_swbd; union enetc_tx_bd *txbd; @@ -818,9 +821,9 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb /* compute the csum over the L4 header */ csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos); - enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len, data_len); + count += enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, + hdr_len, data_len); bd_data_num = 0; - count += ext_bd ? 2 : 1; while (data_len > 0) { int size;
> I'm not sure "correct the statistics" is the best way to describe this > change. Maybe "keep track of correct TXBD count in > enetc_map_tx_tso_buffs()"? Hi Vladimir, Inspired by Michal, I think we don't need to keep the count variable, because we already have index "i", we just need to record the value of the initial i at the beginning. So I plan to do this optimization on the net-next tree in the future. So I don't think it is necessary to modify enetc_map_tx_tso_hdr(). > The bug is that not all TX buffers are freed on error, not that some > statistics are wrong. > > > drivers/net/ethernet/freescale/enetc/enetc.c | 3 ++- > > 1 file changed, 2 insertions(+), 1 deletion(-) > > > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > b/drivers/net/ethernet/freescale/enetc/enetc.c > > index 01c09fd26f9f..0658c06a23c1 100644 > > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > > @@ -759,6 +759,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr > *tx_ring, struct sk_buff *skb) > > static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff > *skb) > > { > > struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev); > > + bool ext_bd = skb_vlan_tag_present(skb); > > int hdr_len, total_len, data_len; > > struct enetc_tx_swbd *tx_swbd; > > union enetc_tx_bd *txbd; > > @@ -792,7 +793,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr > *tx_ring, struct sk_buff *skb > > csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos); > > enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len, > data_len); > > bd_data_num = 0; > > - count++; > > + count += ext_bd ? 2 : 1; > > > > while (data_len > 0) { > > int size; > > -- > > 2.34.1 > > > > stylistic nitpick: I think this implementation choice obscures the fact, > to an unfamiliar reader, that the requirement for an extended TXBD comes > from enetc_map_tx_tso_hdr(). This is because you repeat the condition > for skb_vlan_tag_present(), but it's not obvious it's correlated to the > other one. Something like the change below is more expressive in this > regard, in my opinion: > > diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c > b/drivers/net/ethernet/freescale/enetc/enetc.c > index fe3967268a19..6178157611db 100644 > --- a/drivers/net/ethernet/freescale/enetc/enetc.c > +++ b/drivers/net/ethernet/freescale/enetc/enetc.c > @@ -410,14 +410,15 @@ static int enetc_map_tx_buffs(struct enetc_bdr > *tx_ring, struct sk_buff *skb) > return 0; > } > > -static void enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff > *skb, > - struct enetc_tx_swbd *tx_swbd, > - union enetc_tx_bd *txbd, int *i, int hdr_len, > - int data_len) > +static int enetc_map_tx_tso_hdr(struct enetc_bdr *tx_ring, struct sk_buff > *skb, > + struct enetc_tx_swbd *tx_swbd, > + union enetc_tx_bd *txbd, int *i, int hdr_len, > + int data_len) > { > union enetc_tx_bd txbd_tmp; > u8 flags = 0, e_flags = 0; > dma_addr_t addr; > + int count = 1; > > enetc_clear_tx_bd(&txbd_tmp); > addr = tx_ring->tso_headers_dma + *i * TSO_HEADER_SIZE; > @@ -460,7 +461,10 @@ static void enetc_map_tx_tso_hdr(struct enetc_bdr > *tx_ring, struct sk_buff *skb, > /* Write the BD */ > txbd_tmp.ext.e_flags = e_flags; > *txbd = txbd_tmp; > + count++; > } > + > + return count; > } > > static int enetc_map_tx_tso_data(struct enetc_bdr *tx_ring, struct sk_buff > *skb, > @@ -786,7 +790,6 @@ static int enetc_lso_hw_offload(struct enetc_bdr > *tx_ring, struct sk_buff *skb) > static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff > *skb) > { > struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev); > - bool ext_bd = skb_vlan_tag_present(skb); > int hdr_len, total_len, data_len; > struct enetc_tx_swbd *tx_swbd; > union enetc_tx_bd *txbd; > @@ -818,9 +821,9 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr > *tx_ring, struct sk_buff *skb > > /* compute the csum over the L4 header */ > csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos); > - enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len, > data_len); > + count += enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, > + hdr_len, data_len); > bd_data_num = 0; > - count += ext_bd ? 2 : 1; > > while (data_len > 0) { > int size;
> -----Original Message----- > From: Wei Fang <wei.fang@nxp.com> > Sent: Friday, February 21, 2025 3:42 AM > To: Vladimir Oltean <vladimir.oltean@nxp.com> > Cc: Claudiu Manoil <claudiu.manoil@nxp.com>; Clark Wang > <xiaoning.wang@nxp.com>; andrew+netdev@lunn.ch; > davem@davemloft.net; edumazet@google.com; kuba@kernel.org; > pabeni@redhat.com; Ioana Ciornei <ioana.ciornei@nxp.com>; Y.B. Lu > <yangbo.lu@nxp.com>; michal.swiatkowski@linux.intel.com; > netdev@vger.kernel.org; linux-kernel@vger.kernel.org; imx@lists.linux.dev; > stable@vger.kernel.org > Subject: RE: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics > > > I'm not sure "correct the statistics" is the best way to describe this > > change. Maybe "keep track of correct TXBD count in > > enetc_map_tx_tso_buffs()"? > > Hi Vladimir, > > Inspired by Michal, I think we don't need to keep the count variable, because > we already have index "i", we just need to record the value of the initial i at the > beginning. So I plan to do this optimization on the net-next tree in the future. > So I don't think it is necessary to modify enetc_map_tx_tso_hdr(). > And what if 'i' wraps around at least one time and becomes greater than the initial 'i'? Instead of 'count' you would have to record the number of wraps. Even if not possible now in specific cases, there should be no limitation on whether 'i' can wrap around in the loop or not (i.e. maybe some users want to try very small Tx rings etc.)
> > > I'm not sure "correct the statistics" is the best way to describe this > > > change. Maybe "keep track of correct TXBD count in > > > enetc_map_tx_tso_buffs()"? > > > > Hi Vladimir, > > > > Inspired by Michal, I think we don't need to keep the count variable, because > > we already have index "i", we just need to record the value of the initial i at > the > > beginning. So I plan to do this optimization on the net-next tree in the future. > > So I don't think it is necessary to modify enetc_map_tx_tso_hdr(). > > > > And what if 'i' wraps around at least one time and becomes greater than the > initial 'i'? Instead of 'count' you would have to record the number of wraps. I think this situation will not happen, because when calling enetc_map_tx_tso_buffs()/enetc_map_tx_buffs()/enetc_lso_hw_offload(), we always check whether the current free BDs are enough. The number of free BDs is always <= bdr->bd_count, in the case you mentioned, the frame will occupy more BDs than bdr->bd_count. > Even if not possible now in specific cases, there should be no limitation on > whether 'i' can wrap around in the loop or not (i.e. maybe some users want to > try very small Tx rings etc.)
On Fri, Feb 21, 2025 at 03:42:05AM +0200, Wei Fang wrote: > > I'm not sure "correct the statistics" is the best way to describe this > > change. Maybe "keep track of correct TXBD count in > > enetc_map_tx_tso_buffs()"? > > Hi Vladimir, > > Inspired by Michal, I think we don't need to keep the count variable, because > we already have index "i", we just need to record the value of the initial i at the > beginning. So I plan to do this optimization on the net-next tree in the future. > So I don't think it is necessary to modify enetc_map_tx_tso_hdr(). You are saying this in reply to my observation that the title of the change does not correctly represent the change. But I don't see how what you're saying is connected to that. Currently there exists a "count" variable based on which TX BDs are unmapped, and these patches are not changing that fact. > > stylistic nitpick: I think this implementation choice obscures the fact, > > to an unfamiliar reader, that the requirement for an extended TXBD comes > > from enetc_map_tx_tso_hdr(). This is because you repeat the condition > > for skb_vlan_tag_present(), but it's not obvious it's correlated to the > > other one. Something like the change below is more expressive in this > > regard, in my opinion: It seems you were objecting to this other change suggestion instead. Sure, I mean, ignore it if you want, but you're saying "well I'm going to change the scheme for net-next, so no point in making the code more obviously correct in stable branches", but the stable branches aren't going to pick up the net-next patch - they are essentially frozen except for bug fixes. I would still recommend writing code that makes the most sense for stable (to the extent that this is logically part of fixing a bug), and then writing code that makes most sense for net-next, even if it implies changing some of it back the way it was.
> -----Original Message----- > From: Wei Fang <wei.fang@nxp.com> > Sent: Friday, February 21, 2025 10:34 AM [...] > Subject: RE: [PATCH v2 net 2/9] net: enetc: correct the tx_swbd statistics > > > > > I'm not sure "correct the statistics" is the best way to describe this > > > > change. Maybe "keep track of correct TXBD count in > > > > enetc_map_tx_tso_buffs()"? > > > > > > Hi Vladimir, > > > > > > Inspired by Michal, I think we don't need to keep the count variable, > because > > > we already have index "i", we just need to record the value of the initial i at > > the > > > beginning. So I plan to do this optimization on the net-next tree in the > future. > > > So I don't think it is necessary to modify enetc_map_tx_tso_hdr(). > > > > > > > And what if 'i' wraps around at least one time and becomes greater than the > > initial 'i'? Instead of 'count' you would have to record the number of wraps. > > I think this situation will not happen, because when calling > enetc_map_tx_tso_buffs()/enetc_map_tx_buffs()/enetc_lso_hw_offload(), > we always check whether the current free BDs are enough. The number of > free BDs is always <= bdr->bd_count, in the case you mentioned, the frame > will occupy more BDs than bdr->bd_count. > Ok, let's see the net-next patches and discuss then.
diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c b/drivers/net/ethernet/freescale/enetc/enetc.c index 01c09fd26f9f..0658c06a23c1 100644 --- a/drivers/net/ethernet/freescale/enetc/enetc.c +++ b/drivers/net/ethernet/freescale/enetc/enetc.c @@ -759,6 +759,7 @@ static int enetc_lso_hw_offload(struct enetc_bdr *tx_ring, struct sk_buff *skb) static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb) { struct enetc_ndev_priv *priv = netdev_priv(tx_ring->ndev); + bool ext_bd = skb_vlan_tag_present(skb); int hdr_len, total_len, data_len; struct enetc_tx_swbd *tx_swbd; union enetc_tx_bd *txbd; @@ -792,7 +793,7 @@ static int enetc_map_tx_tso_buffs(struct enetc_bdr *tx_ring, struct sk_buff *skb csum = enetc_tso_hdr_csum(&tso, skb, hdr, hdr_len, &pos); enetc_map_tx_tso_hdr(tx_ring, skb, tx_swbd, txbd, &i, hdr_len, data_len); bd_data_num = 0; - count++; + count += ext_bd ? 2 : 1; while (data_len > 0) { int size;
When creating a TSO header, if the skb is VLAN tagged, the extended BD will be used and the 'count' should be increased by 2 instead of 1. Otherwise, when an error occurs, less tx_swbd will be freed than the actual number. Fixes: fb8629e2cbfc ("net: enetc: add support for software TSO") Cc: stable@vger.kernel.org Signed-off-by: Wei Fang <wei.fang@nxp.com> Reviewed-by: Michal Swiatkowski <michal.swiatkowski@linux.intel.com> --- drivers/net/ethernet/freescale/enetc/enetc.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)