[PATCH v2] Various pktio fixes #397

MatiasElo · 2018-01-17T10:02:59Z

V2:

Added ODP_DBG messages where packets are dropped (Maxim)
Renamed recv/send function argument len -> num (Bill)

muvarov · 2018-01-17T10:36:48Z

platform/linux-generic/pktio/socket.c

@@ -643,6 +643,10 @@ static int sock_mmsg_recv(pktio_entry_t *pktio_entry, int index ODP_UNUSED,
 		uint16_t pkt_len = msgvec[i].msg_len;
 		int ret;

+		if (odp_unlikely(msgvec[i].msg_hdr.msg_flags & MSG_TRUNC)) {
+			odp_packet_free(pkt);


maybe to just set error bit and deliver this packet to application?

The odp_pktin_maxlen() API states:

Maximum frame length in bytes that the packet IO interface can receive.

I interpret this so that the application shouln't see packets which exceed pktin max length.

The error bit simply says the packet was truncated to max length. Perhaps the application is only interested in the first few bytes of a packet?

The most specific error type an application can see is odp_packet_has_l2_error(), so it cannot trust packet contents anyway.

inconsistent with counters. You do not increase drop counter for pktio and silently drop packet. I would add at least ODP_DBG() so people on debug can see what is happening.

Problem is that you cannot increase drop counters manually with socket pktio. The counters are read directly from ethtool or sysfs. ODP_DBG() may be the best option here.

muvarov · 2018-01-17T10:49:18Z

platform/linux-generic/pktio/socket_mmap.c

@@ -202,6 +202,12 @@ static inline unsigned pkt_mmap_v2_rx(pktio_entry_t *pktio_entry,
 		pkt_buf = (uint8_t *)ppd.raw + ppd.v2->tp_h.tp_mac;
 		pkt_len = ppd.v2->tp_h.tp_snaplen;

+		if (odp_unlikely(pkt_len > pkt_sock->mtu)) {


codecov · 2018-01-17T17:56:24Z

Codecov Report

Merging #397 into master will decrease coverage by 0.013%.
The diff coverage is 60.784%.

@@              Coverage Diff              @@
##            master      #397       +/-   ##
=============================================
- Coverage   78.565%   78.552%   -0.014%     
=============================================
  Files          194       194               
  Lines        34818     34852       +34     
=============================================
+ Hits         27355     27377       +22     
- Misses        7463      7475       +12

Impacted Files	Coverage Δ
platform/linux-generic/include/odp_packet_socket.h	`100% <ø> (ø)`	⬆️
platform/linux-generic/pktio/ipc.c	`78.612% <100%> (+0.578%)`	⬆️
platform/linux-generic/pktio/pcap.c	`81.675% <100%> (ø)`	⬆️
platform/linux-generic/pktio/tap.c	`65.596% <100%> (ø)`	⬆️
platform/linux-generic/pktio/netmap.c	`82.092% <100%> (+0.329%)`	⬆️
platform/linux-generic/pktio/socket_mmap.c	`69.491% <34.693%> (-2.632%)`	⬇️
platform/linux-generic/pktio/loop.c	`85.606% <76.19%> (-2.099%)`	⬇️
platform/linux-generic/pktio/socket.c	`62.529% <76.923%> (-0.355%)`	⬇️
platform/linux-generic/test/pktio_ipc/pktio_ipc2.c	`68.217% <0%> (+0.775%)`	⬆️
... and 1 more

Bill-Fischofer-Linaro · 2018-01-18T00:27:05Z

platform/linux-generic/pktio/loop.c

@@ -177,13 +177,15 @@ static int loopback_send(pktio_entry_t *pktio_entry, int index ODP_UNUSED,
 	int i;
 	int ret;
 	uint32_t bytes = 0;
+	uint32_t out_octets_tbl[len];


Since you're doing miscellaneous cleanups anyway, perhaps changing len to num here should be considered? len is confusing since it doesn't represent packet length.

Bill-Fischofer-Linaro · 2018-01-18T00:29:58Z

platform/linux-generic/pktio/loop.c


 	if (odp_unlikely(len > QUEUE_MULTI_MAX))
 		len = QUEUE_MULTI_MAX;

 	for (i = 0; i < len; ++i) {
 		hdr_tbl[i] = packet_to_buf_hdr(pkt_tbl[i]);
 		bytes += odp_packet_len(pkt_tbl[i]);
+		out_octets_tbl[i] = bytes;


Not sure I understand what out_octets_tbl is accumulating here. If I'm sending three 100 byte packets this will set out_octets_tbl[0] to 100, out_octets_tbl[1] to 200, and out_octets_tbl[2] to 300.

This array is used to update stats.out_octets correctly if enq_multi() fails to enqueue all packets. Without it a second for loop would be required inside if (ret > 0) clause below.

Ok, clever. A comment might be useful here.

Bill-Fischofer-Linaro · 2018-01-18T00:42:20Z

platform/linux-generic/pktio/socket_mmap.c

@@ -361,11 +365,13 @@ static void mmap_fill_ring(struct ring *ring, odp_pool_t pool_hdl, int fanout)
 				   pool->tailroom + TPACKET_HDRLEN +
 				   TPACKET_ALIGNMENT + + (pz - 1)) & (-pz);

-	/* Calculate how many pages do we need to hold all pool packets


If we're cleaning up, might as well delete that extraneous + in the above line: TPACKET_ALIGNMENT + + (pz - 1)) & (-pz); I'm surprised the compiler doesn't flag that since it's normally so picky about issuing warnings.

Bill-Fischofer-Linaro · 2018-01-18T00:51:11Z

platform/linux-generic/pktio/socket_mmap.c

@@ -148,6 +155,11 @@ static uint8_t *pkt_mmap_vlan_insert(uint8_t *l2_hdr_ptr,
 	return l2_hdr_ptr;
 }

+static inline unsigned next_frame(unsigned cur_frame, unsigned frame_count)
+{
+	return odp_unlikely(cur_frame + 1 >= frame_count) ? 0 : cur_frame + 1;


Since ring->rd_num is a power of two, is the concern here that the % operator might result in a division if the compiler doesn't know that it's a power of two? In that case:

return ++cur_frame & (frame_count - 1);

solves that problem and avoids conditional branching.

ring->rd_num is not guaranteed to be a power of two. At least on my test system (28 lcores) it is not.

OK. From the original code's use of % I assumed that was the case. Perhaps that's why this is a fix. :)

Bill-Fischofer-Linaro · 2018-01-18T01:03:26Z

platform/linux-generic/pktio/socket_mmap.c

+		frame_num = next_frame(frame_num, frame_count);
+	}
+
+	ring->frame_num = (first_frame_num + nb_tx) % frame_count;


ring->frame_num = next_frame(first_frame_num + nb_tx - 1, frame_count); is an alternative here.

Bill-Fischofer-Linaro · 2018-01-18T01:17:48Z

platform/linux-generic/pktio/socket.c

@@ -643,6 +643,10 @@ static int sock_mmsg_recv(pktio_entry_t *pktio_entry, int index ODP_UNUSED,
 		uint16_t pkt_len = msgvec[i].msg_len;
 		int ret;

+		if (odp_unlikely(msgvec[i].msg_hdr.msg_flags & MSG_TRUNC)) {
+			odp_packet_free(pkt);


The error bit simply says the packet was truncated to max length. Perhaps the application is only interested in the first few bytes of a packet?

The maximum frame length should include Ethernet header. Signed-off-by: Matias Elo <matias.elo@nokia.com>

odp_pktio_stats_t.out_octets was updated incorrectly if all packets were not successfully enqueued. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Use more realistic MTU value. The value is also checked in TX. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Allocating enough memory for storing each packet in the packet pool in each RX/TX block wastes a lot of memory. Also, the following mmap() call in mmap_sock() can start failing when the number of memory blocks increases (large number of CPU cores). Reduce the amount of required memory by limiting the size of memory blocks. Signed-off-by: Matias Elo <matias.elo@nokia.com>

During sock_mmap_send() when all packets are not transmitted by sendto() call the used ring slots have to be marked as TP_STATUS_AVAILABLE or they will be transmitted during the next sock_mmap_send() call, thus breaking the function logic. To reduce the number of failed TX operations (visible to applications) the pktio implementation can call sendto() up to TX_RETRIES times during a single sock_mmap_recv() call. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Drop truncated packets received from kernel during sock_mmsg_recv() call. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Some NICs do not automatically drop packets which exceed MTU. Signed-off-by: Matias Elo <matias.elo@nokia.com>

Use argument named 'num' on all pktio devices to pass the number of packets to receive/transmit. Signed-off-by: Matias Elo <matias.elo@nokia.com>

JannePeltonen · 2018-01-19T10:55:57Z

platform/linux-generic/pktio/socket_mmap.c

@@ -202,6 +202,12 @@ static inline unsigned pkt_mmap_v2_rx(pktio_entry_t *pktio_entry,
 		pkt_buf = (uint8_t *)ppd.raw + ppd.v2->tp_h.tp_mac;
 		pkt_len = ppd.v2->tp_h.tp_snaplen;

+		if (odp_unlikely(pkt_len > pkt_sock->mtu)) {


Why do these extra long packets need to be dropped?

Maybe odp_pktin_maxlen() is not defined clearly enough regarding unexpectedly long packets. Does it merely promise that packets up to the maxlen can always be received (as I thought) or does it also promise that no bigger packets than that ever come out of the pktin? Would the latter be even useful?

At this point the API is quite clear:

Maximum frame length in bytes that the packet IO interface can receive.

muvarov · 2018-01-22T19:47:55Z

Merged.

MatiasElo changed the title ~~Various pktio fixes~~ [PATCH v1] Various pktio fixes Jan 17, 2018

muvarov reviewed Jan 17, 2018

View reviewed changes

muvarov added Email_sent labels Jan 17, 2018

muvarov changed the title ~~[PATCH v1] Various pktio fixes~~ [PATCH v2] Various pktio fixes Jan 17, 2018

muvarov added Email_sent and removed Email_sent labels Jan 17, 2018

Bill-Fischofer-Linaro suggested changes Jan 18, 2018

View reviewed changes

MatiasElo added 9 commits January 19, 2018 10:09

linux-gen: pktio: use correct maximum frame length

98e1d9c

The maximum frame length should include Ethernet header. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: pktio: loop: update stats.out_octets correctly

c256e9a

odp_pktio_stats_t.out_octets was updated incorrectly if all packets were not successfully enqueued. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: pktio: loop: use more realistic MTU value

9137359

Use more realistic MTU value. The value is also checked in TX. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: socket: drop truncated packets on receive

8f5027a

Drop truncated packets received from kernel during sock_mmsg_recv() call. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: socket_mmap: check that packet obeys MTU in receive

5a10cbf

Some NICs do not automatically drop packets which exceed MTU. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: netmap: check that packet obeys MTU in receive

cf69bbf

Some NICs do not automatically drop packets which exceed MTU. Signed-off-by: Matias Elo <matias.elo@nokia.com>

linux-gen: pktio: unify send/recv function argument naming

0d0d4a4

Use argument named 'num' on all pktio devices to pass the number of packets to receive/transmit. Signed-off-by: Matias Elo <matias.elo@nokia.com>

JannePeltonen reviewed Jan 19, 2018

View reviewed changes

muvarov changed the title ~~[PATCH v2] Various pktio fixes~~ [PATCH v3] Various pktio fixes Jan 19, 2018

muvarov removed Email_sent labels Jan 19, 2018

MatiasElo changed the title ~~[PATCH v3] Various pktio fixes~~ [PATCH v2] Various pktio fixes Jan 19, 2018

muvarov added Email_sent labels Jan 19, 2018

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

View reviewed changes

muvarov closed this Jan 22, 2018

MatiasElo deleted the dev/pktio_fixes branch February 20, 2018 07:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[PATCH v2] Various pktio fixes #397

[PATCH v2] Various pktio fixes #397

MatiasElo commented Jan 17, 2018 •

edited

muvarov Jan 17, 2018

MatiasElo Jan 17, 2018

Bill-Fischofer-Linaro Jan 18, 2018

MatiasElo Jan 18, 2018

muvarov Jan 18, 2018

MatiasElo Jan 18, 2018

muvarov Jan 17, 2018

codecov bot commented Jan 17, 2018 •

edited

Bill-Fischofer-Linaro Jan 18, 2018

MatiasElo Jan 18, 2018

Bill-Fischofer-Linaro Jan 18, 2018

MatiasElo Jan 18, 2018

Bill-Fischofer-Linaro Jan 18, 2018

Bill-Fischofer-Linaro Jan 18, 2018

MatiasElo Jan 18, 2018

Bill-Fischofer-Linaro Jan 18, 2018

MatiasElo Jan 18, 2018

Bill-Fischofer-Linaro Jan 18, 2018

Bill-Fischofer-Linaro Jan 18, 2018

MatiasElo Jan 18, 2018

Bill-Fischofer-Linaro Jan 18, 2018

JannePeltonen Jan 19, 2018

MatiasElo Jan 19, 2018

muvarov commented Jan 22, 2018

[PATCH v2] Various pktio fixes #397

[PATCH v2] Various pktio fixes #397

Conversation

MatiasElo commented Jan 17, 2018 • 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

codecov bot commented Jan 17, 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

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 Jan 22, 2018

MatiasElo commented Jan 17, 2018 •

edited

codecov bot commented Jan 17, 2018 •

edited