On Thu, Nov 2, 2023 at 7:58 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
>
> On Wed, Nov 01, 2023 at 03:21:42PM -0700, Andrii Nakryiko wrote:
> > On Fri, Oct 27, 2023 at 7:30 AM Jiri Olsa <olsajiri@xxxxxxxxx> wrote:
> > >
> > > On Thu, Oct 26, 2023 at 10:55:35AM -0700, Song Liu wrote:
> > > > On Wed, Oct 25, 2023 at 1:24 PM Jiri Olsa <jolsa@xxxxxxxxxx> wrote:
> > > > [...]
> >
> > [...]
> >
> > > >
> > > > > + if (upath_max > PATH_MAX)
> > > > > + return -E2BIG;
> > > >
> > > > I don't think we need to fail here. How about we simply do
> > > >
> > > > upath_max = min_ut(u32, upath_max, PATH_MAX);
> > >
> > > ok
> >
> > +1, was going to say the same
> >
> > >
> > > >
> > > > > + buf = kmalloc(upath_max, GFP_KERNEL);
> > > > > + if (!buf)
> > > > > + return -ENOMEM;
> > > > > + p = d_path(&umulti_link->path, buf, upath_max);
> > > > > + if (IS_ERR(p)) {
> > > > > + kfree(buf);
> > > > > + return -ENOSPC;
> > > > > + }
> > > > > + left = copy_to_user(upath, p, buf + upath_max - p);
> > > > > + kfree(buf);
> > > > > + if (left)
> > > > > + return -EFAULT;
> > > > > + }
> > > > > +
> > > > > + if (!uoffsets)
> > > > > + return 0;
> > > > > +
> > > > > + if (ucount < umulti_link->cnt)
> > > > > + err = -ENOSPC;
> > > > > + else
> > > > > + ucount = umulti_link->cnt;
> > > > > +
> > > > > + for (i = 0; i < ucount; i++) {
> > > > > + if (put_user(umulti_link->uprobes[i].offset, uoffsets + i))
> > > > > + return -EFAULT;
> > > > > + if (uref_ctr_offsets &&
> > > > > + put_user(umulti_link->uprobes[i].ref_ctr_offset, uref_ctr_offsets + i))
> > > > > + return -EFAULT;
> > > > > + if (ucookies &&
> > > > > + put_user(umulti_link->uprobes[i].cookie, ucookies + i))
> > > > > + return -EFAULT;
> > > >
> > > > It feels expensive to put_user() 3x in a loop. Maybe we need a new struct
> > > > with offset, ref_ctr_offset, and cookie?
> > >
> > > good idea, I think we could store offsets/uref_ctr_offsets/cookies
> > > together both in kernel and user sapce and use just single put_user
> > > call... will check
> > >
> >
> > hm... only offset is mandatory, and then we can have either cookie or
> > ref_ctr_offset or both, so co-locating them in the same struct seems
> > inconvenient and unnecessary
>
> yes, using struct seems too complicated because of this
>
> but during attach we could store offsets/ref_ctr_offsets/cookies
> in separated arrays (instead of in bpf_uprobe) and just use single
> copy_to_user call for each array in here
I think you are trying to optimize for the wrong thing here. link_info
fetching isn't the most performance critical operation, I wouldn't
bother complicating code just to make it a microsecond faster.
>
> jirka
