Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
MIME-Version: 1.0
References: <20190703170118.196552-1-brianvv@google.com> <20190703170118.196552-3-brianvv@google.com>
 <CAH3MdRU505Er44m460c7y5nxtZxmDmVY4jDrWOYt2=OdP2d5Ow@mail.gmail.com>
In-Reply-To: <CAH3MdRU505Er44m460c7y5nxtZxmDmVY4jDrWOYt2=OdP2d5Ow@mail.gmail.com>
From:   Brian Vazquez <brianvv.kernel@gmail.com>
Date:   Tue, 9 Jul 2019 08:34:11 -0700
Message-ID: <CABCgpaU=H+qOUAx+yRHJjBMqmeAtp5iQL_yv9cXB6HTqB0jXcA@mail.gmail.com>
Subject: Re: [PATCH bpf-next RFC v3 2/6] bpf: add BPF_MAP_DUMP command to dump
 more than one entry per call
To:     Y Song <ys114321@gmail.com>
Cc:     Brian Vazquez <brianvv@google.com>,
        Alexei Starovoitov <ast@kernel.org>,
        Daniel Borkmann <daniel@iogearbox.net>,
        "David S . Miller" <davem@davemloft.net>,
        Stanislav Fomichev <sdf@google.com>,
        Willem de Bruijn <willemb@google.com>,
        Petar Penkov <ppenkov@google.com>,
        LKML <linux-kernel@vger.kernel.org>,
        netdev <netdev@vger.kernel.org>, bpf <bpf@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

> Maybe you can swap map_fd and flags?
> This way, you won't have hole right after map_fd?

Makes sense.

> > +       attr->flags = 0;
> Why do you want attr->flags? This is to modify anonumous struct used by
> BPF_MAP_*_ELEM commands.

Nice catch! This was a mistake I forgot to delete that line.

> In bcc, we have use cases like this. At a certain time interval (e.g.,
> every 2 seconds),
> we get all key/value pairs for a map, we format and print out map
> key/values on the screen,
> and then delete all key/value pairs we retrieved earlier.
>
> Currently, bpf_get_next_key() is used to get all key/value pairs, and
> deletion also happened
> at each key level.
>
> Your batch dump command should help retrieving map key/value pairs.
> What do you think deletions of those just retrieved map entries?
> With an additional flag and fold into BPF_MAP_DUMP?
> or implement a new BPF_MAP_DUMP_AND_DELETE?
>
> I mentioned this so that we can start discussion now.
> You do not need to implement batch deletion part, but let us
> have a design extensible for that.
>
> Thanks.

With a additional flag, code could be racy where you copy an old value
and delete the newest one.
So maybe we could implement BPF_MAP_DUMP_AND_DELETE as a wrapper of
map_get_next_key + map_lookup_and_delete_elem. Last function already
exists but it has not been implemented for maps other than stack and
queue.

Thanks for reviewing it!