From mboxrd@z Thu Jan  1 00:00:00 1970
From: Alexei Starovoitov <alexei.starovoitov@gmail.com>
Subject: Re: [RFC PATCH bpf-next v3 4/7] bpf: add bpf queue and stack maps
Date: Mon, 1 Oct 2018 17:26:41 -0700
Message-ID: <20181002002640.ug7manxi2livgrlo@ast-mbp.dhcp.thefacebook.com>
References: <153724634652.7866.6354309647800281793.stgit@kernel>
 <153724637114.7866.8418882266337744689.stgit@kernel>
 <20180918232753.2vk6u256ag27cjgo@ast-mbp>
 <d6de98eb-9a52-3d48-2566-4fd3a7b981f2@polito.it>
 <12a0d7d2-9590-b44f-803a-a00eefe611c1@polito.it>
Mime-Version: 1.0
Content-Type: text/plain; charset=iso-8859-1
Content-Transfer-Encoding: 8bit
Cc: daniel@iogearbox.net, netdev@vger.kernel.org
To: Mauricio Vasquez <mauricio.vasquez@polito.it>
Return-path: <netdev-owner@vger.kernel.org>
Received: from mail-io1-f67.google.com ([209.85.166.67]:44151 "EHLO
        mail-io1-f67.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org
        with ESMTP id S1725936AbeJBHHH (ORCPT
        <rfc822;netdev@vger.kernel.org>); Tue, 2 Oct 2018 03:07:07 -0400
Received: by mail-io1-f67.google.com with SMTP id x26-v6so141562iog.11
        for <netdev@vger.kernel.org>; Mon, 01 Oct 2018 17:26:45 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <12a0d7d2-9590-b44f-803a-a00eefe611c1@polito.it>
Sender: netdev-owner@vger.kernel.org
List-ID: <netdev.vger.kernel.org>

On Mon, Oct 01, 2018 at 08:11:43AM -0500, Mauricio Vasquez wrote:
> > > > +BPF_CALL_3(bpf_map_pop_elem, struct bpf_map *, map, void *,
> > > > value, u32, size)
> > > > +{
> > > > +    void *ptr;
> > > > +
> > > > +    if (map->value_size != size)
> > > > +        return -EINVAL;
> > > > +
> > > > +    ptr = map->ops->map_lookup_and_delete_elem(map, NULL);
> > > > +    if (!ptr)
> > > > +        return -ENOENT;
> > > > +
> > > > +    switch (size) {
> > > > +    case 1:
> > > > +        *(u8 *) value = *(u8 *) ptr;
> > > > +        break;
> > > > +    case 2:
> > > > +        *(u16 *) value = *(u16 *) ptr;
> > > > +        break;
> > > > +    case 4:
> > > > +        *(u32 *) value = *(u32 *) ptr;
> > > > +        break;
> > > > +    case 8:
> > > > +        *(u64 *) value = *(u64 *) ptr;
> > > > +        break;
> > > this is inefficient. can we pass value ptr into ops and let it
> > > populate it?
> > 
> > I don't think so, doing that implies that look_and_delete will be a
> > per-value op, while other ops in maps are per-reference.
> > For instance, how to change it in the case of peek helper that is using
> > the lookup operation?, we cannot change the signature of the lookup
> > operation.
> > 
> > This is something that worries me a little bit, we are creating new
> > per-value helpers based on already existing per-reference operations,
> > this is not probably the cleanest way.  Here we are at the beginning of
> > the discussion once again, how should we map helpers and syscalls to
> > ops.
> > 
> > What about creating pop/peek/push ops, mapping helpers one to one and
> > adding some logic into syscall.c to call the correct operation in case
> > the map is stack/queue?
> > Syscall mapping would be:
> > bpf_map_lookup_elem() -> peek
> > bpf_map_lookup_and_delete_elem() -> pop
> > bpf_map_update_elem() -> push
> > 
> > Does it make sense?
> 
> Hello Alexei,
> 
> Do you have any feedback on this specific part?

Indeed. It seems push/pop ops will be cleaner.
I still think that peek() is useless due to races.
So BPF_MAP_UPDATE_ELEM syscall cmd will map to 'push' ops
and new BPF_MAP_LOOKUP_AND_DELETE_ELEM will map to 'pop' ops.
right?