diff mbox series

[v3,3/5] lib: move mbuf next pointer to first cache line

Message ID 7cc8107a4a83dbfe16abd1f24150b0db6cf1041b.1663767715.git.sthotton@marvell.com (mailing list archive)
State Superseded, archived
Delegated to: Thomas Monjalon
Headers
Series mbuf dynamic field expansion |

Checks

Context Check Description
ci/checkpatch success coding style OK

Commit Message

Shijith Thotton Sept. 21, 2022, 1:56 p.m. UTC 
  Swapped position of mbuf next pointer and second dynamic field (dynfield2)
if the build is configured to use IOVA as VA. This is to move the mbuf
next pointer to first cache line. kni library is disabled for this
change as it depends on the offset value of next pointer.

Signed-off-by: Shijith Thotton <sthotton@marvell.com>
---
 lib/mbuf/rte_mbuf_core.h | 29 +++++++++++++++++++++--------
 lib/meson.build          |  3 +++
 2 files changed, 24 insertions(+), 8 deletions(-)

Comments

Morten Brørup Sept. 21, 2022, 2:07 p.m. UTC | #1 
> From: Shijith Thotton [mailto:sthotton@marvell.com]
> Sent: Wednesday, 21 September 2022 15.56
> 
> Swapped position of mbuf next pointer and second dynamic field
> (dynfield2)
> if the build is configured to use IOVA as VA. This is to move the mbuf
> next pointer to first cache line. kni library is disabled for this
> change as it depends on the offset value of next pointer.
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---

