On Sat, Jun 29, 2024 at 4:30 PM Masami Hiramatsu [off-list ref] wrote:

On Fri, 28 Jun 2024 09:34:26 -0700
Andrii Nakryiko [off-list ref] wrote:

quoted

On Thu, Jun 27, 2024 at 11:28 PM Masami Hiramatsu [off-list ref] wrote:

quoted

On Thu, 27 Jun 2024 09:47:10 -0700
Andrii Nakryiko [off-list ref] wrote:

quoted

On Thu, Jun 27, 2024 at 6:04 AM Masami Hiramatsu [off-list ref] wrote:

quoted

On Mon, 24 Jun 2024 17:21:38 -0700
Andrii Nakryiko [off-list ref] wrote:

quoted

-static int __uprobe_register(struct inode *inode, loff_t offset,
-                          loff_t ref_ctr_offset, struct uprobe_consumer *uc)
+int uprobe_register_batch(struct inode *inode, int cnt,
+                       uprobe_consumer_fn get_uprobe_consumer, void *ctx)

Is this interface just for avoiding memory allocation? Can't we just
allocate a temporary array of *uprobe_consumer instead?

Yes, exactly, to avoid the need for allocating another array that
would just contain pointers to uprobe_consumer. Consumers would never
just have an array of `struct uprobe_consumer *`, because
uprobe_consumer struct is embedded in some other struct, so the array
interface isn't the most convenient.

OK, I understand it.

quoted

If you feel strongly, I can do an array, but this necessitates
allocating an extra array *and keeping it* for the entire duration of
BPF multi-uprobe link (attachment) existence, so it feels like a
waste. This is because we don't want to do anything that can fail in
the detachment logic (so no temporary array allocation there).

No need to change it, that sounds reasonable.

Great, thanks.

quoted

quoted

Anyways, let me know how you feel about keeping this callback.

IMHO, maybe the interface function is better to change to
`uprobe_consumer *next_uprobe_consumer(void **data)`. If caller
side uses a linked list of structure, index access will need to
follow the list every time.

This would be problematic. Note how we call get_uprobe_consumer(i,
ctx) with i going from 0 to N in multiple independent loops. So if we
are only allowed to ask for the next consumer, then
uprobe_register_batch and uprobe_unregister_batch would need to build
its own internal index and remember ith instance. Which again means
more allocations and possibly failing uprobe_unregister_batch(), which
isn't great.

No, I think we can use a cursor variable as;

int uprobe_register_batch(struct inode *inode,
                 uprobe_consumer_fn get_uprobe_consumer, void *ctx)
{
        void *cur = ctx;

        while ((uc = get_uprobe_consumer(&cur)) != NULL) {
                ...
        }

        cur = ctx;
        while ((uc = get_uprobe_consumer(&cur)) != NULL) {
                ...
        }
}

This can also remove the cnt.

Ok, if you prefer this I'll switch. It's a bit more cumbersome to use
for callers, but we have one right now, and might have another one, so
not a big deal.

Thank you,

quoted

For now this API works well, I propose to keep it as is. For linked
list case consumers would need to allocate one extra array or pay the
price of O(N) search (which might be ok, depending on how many uprobes
are being attached). But we don't have such consumers right now,
thankfully.

quoted

Thank you,

quoted

quoted

Thank you,

--
Masami Hiramatsu (Google) [off-list ref]

--
Masami Hiramatsu (Google) [off-list ref]

--
Masami Hiramatsu (Google) [off-list ref]