netlink: Optimize growing the snl_writer message buffer
Needs ReviewPublic
Actions

Authored by jhb on Wed, Dec 10, 3:32 PM.

Details

Reviewers

igoro
kp
markj
melifaro

Summary

Optimize the case that the current write buffer grown by
snl_realloc_msg_buffer is allocated at the end of the associated
snl_state's current linear buffer if the linear buffer has room for
the new size. In that case, simply reuse the existing space in
the linear buffer by bumping the offset of the linear buffer for
the updated size.

Diff Detail

Repository

rG FreeBSD src repository

Lint

Lint Skipped

Unit

Tests Skipped

Build Status

Buildable 69216
Build 66099: arc lint + arc unit

Event Timeline

jhb created this revision.Wed, Dec 10, 3:32 PM

Herald added a reviewer: melifaro. · View Herald TranscriptWed, Dec 10, 3:32 PM

Herald added subscribers: glebius, melifaro, imp. · View Herald Transcript

jhb requested review of this revision.Wed, Dec 10, 3:32 PM

Harbormaster completed remote builds in B69160: Diff 167817.Wed, Dec 10, 3:32 PM

jhb added inline comments.Wed, Dec 10, 3:36 PM

sys/netlink/netlink_snl.h
126	It is debatable if this should be zeroing any padding due to rounding up the length of the previous allocation. That padding should have already been zeroed by the previous `lb_allocz` or `lb_extendz` which is why I went with the simple approach here. Actually, I just looked and `lb_allocz` doesn't do an explicit `memset` but instead depends on the original `calloc` anyway, so maybe I should drop the `memset` entirely.

glebius added inline comments.Wed, Dec 10, 4:29 PM

sys/netlink/netlink_snl.h
126	I don't think this API/KPI provides any guarantees that allocated memory is zeroed. As consumer fills a message, they are supposed to fill it in a non-sparse manner.

jhb added inline comments.Thu, Dec 11, 5:31 PM

sys/netlink/netlink_snl.h
126	I think the 'z' in `lb_allocz` does imply zeroing, and the way it works today is that `lb_init` uses `calloc` for its backing store when it is first allocated.

Drop memset

Harbormaster completed remote builds in B69216: Diff 167940.Fri, Dec 12, 8:44 PM

melifaro accepted this revision.Fri, Dec 12, 8:57 PM

This revision is now accepted and ready to land.Fri, Dec 12, 8:57 PM

LGTM. The same testing passed on my side, with the same build and test env. The only obstacle I had is this:

In file included from netlink_netlink_snl_route_parsers.c:1:
In file included from /home/igoro/src/sys/netlink/netlink_snl_route_parsers.h:30:
/home/igoro/src/sys/netlink/netlink_snl.h:122:12: error: comparison of integers of different signs: 'int' and 'uint32_t' (aka 'unsigned int') [-Werror,-Wsign-compare]
  122 |         if (delta > lb->size - lb->offset)
      |             ~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~
1 error generated.
*** [netlink_netlink_snl_route_parsers.o] Error code 1

make[3]: stopped in /home/igoro/src/tools/build/test-includes
.ERROR_TARGET='netlink_netlink_snl_route_parsers.o'

As long as we are growing only, the following was used during the testing as a quick solution:

diff --git a/sys/netlink/netlink_snl.h b/sys/netlink/netlink_snl.h
index f87abbe80981..cccff21a82d6 100644
--- a/sys/netlink/netlink_snl.h
+++ b/sys/netlink/netlink_snl.h
@@ -109,7 +109,7 @@ lb_allocz(struct linear_buffer *lb, int len)
 static inline bool
 lb_extendz(struct linear_buffer *lb, void *old, int old_len, int new_len)
 {
-       int delta;
+       uint32_t delta;

        /* Is the old buffer the last allocated region in the lb? */
        old_len = roundup2(old_len, alignof(__max_align_t));

I actually have a much larger diff I'm working on to consistently use unsigned types for lengths in this header as well as fixing some potential integer overflow issues. I'll try to upload that later this week. I think I also fixed an overflow check in this patch as well as part of that.

Rebase on D54397

This revision now requires review to proceed.Tue, Dec 30, 4:32 PM

Harbormaster completed remote builds in B69530: Diff 168780.Tue, Dec 30, 4:32 PM

jhb added a parent revision: D54397: netlink: Consistently use unsigned types for message and buffer lengths.Tue, Dec 30, 4:32 PM