[PATCH v2] Packet function inlines #437

psavol · 2018-01-30T15:49:47Z

Continue inlining of commonly used small packet and pktio API functions. These improve few percents packet rate of L2fwd (both direct/scheduled mode) and OFP burst test. Especially, in non-ABI compat mode and with copying pktios (test with DPDK).

Bill-Fischofer-Linaro · 2018-01-31T01:39:10Z

platform/linux-generic/odp_packet.c

 }

 void odp_packet_ts_set(odp_packet_t pkt, odp_time_t timestamp)
 {
 	odp_packet_hdr_t *pkt_hdr = packet_hdr(pkt);

-	pkt_hdr->timestamp = timestamp;
-	pkt_hdr->p.input_flags.timestamp = 1;
+	packet_set_ts(pkt_hdr, &timestamp);


Should this be in its own commit? Different function.

I could rename the patch to:
"linux-gen: packet: use inlined flow hash and ts set"

... if necessary. The comment says already that also ts set is modified the same way.

I'm OK with just a modified commit name. I agree we needn't be too granular about these things.

Bill-Fischofer-Linaro · 2018-01-31T02:02:38Z

platform/linux-generic/odp_packet.c

+#ifndef __clang__
+ODP_STATIC_ASSERT(ODP_PACKET_INVALID == 0, "Packet invalid not 0");
+ODP_STATIC_ASSERT(ODP_BUFFER_INVALID == 0, "Buffer invalid not 0");
+ODP_STATIC_ASSERT(ODP_EVENT_INVALID  == 0, "Event invalid not 0");


Since you want to breech the strong type you need to use the internal odp_typeval() macro here.

ODP_STATIC_ASSERT(_odp_typeval(ODP_EVENT_INVALID) == 0, "Event invalid not 0");

etc. I've verified that this works fine for clang, so no need for the conditional compilation.

I did try several combinations of type casts of invalid values, also cast to uintptr_t. The problem is that gcc and clang (clang version 3.8.0-2ubuntu4) of my machine accepted all of those casts, but the clang version in Travis (clang version 3.8.0-2ubuntu3~trusty5) does not accept anything.

Maybe it's a bug in the particular version of clang. Anyway, addition of this check even only on gcc side is improvement against what we currently have (no check). Packet invalid is currently 0xfffffff in ABI spec, but event/buffer invalid are 0.

odp_packet.c:55:19: error: static_assert expression is not an integral constant
expression
ODP_STATIC_ASSERT(((uintptr_t)ODP_PACKET_INVALID) == 0, "Packet invalid not 0");

../../include/odp/api/abi-default/debug.h:26:53: note: expanded from macro 'ODP_STATIC_ASSERT' #define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg) ^~~~ odp_packet.c:55:20: note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression ODP_STATIC_ASSERT(((uintptr_t)ODP_PACKET_INVALID) == 0, "Packet invalid not 0"); ^

_odp_typeval() is defined as:

#define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))

It's used elsewhere in the ODP code and clang doesn't have any problem with it. I've verified this works for clang 4.2.1. Since it works elsewhere on Travis I'm not sure why it wouldn't in this instance.

static_assert() seems special. The clang version in Travis does not accept type casts with static_assert(). The cast confuses it to think that (uintptr_t)0 == 0 is not an integral constant.

odp_packet.c:52:19: error: static_assert expression is not an integral constant
expression
ODP_STATIC_ASSERT(_odp_typeval(ODP_BUFFER_INVALID) == 0, "Buffer inval not 0");

../../platform/linux-generic/include/odp/api/plat/strong_types.h:32:30: note: expanded from macro '_odp_typeval' #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle)) ^ ../../include/odp/api/abi-default/debug.h:26:53: note: expanded from macro 'ODP_STATIC_ASSERT' #define ODP_STATIC_ASSERT(cond, msg) _Static_assert(cond, msg) ^~~~ odp_packet.c:52:19: note: cast that performs the conversions of a reinterpret_cast is not allowed in a constant expression ../../platform/linux-generic/include/odp/api/plat/strong_types.h:32:41: note: expanded from macro '_odp_typeval' #define _odp_typeval(handle) ((uint32_t)(uintptr_t)(handle))

Very strange. I'm Ok with this workaround, but a comment about clang version might be appropriate. Don't know if it's worth doing a clang version check. We do things like that in the ODP_STATIC_ASSERT() macro itself for similar reasons.

Maybe (uintptr_t)(handle) == (uintptr_t)0 would do the trick?

psavol · 2018-01-31T08:33:14Z

Travis failure is a false positive checkpatch issue:
ERROR: Macros with complex values should be enclosed in parentheses
#53: FILE: platform/linux-generic/include-abi/odp/api/abi/packet_io.h:54:
+#define _ODP_INLINE static inline

Use inlined version of flow hash set function. Also changed API to use inlined timestamp function, so that the same function is not implemented twice. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

@cond

Use @cond tag to hide internal functions from Doxygen. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

Inline commonly used packet to/from memory copy functions. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

Use always inlined versions of packet to/from memory copy functions. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

Use only one function (packet_hdr) in handle to header pointer conversion. odp_packet_hdr is bad name for an implementation internal function as API function prefix is odp_packet_. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

For minimal code complexity and negative impact, prefetch only when the requested address is in the first segment and prefetch only one cache line. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

Inline implementation of packet_from_event and packet_to_event functions. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

Use multi versions of packet / event conversion functions. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

Fast path functions cannot be expected to always check against invalid handles. Also a handle that has been closed (or destroyed) must not be passed to any API function. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

Inline packet IO interface handle to index conversion function which may be used frequently. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

Inline packet input interface index function which may be used frequently. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

codecov · 2018-01-31T13:54:35Z

Codecov Report

Merging #437 into master will increase coverage by 0.072%.
The diff coverage is 91.428%.

@@              Coverage Diff              @@
##            master      #437       +/-   ##
=============================================
+ Coverage   77.774%   77.847%   +0.072%     
=============================================
  Files          199       201        +2     
  Lines        35766     35987      +221     
=============================================
+ Hits         27817     28015      +198     
- Misses        7949      7972       +23

Impacted Files	Coverage Δ
test/validation/api/pktio/pktio.c	`90.499% <ø> (-0.028%)`	⬇️
platform/linux-generic/odp_traffic_mngr.c	`78.98% <0%> (ø)`	⬆️
platform/linux-generic/pktio/loop.c	`85.606% <100%> (ø)`	⬆️
platform/linux-generic/pktio/pcap.c	`81.675% <100%> (ø)`	⬆️
...orm/linux-generic/include/odp_packet_io_internal.h	`83.333% <100%> (-1.283%)`	⬇️
platform/linux-generic/pktio/socket.c	`62.529% <100%> (ø)`	⬆️
platform/linux-generic/pktio/ipc.c	`78.612% <100%> (ø)`	⬆️
...-generic/include/odp/api/plat/packet_inlines_api.h	`100% <100%> (ø)`	⬆️
platform/linux-generic/odp_packet_flags.c	`100% <100%> (ø)`	⬆️
platform/linux-generic/pktio/netmap.c	`82.092% <100%> (ø)`	⬆️
... and 14 more

lumag · 2018-01-31T16:44:08Z

platform/linux-generic/odp_packet.c

+	}
+
+	return 0;
+}


