net: buf: resolve issues with fixed size network buffer pool

[kderda]

This PR fixes issue discussed in #66870 (fix proposed by @jhedberg) with new test case.
Additionally, it resolves an additional issue where fixed buffer would set it's size to requested length instead of the actual size (with additional check in existing unit test) .

Fixes #66870

[jhedberg]

Looks pretty good, thanks! Do take a look at my inline comments, however.

Also, I think this might justify a mention in doc/releases/migration-guide-3.6.rst since it's a change to a public API (even though custom out-of-tree net_buf_pool implementations are likely quite rare)

[jhedberg]

minor: add a , to the end. It's not needed now, but if this ever gets extended with another struct member, then you just need to add another line instead of also modifying this existing one.

I was also thinking that maybe 0 could have been used to mean "unrestricted" but then again since we do have this define for the system heap I guess we can use that here. Actually, now that I think about it, maybe we anyway want 0 to have this special meaning, since that would give some backwards compatibility to any existing definitions of custom allocators, since any not explicitly initialized struct members are 0 by default.

[kderda]

To my mind, using 0 as the special value is great. I was thinking about using a max value but it could confuse someone. The solution with holding an initial value seemed better but was still a lie as in theory this value could be interpreted as maximum contiguous memory block. With 0 it is also easy to use in net_buf_append_bytes().

[kderda]

@jhedberg I have added a point to the migration guide

[jhedberg]

@konradderda please fix the CI failure:

/__w/zephyr/zephyr/subsys/net/buf.c:681:15: error: variable 'pool' is uninitialized when used here [-Werror,-Wuninitialized]
                        max_size = pool->alloc->max_alloc_size;
                                   ^~~~
/__w/zephyr/zephyr/subsys/net/buf.c:676:29: note: initialize the variable 'pool' to silence this warning
                        struct net_buf_pool *pool;
                                                 ^
                                                  = NULL
1 error generated.

[kderda]

@jhedberg fixed

[kderda]

It seems that I need also to align few test cases for net_pkt as few asserts assume the requested size, not the actual one.

[kderda]

@jhedberg I have added a commit that changes failing tests so they use VAR pool instead of FIXED as it does not impact the test cases. I moved it to the beginning of commits in this PR so Zephyr build and tests will pass at every commit.

[henrikbrixandersen]

@jhedberg Is this targeting v3.6.0? If so, please add it as milestone.

[jhedberg]

@jhedberg Is this targeting v3.6.0? If so, please add it as milestone.

Done. It's fixing an issue with allocations from fixed-size buffer pools, so I think it'd be good to get it merged. There's also the appropriate bug report reference.

[M1cha]

@konradderda

Moreover, this commit fixes an issue where allocation of less than
maximum number of bytes from a fixed buffer pool would set buffer's
size to this number instead of the whole buffer size.

But why is that in issue? If I allocate 2 bytes, I expect the allocation to be 2 bytes, no matter how large the internal(fixed) buffer is.

[M1cha]

Mh I just learned about the implementation of net_buf_alloc. That function does not take a length parameter and simplfy forwards to net_buf_alloc_fixed, so that function would be unsafe to call, if you don't know the pool type.

While that kinda makes sense and would fit your expectations, the weird part is, that net_buf_alloc_fixed internally calls the generic API net_buf_alloc_len, but with the fixed chunk length. So a fixed-specific function uses a generic function to do the allocation and by doing so has to pass a parameter that's conceptionally not needed.

I always thought, net_buf_alloc would just allocate a buffer without data, because that's actually supported by calling net_buf_alloc_len with a size of 0.

To me this looks like the allocator API went through some changes and that these functions implement the old behavior for compatibility reasons.

[kderda]

@M1cha please do not edit my comments...

It makes sense to have more memory allocated then requested for example in case of network packets when someone write fields one by one (addresses, options etc.) using Zephyr's net_pkt_* API it would end up with a chain of small buffer fragments instead of larger one.

Before this PR such behavior led to:

1. Unnecessary number of memory allocation
2. Slower packet processing
3. Higher memory consumption - any packet write would allocate a new fixed buffer.

You can checkout the commit before this PR was merged and play with unit tests and printk() or GDB to better understand the change. Other than that if you would like discuss something related to the design of implementation of buffers please open a new issue instead.

[M1cha]

@konradderda

@M1cha please do not edit my comments...

Sorry about that, I don't know how that happened. I wanted to restore your comment, but currently can't see your original comment at all. Do you know what happened to it?

It makes sense to have more memory allocated then requested for example in case of network packets when someone write fields one by one (addresses, options etc.) using Zephyr's net_pkt_* API it would end up with a chain of small buffer fragments instead of larger one.

Before this PR such behavior led to:

1. Unnecessary number of memory allocation

2. Slower packet processing

3. Higher memory consumption - any packet write would allocate a new fixed buffer.

You can checkout the commit before this PR was merged and play with unit tests and printk() or GDB to better understand the change. Other than that if you would like discuss something related to the design of implementation of buffers please open a new issue instead.

I didn't know about that use-case. That makes sense, thx.