Re: [bpf-next V1-RFC PATCH 01/14] xdp: base API for new XDP rx-queue info concept

[Jesper Dangaard Brouer <brouer@redhat.com>]

On Mon, 18 Dec 2017 11:55:01 +0100
Jesper Dangaard Brouer <brouer@redhat.com> wrote:

> On Wed, 13 Dec 2017 19:34:40 -0700
> David Ahern <dsahern@gmail.com> 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 will take Ahern's suggestion of returning an err-code, but only from
xdp_rxq_info_reg().  And I'm going to move xdp_rxq_info_init to be an
internal function (which Saeed also implicitly suggested).
I'm working through the drivers now, and only two drivers don't have a
proper error-return for handling xdp_rxq_info_reg() could fail.

I've also extended xdp_rxq_info_reg() to take args dev + idx, to reduce
the code-lines (given we now also have to check return code, this got
too big).  Thus, reg is a single call with if-return-check.


> 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...

I'm mostly doing it for above reason, as I'm hoping to avoid touching
every XDP driver once again.  It is a real pain.

> 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).

I've dropped my idea of (slab) allocating the xdp_rxq_info structure.
I started coding this up, but realized the number of lines added per
driver got too excessive for no apparent gain. (e.g. I also needed to
take the numa-node into account in some drivers).

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Principal Kernel Engineer at Red Hat
  LinkedIn: http://www.linkedin.com/in/brouer