DPDK patches and discussions
 help / color / mirror / Atom feed
* [dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well
@ 2020-05-27 14:32 Andrew Rybchenko
  2020-07-13 13:35 ` Olivier Matz
  2020-07-13 14:22 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
  0 siblings, 2 replies; 5+ messages in thread
From: Andrew Rybchenko @ 2020-05-27 14:32 UTC (permalink / raw)
  To: Olivier Matz; +Cc: dev

Pseudo-header checksum calculation requires contiguous headers.
There is no any formal requirements on data location and mbuf
structure which could be used by the application.

Make corresponding check to be done in non-debug build as well
to avoid bad accesses, incorrect checksum caclculation and to
return appropriate error from Tx prepare.

Make no-offloads check more precise and do it in non-debug build
as well to avoid contiguous headers check and Tx prepare failure
if it is not actually required.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_net/rte_net.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index 1560ecfa46..1edc283a47 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -120,20 +120,17 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	struct rte_udp_hdr *udp_hdr;
 	uint64_t inner_l3_offset = m->l2_len;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
 	/*
 	 * Does packet set any of available offloads?
 	 * Mainly it is required to avoid fragmented headers check if
 	 * no offloads are requested.
 	 */
-	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
+	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK)))
 		return 0;
-#endif
 
 	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
 		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
 	/*
 	 * Check if headers are fragmented.
 	 * The check could be less strict depending on which offloads are
@@ -142,7 +139,6 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	if (unlikely(rte_pktmbuf_data_len(m) <
 		     inner_l3_offset + m->l3_len + m->l4_len))
 		return -ENOTSUP;
-#endif
 
 	if (ol_flags & PKT_TX_IPV4) {
 		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well
  2020-05-27 14:32 [dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well Andrew Rybchenko
@ 2020-07-13 13:35 ` Olivier Matz
  2020-07-13 14:22   ` Andrew Rybchenko
  2020-07-13 14:22 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
  1 sibling, 1 reply; 5+ messages in thread
From: Olivier Matz @ 2020-07-13 13:35 UTC (permalink / raw)
  To: Andrew Rybchenko; +Cc: dev

Hi Andrew,

On Wed, May 27, 2020 at 03:32:56PM +0100, Andrew Rybchenko wrote:
> Pseudo-header checksum calculation requires contiguous headers.
> There is no any formal requirements on data location and mbuf
> structure which could be used by the application.
> 
> Make corresponding check to be done in non-debug build as well
> to avoid bad accesses, incorrect checksum caclculation and to

typo: caclculation -> calculation

> return appropriate error from Tx prepare.
> 
> Make no-offloads check more precise and do it in non-debug build
> as well to avoid contiguous headers check and Tx prepare failure
> if it is not actually required.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
>  lib/librte_net/rte_net.h | 6 +-----
>  1 file changed, 1 insertion(+), 5 deletions(-)
> 
> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> index 1560ecfa46..1edc283a47 100644
> --- a/lib/librte_net/rte_net.h
> +++ b/lib/librte_net/rte_net.h
> @@ -120,20 +120,17 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>  	struct rte_udp_hdr *udp_hdr;
>  	uint64_t inner_l3_offset = m->l2_len;
>  
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>  	/*
>  	 * Does packet set any of available offloads?
>  	 * Mainly it is required to avoid fragmented headers check if
>  	 * no offloads are requested.
>  	 */
> -	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
> +	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK)))
>  		return 0;

Agree, the packet is modified only if one of these flag is set.

> -#endif
>  
>  	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
>  		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>  
> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>  	/*
>  	 * Check if headers are fragmented.
>  	 * The check could be less strict depending on which offloads are
> @@ -142,7 +139,6 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>  	if (unlikely(rte_pktmbuf_data_len(m) <
>  		     inner_l3_offset + m->l3_len + m->l4_len))
>  		return -ENOTSUP;
> -#endif

Yes, despite the documentation of thus function says that used headers
should be in the first data segment of the mbuf, when it is used through
the ethdev tx_prepare() API there is no such requirement.

So yes, it looks safer to do these checks even if debug is off. They
will only be done when doing tx offload, so I guess it is ok in terms of
performance.

Maybe it is worth mentioning commit dfc6b2fd8da3 ("mbuf: remove Intel
offload checks from generic API") in the commit log?

>  
>  	if (ol_flags & PKT_TX_IPV4) {
>  		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
> -- 
> 2.17.1
> 

Acked-by: Olivier Matz <olivier.matz@6wind.com>

^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well
  2020-07-13 13:35 ` Olivier Matz
@ 2020-07-13 14:22   ` Andrew Rybchenko
  0 siblings, 0 replies; 5+ messages in thread
From: Andrew Rybchenko @ 2020-07-13 14:22 UTC (permalink / raw)
  To: Olivier Matz; +Cc: dev

Hi Olivier,

On 7/13/20 4:35 PM, Olivier Matz wrote:
> Hi Andrew,
>
> On Wed, May 27, 2020 at 03:32:56PM +0100, Andrew Rybchenko wrote:
>> Pseudo-header checksum calculation requires contiguous headers.
>> There is no any formal requirements on data location and mbuf
>> structure which could be used by the application.
>>
>> Make corresponding check to be done in non-debug build as well
>> to avoid bad accesses, incorrect checksum caclculation and to
> typo: caclculation -> calculation

Will fix in v2.

>> return appropriate error from Tx prepare.
>>
>> Make no-offloads check more precise and do it in non-debug build
>> as well to avoid contiguous headers check and Tx prepare failure
>> if it is not actually required.
>>
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>>  lib/librte_net/rte_net.h | 6 +-----
>>  1 file changed, 1 insertion(+), 5 deletions(-)
>>
>> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
>> index 1560ecfa46..1edc283a47 100644
>> --- a/lib/librte_net/rte_net.h
>> +++ b/lib/librte_net/rte_net.h
>> @@ -120,20 +120,17 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>>  	struct rte_udp_hdr *udp_hdr;
>>  	uint64_t inner_l3_offset = m->l2_len;
>>  
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	/*
>>  	 * Does packet set any of available offloads?
>>  	 * Mainly it is required to avoid fragmented headers check if
>>  	 * no offloads are requested.
>>  	 */
>> -	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
>> +	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK)))
>>  		return 0;
> Agree, the packet is modified only if one of these flag is set.
>
>> -#endif
>>  
>>  	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
>>  		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>>  
>> -#ifdef RTE_LIBRTE_ETHDEV_DEBUG
>>  	/*
>>  	 * Check if headers are fragmented.
>>  	 * The check could be less strict depending on which offloads are
>> @@ -142,7 +139,6 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>>  	if (unlikely(rte_pktmbuf_data_len(m) <
>>  		     inner_l3_offset + m->l3_len + m->l4_len))
>>  		return -ENOTSUP;
>> -#endif
> Yes, despite the documentation of thus function says that used headers
> should be in the first data segment of the mbuf, when it is used through
> the ethdev tx_prepare() API there is no such requirement.
>
> So yes, it looks safer to do these checks even if debug is off. They
> will only be done when doing tx offload, so I guess it is ok in terms of
> performance.
>
> Maybe it is worth mentioning commit dfc6b2fd8da3 ("mbuf: remove Intel
> offload checks from generic API") in the commit log?

Yes, I think it could be useful will add a bit of history in v2.

>>  
>>  	if (ol_flags & PKT_TX_IPV4) {
>>  		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
>> -- 
>> 2.17.1
>>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Thanks for the review,
Andrew.

^ permalink raw reply	[flat|nested] 5+ messages in thread

* [dpdk-dev] [PATCH v2] net: do fragmented headers check in non-debug build as well
  2020-05-27 14:32 [dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well Andrew Rybchenko
  2020-07-13 13:35 ` Olivier Matz
@ 2020-07-13 14:22 ` Andrew Rybchenko
  2020-07-21  0:53   ` Ferruh Yigit
  1 sibling, 1 reply; 5+ messages in thread
From: Andrew Rybchenko @ 2020-07-13 14:22 UTC (permalink / raw)
  To: dev; +Cc: Olivier Matz

Pseudo-header checksum calculation requires contiguous headers.
There is no any formal requirements on data location and mbuf
structure which could be used by the application.

Since

commit dfc6b2fd8da3 ("mbuf: remove Intel offload checks from generic API")

fragmented headers checks are done inside
rte_net_intel_cksum_flags_prepare() in RTE_LIBRTE_ETHDEV_DEBUG build
because it is moved from rte_validate_tx_offload() which is called
under debug only.

Make corresponding check to be done in non-debug build as well
to avoid bad accesses, incorrect checksum calculation and to
return appropriate error from Tx prepare.

Make no-offloads check more precise and do it in non-debug build
as well to avoid contiguous headers check and Tx prepare failure
if it is not actually required.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
Acked-by: Olivier Matz <olivier.matz@6wind.com>
---
v2:
 - fix typo in description
 - mention dfc6b2fd8da3 in the description

 lib/librte_net/rte_net.h | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index 1560ecfa46..1edc283a47 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -120,20 +120,17 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	struct rte_udp_hdr *udp_hdr;
 	uint64_t inner_l3_offset = m->l2_len;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
 	/*
 	 * Does packet set any of available offloads?
 	 * Mainly it is required to avoid fragmented headers check if
 	 * no offloads are requested.
 	 */
-	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
+	if (!(ol_flags & (PKT_TX_IP_CKSUM | PKT_TX_L4_MASK)))
 		return 0;
-#endif
 
 	if (ol_flags & (PKT_TX_OUTER_IPV4 | PKT_TX_OUTER_IPV6))
 		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
 
-#ifdef RTE_LIBRTE_ETHDEV_DEBUG
 	/*
 	 * Check if headers are fragmented.
 	 * The check could be less strict depending on which offloads are
@@ -142,7 +139,6 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	if (unlikely(rte_pktmbuf_data_len(m) <
 		     inner_l3_offset + m->l3_len + m->l4_len))
 		return -ENOTSUP;
-#endif
 
 	if (ol_flags & PKT_TX_IPV4) {
 		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct rte_ipv4_hdr *,
-- 
2.17.1


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [dpdk-dev] [PATCH v2] net: do fragmented headers check in non-debug build as well
  2020-07-13 14:22 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
@ 2020-07-21  0:53   ` Ferruh Yigit
  0 siblings, 0 replies; 5+ messages in thread
From: Ferruh Yigit @ 2020-07-21  0:53 UTC (permalink / raw)
  To: Andrew Rybchenko, dev; +Cc: Olivier Matz

On 7/13/2020 3:22 PM, Andrew Rybchenko wrote:
> Pseudo-header checksum calculation requires contiguous headers.
> There is no any formal requirements on data location and mbuf
> structure which could be used by the application.
> 
> Since
> 
> commit dfc6b2fd8da3 ("mbuf: remove Intel offload checks from generic API")
> 
> fragmented headers checks are done inside
> rte_net_intel_cksum_flags_prepare() in RTE_LIBRTE_ETHDEV_DEBUG build
> because it is moved from rte_validate_tx_offload() which is called
> under debug only.
> 
> Make corresponding check to be done in non-debug build as well
> to avoid bad accesses, incorrect checksum calculation and to
> return appropriate error from Tx prepare.
> 
> Make no-offloads check more precise and do it in non-debug build
> as well to avoid contiguous headers check and Tx prepare failure
> if it is not actually required.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Acked-by: Olivier Matz <olivier.matz@6wind.com>

Applied to dpdk-next-net/master, thanks.

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2020-07-21  0:53 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-05-27 14:32 [dpdk-dev] [PATCH] net: do fragmented headers check in non-debug build as well Andrew Rybchenko
2020-07-13 13:35 ` Olivier Matz
2020-07-13 14:22   ` Andrew Rybchenko
2020-07-13 14:22 ` [dpdk-dev] [PATCH v2] " Andrew Rybchenko
2020-07-21  0:53   ` Ferruh Yigit

DPDK patches and discussions

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://inbox.dpdk.org/dev/0 dev/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 dev dev/ https://inbox.dpdk.org/dev \
		dev@dpdk.org
	public-inbox-index dev

Example config snippet for mirrors.
Newsgroup available over NNTP:
	nntp://inbox.dpdk.org/inbox.dpdk.dev


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git