On Mon, 18 Dec 2017 11:55:01 +0100
Jesper Dangaard Brouer <bro...@redhat.com> wrote:
> On Wed, 13 Dec 2017 19:34:40 -0700
> David Ahern <dsah...@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
