net: buf: deprecate net_buf_slist_get, net_buf_slist_put

[kamnxt]

Deprecate net_buf_slist_get() and net_buf_slist_put().
Commit 3d306c1 back in December 2022 removed the need for these helper functions, by storing the fragments separately from nodes.
Also removed the part of the docs stating these must be used when storing net_bufs in slists instead of sys_slist_get()/sys_slist_append().

[kamnxt]

(should probably also be added to #86000)

[jhedberg]

This makes me a little bit uneasy, since it's not quite the same as the original code. The node struct member happens to be the first one in struct net_buf, i.e. its address coincides with the address of the struct. However, using CONTAINER_OF() makes it look like it would now be safe to move node to some other position within the struct. This is not true however, since you're doing the NULL check: the slist API could return NULL, but then CONTAINER_OF() would convert the pointer to something non-NULL, making it seem like you got a valid pointer when in fact you didn't.

[kamnxt]

That's a pretty good point, didn't think of that. Should I change the relevant locations to get a sys_snode_t, and only use CONTAINER_OF later after the NULL check?

For example, in another location:

/* Discard queued buffers */
while ((buf = CONTAINER_OF(sys_slist_get(&att->prep_queue), struct net_buf, node))) {
	net_buf_unref(buf);
}

to

/* Discard queued buffers */
while ((node = sys_slist_get(&att->prep_queue))) {
	net_buf_unref(CONTAINER_OF(node, struct net_buf, node));
}

?

[Thalley]

Just FYI, both loops you've suggested would break rule 79 of https://docs.zephyrproject.org/latest/contribute/coding_guidelines/index.html I think.

You probably need to use a do { } while instead

[jhedberg]

@kamnxt your proposal looks ok to me. As for the comment from @Thalley, what should hopefully keep the code size the same, would be to use !sys_slist_is_empty() as the while-loop condition, after which you can use sys_slist_get_not_empty() inside the loop.

[jhedberg]

Btw, with the separate !sys_slist_is_empty() check, the CONTAINER_OF() actually does become safe since it'd never be called on a NULL pointer.

[Thalley]

Looks better to me at least. I haven't looked enough into it to determine if it's done in a safe manner; doing a sys_slist_is_empty followed by a sys_slist_get_not_empty doesn't sound thread safe, so only if a single thread is ever accessing the lists can this be considered safe.

Maybe @jhedberg or @alwa-nordic know more about whether the lists changed in Bluetooth are accessed by multiple threads

[jhedberg]

doing a sys_slist_is_empty followed by a sys_slist_get_not_empty doesn't sound thread safe, so only if a single thread is ever accessing the lists can this be considered safe

This is just as safe/unsafe as a single sys_slist_get() call. The slist API is documented as not being multithread safe. Since the old net_buf_slist API did have a lock assume @kamnxt has done some level of analysis and concluded that locking is not needed?

[kamnxt]

Haven't looked too much into this no. I also see a few places in friend.c where several operations are done without a lock, for example here

zephyr/subsys/bluetooth/mesh/friend.c

Line 754 in 1ac19d3

if (sys_slist_is_empty(&frnd->queue)) {

I'll look through and see if there are any places where a lock might be needed with this change.

[jhedberg]

@kamnxt note that in most places locking in the Bluetooth stack is implicit, since almost everything runs in cooperative threads. The places that need protection are where a preemptible app-side thread might call into the stack or some shared data might be accessed in ISR context.

[alwa-nordic]

I'm fairly confident we rely on a thread safe net_buf_slist_get, so we need to make sure a refactor does the right thing.

@kamnxt I think we regard sys_slist_is_empty as thread safe, even if not explicitly documented. The implementation is thread safe (at least on architectures we care about).

[Thalley]

Minor formatting comment

[Thalley]

Suggested change

	* Deprecated the ``net_buf_slist_put()`` and ``net_buf_slist_get()`` API functions,
	as they are no longer needed. Use ``sys_slist_get()``/``sys_slist_append()``.
	* Deprecated the :c:func:`net_buf_slist_put()` and :c:func:`net_buf_slist_get()` API functions,
	as they are no longer needed. Use :c:func:`sys_slist_append()` and :c:func:`sys_slist_get()`.