-
Notifications
You must be signed in to change notification settings - Fork 8.1k
net_buf: support large size #97440
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
net_buf: support large size #97440
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please see https://github.com/zephyrproject-rtos/zephyr/pull/97440/files#r2425645621
Besides that, the following functions/structs need to be updated too:
uint16_t net_buf_simple_max_len(const struct net_buf_simple *buf);
struct net_buf_simple_state;
static inline void net_buf_simple_save(const struct net_buf_simple *buf,
struct net_buf_simple_state *state);
struct net_buf;
Make sure the tests in |
3a3a6a6
to
938fd0f
Compare
Tested
|
include/zephyr/net_buf.h
Outdated
|
||
#if defined(CONFIG_NET_BUF_LARGE_SIZE) | ||
/** @brief net buf size type */ | ||
typedef size_t net_buf_size_t; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we look at #96903, should we prefer uint32_t
?
Not blocking, just a question.
include/zephyr/net_buf.h
Outdated
#if defined(CONFIG_NET_BUF_LARGE_SIZE) | ||
/** @brief net buf size type */ | ||
typedef size_t net_buf_size_t; | ||
#else | ||
/** @brief net buf size type */ | ||
typedef uint16_t net_buf_size_t; | ||
#endif | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that it makes any sense to have it configurable, nor use size_t. This will introduce a lot of mess because no one would know what actual maximal size is. Either it remains uint16_t, or we change it to uint32_t. And USB in Zephyr for sure does not require it to be larger than uint16_t.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And USB in Zephyr for sure does not require it to be larger than uint16_t.
Could you explain more about it?
Windows will write/read 64KiB data to/from usb disk. If the buffer of msc is configured as 64KiB and give the 64KiB buffer to controller at one time, then the controller can transfer all the 64 KiB bytes without host stack and msc class involved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do not think that it makes any sense to have it configurable, nor use size_t. This will introduce a lot of mess because no one would know what actual maximal size is.
@jfischer-no I agree about not using size_t
, since it needs to be more explicit than that, however no code should be using the struct member types for doing any "maximal size" derminations. net_buf offers separate APIs like net_buf_max_len()
, net_buf_headroom()
and net_buf_tailroom()
to determine how much storage space is available.
include/zephyr/net_buf.h
Outdated
uint16_t net_buf_simple_max_len(const struct net_buf_simple *buf); | ||
net_buf_size_t net_buf_simple_max_len(const struct net_buf_simple *buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is an oversight in the API. The public API has strived to keep the uint16_t
as much of an internal detail as possible, while using size_t
for both input and return parameters. I think this one should probably just be changed to size_t
to be consistent with the rest of the API.
As for the point @jfischer-no raised regarding the predictability of what the buffer can actually contain, I agree that internal (within the structure) a fixed size type (e.g. uint32_t
) makes sense.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jhedberg , I do the changes, please help to review whether it is your thought.
change uint16_t as uint32_t to support large size of data. Signed-off-by: Mark Wang <yichang.wang@nxp.com>
938fd0f
to
e78e3dc
Compare
|
uint16_t len; | ||
uint32_t len; | ||
|
||
/** Amount of data that net_buf_simple#__buf can store. */ | ||
uint16_t size; | ||
uint32_t size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you misunderstood me a little bit. I wasn't proposing to change this unconditionally to uint32_t
, rather you'd need some explicit enabling of the larger size, through Kconfig, like you had before. Otherwise you're causing memory overflows to platforms what are very constrained (we have at least some build configurations for 16k platforms where even just a few bytes increase in RAM consumption will cause the build to fail).
* @return Number of bytes usable behind the net_buf_simple::data pointer. | ||
*/ | ||
uint16_t net_buf_simple_max_len(const struct net_buf_simple *buf); | ||
size_t net_buf_simple_max_len(const struct net_buf_simple *buf); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This should be in a separate commit, with the commit message explaining that it's just aligning this with the rest of the net_buf APIs.
uint16_t offset; | ||
uint32_t offset; | ||
/** Length of data */ | ||
uint16_t len; | ||
uint32_t len; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here. This should be build-time conditional.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Got it, I will add back the net_buf_size_t
too. Do you have concern about using typedef
? IMO, both are OK.
#if defined(CONFIG_NET_BUF_LARGE_SIZE)
/** @brief net buf size type */
typedef uint32_t net_buf_size_t;
#else
/** @brief net buf size type */
typedef uint16_t net_buf_size_t;
#endif
...
net_buf_size_t len;
...
or
...
#if defined(CONFIG_NET_BUF_LARGE_SIZE)
uint16_t len;
#else
uint32_t len;
#endif
...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Considering that you need to have the correct type in multiple structs and multiple member variables, I think the typedef would be the simplest option.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That said, I'd appreciate some more thoughts from @jfischer-no, since he seemed to be against this. @jfischer-no changing unconditionally to a larger size will not work without also finding a solution for our memory constrained platforms that can't currently take any additional memory hit wrt to net_buf sizes.
uint16_t len; | ||
uint32_t len; | ||
|
||
/** Amount of data that this buffer can store. */ | ||
uint16_t size; | ||
uint32_t size; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And here.
Initially I think we should make the larger size opt in. We can then discuss if eventually it'd be opt-out, and then we make the memory-constrained configurations explicitly select the smaller size.
* @return Number of bytes usable behind the net_buf::data pointer. | ||
*/ | ||
static inline uint16_t net_buf_max_len(const struct net_buf *buf) | ||
static inline size_t net_buf_max_len(const struct net_buf *buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate commit.
} | ||
|
||
uint16_t net_buf_simple_max_len(const struct net_buf_simple *buf) | ||
size_t net_buf_simple_max_len(const struct net_buf_simple *buf) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Separate commit
change uint16_t as size_t to support large size of data.
The user case:
In USB, the net_buf is used. usb msc may transfer 64KB (65536) data in one net_buf. So the
uint16_t
can't handle 65536.