[PATCH v2] Misc fast path optimizations #281

MatiasElo · 2017-11-07T14:37:22Z

Improve fast path performance by reorganizing and cleaning buffer headers.

V2:

Add pktio capability to pktio_entry_t (Maxim)

lumag · 2017-11-07T19:09:33Z

platform/linux-generic/include/odp/api/plat/packet_inlines.h

@@ -21,6 +21,9 @@
 /** @internal Inline function offsets */
 extern const _odp_packet_inline_offset_t _odp_packet_inline;

+/** @internal Pool inline function offsets */
+extern const _odp_pool_inline_offset_t _odp_pool_inline;


This should go to pool_inlines.h

pool_inlines.h would only include these two lines and looking at pool API there aren't any additional functions which should be inlined. Do you have a particular use case in mind for this header?

Just wanted to have pool functions in pool header, etc.

Since there are currently no functions in sight to put into pool_inlines.h I would suggest not to add a new header file. If/when some pool inline function are added later on the header can be created them. Is this OK for you?

@MatiasElo ok.

The API being inlined is odp_packet_pool() so this is an artifact of the implementation of that API. So I'm OK with it being here since this is supposed to be fastpath code.

lumag · 2017-11-07T19:11:09Z

platform/linux-generic/include/odp/api/plat/pool_types.h

+	uint16_t uarea_size;
+
+} _odp_pool_inline_offset_t;
+


And this to pool_inline_types.h.

I followed the same pattern as the packet inlines. _odp_packet_inline_offset_t is defined in packet_types.h.

@MatiasElo I agree this is correctly placed here.

lumag · 2017-11-07T19:11:32Z

@MatiasElo first patches look good, Reviewed-by: Dmitry Eremin-Solenikov

Last one might clash with #250

nagarahalli

Do you have any performance measurement results with these changes?

nagarahalli · 2017-11-08T16:01:47Z

platform/linux-generic/include/odp_buffer_internal.h

