Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20200727184506.2279656-1-guro@fb.com> <20200727184506.2279656-28-guro@fb.com>
 <CAPhsuW7jWztOVeeiRNBRK4JC_MS41qUSxzEDMywb-6=Don-ndA@mail.gmail.com>
 <CAEf4BzaOX_gc8F20xrHxiKFxYbwULK130m1A49rnMoT7T74T3Q@mail.gmail.com>
 <CAPhsuW5qBxWibkYMAvS0s6yLj-gijHqy9rVxSWCk5Xr+bXqtJg@mail.gmail.com> <20200728190830.GB410810@carbon.DHCP.thefacebook.com>
In-Reply-To: <20200728190830.GB410810@carbon.DHCP.thefacebook.com>
From:   Andrii Nakryiko <andrii.nakryiko@gmail.com>
Date:   Tue, 28 Jul 2020 12:16:51 -0700
Message-ID: <CAEf4BzZuj9d_WT4nJ6c_W4uAnT2_4mBOXCbi1q1w97568rd4eg@mail.gmail.com>
Subject: Re: [PATCH bpf-next v2 27/35] bpf: eliminate rlimit-based memory
 accounting infra for bpf maps
To:     Roman Gushchin <guro@fb.com>
Cc:     Song Liu <song@kernel.org>, bpf <bpf@vger.kernel.org>,
        Networking <netdev@vger.kernel.org>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        Kernel Team <kernel-team@fb.com>,
        open list <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Tue, Jul 28, 2020 at 12:09 PM Roman Gushchin <guro@fb.com> wrote:
>
> On Mon, Jul 27, 2020 at 11:06:42PM -0700, Song Liu wrote:
> > On Mon, Jul 27, 2020 at 10:58 PM Andrii Nakryiko
> > <andrii.nakryiko@gmail.com> wrote:
> > >
> > > On Mon, Jul 27, 2020 at 10:47 PM Song Liu <song@kernel.org> wrote:
> > > >
> > > > On Mon, Jul 27, 2020 at 12:26 PM Roman Gushchin <guro@fb.com> wrote:
> > > > >
> > > > > Remove rlimit-based accounting infrastructure code, which is not used
> > > > > anymore.
> > > > >
> > > > > Signed-off-by: Roman Gushchin <guro@fb.com>
> > > > [...]
> > > > >
> > > > >  static void bpf_map_put_uref(struct bpf_map *map)
> > > > > @@ -541,7 +484,7 @@ static void bpf_map_show_fdinfo(struct seq_file *m, struct file *filp)
> > > > >                    "value_size:\t%u\n"
> > > > >                    "max_entries:\t%u\n"
> > > > >                    "map_flags:\t%#x\n"
> > > > > -                  "memlock:\t%llu\n"
> > > > > +                  "memlock:\t%llu\n" /* deprecated */
> > > >
> > > > I am not sure whether we can deprecate this one.. How difficult is it
> > > > to keep this statistics?
> > > >
> > >
> > > It's factually correct now, that BPF map doesn't use any memlock memory, no?
>
> Right.
>
> >
> > I am not sure whether memlock really means memlock for all users... I bet there
> > are users who use memlock to check total memory used by the map.
>
> But this is just the part of struct bpf_map, so I agree with Andrii,
> it's a safe check.
>
> >
> > >
> > > This is actually one way to detect whether RLIMIT_MEMLOCK is necessary
> > > or not: create a small map, check if it's fdinfo has memlock: 0 or not
> > > :)
> >
> > If we do show memlock=0, this is a good check...
>
> The only question I have if it's worth checking at all? Bumping the rlimit
> is a way cheaper operation than creating a temporarily map and checking its
> properties.
>

for perf and libbpf -- I think it's totally worth it. Bumping
RLIMIT_MEMLOCK automatically means potentially messing up some other
parts of the system (e.g., BCC just bumps it to INFINITY allowing to
over-allocate too much memory, potentially, for unrelated applications
that do rely on RLIMIT_MEMLOCK). It's one of the reasons why libbpf
doesn't do it automatically, actually. So knowing when this is not
necessary, will allow to improve diagnostic messages by libbpf, and
would just avoid potentially risky operation by perf/BCC/etc.

> So is there any win in comparison to just leaving the userspace code* as it is
> for now?
>
> * except runqslower and samples