All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC PATCH] mbuf: move headers not fragmented check to checksum
@ 2019-01-29  8:49 Andrew Rybchenko
  2019-02-13  9:50 ` Andrew Rybchenko
                   ` (3 more replies)
  0 siblings, 4 replies; 11+ messages in thread
From: Andrew Rybchenko @ 2019-01-29  8:49 UTC (permalink / raw)
  To: Tomasz Kulasek, Olivier Matz
  Cc: dev, Konstantin Ananyev, Thomas Monjalon, Stephen Hemminger

rte_validate_tx_offload() is used in Tx prepare callbacks
(RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
Requirement that packet headers should not be fragmented is not
documented and unclear where it comes from except
rte_net_intel_cksum_prepare() functions which relies on it.

It could be NIC vendor specific driver or hardware limitation, but,
if so, it should be documented and checked in corresponding Tx
prepare callbacks.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
May be the check should be done in rte_net_intel_cksum_prepare()
under RTE_LIBRTE_ETHDEV_DEBUG only. Mainly I'd like to get feedback
on where the limitation comes from and idea to remove it from
rte_validate_tx_offload().

 lib/librte_mbuf/rte_mbuf.h | 12 ------------
 lib/librte_net/rte_net.h   | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index a7f67023a..14a3b472b 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2257,23 +2257,11 @@ static inline int
 rte_validate_tx_offload(const struct rte_mbuf *m)
 {
 	uint64_t ol_flags = m->ol_flags;
-	uint64_t inner_l3_offset = m->l2_len;
 
 	/* Does packet set any of available offloads? */
 	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
 		return 0;
 
-	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
-		/* NB: elaborating the addition like this instead of using
-		 *     += gives the result uint64_t type instead of int,
-		 *     avoiding compiler warnings on gcc 8.1 at least */
-		inner_l3_offset = inner_l3_offset + m->outer_l2_len +
-				  m->outer_l3_len;
-
-	/* Headers are fragmented */
-	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
-		return -ENOTSUP;
-
 	/* IP checksum can be counted only for IPv4 packet */
 	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
 		return -EINVAL;
diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index e59760a0a..bd75aea8e 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -118,10 +118,27 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	struct udp_hdr *udp_hdr;
 	uint64_t inner_l3_offset = m->l2_len;
 
+	/*
+	 * 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))
+		return 0;
+
 	if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) ||
 		(ol_flags & PKT_TX_OUTER_IPV6))
 		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
 
+	/*
+	 * Check if headers are fragmented.
+	 * The check could be less strict depending on which offloads are
+	 * requested and headers to be used, but let's keep it simple.
+	 */
+	if (unlikely(rte_pktmbuf_data_len(m) <
+		     inner_l3_offset + m->l3_len + m->l4_len))
+		return -ENOTSUP;
+
 	if (ol_flags & PKT_TX_IPV4) {
 		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
 				inner_l3_offset);
-- 
2.17.1

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

* Re: [RFC PATCH] mbuf: move headers not fragmented check to checksum
  2019-01-29  8:49 [RFC PATCH] mbuf: move headers not fragmented check to checksum Andrew Rybchenko
@ 2019-02-13  9:50 ` Andrew Rybchenko
  2019-02-13 14:48   ` Wiles, Keith
  2019-02-13 23:27 ` Ananyev, Konstantin
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 11+ messages in thread
From: Andrew Rybchenko @ 2019-02-13  9:50 UTC (permalink / raw)
  To: Tomasz Kulasek, Olivier Matz
  Cc: dev, Konstantin Ananyev, Thomas Monjalon, Stephen Hemminger

Ping.

Do 2 weeks without reply mean that it looks good and I should send 
non-RCF version?

Andrew.

On 1/29/19 11:49 AM, Andrew Rybchenko wrote:
> rte_validate_tx_offload() is used in Tx prepare callbacks
> (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
> Requirement that packet headers should not be fragmented is not
> documented and unclear where it comes from except
> rte_net_intel_cksum_prepare() functions which relies on it.
>
> It could be NIC vendor specific driver or hardware limitation, but,
> if so, it should be documented and checked in corresponding Tx
> prepare callbacks.
>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> May be the check should be done in rte_net_intel_cksum_prepare()
> under RTE_LIBRTE_ETHDEV_DEBUG only. Mainly I'd like to get feedback
> on where the limitation comes from and idea to remove it from
> rte_validate_tx_offload().
>
>   lib/librte_mbuf/rte_mbuf.h | 12 ------------
>   lib/librte_net/rte_net.h   | 17 +++++++++++++++++
>   2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index a7f67023a..14a3b472b 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -2257,23 +2257,11 @@ static inline int
>   rte_validate_tx_offload(const struct rte_mbuf *m)
>   {
>   	uint64_t ol_flags = m->ol_flags;
> -	uint64_t inner_l3_offset = m->l2_len;
>   
>   	/* Does packet set any of available offloads? */
>   	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
>   		return 0;
>   
> -	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
> -		/* NB: elaborating the addition like this instead of using
> -		 *     += gives the result uint64_t type instead of int,
> -		 *     avoiding compiler warnings on gcc 8.1 at least */
> -		inner_l3_offset = inner_l3_offset + m->outer_l2_len +
> -				  m->outer_l3_len;
> -
> -	/* Headers are fragmented */
> -	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
> -		return -ENOTSUP;
> -
>   	/* IP checksum can be counted only for IPv4 packet */
>   	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
>   		return -EINVAL;
> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> index e59760a0a..bd75aea8e 100644
> --- a/lib/librte_net/rte_net.h
> +++ b/lib/librte_net/rte_net.h
> @@ -118,10 +118,27 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>   	struct udp_hdr *udp_hdr;
>   	uint64_t inner_l3_offset = m->l2_len;
>   
> +	/*
> +	 * 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))
> +		return 0;
> +
>   	if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) ||
>   		(ol_flags & PKT_TX_OUTER_IPV6))
>   		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>   
> +	/*
> +	 * Check if headers are fragmented.
> +	 * The check could be less strict depending on which offloads are
> +	 * requested and headers to be used, but let's keep it simple.
> +	 */
> +	if (unlikely(rte_pktmbuf_data_len(m) <
> +		     inner_l3_offset + m->l3_len + m->l4_len))
> +		return -ENOTSUP;
> +
>   	if (ol_flags & PKT_TX_IPV4) {
>   		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
>   				inner_l3_offset);

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

* Re: [RFC PATCH] mbuf: move headers not fragmented check to checksum
  2019-02-13  9:50 ` Andrew Rybchenko
@ 2019-02-13 14:48   ` Wiles, Keith
  0 siblings, 0 replies; 11+ messages in thread
From: Wiles, Keith @ 2019-02-13 14:48 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Kulasek, TomaszX, Olivier Matz, dev, Ananyev, Konstantin,
	Thomas Monjalon, Stephen Hemminger



> On Feb 13, 2019, at 3:50 AM, Andrew Rybchenko <arybchenko@solarflare.com> wrote:
> 
> Ping.
> 
> Do 2 weeks without reply mean that it looks good and I should send non-RCF version?

Just send the non-RFC patch as it seems most do not even look at the RFC’s anyway.
> 
> Andrew.
> 
> On 1/29/19 11:49 AM, Andrew Rybchenko wrote:
>> rte_validate_tx_offload() is used in Tx prepare callbacks
>> (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
>> Requirement that packet headers should not be fragmented is not
>> documented and unclear where it comes from except
>> rte_net_intel_cksum_prepare() functions which relies on it.
>> 
>> It could be NIC vendor specific driver or hardware limitation, but,
>> if so, it should be documented and checked in corresponding Tx
>> prepare callbacks.
>> 
>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>> ---
>> May be the check should be done in rte_net_intel_cksum_prepare()
>> under RTE_LIBRTE_ETHDEV_DEBUG only. Mainly I'd like to get feedback
>> on where the limitation comes from and idea to remove it from
>> rte_validate_tx_offload().
>> 
>>  lib/librte_mbuf/rte_mbuf.h | 12 ------------
>>  lib/librte_net/rte_net.h   | 17 +++++++++++++++++
>>  2 files changed, 17 insertions(+), 12 deletions(-)
>> 
>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>> index a7f67023a..14a3b472b 100644
>> --- a/lib/librte_mbuf/rte_mbuf.h
>> +++ b/lib/librte_mbuf/rte_mbuf.h
>> @@ -2257,23 +2257,11 @@ static inline int
>>  rte_validate_tx_offload(const struct rte_mbuf *m)
>>  {
>>  	uint64_t ol_flags = m->ol_flags;
>> -	uint64_t inner_l3_offset = m->l2_len;
>>    	/* Does packet set any of available offloads? */
>>  	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
>>  		return 0;
>>  -	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
>> -		/* NB: elaborating the addition like this instead of using
>> -		 *     += gives the result uint64_t type instead of int,
>> -		 *     avoiding compiler warnings on gcc 8.1 at least */
>> -		inner_l3_offset = inner_l3_offset + m->outer_l2_len +
>> -				  m->outer_l3_len;
>> -
>> -	/* Headers are fragmented */
>> -	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
>> -		return -ENOTSUP;
>> -
>>  	/* IP checksum can be counted only for IPv4 packet */
>>  	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
>>  		return -EINVAL;
>> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
>> index e59760a0a..bd75aea8e 100644
>> --- a/lib/librte_net/rte_net.h
>> +++ b/lib/librte_net/rte_net.h
>> @@ -118,10 +118,27 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>>  	struct udp_hdr *udp_hdr;
>>  	uint64_t inner_l3_offset = m->l2_len;
>>  +	/*
>> +	 * 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))
>> +		return 0;
>> +
>>  	if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) ||
>>  		(ol_flags & PKT_TX_OUTER_IPV6))
>>  		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>>  +	/*
>> +	 * Check if headers are fragmented.
>> +	 * The check could be less strict depending on which offloads are
>> +	 * requested and headers to be used, but let's keep it simple.
>> +	 */
>> +	if (unlikely(rte_pktmbuf_data_len(m) <
>> +		     inner_l3_offset + m->l3_len + m->l4_len))
>> +		return -ENOTSUP;
>> +
>>  	if (ol_flags & PKT_TX_IPV4) {
>>  		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
>>  				inner_l3_offset);
> 

Regards,
Keith


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

* Re: [RFC PATCH] mbuf: move headers not fragmented check to checksum
  2019-01-29  8:49 [RFC PATCH] mbuf: move headers not fragmented check to checksum Andrew Rybchenko
  2019-02-13  9:50 ` Andrew Rybchenko
@ 2019-02-13 23:27 ` Ananyev, Konstantin
  2019-02-19  6:30 ` [PATCH] " Andrew Rybchenko
  2019-03-29 13:42 ` [PATCH v2] " Andrew Rybchenko
  3 siblings, 0 replies; 11+ messages in thread
From: Ananyev, Konstantin @ 2019-02-13 23:27 UTC (permalink / raw)
  To: Andrew Rybchenko, Kulasek, TomaszX, Olivier Matz
  Cc: dev, Thomas Monjalon, Stephen Hemminger


Hi Andrew,
 
> rte_validate_tx_offload() is used in Tx prepare callbacks
> (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
> Requirement that packet headers should not be fragmented is not
> documented and unclear where it comes from except
> rte_net_intel_cksum_prepare() functions which relies on it.
> 
> It could be NIC vendor specific driver or hardware limitation, but,
> if so, it should be documented and checked in corresponding Tx
> prepare callbacks.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> ---
> May be the check should be done in rte_net_intel_cksum_prepare()
> under RTE_LIBRTE_ETHDEV_DEBUG only. Mainly I'd like to get feedback
> on where the limitation comes from and idea to remove it from
> rte_validate_tx_offload().
> 
>  lib/librte_mbuf/rte_mbuf.h | 12 ------------
>  lib/librte_net/rte_net.h   | 17 +++++++++++++++++
>  2 files changed, 17 insertions(+), 12 deletions(-)
> 
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index a7f67023a..14a3b472b 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -2257,23 +2257,11 @@ static inline int
>  rte_validate_tx_offload(const struct rte_mbuf *m)
>  {
>  	uint64_t ol_flags = m->ol_flags;
> -	uint64_t inner_l3_offset = m->l2_len;
> 
>  	/* Does packet set any of available offloads? */
>  	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
>  		return 0;
> 
> -	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
> -		/* NB: elaborating the addition like this instead of using
> -		 *     += gives the result uint64_t type instead of int,
> -		 *     avoiding compiler warnings on gcc 8.1 at least */
> -		inner_l3_offset = inner_l3_offset + m->outer_l2_len +
> -				  m->outer_l3_len;
> -
> -	/* Headers are fragmented */
> -	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
> -		return -ENOTSUP;
> -
>  	/* IP checksum can be counted only for IPv4 packet */
>  	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
>  		return -EINVAL;
> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> index e59760a0a..bd75aea8e 100644
> --- a/lib/librte_net/rte_net.h
> +++ b/lib/librte_net/rte_net.h
> @@ -118,10 +118,27 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
>  	struct udp_hdr *udp_hdr;
>  	uint64_t inner_l3_offset = m->l2_len;
> 
> +	/*
> +	 * 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))
> +		return 0;
> +
>  	if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) ||
>  		(ol_flags & PKT_TX_OUTER_IPV6))
>  		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> 
> +	/*
> +	 * Check if headers are fragmented.
> +	 * The check could be less strict depending on which offloads are
> +	 * requested and headers to be used, but let's keep it simple.
> +	 */
> +	if (unlikely(rte_pktmbuf_data_len(m) <
> +		     inner_l3_offset + m->l3_len + m->l4_len))
> +		return -ENOTSUP;
> +
>  	if (ol_flags & PKT_TX_IPV4) {
>  		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
>  				inner_l3_offset);
> --

Looks good to me, though extra-testing would be needed.
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>

> 2.17.1

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

* [PATCH] mbuf: move headers not fragmented check to checksum
  2019-01-29  8:49 [RFC PATCH] mbuf: move headers not fragmented check to checksum Andrew Rybchenko
  2019-02-13  9:50 ` Andrew Rybchenko
  2019-02-13 23:27 ` Ananyev, Konstantin
@ 2019-02-19  6:30 ` Andrew Rybchenko
  2019-03-28 17:04   ` Andrew Rybchenko
  2019-03-29 13:42 ` [PATCH v2] " Andrew Rybchenko
  3 siblings, 1 reply; 11+ messages in thread
From: Andrew Rybchenko @ 2019-02-19  6:30 UTC (permalink / raw)
  To: Tomasz Kulasek, Olivier Matz; +Cc: dev, Konstantin Ananyev

rte_validate_tx_offload() is used in Tx prepare callbacks
(RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
Requirement that packet headers should not be fragmented is not
documented and unclear where it comes from except
rte_net_intel_cksum_prepare() functions which relies on it.

It could be NIC vendor specific driver or hardware limitation, but,
if so, it should be documented and checked in corresponding Tx
prepare callbacks.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
---
Looks good to me, though extra-testing would be needed.
Konstantin Ananyev <konstantin.ananyev@intel.com>

 lib/librte_mbuf/rte_mbuf.h | 12 ------------
 lib/librte_net/rte_net.h   | 17 +++++++++++++++++
 2 files changed, 17 insertions(+), 12 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index d961cca..73daa81 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2257,23 +2257,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
 rte_validate_tx_offload(const struct rte_mbuf *m)
 {
 	uint64_t ol_flags = m->ol_flags;
-	uint64_t inner_l3_offset = m->l2_len;
 
 	/* Does packet set any of available offloads? */
 	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
 		return 0;
 
-	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
-		/* NB: elaborating the addition like this instead of using
-		 *     += gives the result uint64_t type instead of int,
-		 *     avoiding compiler warnings on gcc 8.1 at least */
-		inner_l3_offset = inner_l3_offset + m->outer_l2_len +
-				  m->outer_l3_len;
-
-	/* Headers are fragmented */
-	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
-		return -ENOTSUP;
-
 	/* IP checksum can be counted only for IPv4 packet */
 	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
 		return -EINVAL;
diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index e59760a..bd75aea 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -118,10 +118,27 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
 	struct udp_hdr *udp_hdr;
 	uint64_t inner_l3_offset = m->l2_len;
 
+	/*
+	 * 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))
+		return 0;
+
 	if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) ||
 		(ol_flags & PKT_TX_OUTER_IPV6))
 		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
 
+	/*
+	 * Check if headers are fragmented.
+	 * The check could be less strict depending on which offloads are
+	 * requested and headers to be used, but let's keep it simple.
+	 */
+	if (unlikely(rte_pktmbuf_data_len(m) <
+		     inner_l3_offset + m->l3_len + m->l4_len))
+		return -ENOTSUP;
+
 	if (ol_flags & PKT_TX_IPV4) {
 		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
 				inner_l3_offset);
-- 
1.8.3.1

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

* Re: [PATCH] mbuf: move headers not fragmented check to checksum
  2019-02-19  6:30 ` [PATCH] " Andrew Rybchenko
@ 2019-03-28 17:04   ` Andrew Rybchenko
  2019-03-29 13:09     ` Olivier Matz
  0 siblings, 1 reply; 11+ messages in thread
From: Andrew Rybchenko @ 2019-03-28 17:04 UTC (permalink / raw)
  To: Tomasz Kulasek, Olivier Matz
  Cc: dev, Konstantin Ananyev, Thomas Monjalon, Ferruh Yigit

Ping? (I have a number of net/sfc patches which heavily depend on this
one and must not be applied without this one)

Andrew.

On 2/19/19 9:30 AM, Andrew Rybchenko wrote:
> rte_validate_tx_offload() is used in Tx prepare callbacks
> (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
> Requirement that packet headers should not be fragmented is not
> documented and unclear where it comes from except
> rte_net_intel_cksum_prepare() functions which relies on it.
>
> It could be NIC vendor specific driver or hardware limitation, but,
> if so, it should be documented and checked in corresponding Tx
> prepare callbacks.
>
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> ---
> Looks good to me, though extra-testing would be needed.
> Konstantin Ananyev <konstantin.ananyev@intel.com>
>
>   lib/librte_mbuf/rte_mbuf.h | 12 ------------
>   lib/librte_net/rte_net.h   | 17 +++++++++++++++++
>   2 files changed, 17 insertions(+), 12 deletions(-)
>
> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> index d961cca..73daa81 100644
> --- a/lib/librte_mbuf/rte_mbuf.h
> +++ b/lib/librte_mbuf/rte_mbuf.h
> @@ -2257,23 +2257,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
>   rte_validate_tx_offload(const struct rte_mbuf *m)
>   {
>   	uint64_t ol_flags = m->ol_flags;
> -	uint64_t inner_l3_offset = m->l2_len;
>   
>   	/* Does packet set any of available offloads? */
>   	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
>   		return 0;
>   
> -	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
> -		/* NB: elaborating the addition like this instead of using
> -		 *     += gives the result uint64_t type instead of int,
> -		 *     avoiding compiler warnings on gcc 8.1 at least */
> -		inner_l3_offset = inner_l3_offset + m->outer_l2_len +
> -				  m->outer_l3_len;
> -
> -	/* Headers are fragmented */
> -	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
> -		return -ENOTSUP;
> -
>   	/* IP checksum can be counted only for IPv4 packet */
>   	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
>   		return -EINVAL;
> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> index e59760a..bd75aea 100644
> --- a/lib/librte_net/rte_net.h
> +++ b/lib/librte_net/rte_net.h
> @@ -118,10 +118,27 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>   	struct udp_hdr *udp_hdr;
>   	uint64_t inner_l3_offset = m->l2_len;
>   
> +	/*
> +	 * 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))
> +		return 0;
> +
>   	if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) ||
>   		(ol_flags & PKT_TX_OUTER_IPV6))
>   		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>   
> +	/*
> +	 * Check if headers are fragmented.
> +	 * The check could be less strict depending on which offloads are
> +	 * requested and headers to be used, but let's keep it simple.
> +	 */
> +	if (unlikely(rte_pktmbuf_data_len(m) <
> +		     inner_l3_offset + m->l3_len + m->l4_len))
> +		return -ENOTSUP;
> +
>   	if (ol_flags & PKT_TX_IPV4) {
>   		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
>   				inner_l3_offset);

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

* Re: [PATCH] mbuf: move headers not fragmented check to checksum
  2019-03-28 17:04   ` Andrew Rybchenko
@ 2019-03-29 13:09     ` Olivier Matz
  2019-03-29 13:30       ` Andrew Rybchenko
  0 siblings, 1 reply; 11+ messages in thread
From: Olivier Matz @ 2019-03-29 13:09 UTC (permalink / raw)
  To: Andrew Rybchenko
  Cc: Tomasz Kulasek, dev, Konstantin Ananyev, Thomas Monjalon, Ferruh Yigit

Hi Andrew,

On Thu, Mar 28, 2019 at 08:04:31PM +0300, Andrew Rybchenko wrote:
> Ping? (I have a number of net/sfc patches which heavily depend on this
> one and must not be applied without this one)
> 
> Andrew.
> 
> On 2/19/19 9:30 AM, Andrew Rybchenko wrote:
> > rte_validate_tx_offload() is used in Tx prepare callbacks
> > (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
> > Requirement that packet headers should not be fragmented is not
> > documented and unclear where it comes from except
> > rte_net_intel_cksum_prepare() functions which relies on it.
> > 
> > It could be NIC vendor specific driver or hardware limitation, but,
> > if so, it should be documented and checked in corresponding Tx
> > prepare callbacks.
> > 
> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> > Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
> > ---
> > Looks good to me, though extra-testing would be needed.
> > Konstantin Ananyev <konstantin.ananyev@intel.com>
> > 
> >   lib/librte_mbuf/rte_mbuf.h | 12 ------------
> >   lib/librte_net/rte_net.h   | 17 +++++++++++++++++
> >   2 files changed, 17 insertions(+), 12 deletions(-)
> > 
> > diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
> > index d961cca..73daa81 100644
> > --- a/lib/librte_mbuf/rte_mbuf.h
> > +++ b/lib/librte_mbuf/rte_mbuf.h
> > @@ -2257,23 +2257,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
> >   rte_validate_tx_offload(const struct rte_mbuf *m)
> >   {
> >   	uint64_t ol_flags = m->ol_flags;
> > -	uint64_t inner_l3_offset = m->l2_len;
> >   	/* Does packet set any of available offloads? */
> >   	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
> >   		return 0;
> > -	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
> > -		/* NB: elaborating the addition like this instead of using
> > -		 *     += gives the result uint64_t type instead of int,
> > -		 *     avoiding compiler warnings on gcc 8.1 at least */
> > -		inner_l3_offset = inner_l3_offset + m->outer_l2_len +
> > -				  m->outer_l3_len;
> > -
> > -	/* Headers are fragmented */
> > -	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
> > -		return -ENOTSUP;
> > -
> >   	/* IP checksum can be counted only for IPv4 packet */
> >   	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
> >   		return -EINVAL;
> > diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
> > index e59760a..bd75aea 100644
> > --- a/lib/librte_net/rte_net.h
> > +++ b/lib/librte_net/rte_net.h
> > @@ -118,10 +118,27 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
> >   	struct udp_hdr *udp_hdr;
> >   	uint64_t inner_l3_offset = m->l2_len;
> > +	/*
> > +	 * 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))
> > +		return 0;
> > +
> >   	if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) ||
> >   		(ol_flags & PKT_TX_OUTER_IPV6))
> >   		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
> > +	/*
> > +	 * Check if headers are fragmented.
> > +	 * The check could be less strict depending on which offloads are
> > +	 * requested and headers to be used, but let's keep it simple.
> > +	 */
> > +	if (unlikely(rte_pktmbuf_data_len(m) <
> > +		     inner_l3_offset + m->l3_len + m->l4_len))
> > +		return -ENOTSUP;
> > +
> >   	if (ol_flags & PKT_TX_IPV4) {
> >   		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
> >   				inner_l3_offset);
> 


To summarize, the previous code was in a generic part, only enabled if
RTE_LIBRTE_ETHDEV_DEBUG is set, and it is moved in an intel-specific part,
but always enabled. Am I correct?

So it may have a performance impact on intel NICs. Shouldn't it be under
a debug option?

Regards,
Olivier

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

* Re: [PATCH] mbuf: move headers not fragmented check to checksum
  2019-03-29 13:09     ` Olivier Matz
@ 2019-03-29 13:30       ` Andrew Rybchenko
  0 siblings, 0 replies; 11+ messages in thread
From: Andrew Rybchenko @ 2019-03-29 13:30 UTC (permalink / raw)
  To: Olivier Matz
  Cc: Tomasz Kulasek, dev, Konstantin Ananyev, Thomas Monjalon, Ferruh Yigit

Hi Olivier,

On 3/29/19 4:09 PM, Olivier Matz wrote:
> Hi Andrew,
>
> On Thu, Mar 28, 2019 at 08:04:31PM +0300, Andrew Rybchenko wrote:
>> Ping? (I have a number of net/sfc patches which heavily depend on this
>> one and must not be applied without this one)
>>
>> Andrew.
>>
>> On 2/19/19 9:30 AM, Andrew Rybchenko wrote:
>>> rte_validate_tx_offload() is used in Tx prepare callbacks
>>> (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
>>> Requirement that packet headers should not be fragmented is not
>>> documented and unclear where it comes from except
>>> rte_net_intel_cksum_prepare() functions which relies on it.
>>>
>>> It could be NIC vendor specific driver or hardware limitation, but,
>>> if so, it should be documented and checked in corresponding Tx
>>> prepare callbacks.
>>>
>>> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
>>> Acked-by: Konstantin Ananyev <konstantin.ananyev@intel.com>
>>> ---
>>> Looks good to me, though extra-testing would be needed.
>>> Konstantin Ananyev <konstantin.ananyev@intel.com>
>>>
>>>    lib/librte_mbuf/rte_mbuf.h | 12 ------------
>>>    lib/librte_net/rte_net.h   | 17 +++++++++++++++++
>>>    2 files changed, 17 insertions(+), 12 deletions(-)
>>>
>>> diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
>>> index d961cca..73daa81 100644
>>> --- a/lib/librte_mbuf/rte_mbuf.h
>>> +++ b/lib/librte_mbuf/rte_mbuf.h
>>> @@ -2257,23 +2257,11 @@ static inline int rte_pktmbuf_chain(struct rte_mbuf *head, struct rte_mbuf *tail
>>>    rte_validate_tx_offload(const struct rte_mbuf *m)
>>>    {
>>>    	uint64_t ol_flags = m->ol_flags;
>>> -	uint64_t inner_l3_offset = m->l2_len;
>>>    	/* Does packet set any of available offloads? */
>>>    	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
>>>    		return 0;
>>> -	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
>>> -		/* NB: elaborating the addition like this instead of using
>>> -		 *     += gives the result uint64_t type instead of int,
>>> -		 *     avoiding compiler warnings on gcc 8.1 at least */
>>> -		inner_l3_offset = inner_l3_offset + m->outer_l2_len +
>>> -				  m->outer_l3_len;
>>> -
>>> -	/* Headers are fragmented */
>>> -	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
>>> -		return -ENOTSUP;
>>> -
>>>    	/* IP checksum can be counted only for IPv4 packet */
>>>    	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
>>>    		return -EINVAL;
>>> diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
>>> index e59760a..bd75aea 100644
>>> --- a/lib/librte_net/rte_net.h
>>> +++ b/lib/librte_net/rte_net.h
>>> @@ -118,10 +118,27 @@ uint32_t rte_net_get_ptype(const struct rte_mbuf *m,
>>>    	struct udp_hdr *udp_hdr;
>>>    	uint64_t inner_l3_offset = m->l2_len;
>>> +	/*
>>> +	 * 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))
>>> +		return 0;
>>> +
>>>    	if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) ||
>>>    		(ol_flags & PKT_TX_OUTER_IPV6))
>>>    		inner_l3_offset += m->outer_l2_len + m->outer_l3_len;
>>> +	/*
>>> +	 * Check if headers are fragmented.
>>> +	 * The check could be less strict depending on which offloads are
>>> +	 * requested and headers to be used, but let's keep it simple.
>>> +	 */
>>> +	if (unlikely(rte_pktmbuf_data_len(m) <
>>> +		     inner_l3_offset + m->l3_len + m->l4_len))
>>> +		return -ENOTSUP;
>>> +
>>>    	if (ol_flags & PKT_TX_IPV4) {
>>>    		ipv4_hdr = rte_pktmbuf_mtod_offset(m, struct ipv4_hdr *,
>>>    				inner_l3_offset);
>
> To summarize, the previous code was in a generic part, only enabled if
> RTE_LIBRTE_ETHDEV_DEBUG is set, and it is moved in an intel-specific part,
> but always enabled. Am I correct?

Yes, correct.

> So it may have a performance impact on intel NICs. Shouldn't it be under
> a debug option?

Yes, to be 100% equivalent.

May be making these checks non-debug is a separate story since IMHO
these checks should be non-debug. Below code really depends on these
checks and if the condition is violated it will read and could write outside
of provided buffer (bad checksums, spoiled memory etc).

I'll send v2 shortly with RTE_LIBRTE_ETHDEV_DEBUG to make it easy to
pickup finally chosen version.

Thanks,
Andrew.

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

* [PATCH v2] mbuf: move headers not fragmented check to checksum
  2019-01-29  8:49 [RFC PATCH] mbuf: move headers not fragmented check to checksum Andrew Rybchenko
                   ` (2 preceding siblings ...)
  2019-02-19  6:30 ` [PATCH] " Andrew Rybchenko
@ 2019-03-29 13:42 ` Andrew Rybchenko
  2019-03-29 14:18   ` Olivier Matz
  3 siblings, 1 reply; 11+ messages in thread
From: Andrew Rybchenko @ 2019-03-29 13:42 UTC (permalink / raw)
  To: Tomasz Kulasek, Olivier Matz; +Cc: dev, Konstantin Ananyev

rte_validate_tx_offload() is used in Tx prepare callbacks
(RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
Requirement that packet headers should not be fragmented is not
documented and unclear where it comes from except
rte_net_intel_cksum_prepare() functions which relies on it.

It could be NIC vendor specific driver or hardware limitation, but,
if so, it should be documented and checked in corresponding Tx
prepare callbacks.

Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
---
 lib/librte_mbuf/rte_mbuf.h | 12 ------------
 lib/librte_net/rte_net.h   | 21 +++++++++++++++++++++
 2 files changed, 21 insertions(+), 12 deletions(-)

diff --git a/lib/librte_mbuf/rte_mbuf.h b/lib/librte_mbuf/rte_mbuf.h
index d961ccaf6..73daa8140 100644
--- a/lib/librte_mbuf/rte_mbuf.h
+++ b/lib/librte_mbuf/rte_mbuf.h
@@ -2257,23 +2257,11 @@ static inline int
 rte_validate_tx_offload(const struct rte_mbuf *m)
 {
 	uint64_t ol_flags = m->ol_flags;
-	uint64_t inner_l3_offset = m->l2_len;
 
 	/* Does packet set any of available offloads? */
 	if (!(ol_flags & PKT_TX_OFFLOAD_MASK))
 		return 0;
 
-	if (ol_flags & PKT_TX_OUTER_IP_CKSUM)
-		/* NB: elaborating the addition like this instead of using
-		 *     += gives the result uint64_t type instead of int,
-		 *     avoiding compiler warnings on gcc 8.1 at least */
-		inner_l3_offset = inner_l3_offset + m->outer_l2_len +
-				  m->outer_l3_len;
-
-	/* Headers are fragmented */
-	if (rte_pktmbuf_data_len(m) < inner_l3_offset + m->l3_len + m->l4_len)
-		return -ENOTSUP;
-
 	/* IP checksum can be counted only for IPv4 packet */
 	if ((ol_flags & PKT_TX_IP_CKSUM) && (ol_flags & PKT_TX_IPV6))
 		return -EINVAL;
diff --git a/lib/librte_net/rte_net.h b/lib/librte_net/rte_net.h
index e59760a0a..16970711b 100644
--- a/lib/librte_net/rte_net.h
+++ b/lib/librte_net/rte_net.h
@@ -118,10 +118,31 @@ rte_net_intel_cksum_flags_prepare(struct rte_mbuf *m, uint64_t ol_flags)
 	struct 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))
+		return 0;
+#endif
+
 	if ((ol_flags & PKT_TX_OUTER_IP_CKSUM) ||
 		(ol_flags & 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
+	 * requested and headers to be used, but let's keep it simple.
+	 */
+	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 ipv4_hdr *,
 				inner_l3_offset);
-- 
2.17.1

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

* Re: [PATCH v2] mbuf: move headers not fragmented check to checksum
  2019-03-29 13:42 ` [PATCH v2] " Andrew Rybchenko
@ 2019-03-29 14:18   ` Olivier Matz
  2019-04-02 14:48     ` Thomas Monjalon
  0 siblings, 1 reply; 11+ messages in thread
From: Olivier Matz @ 2019-03-29 14:18 UTC (permalink / raw)
  To: Andrew Rybchenko; +Cc: Tomasz Kulasek, dev, Konstantin Ananyev

On Fri, Mar 29, 2019 at 01:42:14PM +0000, Andrew Rybchenko wrote:
> rte_validate_tx_offload() is used in Tx prepare callbacks
> (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
> Requirement that packet headers should not be fragmented is not
> documented and unclear where it comes from except
> rte_net_intel_cksum_prepare() functions which relies on it.
> 
> It could be NIC vendor specific driver or hardware limitation, but,
> if so, it should be documented and checked in corresponding Tx
> prepare callbacks.
> 
> Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>

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

Thanks!

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

* Re: [PATCH v2] mbuf: move headers not fragmented check to checksum
  2019-03-29 14:18   ` Olivier Matz
@ 2019-04-02 14:48     ` Thomas Monjalon
  0 siblings, 0 replies; 11+ messages in thread
From: Thomas Monjalon @ 2019-04-02 14:48 UTC (permalink / raw)
  To: Andrew Rybchenko; +Cc: dev, Olivier Matz, Tomasz Kulasek, Konstantin Ananyev

29/03/2019 15:18, Olivier Matz:
> On Fri, Mar 29, 2019 at 01:42:14PM +0000, Andrew Rybchenko wrote:
> > rte_validate_tx_offload() is used in Tx prepare callbacks
> > (RTE_LIBRTE_ETHDEV_DEBUG only) to check Tx offloads consistency.
> > Requirement that packet headers should not be fragmented is not
> > documented and unclear where it comes from except
> > rte_net_intel_cksum_prepare() functions which relies on it.
> > 
> > It could be NIC vendor specific driver or hardware limitation, but,
> > if so, it should be documented and checked in corresponding Tx
> > prepare callbacks.
> > 
> > Signed-off-by: Andrew Rybchenko <arybchenko@solarflare.com>
> 
> Acked-by: Olivier Matz <olivier.matz@6wind.com>
> 
> Thanks!

Applied with a more explicit title:
	mbuf: remove Intel offload checks from generic API

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

end of thread, other threads:[~2019-04-02 14:48 UTC | newest]

Thread overview: 11+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-29  8:49 [RFC PATCH] mbuf: move headers not fragmented check to checksum Andrew Rybchenko
2019-02-13  9:50 ` Andrew Rybchenko
2019-02-13 14:48   ` Wiles, Keith
2019-02-13 23:27 ` Ananyev, Konstantin
2019-02-19  6:30 ` [PATCH] " Andrew Rybchenko
2019-03-28 17:04   ` Andrew Rybchenko
2019-03-29 13:09     ` Olivier Matz
2019-03-29 13:30       ` Andrew Rybchenko
2019-03-29 13:42 ` [PATCH v2] " Andrew Rybchenko
2019-03-29 14:18   ` Olivier Matz
2019-04-02 14:48     ` Thomas Monjalon

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.