Series-Acked-by: Morten Brørup <mb@smartsharesystems.com>
Olivier Matz Sept. 28, 2022, 12:52 p.m. UTC | #2 
On Wed, Sep 21, 2022 at 07:26:19PM +0530, Shijith Thotton wrote:
> Swapped position of mbuf next pointer and second dynamic field (dynfield2)
> if the build is configured to use IOVA as VA. This is to move the mbuf
> next pointer to first cache line. kni library is disabled for this
> change as it depends on the offset value of next pointer.
> 
> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
> ---
>  lib/mbuf/rte_mbuf_core.h | 29 +++++++++++++++++++++--------
>  lib/meson.build          |  3 +++
>  2 files changed, 24 insertions(+), 8 deletions(-)
> 
> diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
> index 94907f301d..915dcd8653 100644
> --- a/lib/mbuf/rte_mbuf_core.h
> +++ b/lib/mbuf/rte_mbuf_core.h
> @@ -590,11 +590,14 @@ struct rte_mbuf {
>  		 * working on vector drivers easier.
>  		 */
>  		rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
> +#if RTE_IOVA_AS_VA
>  		/**
> -		 * Reserved for dynamic field in builds where physical address
> -		 * field is invalid.
> +		 * Next segment of scattered packet.
> +		 * This field is valid when physical address field is invalid.
> +		 * Otherwise next pointer in the second cache line will be used.
>  		 */
> -		uint64_t dynfield2;
> +		struct rte_mbuf *next;
> +#endif
>  	};
>  
>  	/* next 8 bytes are initialised on RX descriptor rearm */
> @@ -711,11 +714,21 @@ struct rte_mbuf {
>  	/* second cache line - fields only used in slow path or on TX */
>  	RTE_MARKER cacheline1 __rte_cache_min_aligned;
>  
> -	/**
> -	 * Next segment of scattered packet. Must be NULL in the last segment or
> -	 * in case of non-segmented packet.
> -	 */
> -	struct rte_mbuf *next;
> +	RTE_STD_C11
> +	union {
> +#if !RTE_IOVA_AS_VA
> +		/**
> +		 * Next segment of scattered packet. Must be NULL in the last
> +		 * segment or in case of non-segmented packet.
> +		 */
> +		struct rte_mbuf *next;
> +#endif
> +		/**
> +		 * Reserved for dynamic field when the next pointer is in first
> +		 * cache line (i.e. RTE_IOVA_AS_VA is 1).
> +		 */
> +		uint64_t dynfield2;
> +	};

Same comment than other patches about union vs #if.

>  
>  	/* fields to support TX offloads */
>  	RTE_STD_C11
> diff --git a/lib/meson.build b/lib/meson.build
> index c648f7d800..73d93bc803 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -88,6 +88,9 @@ optional_libs = [
>  disabled_libs = []
>  opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
>          check: true).stdout().split()
> +if dpdk_conf.get('RTE_IOVA_AS_VA') == 1
> +    opt_disabled_libs += ['kni']
> +endif

I guess this should be in the previous patch instead, since kni uses
m->buf_iova

>  foreach l:opt_disabled_libs
>      if not optional_libs.contains(l)
>          warning('Cannot disable mandatory library "@0@"'.format(l))


After this change, the documentation of RTE_IOVA_AS_VA can be enhanced to
explain that it also moves the next pointer to the first cache line, possibly
increasing the performance.
Shijith Thotton Sept. 29, 2022, 6:14 a.m. UTC | #3 
>> Swapped position of mbuf next pointer and second dynamic field (dynfield2)
>> if the build is configured to use IOVA as VA. This is to move the mbuf
>> next pointer to first cache line. kni library is disabled for this
>> change as it depends on the offset value of next pointer.
>>
>> Signed-off-by: Shijith Thotton <sthotton@marvell.com>
>> ---
>>  lib/mbuf/rte_mbuf_core.h | 29 +++++++++++++++++++++--------
>>  lib/meson.build          |  3 +++
>>  2 files changed, 24 insertions(+), 8 deletions(-)
>>
>> diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
>> index 94907f301d..915dcd8653 100644
>> --- a/lib/mbuf/rte_mbuf_core.h
>> +++ b/lib/mbuf/rte_mbuf_core.h
>> @@ -590,11 +590,14 @@ struct rte_mbuf {
>>  		 * working on vector drivers easier.
>>  		 */
>>  		rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
>> +#if RTE_IOVA_AS_VA
>>  		/**
>> -		 * Reserved for dynamic field in builds where physical address
>> -		 * field is invalid.
>> +		 * Next segment of scattered packet.
>> +		 * This field is valid when physical address field is invalid.
>> +		 * Otherwise next pointer in the second cache line will be used.
>>  		 */
>> -		uint64_t dynfield2;
>> +		struct rte_mbuf *next;
>> +#endif
>>  	};
>>
>>  	/* next 8 bytes are initialised on RX descriptor rearm */
>> @@ -711,11 +714,21 @@ struct rte_mbuf {
>>  	/* second cache line - fields only used in slow path or on TX */
>>  	RTE_MARKER cacheline1 __rte_cache_min_aligned;
>>
>> -	/**
>> -	 * Next segment of scattered packet. Must be NULL in the last segment or
>> -	 * in case of non-segmented packet.
>> -	 */
>> -	struct rte_mbuf *next;
>> +	RTE_STD_C11
>> +	union {
>> +#if !RTE_IOVA_AS_VA
>> +		/**
>> +		 * Next segment of scattered packet. Must be NULL in the last
>> +		 * segment or in case of non-segmented packet.
>> +		 */
>> +		struct rte_mbuf *next;
>> +#endif
>> +		/**
>> +		 * Reserved for dynamic field when the next pointer is in first
>> +		 * cache line (i.e. RTE_IOVA_AS_VA is 1).
>> +		 */
>> +		uint64_t dynfield2;
>> +	};
>
>Same comment than other patches about union vs #if.
 
Okay. Will change.

>
>>
>>  	/* fields to support TX offloads */
>>  	RTE_STD_C11
>> diff --git a/lib/meson.build b/lib/meson.build
>> index c648f7d800..73d93bc803 100644
>> --- a/lib/meson.build
>> +++ b/lib/meson.build
>> @@ -88,6 +88,9 @@ optional_libs = [
>>  disabled_libs = []
>>  opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
>>          check: true).stdout().split()
>> +if dpdk_conf.get('RTE_IOVA_AS_VA') == 1
>> +    opt_disabled_libs += ['kni']
>> +endif
>
>I guess this should be in the previous patch instead, since kni uses
>m->buf_iova
>
 
Ack.

>>  foreach l:opt_disabled_libs
>>      if not optional_libs.contains(l)
>>          warning('Cannot disable mandatory library "@0@"'.format(l))
>
>
>After this change, the documentation of RTE_IOVA_AS_VA can be enhanced to
>explain that it also moves the next pointer to the first cache line, possibly
>increasing the performance.
 
Okay.
diff mbox series

Patch

diff --git a/lib/mbuf/rte_mbuf_core.h b/lib/mbuf/rte_mbuf_core.h
index 94907f301d..915dcd8653 100644
--- a/lib/mbuf/rte_mbuf_core.h
+++ b/lib/mbuf/rte_mbuf_core.h
@@ -590,11 +590,14 @@  struct rte_mbuf {
 		 * working on vector drivers easier.
 		 */
 		rte_iova_t buf_iova __rte_aligned(sizeof(rte_iova_t));
+#if RTE_IOVA_AS_VA
 		/**
-		 * Reserved for dynamic field in builds where physical address
-		 * field is invalid.
+		 * Next segment of scattered packet.
+		 * This field is valid when physical address field is invalid.
+		 * Otherwise next pointer in the second cache line will be used.
 		 */
-		uint64_t dynfield2;
+		struct rte_mbuf *next;
+#endif
 	};
 
 	/* next 8 bytes are initialised on RX descriptor rearm */
@@ -711,11 +714,21 @@  struct rte_mbuf {
 	/* second cache line - fields only used in slow path or on TX */
 	RTE_MARKER cacheline1 __rte_cache_min_aligned;
 
-	/**
-	 * Next segment of scattered packet. Must be NULL in the last segment or
-	 * in case of non-segmented packet.
-	 */
-	struct rte_mbuf *next;
+	RTE_STD_C11
+	union {
+#if !RTE_IOVA_AS_VA
+		/**
+		 * Next segment of scattered packet. Must be NULL in the last
+		 * segment or in case of non-segmented packet.
+		 */
+		struct rte_mbuf *next;
+#endif
+		/**
+		 * Reserved for dynamic field when the next pointer is in first
+		 * cache line (i.e. RTE_IOVA_AS_VA is 1).
+		 */
+		uint64_t dynfield2;
+	};
 
 	/* fields to support TX offloads */
 	RTE_STD_C11
diff --git a/lib/meson.build b/lib/meson.build
index c648f7d800..73d93bc803 100644
--- a/lib/meson.build
+++ b/lib/meson.build
@@ -88,6 +88,9 @@  optional_libs = [
 disabled_libs = []
 opt_disabled_libs = run_command(list_dir_globs, get_option('disable_libs'),
         check: true).stdout().split()
+if dpdk_conf.get('RTE_IOVA_AS_VA') == 1
+    opt_disabled_libs += ['kni']
+endif
 foreach l:opt_disabled_libs
     if not optional_libs.contains(l)
         warning('Cannot disable mandatory library "@0@"'.format(l))