net/iavf: fix checksum offloading
Checks
Commit Message
The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator
that a checksum offload has been requested by an application.
Check that RTE_MBUF_F_TX_IP_CKSUM or others flags have been set.
Fixes: 1e728b01120c ("net/iavf: rework Tx path")
Cc: stable@dpdk.org
Signed-off-by: David Marchand <david.marchand@redhat.com>
---
drivers/net/iavf/iavf_rxtx.c | 4 ++++
1 file changed, 4 insertions(+)
Comments
On 18 Aug 2023, at 11:03, David Marchand wrote:
> The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator
> that a checksum offload has been requested by an application.
> Check that RTE_MBUF_F_TX_IP_CKSUM or others flags have been set.
>
> Fixes: 1e728b01120c ("net/iavf: rework Tx path")
> Cc: stable@dpdk.org
Thanks for fixing this David! I tested and reviewed this change and it works now.
Tested-by: Eelco Chaudron <echaudro@redhat.com>
Acked-by: Eelco Chaudron <echaudro@redhat.com>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> drivers/net/iavf/iavf_rxtx.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c
> index f7df4665d1..b9e2879764 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -2652,6 +2652,9 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
> offset |= (m->l2_len >> 1)
> << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
>
> + if ((m->ol_flags & IAVF_TX_CKSUM_OFFLOAD_MASK) == 0)
> + goto skip_cksum;
> +
> /* Enable L3 checksum offloading inner */
> if (m->ol_flags & RTE_MBUF_F_TX_IP_CKSUM) {
> if (m->ol_flags & RTE_MBUF_F_TX_IPV4) {
> @@ -2702,6 +2705,7 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
> break;
> }
>
> +skip_cksum:
> *qw1 = rte_cpu_to_le_64((((uint64_t)command <<
> IAVF_TXD_DATA_QW1_CMD_SHIFT) & IAVF_TXD_DATA_QW1_CMD_MASK) |
> (((uint64_t)offset << IAVF_TXD_DATA_QW1_OFFSET_SHIFT) &
> --
> 2.41.0
On Mon, Aug 21, 2023 at 10:03 AM Eelco Chaudron <echaudro@redhat.com> wrote:
> On 18 Aug 2023, at 11:03, David Marchand wrote:
>
> > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator
> > that a checksum offload has been requested by an application.
> > Check that RTE_MBUF_F_TX_IP_CKSUM or others flags have been set.
> >
> > Fixes: 1e728b01120c ("net/iavf: rework Tx path")
> > Cc: stable@dpdk.org
>
> Thanks for fixing this David! I tested and reviewed this change and it works now.
For the record, Eelco encountered an issue with OVS 3.2 which now uses
ip checksum offloading.
Packets requiring such offloading were dropped by either the net/iavf
driver or the i40e VF hw itself.
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Friday, August 18, 2023 5:04 PM
> To: dev@dpdk.org
> Cc: echaudro@redhat.com; mkp@redhat.com; stable@dpdk.org; Wu, Jingjing
> <jingjing.wu@intel.com>; Xing, Beilei <beilei.xing@intel.com>; Doherty, Declan
> <declan.doherty@intel.com>; Sinha, Abhijit <abhijit.sinha@intel.com>; Nicolau,
> Radu <radu.nicolau@intel.com>
> Subject: [PATCH] net/iavf: fix checksum offloading
>
> The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator that
> a checksum offload has been requested by an application.
According to current implementation, actually the only presence of RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds to an 'IPv4 packet with no IP checksum offload,' according to datasheet.
So, I assume in this situation, the PMD continues to operate under the assumption that the application has not requested checksum offloading.
Could you share more insight what is the failure, maybe we can perform a more comprehensive investigation?
Thanks
Qi
> Check that RTE_MBUF_F_TX_IP_CKSUM or others flags have been set.
>
> Fixes: 1e728b01120c ("net/iavf: rework Tx path")
> Cc: stable@dpdk.org
>
> Signed-off-by: David Marchand <david.marchand@redhat.com>
> ---
> drivers/net/iavf/iavf_rxtx.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/drivers/net/iavf/iavf_rxtx.c b/drivers/net/iavf/iavf_rxtx.c index
> f7df4665d1..b9e2879764 100644
> --- a/drivers/net/iavf/iavf_rxtx.c
> +++ b/drivers/net/iavf/iavf_rxtx.c
> @@ -2652,6 +2652,9 @@ iavf_build_data_desc_cmd_offset_fields(volatile
> uint64_t *qw1,
> offset |= (m->l2_len >> 1)
> << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
>
> + if ((m->ol_flags & IAVF_TX_CKSUM_OFFLOAD_MASK) == 0)
> + goto skip_cksum;
> +
> /* Enable L3 checksum offloading inner */
> if (m->ol_flags & RTE_MBUF_F_TX_IP_CKSUM) {
> if (m->ol_flags & RTE_MBUF_F_TX_IPV4) { @@ -2702,6
> +2705,7 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
> break;
> }
>
> +skip_cksum:
> *qw1 = rte_cpu_to_le_64((((uint64_t)command <<
> IAVF_TXD_DATA_QW1_CMD_SHIFT) &
> IAVF_TXD_DATA_QW1_CMD_MASK) |
> (((uint64_t)offset << IAVF_TXD_DATA_QW1_OFFSET_SHIFT) &
> --
> 2.41.0
On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > Subject: [PATCH] net/iavf: fix checksum offloading
> >
> > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an indicator that
> > a checksum offload has been requested by an application.
>
> According to current implementation, actually the only presence of RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds to an 'IPv4 packet with no IP checksum offload,' according to datasheet.
> So, I assume in this situation, the PMD continues to operate under the assumption that the application has not requested checksum offloading.
>
> Could you share more insight what is the failure, maybe we can perform a more comprehensive investigation?
I think the missing piece is that OVS passes a l2_len == l3_len == 0.
In our tests, we could see that tx_errors get incremented for each
failed packet to transmit.
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, August 22, 2023 1:29 AM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Subject: Re: [PATCH] net/iavf: fix checksum offloading
>
> On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > > Subject: [PATCH] net/iavf: fix checksum offloading
> > >
> > > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an
> > > indicator that a checksum offload has been requested by an application.
> >
> > According to current implementation, actually the only presence of
> RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds to an
> 'IPv4 packet with no IP checksum offload,' according to datasheet.
> > So, I assume in this situation, the PMD continues to operate under the
> assumption that the application has not requested checksum offloading.
> >
> > Could you share more insight what is the failure, maybe we can perform a
> more comprehensive investigation?
>
> I think the missing piece is that OVS passes a l2_len == l3_len == 0.
> In our tests, we could see that tx_errors get incremented for each failed packet
> to transmit.
OK, do you think to ignore RTE_MBUF_F_TX_IPV4 when l3_len = 0 is a better fix?
>
>
> --
> David Marchand
On Tue, Aug 22, 2023 at 3:52 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Tuesday, August 22, 2023 1:29 AM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> > Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> > Subject: Re: [PATCH] net/iavf: fix checksum offloading
> >
> > On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > > > Subject: [PATCH] net/iavf: fix checksum offloading
> > > >
> > > > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an
> > > > indicator that a checksum offload has been requested by an application.
> > >
> > > According to current implementation, actually the only presence of
> > RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds to an
> > 'IPv4 packet with no IP checksum offload,' according to datasheet.
> > > So, I assume in this situation, the PMD continues to operate under the
> > assumption that the application has not requested checksum offloading.
> > >
> > > Could you share more insight what is the failure, maybe we can perform a
> > more comprehensive investigation?
> >
> > I think the missing piece is that OVS passes a l2_len == l3_len == 0.
> > In our tests, we could see that tx_errors get incremented for each failed packet
> > to transmit.
>
> OK, do you think to ignore RTE_MBUF_F_TX_IPV4 when l3_len = 0 is a better fix?
Looking at the mbuf API, l2_len and l3_len should be considered by a
driver if ol_flags contains at least one of RTE_MBUF_F_TX_SEC_OFFLOAD,
RTE_MBUF_F_TX_TUNNEL_*, RTE_MBUF_F_TX_TCP_SEG,
RTE_MBUF_F_TX_(IP|TCP|UDP|SCTP)_CKSUM.
Here, it is not the case.
If the driver reads l2_len or l3_len, this is an undefined behavior:
for example, OVS might have been using l2_len or l3_len for its
internal uses (though I agree it would be risky for an application to
do so).
We probably need to fix access to l2_len a few lines before my patch.
if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
!(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
offset |= (m->outer_l2_len >> 1)
<< IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
else
offset |= (m->l2_len >> 1)
<< IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
But to be clear, no I don't think looking at l3_len value is better:
it should not be read at all.
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, August 22, 2023 2:12 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Subject: Re: [PATCH] net/iavf: fix checksum offloading
>
> On Tue, Aug 22, 2023 at 3:52 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> >
> >
> >
> > > -----Original Message-----
> > > From: David Marchand <david.marchand@redhat.com>
> > > Sent: Tuesday, August 22, 2023 1:29 AM
> > > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > > Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> > > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > > <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> > > Sinha, Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu
> > > <radu.nicolau@intel.com>
> > > Subject: Re: [PATCH] net/iavf: fix checksum offloading
> > >
> > > On Mon, Aug 21, 2023 at 1:54 PM Zhang, Qi Z <qi.z.zhang@intel.com>
> wrote:
> > > > > Subject: [PATCH] net/iavf: fix checksum offloading
> > > > >
> > > > > The only presence of RTE_MBUF_F_TX_IPV4 can't be used as an
> > > > > indicator that a checksum offload has been requested by an application.
> > > >
> > > > According to current implementation, actually the only presence of
> > > RTE_MBUF_F_TX_IPV4 will cause IIPT = 10b, this scenario corresponds
> > > to an
> > > 'IPv4 packet with no IP checksum offload,' according to datasheet.
> > > > So, I assume in this situation, the PMD continues to operate
> > > > under the
> > > assumption that the application has not requested checksum offloading.
> > > >
> > > > Could you share more insight what is the failure, maybe we can
> > > > perform a
> > > more comprehensive investigation?
> > >
> > > I think the missing piece is that OVS passes a l2_len == l3_len == 0.
> > > In our tests, we could see that tx_errors get incremented for each
> > > failed packet to transmit.
> >
> > OK, do you think to ignore RTE_MBUF_F_TX_IPV4 when l3_len = 0 is a better
> fix?
>
> Looking at the mbuf API, l2_len and l3_len should be considered by a driver if
> ol_flags contains at least one of RTE_MBUF_F_TX_SEC_OFFLOAD,
> RTE_MBUF_F_TX_TUNNEL_*, RTE_MBUF_F_TX_TCP_SEG,
> RTE_MBUF_F_TX_(IP|TCP|UDP|SCTP)_CKSUM.
> Here, it is not the case.
>
> If the driver reads l2_len or l3_len, this is an undefined behavior:
> for example, OVS might have been using l2_len or l3_len for its internal uses
> (though I agree it would be risky for an application to do so).
>
> We probably need to fix access to l2_len a few lines before my patch.
>
> if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
> !(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
> offset |= (m->outer_l2_len >> 1)
> << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> else
> offset |= (m->l2_len >> 1)
> << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
>
>
> But to be clear, no I don't think looking at l3_len value is better:
> it should not be read at all.
Yes, you may be correct; it appears that this issue is unrelated to l3_len. The primary concern is to prevent the configuration of Tx descriptors with incorrect values.
Based on your description, it seems the problem arises when the PMD sets MACLEN to 0 and also configures IIPT as 01b, Is this correct?
To prevent this issue, we could implement a check where, if l2_len is 0, we simply ignore the IIPT configuration and keep it at 0. (which leads to same configure with your patch)
Regarding your mention of 'fix access to l2_len,' if l2_len is 0, there's no change in the offset regardless of whether l2_len is accessed or not. Did you mean setting a fixed value of MACLEN to 14?"
>
>
> --
> David Marchand
On Tue, Aug 22, 2023 at 9:33 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > If the driver reads l2_len or l3_len, this is an undefined behavior:
> > for example, OVS might have been using l2_len or l3_len for its internal uses
> > (though I agree it would be risky for an application to do so).
> >
> > We probably need to fix access to l2_len a few lines before my patch.
> >
> > if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
> > !(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
> > offset |= (m->outer_l2_len >> 1)
> > << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> > else
> > offset |= (m->l2_len >> 1)
> > << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> >
> >
> > But to be clear, no I don't think looking at l3_len value is better:
> > it should not be read at all.
>
> Yes, you may be correct; it appears that this issue is unrelated to l3_len. The primary concern is to prevent the configuration of Tx descriptors with incorrect values.
>
> Based on your description, it seems the problem arises when the PMD sets MACLEN to 0 and also configures IIPT as 01b, Is this correct?
>
> To prevent this issue, we could implement a check where, if l2_len is 0, we simply ignore the IIPT configuration and keep it at 0. (which leads to same configure with your patch)
The driver is _not_ supposed to read l2_len or l3_len if no valid
ol_flags is set.
I will reject any suggestion where you consider their values.
> -----Original Message-----
> From: David Marchand <david.marchand@redhat.com>
> Sent: Tuesday, August 22, 2023 3:40 PM
> To: Zhang, Qi Z <qi.z.zhang@intel.com>
> Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Subject: Re: [PATCH] net/iavf: fix checksum offloading
>
> On Tue, Aug 22, 2023 at 9:33 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > > If the driver reads l2_len or l3_len, this is an undefined behavior:
> > > for example, OVS might have been using l2_len or l3_len for its
> > > internal uses (though I agree it would be risky for an application to do so).
> > >
> > > We probably need to fix access to l2_len a few lines before my patch.
> > >
> > > if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
> > > !(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
> > > offset |= (m->outer_l2_len >> 1)
> > > << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> > > else
> > > offset |= (m->l2_len >> 1)
> > > << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> > >
> > >
> > > But to be clear, no I don't think looking at l3_len value is better:
> > > it should not be read at all.
> >
> > Yes, you may be correct; it appears that this issue is unrelated to l3_len. The
> primary concern is to prevent the configuration of Tx descriptors with incorrect
> values.
> >
> > Based on your description, it seems the problem arises when the PMD sets
> MACLEN to 0 and also configures IIPT as 01b, Is this correct?
> >
> > To prevent this issue, we could implement a check where, if l2_len is
> > 0, we simply ignore the IIPT configuration and keep it at 0. (which
> > leads to same configure with your patch)
>
> The driver is _not_ supposed to read l2_len or l3_len if no valid ol_flags is set.
> I will reject any suggestion where you consider their values.
Alright, we could directly verify the MACLEN value in offset and decide if IIPT should be configured or not, regardless of the values of l2_len and l3_len
And before that, l2_len / l3_len should not be accessed by the driver if no related offload is required.
>
>
> --
> David Marchand
> -----Original Message-----
> From: Zhang, Qi Z
> Sent: Tuesday, August 22, 2023 3:59 PM
> To: 'David Marchand' <david.marchand@redhat.com>
> Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>; Sinha,
> Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu <radu.nicolau@intel.com>
> Subject: RE: [PATCH] net/iavf: fix checksum offloading
>
>
>
> > -----Original Message-----
> > From: David Marchand <david.marchand@redhat.com>
> > Sent: Tuesday, August 22, 2023 3:40 PM
> > To: Zhang, Qi Z <qi.z.zhang@intel.com>
> > Cc: dev@dpdk.org; echaudro@redhat.com; mkp@redhat.com;
> > stable@dpdk.org; Wu, Jingjing <jingjing.wu@intel.com>; Xing, Beilei
> > <beilei.xing@intel.com>; Doherty, Declan <declan.doherty@intel.com>;
> > Sinha, Abhijit <abhijit.sinha@intel.com>; Nicolau, Radu
> > <radu.nicolau@intel.com>
> > Subject: Re: [PATCH] net/iavf: fix checksum offloading
> >
> > On Tue, Aug 22, 2023 at 9:33 AM Zhang, Qi Z <qi.z.zhang@intel.com> wrote:
> > > > If the driver reads l2_len or l3_len, this is an undefined behavior:
> > > > for example, OVS might have been using l2_len or l3_len for its
> > > > internal uses (though I agree it would be risky for an application to do so).
> > > >
> > > > We probably need to fix access to l2_len a few lines before my patch.
> > > >
> > > > if (m->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK &&
> > > > !(m->ol_flags & RTE_MBUF_F_TX_SEC_OFFLOAD))
> > > > offset |= (m->outer_l2_len >> 1)
> > > > << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> > > > else
> > > > offset |= (m->l2_len >> 1)
> > > > << IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
> > > >
After second thought, I think your patch looks good, will you submit a new version to cover above fix?
@@ -2652,6 +2652,9 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
offset |= (m->l2_len >> 1)
<< IAVF_TX_DESC_LENGTH_MACLEN_SHIFT;
+ if ((m->ol_flags & IAVF_TX_CKSUM_OFFLOAD_MASK) == 0)
+ goto skip_cksum;
+
/* Enable L3 checksum offloading inner */
if (m->ol_flags & RTE_MBUF_F_TX_IP_CKSUM) {
if (m->ol_flags & RTE_MBUF_F_TX_IPV4) {
@@ -2702,6 +2705,7 @@ iavf_build_data_desc_cmd_offset_fields(volatile uint64_t *qw1,
break;
}
+skip_cksum:
*qw1 = rte_cpu_to_le_64((((uint64_t)command <<
IAVF_TXD_DATA_QW1_CMD_SHIFT) & IAVF_TXD_DATA_QW1_CMD_MASK) |
(((uint64_t)offset << IAVF_TXD_DATA_QW1_OFFSET_SHIFT) &