Features/net pkt append memset #9393

therealprof · 2018-08-12T19:21:30Z

codecov-io · 2018-08-12T19:55:36Z

Codecov Report

Merging #9393 into master will increase coverage by 0.01%.
The diff coverage is 47.61%.

@@            Coverage Diff             @@
##           master    #9393      +/-   ##
==========================================
+ Coverage   52.53%   52.55%   +0.01%     
==========================================
  Files         212      212              
  Lines       25821    25855      +34     
  Branches     5428     5437       +9     
==========================================
+ Hits        13565    13588      +23     
- Misses      10071    10077       +6     
- Partials     2185     2190       +5

Impacted Files	Coverage Δ
include/net/buf.h	`88.88% <ø> (ø)`	⬆️
include/net/net_pkt.h	`85.64% <ø> (ø)`	⬆️
subsys/net/ip/dhcpv4.c	`6.93% <0%> (+0.03%)`	⬆️
subsys/net/ip/net_pkt.c	`62.32% <33.33%> (-0.81%)`	⬇️
subsys/net/buf.c	`59.24% <86.66%> (+1.98%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4b9d980...83a2873. Read the comment docs.

jukkar · 2018-08-13T12:04:35Z

include/net/buf.h

+ *         were no free fragments in a pool to accommodate all data.
+ */
+u16_t net_buf_append_set_bytes(struct net_buf *buf, u16_t len,
+			       const u8_t value, s32_t timeout,


Looks like indentation is off here.

Hm, looks perfectly indented here. Checkpatch also approves. Not sure how to improve that.

Yep, my bad, no issue in this line.

This sounds so specific that I'd rather move it to the net_pkt layer. Some comments nevertheless:

The verb used for adding stuff to the end of buffers is "add", not "append". I noticed that net_buf_append_bytes() has slipped past me. Was there a specific reason to change the verb for that API?

const for the u8_t value does nothing, so I'd leave it out.

It should be size_t len and not u16_t len to be consistent throughout the API (not to mention it generates less code for our 32-bit architectures). net_buf_append_bytes() got this wrong also - needs fixing.

jukkar · 2018-08-13T12:04:45Z

include/net/net_pkt.h

+ *         were no free fragments in a pool to accommodate all data.
+ */
+u16_t net_pkt_append_memset(struct net_pkt *pkt, u16_t len, const u8_t data,
+		     s32_t timeout);


Same here. Could you check all the functions in this PR and make sure that indentation is correct.

jukkar · 2018-08-13T12:07:41Z

subsys/net/ip/dhcpv4.c

-		if (!net_pkt_append_u8(pkt, 0)) {
-			return false;
-		}
+	if (net_pkt_append_memset(pkt, size, 0, K_FOREVER) != size) {


We are trying to migrate away from K_FOREVER timeout in #9118 so please use here a proper timeout value.

jukkar · 2018-08-13T12:07:51Z

subsys/net/ip/dhcpv4.c

-		if (!net_pkt_append_u8(pkt, 0)) {
-			return false;
-		}
+	if (net_pkt_append_memset(pkt, size, 0, K_FOREVER) != size) {


jukkar · 2018-08-13T14:09:32Z

include/net/net_pkt.h

+ *         were no free fragments in a pool to accommodate all data.
+ */
+u16_t net_pkt_append_memset(struct net_pkt *pkt, u16_t len, const u8_t data,
+		     s32_t timeout);


There is indentation issue here.

jukkar · 2018-08-13T14:09:40Z

subsys/net/buf.c

+ * assumes that the buffer has at least one fragment.
+ */
+u16_t net_buf_append_set_bytes(struct net_buf *buf, u16_t len,
+			   const u8_t value, s32_t timeout,


jukkar · 2018-08-13T14:09:58Z

subsys/net/ip/net_pkt.c

@@ -1201,6 +1201,59 @@ u16_t net_pkt_append(struct net_pkt *pkt, u16_t len, const u8_t *data,
 	return appended;
 }

+u16_t net_pkt_append_memset(struct net_pkt *pkt, u16_t len, const u8_t data,
+		    s32_t timeout)


jukkar · 2018-08-13T14:10:08Z

subsys/net/ip/net_pkt.c

+	}
+
+	appended = net_buf_append_set_bytes(net_buf_frag_last(pkt->frags),
+					len, data, timeout,


jukkar · 2018-08-13T14:10:19Z

tests/net/net_pkt/src/main.c

+	zassert_true(ret == 50, "Linearize failed failed");
+	for (cur_pos = 0; cur_pos < 50; ++cur_pos) {
+		zassert_true(read_data[cur_pos] == 0,
+				"Byte was expected to read 0");


Please indent these three lines in the main.c properly too.

jukkar · 2018-08-13T14:10:26Z

tests/net/net_pkt/src/main.c

+	zassert_true(ret == 128, "Linearize failed failed");
+	for (cur_pos = 0; cur_pos < 128; ++cur_pos) {
+		zassert_true(read_data[cur_pos] == 255,
+				"Byte was expected to read 255");


jukkar · 2018-08-13T14:10:35Z

tests/net/net_pkt/src/main.c

+	zassert_true(ret == 128, "Linearize failed failed");
+	for (cur_pos = 0; cur_pos < 128; ++cur_pos) {
+		zassert_true(read_data[cur_pos] == 0,
+				"Byte was expected to read 0");


and also here

pfalcon · 2018-08-13T20:27:23Z

Most importantly - is this change useful? The IP stack seems to move to BSD Sockets API, which wouldn't be able to expose that functionality. And otherwise, 2 functions of 30+50 lines are used to replace cases of 5 lines of code in 2 places. That doesn't look to tally to positive befits in terms of code size, and unlikely will due to note on the emerging standard API above.

therealprof · 2018-08-13T20:56:29Z

@pfalcon My motivation for this is the wasteful handling of resources I noticed in the DHCPv4 code. Because the net_pkt API currently only allows for appending single integral types or copy from a range of memory. I thought it was a pretty glaring omission that it is not possible to efficiently fill a packet buffer with a large number of constant values (typically zero). I only converted DHCPv4 because that is where I noticed it but I'm certain it can/should be applied in a lot of other places. Code size wise it's pretty much a wash, depending on which application/example you look at.

I neither see how the BSD API will fix the particular problem at hand (with buffer handling) nor how and whether all subsystems will convert to to the BSD API soon.

pfalcon · 2018-08-13T21:26:31Z

My motivation for this is the wasteful handling of resources I noticed in the DHCPv4 code.

I understand the motivation and agree that thinking of an abstract "the most general API", that's a useful addition. But practically, the code size is also a resource. And I have a hunch feeling that Zephyr stack is not competitive in the terms of code size, and sooner or later we'll need to address that. On the other hand, DHCPv4 is not on the critical path, so the fact that its packets are constructed "slowly" (but with less code) shouldn't be a big problem.

I neither see how the BSD API will fix the particular problem at hand

It doesn't, that's the point. The typical Unixy (now POSIXy) way is to have API erring on the side minimality, rather than on "abstract completeness". A POSIX way to deal with that problem would be to allocate a static buffer of size a particular application tolerates. Then either write that buffer multiple times, or write a subset of it. E.g., if you have char buf[32] = {0}; and need to write 80 bytes, you'd write buf 2 times, then 3rd time - 16 bytes of it.

and whether all subsystems will convert to to the BSD API soon.

I only know what was posted, e.g. https://github.com/zephyrproject-rtos/zephyr /issues/7591 , and yes, I'm not sure about how soon either.

All in all, I can only call for your own consideration of these matter, and not give -1, as with tests included, it's hard to argue that it does anything "bad", even if I have IMHO that it doesn't do much good either.

therealprof · 2018-08-13T22:10:56Z

On the other hand, DHCPv4 is not on the critical path, so the fact that its packets are constructed "slowly" (but with less code) shouldn't be a big problem.

It's not quite that simple. A lot of those small functions are actually inlined and the call stack is somewhat deep. I'm also not quite sure whether the single appends have any negative impact on fragmentation.

The typical Unixy (now POSIXy)

Oh, I'm quite aware how BSD sockets work, I've written a lot of small and big scale applications in my life.

A POSIX way to deal with that problem would be to allocate a static buffer of size a particular application tolerates.

The problem is: You always have to send one full datagram at a time and this is embedded, so using a regular BSD API you need to allocate the full buffer at a time which may or may not possible which is what the buf/net_pkt API is solving. Not everything is streamed TCP, in fact rather little in the embedded world is and for a reason...

jukkar

Very minor tweak still needed.

jukkar · 2018-08-14T06:11:54Z

tests/net/net_pkt/src/main.c

@@ -1152,8 +1152,6 @@ static void test_pkt_pull(void)
 static void test_net_pkt_append_memset(void)
 {
 	struct net_pkt *pkt;
-	struct net_buf *read_frag;


This change is in wrong commit, should be in "net: pkt: Added new function net_pkt_append_memset() to prefill packet"

jukkar · 2018-08-14T06:13:24Z

tests/net/net_pkt/src/main.c

+	zassert_true(ret == 50, "Linearize failed failed");
+	for (cur_pos = 0; cur_pos < 50; ++cur_pos) {
+		zassert_true(read_data[cur_pos] == 0,
+				"Byte was expected to read 0");


Please indent these three lines in the main.c properly too.

Some locations like DHCPv4 client create a prefilled packet by appending new fragments in a loop with one byte each via net_pkt_append_u8() which is wasteful and noisy. This patch adds the new functions net_pkt_append_memset() which creates fragments as needed in the desired size and initialises it to the specified value. To facilitate this function, net/buf also receives a new net_buf_append_set_bytes() function and this also adds a unittest for the former. Prerequisite for zephyrproject-rtos#9287 Signed-off-by: Daniel Egger <daniel@eggers-club.de>

Thise uses the new net_pkt_append_memset() function to generate the required zero filling instead of calling net_pkt_append_u8() in loops. Fixes zephyrproject-rtos#9287 Signed-off-by: Daniel Egger <daniel@eggers-club.de>

jhedberg · 2018-08-14T09:28:13Z

include/net/buf.h

+ *         were no free fragments in a pool to accommodate all data.
+ */
+u16_t net_buf_append_set_bytes(struct net_buf *buf, u16_t len,
+			       const u8_t value, s32_t timeout,


This sounds so specific that I'd rather move it to the net_pkt layer. Some comments nevertheless:

The verb used for adding stuff to the end of buffers is "add", not "append". I noticed that net_buf_append_bytes() has slipped past me. Was there a specific reason to change the verb for that API?

const for the u8_t value does nothing, so I'd leave it out.

It should be size_t len and not u16_t len to be consistent throughout the API (not to mention it generates less code for our 32-bit architectures). net_buf_append_bytes() got this wrong also - needs fixing.

jhedberg · 2018-08-14T09:31:18Z

Regarding "append" vs "add", was the idea that "append" implies that it'll potentially go to new buffers created in a chain? That might be a valid reason. However, the object should be "mem" and not "bytes" to be consistent with the likes of net_buf_add_mem()

jhedberg · 2018-08-14T09:33:32Z

One other thing: net_buf is an independent module, which should have it's own independent commit.

therealprof · 2018-08-14T09:50:48Z

@jhedberg

However, the object should be "mem" and not "bytes" to be consistent with the likes of net_buf_add_mem()

The "mem" functions copy bytes from another memory location, whereas this initialises with a fixed constant, maybe just use net_buf_append_set or net_buf_append_init?. I'm also not too happy with the nomenclature, but net_buf_append_bytes was there before so I just rolled with it.

This sounds so specific that I'd rather move it to the net_pkt layer.

You mean assimilate into net_pkt_append_memset?

jhedberg · 2018-08-14T11:20:56Z

You mean assimilate into net_pkt_append_memset?

Yes. It'd then also be a private API for now, which reduces the pressure for "getting it right" from the start. If there later comes wider users we could revisit making this a net_buf API.

therealprof · 2018-08-15T11:10:22Z

@jhedberg @jukkar I created PR #9438 as an alternative implementation to this PR addressing the request of @jhedberg to assimilate the additional net_buf_append_set_bytes() function into the net_pkt_append_memset() function.

jukkar · 2018-08-16T07:35:55Z

Closing because alternate solution in #9438 got merged.

therealprof requested review from jhedberg, jukkar, pfalcon, tarunkum and tbursztyka as code owners August 12, 2018 19:21

therealprof force-pushed the features/net_pkt_append_memset branch 2 times, most recently from 75d5eb7 to c5be16b Compare August 13, 2018 07:55

jukkar added the area: Networking label Aug 13, 2018

jukkar requested a review from rveerama1 August 13, 2018 12:03

jukkar requested changes Aug 13, 2018

View reviewed changes

therealprof force-pushed the features/net_pkt_append_memset branch from c5be16b to c5b7235 Compare August 13, 2018 12:33

jukkar requested changes Aug 13, 2018

View reviewed changes

therealprof force-pushed the features/net_pkt_append_memset branch from c5b7235 to b4c1127 Compare August 13, 2018 14:21

jukkar requested changes Aug 14, 2018

View reviewed changes

therealprof added 2 commits August 14, 2018 10:15

net/dhcpv4: Use new net_pkt_append_memset() function

83a2873

Thise uses the new net_pkt_append_memset() function to generate the required zero filling instead of calling net_pkt_append_u8() in loops. Fixes zephyrproject-rtos#9287 Signed-off-by: Daniel Egger <daniel@eggers-club.de>

therealprof force-pushed the features/net_pkt_append_memset branch from b4c1127 to 83a2873 Compare August 14, 2018 08:16

jukkar approved these changes Aug 14, 2018

View reviewed changes

jhedberg requested changes Aug 14, 2018

View reviewed changes

therealprof mentioned this pull request Aug 15, 2018

Features/net pkt append memset2 #9438

Merged

jukkar closed this Aug 16, 2018

therealprof deleted the features/net_pkt_append_memset branch August 16, 2018 07:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Features/net pkt append memset #9393

Features/net pkt append memset #9393

therealprof commented Aug 12, 2018 •

edited by nashif

Loading

codecov-io commented Aug 12, 2018 •

edited

Loading

jukkar Aug 13, 2018

therealprof Aug 13, 2018

jukkar Aug 13, 2018

jhedberg Aug 14, 2018

jukkar Aug 13, 2018

jukkar Aug 13, 2018

jukkar Aug 13, 2018

jukkar Aug 13, 2018

jukkar Aug 13, 2018

jukkar Aug 13, 2018

jukkar Aug 13, 2018

jukkar Aug 13, 2018

jukkar Aug 14, 2018

jukkar Aug 13, 2018

jukkar Aug 13, 2018

pfalcon commented Aug 13, 2018

therealprof commented Aug 13, 2018

pfalcon commented Aug 13, 2018

therealprof commented Aug 13, 2018

jukkar left a comment

jukkar Aug 14, 2018

jukkar Aug 14, 2018

jhedberg Aug 14, 2018

jhedberg commented Aug 14, 2018

jhedberg commented Aug 14, 2018

therealprof commented Aug 14, 2018

jhedberg commented Aug 14, 2018

therealprof commented Aug 15, 2018

jukkar commented Aug 16, 2018

Features/net pkt append memset #9393

Features/net pkt append memset #9393

Conversation

therealprof commented Aug 12, 2018 • edited by nashif Loading

codecov-io commented Aug 12, 2018 • edited Loading

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

pfalcon commented Aug 13, 2018

therealprof commented Aug 13, 2018

pfalcon commented Aug 13, 2018

therealprof commented Aug 13, 2018

jukkar 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

jhedberg commented Aug 14, 2018

jhedberg commented Aug 14, 2018

therealprof commented Aug 14, 2018

jhedberg commented Aug 14, 2018

therealprof commented Aug 15, 2018

jukkar commented Aug 16, 2018

therealprof commented Aug 12, 2018 •

edited by nashif

Loading

codecov-io commented Aug 12, 2018 •

edited

Loading