Is there a reason to move function bodies?

Those need to be inside #include <odp/visibility_begin.h> #include <odp/visibility_end.h>. It's cleaner to have one vs multiple such regions in one c file.

lumag · 2018-01-31T16:54:11Z

test/validation/api/pktio/pktio.c


 	CU_ASSERT(odp_pktio_close(pktio) == 0);
-	CU_ASSERT(odp_pktio_index(pktio) < 0);


Generally I would agree. However we explicitly have the invalid usecase in API:

@retval <0 On failure (e.g., handle not valid)

We can have _odp_pktio_index() which does not perform such checks and odp_pktio_index() which does ODP_PKTIO_INVALID check.

API does not (and should not) guarantee that invalid or stale handles are checked on fast path functions, such as this conversion to index. Especially, any API function is not specified to check against stale handles. Function may return -1, if it finds an error but we don't guarantee which errors are checked. For example, there may be check against invalid handle, but there may not be either. Invalid values are mostly needed for API function to be able return that e.g. create/open/alloc failed. E.g. here, application should check that pktio open succeeded, if it did not succeed it should not continue and pass that invalid handle to other pktio calls. That way every API call does not need to check the handle, only application needs to check it once after open/create/alloc.

This close + index test above it basically the same thing as doing:
odp_packet_free(pkt);
data = odp_packet_data(pkt);
// now we can assume that data is NULL, since pkt packet was freed and this function always searches through valid handles and notices that this handle value is not valid anymore.

