On Fri, 28 Jun 2024 09:34:26 -0700 Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > On Thu, Jun 27, 2024 at 11:28 PM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote: > > > > On Thu, 27 Jun 2024 09:47:10 -0700 > > Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > > > On Thu, Jun 27, 2024 at 6:04 AM Masami Hiramatsu <mhiramat@xxxxxxxxxx> wrote: > > > > > > > > On Mon, 24 Jun 2024 17:21:38 -0700 > > > > Andrii Nakryiko <andrii@xxxxxxxxxx> wrote: > > > > > > > > > -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. > > > > > > > > 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. > > > > > > > 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. Thank you, > > 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. > > > > > Thank you, > > > > > > > > > > > > > > > Thank you, > > > > > > > > -- > > > > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> > > > > > > -- > > Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx> -- Masami Hiramatsu (Google) <mhiramat@xxxxxxxxxx>