@@ -104,17 +104,10 @@ struct odp_buffer_hdr_t {
 	/* User area pointer */
 	void    *uarea_addr;

-	/* User area size */


This change is done already in 2.0.

@nagarahalli Any reason why we shouldn't have it in master?

It is about 2.0 merge to master, it will happen sometime in the future.

This is not planned untill TigerMoth.

2.0 is based on api-next, not master. This will be one of several conflicts that will need to be resolved at merge time.

muvarov · 2017-11-08T19:16:49Z

platform/linux-generic/pktio/dpdk.c

@@ -1079,7 +1079,7 @@ static int dpdk_input_queues_config(pktio_entry_t *pktio_entry,
 				    const odp_pktin_queue_param_t *p)
 {
 	odp_pktin_mode_t mode = pktio_entry->s.param.in_mode;
-	odp_bool_t lockless;
+	uint8_t lockless;


why is that needed?

While packing the pkt_dpdk_t struct I changed pkt_dpdk_t.lockless_rx and pkt_dpdk_t.lockless_tx to uint8_t. I'm using the same type here.

@MatiasElo yes, I see. While reading patches in that web interface sometime I miss some pieces of code. Just above you changed that.

MatiasElo · 2017-11-09T08:33:36Z

@nagarahalli In l2fwd this patch set provides ~2-4% improvement (mainly in scheduled mode).

Bill-Fischofer-Linaro

Reviewed-by: Bill Fischofer bill.fischofer@linaro.org

Bill-Fischofer-Linaro · 2017-11-13T22:07:07Z

platform/linux-generic/include/odp_buffer_internal.h

@@ -104,17 +104,10 @@ struct odp_buffer_hdr_t {
 	/* User area pointer */
 	void    *uarea_addr;

-	/* User area size */


2.0 is based on api-next, not master. This will be one of several conflicts that will need to be resolved at merge time.

Bill-Fischofer-Linaro · 2017-11-13T22:08:19Z

platform/linux-generic/include/odp/api/plat/pool_types.h

+	uint16_t uarea_size;
+
+} _odp_pool_inline_offset_t;
+


@MatiasElo I agree this is correctly placed here.

Bill-Fischofer-Linaro · 2017-11-13T22:10:10Z

platform/linux-generic/include/odp/api/plat/packet_inlines.h

@@ -21,6 +21,9 @@
 /** @internal Inline function offsets */
 extern const _odp_packet_inline_offset_t _odp_packet_inline;

+/** @internal Pool inline function offsets */
+extern const _odp_pool_inline_offset_t _odp_pool_inline;


The API being inlined is odp_packet_pool() so this is an artifact of the implementation of that API. So I'm OK with it being here since this is supposed to be fastpath code.

muvarov · 2017-11-14T19:09:14Z

platform/linux-generic/include/odp/api/plat/packet_inlines.h

@@ -21,6 +21,9 @@
 /** @internal Inline function offsets */
 extern const _odp_packet_inline_offset_t _odp_packet_inline;

+/** @internal Pool inline function offsets */
+extern const _odp_pool_inline_offset_t _odp_pool_inline;


extern is not needed in .h file, also extern 2 lines above.

In this case it is needed. _odp_pool_inline is defined in odp_pool.c and _odp_packet_inline in odp_packet.c. ODP build with '--disable-abi-compat' will fail without extern declarations.

@muvarov extern is not needed for function prototypes.

muvarov · 2017-11-14T19:11:34Z

platform/linux-generic/odp_packet.c

-		       dsthdr->buf_hdr.uarea_size :
-		       srchdr->buf_hdr.uarea_size);
+	    srchdr->buf_hdr.uarea_addr != NULL) {
+		memcpy(dsthdr->buf_hdr.uarea_addr, srchdr->buf_hdr.uarea_addr,


odp_memcpy() might be good fit here.

odp_memcpy() is just a wrapper for memcpy() and I would avoid using api functions in the implementation unless there is a clear benefit.

ok. I just thought maybe some other implementations already speedup this call and we can get this benefits for free. But I don't might if it will be simple memcpy().

muvarov · 2017-11-14T19:18:27Z

platform/linux-generic/include/odp_packet_dpdk.h

+	uint8_t lockless_rx;		  /**< no locking for rx */
+	uint8_t lockless_tx;		  /**< no locking for tx */
+	uint8_t port_id;			  /**< DPDK port identifier */
+	/* --- 34 bytes --- */


what is propose of sayng 34 bytes here? Cache line is 64 so maybe comments for cache lines borders are more interesting here? vdev_sysc_promisc, lockless_rx, lockless_tx can be packet to bit field. That will also reduce structure size. Is there any reason to do that?

The next member in the struct named 'capa' is 40 bytes long, so it will cross a cache line boundary. I was thinking this comment was more useful than an ambiguous cache line comment.

There is still space in the first cache line for additional fast path variables, so I wouldn't use a bitmask before it is actually needed.

muvarov · 2017-11-14T19:28:09Z

platform/linux-generic/include/odp_packet_dpdk.h

+	uint8_t lockless_tx;		  /**< no locking for tx */
+	uint8_t port_id;			  /**< DPDK port identifier */
+	/* --- 34 bytes --- */
+	odp_pktio_capability_t	capa;	 /**< interface capabilities */


pktio capability is big struct and it's same for all packets. Is it really needs to be here? as well as uint16_t mtu? why not in pktio_entry_t ?

A valid point, at least I don't see any reason why capability struct is stored inside each pktio implementation. MTU is used in the fast path so I wouldn't move that. Should I move the capability in this PR or create a new one?

I think if it will be in this request it will make easy to review all changes at once.

muvarov · 2017-11-14T19:30:52Z

@MatiasElo few questions in that PR.

Disable debug checks by default for optimal performance. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Reorder pkt_dpdk_t members so that all struct members used in fast path are located on the same cache line. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Data size is constant for all buffers from the same pool, so there is no need to store the value in the buffer header. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Pool entry pointer is already stored in the odp_buffer_hdr_t struct. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Add inlined accessors for pool_t members pool_hdl and uarea_size. This enables removing the matching members from odp_buffer_hdr_t, and thus reduces per buffer overhead. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Since odp_pktio_capability_t is common for all pktio implementations move it inside pktio_entry_t. Signed-off-by: Matias Elo <matias.elo@nokia.com>

muvarov · 2017-11-16T16:00:54Z

Merged.

muvarov changed the title ~~Misc fast path optimizations~~ [PATCH v1] Misc fast path optimizations Nov 7, 2017

muvarov added Email_sent labels Nov 7, 2017

lumag reviewed Nov 7, 2017

View reviewed changes

nagarahalli approved these changes Nov 8, 2017

View reviewed changes

muvarov reviewed Nov 8, 2017

View reviewed changes

Bill-Fischofer-Linaro approved these changes Nov 13, 2017

View reviewed changes

muvarov reviewed Nov 14, 2017

View reviewed changes

MatiasElo added 6 commits November 15, 2017 10:46

configure: disable debug by default and print configured value

4098581

Disable debug checks by default for optimal performance. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: dpdk: reorganize pkt_dpdk_t struct

5a7ff72

Reorder pkt_dpdk_t members so that all struct members used in fast path are located on the same cache line. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: buffer: remove data size member from odp_buffer_hdr_t

4627a3a

Data size is constant for all buffers from the same pool, so there is no need to store the value in the buffer header. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: pool: remove unnecessary pool_entry_from_hdl() calls

8d8038c

Pool entry pointer is already stored in the odp_buffer_hdr_t struct. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: pool: add inlined accessors for selected pool parameters

f8d68d6

Add inlined accessors for pool_t members pool_hdl and uarea_size. This enables removing the matching members from odp_buffer_hdr_t, and thus reduces per buffer overhead. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: pktio: add pktio capability to pktio_entry_t

0bd28be

Since odp_pktio_capability_t is common for all pktio implementations move it inside pktio_entry_t. Signed-off-by: Matias Elo <matias.elo@nokia.com>

muvarov changed the title ~~[PATCH v1] Misc fast path optimizations~~ [PATCH v2] Misc fast path optimizations Nov 15, 2017

muvarov added Email_sent and removed Email_sent labels Nov 15, 2017

muvarov closed this Nov 16, 2017

MatiasElo deleted the dev/misc_perf_opt branch November 17, 2017 08:01

[PATCH v2] Misc fast path optimizations #281

[PATCH v2] Misc fast path optimizations #281

Conversation

MatiasElo commented Nov 7, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lumag commented Nov 7, 2017

nagarahalli left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatiasElo Nov 9, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MatiasElo commented Nov 9, 2017

Bill-Fischofer-Linaro left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

muvarov commented Nov 14, 2017

muvarov commented Nov 16, 2017

MatiasElo commented Nov 7, 2017 •

edited

MatiasElo Nov 9, 2017 •

edited