To me "(e.g. handle not valid)" does not yet guarantee the check. This would:
"<0 When pktio handle is invalid, or some other error happened".

In general, we should remove all such "e.g. handle not valid" comments.

@psavol Totally agree about close + index.

lumag · 2018-01-31T16:56:05Z

With minor comments:
Reviewed-by: Dmitry Eremin-Solenikov

psavol · 2018-02-05T07:34:48Z

Ping. This has been 4 days ready to merge.

muvarov · 2018-02-05T08:54:45Z

Merged.

muvarov changed the title ~~Packet function inlines~~ [PATCH v1] Packet function inlines Jan 30, 2018

muvarov added Email_sent labels Jan 30, 2018

Bill-Fischofer-Linaro suggested changes Jan 31, 2018

View reviewed changes

Petri Savolainen added 11 commits January 31, 2018 15:53

linux-gen: packet: use inlined flow hash and ts set

45c5d35

Use inlined version of flow hash set function. Also changed API to use inlined timestamp function, so that the same function is not implemented twice. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

linux-gen: packet: remove dummy doxygen documentation

ba6eb85

Use @cond tag to hide internal functions from Doxygen. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

linux-gen: packet: inline copy functions

4fb3636

Inline commonly used packet to/from memory copy functions. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

linux-gen: packet: use inlined copy functions

50ee340

Use always inlined versions of packet to/from memory copy functions. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

linux-gen: packet: implement packet prefetch

5a85368

For minimal code complexity and negative impact, prefetch only when the requested address is in the first segment and prefetch only one cache line. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

linux-gen: packet: inline conversions between packet and event

13f6cf5

Inline implementation of packet_from_event and packet_to_event functions. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

test: l2fwd: use multi conversion functions

45db22d

Use multi versions of packet / event conversion functions. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

linux-gen: pktio: inline pktio index function

7b2317e

Inline packet IO interface handle to index conversion function which may be used frequently. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

linux-gen: packet: inline packet input index function

559591f

Inline packet input interface index function which may be used frequently. Signed-off-by: Petri Savolainen <petri.savolainen@linaro.org>

psavol force-pushed the master-packet-optim branch from e0d7d50 to 559591f Compare January 31, 2018 13:54

muvarov changed the title ~~[PATCH v1] Packet function inlines~~ [PATCH v2] Packet function inlines Jan 31, 2018

muvarov removed Email_sent labels Jan 31, 2018

muvarov added Email_sent labels Jan 31, 2018

Bill-Fischofer-Linaro approved these changes Jan 31, 2018

View reviewed changes

lumag reviewed Jan 31, 2018

View reviewed changes

muvarov closed this Feb 5, 2018

psavol deleted the master-packet-optim branch November 7, 2018 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PATCH v2] Packet function inlines #437

[PATCH v2] Packet function inlines #437

psavol commented Jan 30, 2018

Bill-Fischofer-Linaro Jan 31, 2018

psavol Jan 31, 2018

Bill-Fischofer-Linaro Jan 31, 2018

Bill-Fischofer-Linaro Jan 31, 2018

psavol Jan 31, 2018

Bill-Fischofer-Linaro Jan 31, 2018

psavol Jan 31, 2018

psavol Jan 31, 2018

Bill-Fischofer-Linaro Jan 31, 2018

lumag Jan 31, 2018

psavol commented Jan 31, 2018

codecov bot commented Jan 31, 2018 •

edited

lumag Jan 31, 2018

psavol Feb 1, 2018

lumag Jan 31, 2018

lumag Jan 31, 2018

psavol Feb 1, 2018

psavol Feb 1, 2018 •

edited

lumag Feb 1, 2018

lumag commented Jan 31, 2018

psavol commented Feb 5, 2018

muvarov commented Feb 5, 2018


		CU_ASSERT(odp_pktio_close(pktio) == 0);
		CU_ASSERT(odp_pktio_index(pktio) < 0);

[PATCH v2] Packet function inlines #437

[PATCH v2] Packet function inlines #437

Conversation

psavol commented Jan 30, 2018

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

psavol commented Jan 31, 2018

codecov bot commented Jan 31, 2018 • edited

Codecov Report

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

psavol Feb 1, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lumag commented Jan 31, 2018

psavol commented Feb 5, 2018

muvarov commented Feb 5, 2018

codecov bot commented Jan 31, 2018 •

edited

psavol Feb 1, 2018 •

edited