Re: [PATCH bpf-next 3/6] bpf: Add link_info support for uprobe multi link

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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





[Index of Archives]     [Linux Samsung SoC]     [Linux Rockchip SoC]     [Linux Actions SoC]     [Linux for Synopsys ARC Processors]     [Linux NFS]     [Linux NILFS]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]


  Powered by Linux