Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755529AbaKEO5b (ORCPT ); Wed, 5 Nov 2014 09:57:31 -0500 Received: from mx1.redhat.com ([209.132.183.28]:38299 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751198AbaKEO53 (ORCPT ); Wed, 5 Nov 2014 09:57:29 -0500 Message-ID: <545A3ACC.3080101@redhat.com> Date: Wed, 05 Nov 2014 15:57:16 +0100 From: Daniel Borkmann User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:17.0) Gecko/17.0 Thunderbird/17.0 MIME-Version: 1.0 To: Alexei Starovoitov CC: "David S. Miller" , Ingo Molnar , Andy Lutomirski , Hannes Frederic Sowa , Eric Dumazet , Linux API , Network Development , LKML Subject: Re: [PATCH net-next 1/7] bpf: add 'flags' attribute to BPF_MAP_UPDATE_ELEM command References: <1415069656-14138-1-git-send-email-ast@plumgrid.com> <1415069656-14138-2-git-send-email-ast@plumgrid.com> <54589B89.5000309@redhat.com> In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 11/05/2014 12:04 AM, Alexei Starovoitov wrote: > On Tue, Nov 4, 2014 at 1:25 AM, Daniel Borkmann wrote: >> On 11/04/2014 03:54 AM, Alexei Starovoitov wrote: >>> >>> the current meaning of BPF_MAP_UPDATE_ELEM syscall command is: >>> either update existing map element or create a new one. >>> Initially the plan was to add a new command to handle the case of >>> 'create new element if it didn't exist', but 'flags' style looks >>> cleaner and overall diff is much smaller (more code reused), so add 'flags' >>> attribute to BPF_MAP_UPDATE_ELEM command with the following meaning: >>> enum { >>> BPF_MAP_UPDATE_OR_CREATE = 0, /* add new element or update existing */ >>> BPF_MAP_CREATE_ONLY, /* add new element if it didn't exist */ >>> BPF_MAP_UPDATE_ONLY /* update existing element */ >>> }; >> >> From you commit message/code I currently don't see an explanation why >> it cannot be done in typical ``flags style'' as various syscalls do, >> i.e. BPF_MAP_UPDATE_OR_CREATE rather represented as ... >> >> BPF_MAP_CREATE | BPF_MAP_UPDATE >> >> Do you expect more than 64 different flags to be passed from user space >> for BPF_MAP? > > several reasons: > - preserve flags==0 as default behavior > - avoid holes and extra checks for invalid combinations, so > if (flags > BPF_MAP_UPDATE_ONLY) goto err, is enough. > - it looks much neater when user space uses > BPF_MAP_UPDATE_OR_CREATE instead of ORing bits. > > Note this choice doesn't prevent adding bit-like flags > in the future. Today I cannot think of any new flags > for the update() command, but if somebody comes up with > a new selector that can apply to all three combinations, > we can add it as 3rd bit that can be ORed. Hm, mixing enums together with bitfield-like flags seems kind of hacky ... :/ Or, do you mean to say you're adding a 2nd flag field, i.e. splitting the 64bits into a 32bit ``cmd enum'' and 32bit ``flag section''? I see the point with flags == 0 as default behavior though, but at this point in time we won't get burnt by it since the API is not yet in a usable state and defaults to be compiled-out. > Default will stay zero and 'if >' check in older > kernels will seamlessly work with new userspace. > I don't like holes in flags and combinatorial > explosion of bits and checks for them unless > absolutely necessary. Hm, my concern is that we start to add many *_OR_* enum elements once we find that a flag might be a useful in combination with many other flags ... even though if we only can think of 3 flags /right now/. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/