Re: [PATCH v7 bpf] xsk: fix immature cq descriptor production

On Wed, Sep 3, 2025 at 12:22 AM Alexei Starovoitov
[off-list ref] wrote:

On Tue, Sep 2, 2025 at 6:39 AM Jason Xing [off-list ref] wrote:

quoted

quoted

quoted

quoted

quoted

quoted

+               list_for_each_entry_safe(pos, tmp, &XSKCB(skb)->addrs_list, addr_node) {

It seems no need to use xxx_safe() since the whole process (from
allocating skb to freeing skb) makes sure each skb can be processed
atomically?

We're deleting nodes from linked list so we need the @tmp for further list
traversal, I'm not following your statement about atomicity here?

I mean this list is chained around each skb. It's not possible for one
skb to do the allocation operation and free operation at the same
time, right? That means it's not possible for one list to do the
delete operation and add operation at the same time. If so, the
xxx_safe() seems unneeded.

_safe() variants are meant to allow you to delete nodes while traversing
the list.
You wouldn't be able to traverse the list when in body of the loop nodes
are deleted as the ->next pointer is poisoned by list_del(). _safe()
variant utilizes additional 'tmp' parameter to allow you doing this
operation.

Sure, this is exactly how _safe() works. My take is we don't need to
use _safe() to keep safety because it's not possible for one reader
traversing the entire addr list while another one is trying to delete
node. If it can happen, then _safe() does make sense.

Jason,
sounds like you're still confused what "_safe" suffix does.
"_safe" doesn't help with concurrent access at all.

Hi, Alex.

Quoting Maciej to explain the function of _safe(): _safe() variants
are meant to allow you to delete nodes while traversing the list.

I meant the _safe is not needed at all as I explained above. The
af_xdp logic makes sure processes (like reading/adding/deleting) nodes
of this addr list are serialized. So why add _safe here, I wonder?
Just remove the _safe suffix then.

The moment you jump into the conversation, I feel I might get stuck
somehow, but I'm not aware of it... Please correct me if I'm wrong.

Sure, it's a trivial thing because it has no impact on the whole patch.

Thanks,
Jason