From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesper Dangaard Brouer Subject: Re: [bpf-next V1-RFC PATCH 01/14] xdp: base API for new XDP rx-queue info concept Date: Mon, 18 Dec 2017 11:55:01 +0100 Message-ID: <20171218115501.3f1fcf36@redhat.com> References: <151316391502.14967.13292358380181773729.stgit@firesoul> <151316396600.14967.5648145904814220715.stgit@firesoul> <3b781865-be2a-740b-8403-fe47fea929bc@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8BIT Cc: Daniel Borkmann , Alexei Starovoitov , netdev@vger.kernel.org, gospo@broadcom.com, bjorn.topel@intel.com, michael.chan@broadcom.com, brouer@redhat.com To: David Ahern Return-path: Received: from mx1.redhat.com ([209.132.183.28]:52898 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752200AbdLRKzL (ORCPT ); Mon, 18 Dec 2017 05:55:11 -0500 In-Reply-To: <3b781865-be2a-740b-8403-fe47fea929bc@gmail.com> Sender: netdev-owner@vger.kernel.org List-ID: On Wed, 13 Dec 2017 19:34:40 -0700 David Ahern wrote: > On 12/13/17 4:19 AM, Jesper Dangaard Brouer wrote: > > + > > +void xdp_rxq_info_unreg(struct xdp_rxq_info *xdp_rxq) > > +{ > > + xdp_rxq->reg_state = REG_STATE_UNREGISTRED; > > +} > > +EXPORT_SYMBOL_GPL(xdp_rxq_info_unreg); > > + > > +void xdp_rxq_info_init(struct xdp_rxq_info *xdp_rxq) > > +{ > > + if (xdp_rxq->reg_state == REG_STATE_REGISTRED) { > > + WARN(1, "Missing unregister, handled but fix driver\n"); > > + xdp_rxq_info_unreg(xdp_rxq); > > + } > > + memset(xdp_rxq, 0, sizeof(*xdp_rxq)); > > + xdp_rxq->queue_index = U32_MAX; > > + xdp_rxq->reg_state = REG_STATE_NEW; > > +} > > +EXPORT_SYMBOL_GPL(xdp_rxq_info_init); > > + > > +void xdp_rxq_info_reg(struct xdp_rxq_info *xdp_rxq) > > +{ > > + WARN(!xdp_rxq->dev, "Missing net_device from driver"); > > + WARN(xdp_rxq->queue_index == U32_MAX, "Miss queue_index from driver"); > > + WARN(!(xdp_rxq->reg_state == REG_STATE_NEW),"API violation, miss init"); > > + xdp_rxq->reg_state = REG_STATE_REGISTRED; > > +} > > +EXPORT_SYMBOL_GPL(xdp_rxq_info_reg); > > > > Rather than WARN()'s why not make the _reg and _init functions return an > int that indicates an error? For example you don't want to continue if > the dev is expected but missing. Handling return-errors in the drivers complicated the driver code, as it involves unraveling and deallocating other RX-rings etc (that were already allocated) if the reg fails. (Also notice next patch will allow dev == NULL, if right ptype is set). I'm not completely rejecting you idea, as this is a good optimization trick, which is to move validation checks to setup-time, thus allowing less validation checks at runtime. I sort-of actually already did this, as I allow bpf to deref dev without NULL check. I would argue this is good enough, as we will crash in a predictable way, as above WARN will point to which driver violated the API. If people think it is valuable I can change this API to return an err? I guess, it would be more future-proof to do this, as we (Bjørn, Michael, Andy) want to extend this to implement a XDP frame/mem return code-path. And the register call will likely have to allocate some resource that could fail, which need to be handled... If we do this, we might as well (slab) alloc the xdp_rxq_info structure to reduce the bloat in the drivers RX-rings to a single pointer (and a pointer to xdp_rxq_info is what xdp_buff.rxq need). -- Best regards, Jesper Dangaard Brouer MSc.CS, Principal Kernel Engineer at Red Hat LinkedIn: http://www.linkedin.com/in/brouer