Re: [bpf-next V2 PATCH 05/14] xdp/qede: setup xdp_rxq_info and intro xdp_rxq_info_is_reg

[Jesper Dangaard Brouer]

On Wed, 27 Dec 2017 10:37:10 +
"Chopra, Manish"  wrote:

> > -Original Message-
> > From: Jesper Dangaard Brouer [mailto:bro...@redhat.com]
> > Sent: Friday, December 22, 2017 10:42 PM
> > To: Daniel Borkmann ; Alexei Starovoitov
> > 
> > Cc: bjorn.to...@intel.com; netdev@vger.kernel.org; Jesper Dangaard Brouer
> > ; Elior, Ariel ;
> > dsah...@gmail.com; go...@broadcom.com; Dept-Eng Everest Linux L2  > engeverestlinu...@cavium.com>; michael.c...@broadcom.com  
> > Subject: [bpf-next V2 PATCH 05/14] xdp/qede: setup xdp_rxq_info and intro
> > xdp_rxq_info_is_reg
> > 
> > The driver code qede_free_fp_array() depend on kfree() can be called with a
> > NULL pointer. This stems from the qede_alloc_fp_array() function which 
> > either
> > (kz)alloc memory for fp->txq or fp->rxq.
> > This also simplifies error handling code in case of memory allocation 
> > failures,
> > but xdp_rxq_info_unreg need to know the difference.
> > 
> > Introduce xdp_rxq_info_is_reg() to handle if a memory allocation fails and
> > detect this is the failure path by seeing that xdp_rxq_info was not 
> > registred yet,
> > which first happens after successful alloaction in qede_init_fp().
> > 
> > Driver hook points for xdp_rxq_info:
> >  * reg  : qede_init_fp
> >  * unreg: qede_free_fp_array
> > 
> > Tested on actual hardware with samples/bpf program.
> > 
> > V2: Driver have no proper error path for failed XDP RX-queue info reg, as
> > qede_init_fp() is a void function.
> > 
> > Cc: everest-linux...@cavium.com
> > Cc: Ariel Elior 
> > Signed-off-by: Jesper Dangaard Brouer 
> > ---
> >  drivers/net/ethernet/qlogic/qede/qede.h  |2 ++
> >  drivers/net/ethernet/qlogic/qede/qede_fp.c   |1 +
> >  drivers/net/ethernet/qlogic/qede/qede_main.c |   10 ++
> >  include/net/xdp.h|1 +
> >  net/core/xdp.c   |6 ++
> >  5 files changed, 20 insertions(+)
> > 
> > diff --git a/drivers/net/ethernet/qlogic/qede/qede.h
> > b/drivers/net/ethernet/qlogic/qede/qede.h
> > index a3a70ade411f..62f47736511b 100644
> > --- a/drivers/net/ethernet/qlogic/qede/qede.h
> > +++ b/drivers/net/ethernet/qlogic/qede/qede.h
> > @@ -40,6 +40,7 @@
> >  #include 
> >  #include 
> >  #include 
> > +#include 
> >  #include 
> >  #include 
> >  #ifdef CONFIG_RFS_ACCEL
> > @@ -345,6 +346,7 @@ struct qede_rx_queue {
> > u64 xdp_no_pass;
> > 
> > void *handle;
> > +   struct xdp_rxq_info xdp_rxq;
> >  };
> > 
> >  union db_prod {
> > diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> > b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> > index 48ec4c56cddf..dafc079ab6b9 100644
> > --- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> > +++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> > @@ -1006,6 +1006,7 @@ static bool qede_rx_xdp(struct qede_dev *edev,
> > xdp.data = xdp.data_hard_start + *data_offset;
> > xdp_set_data_meta_invalid();
> > xdp.data_end = xdp.data + *len;
> > +   xdp.rxq = >xdp_rxq;
> > 
> > /* Queues always have a full reset currently, so for the time
> >  * being until there's atomic program replace just mark read diff --git
> > a/drivers/net/ethernet/qlogic/qede/qede_main.c
> > b/drivers/net/ethernet/qlogic/qede/qede_main.c
> > index 57332b3e5e64..24160f1fd0e5 100644
> > --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> > +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> > @@ -762,6 +762,12 @@ static void qede_free_fp_array(struct qede_dev *edev)
> > fp = >fp_array[i];
> > 
> > kfree(fp->sb_info);
> > +   /* Handle mem alloc failure case where qede_init_fp
> > +* didn't register xdp_rxq_info yet.
> > +* Implicit only (fp->type & QEDE_FASTPATH_RX)
> > +*/
> > +   if (fp->rxq && xdp_rxq_info_is_reg(>rxq->xdp_rxq))
> > +   xdp_rxq_info_unreg(>rxq->xdp_rxq);  
> 
> Isn't this sufficient just ?
> If (fp->rxq)
>   xdp_rxq_info_unreg(>rxq->xdp_rxq);

No, it is not sufficient.

Your driver have qede_alloc_fp_array() which for_each_queue (kz)alloc
memory.  If one of the memory allocations fail, then memory is
free'ed/"rolled-back" in qede_free_fp_array().  Thus, some of the
fp->rxq pointers will be allocated, but xdp_rxq_info_reg() have not
"registered" it yet.  Thus, this _is_ needed to correctly handle memory
failures.

> xdp_rxq_info_is_reg() API seems unnecessary to me,
> xdp_rxq_info_unreg() seems to be taking care of the case if it's not
> registered then WARN already, I think instead of adding these checks
> in individual drivers, better handle it properly in
> xdp_rxq_info_unreg() ?

The xdp_rxq_info_is_reg() API is needed in your driver, and I also found
a need for this in the Broadcom bnxt_en driver.


> >  

RE: [bpf-next V2 PATCH 05/14] xdp/qede: setup xdp_rxq_info and intro xdp_rxq_info_is_reg

[Chopra, Manish]

> -Original Message-
> From: Jesper Dangaard Brouer [mailto:bro...@redhat.com]
> Sent: Friday, December 22, 2017 10:42 PM
> To: Daniel Borkmann ; Alexei Starovoitov
> 
> Cc: bjorn.to...@intel.com; netdev@vger.kernel.org; Jesper Dangaard Brouer
> ; Elior, Ariel ;
> dsah...@gmail.com; go...@broadcom.com; Dept-Eng Everest Linux L2  engeverestlinu...@cavium.com>; michael.c...@broadcom.com
> Subject: [bpf-next V2 PATCH 05/14] xdp/qede: setup xdp_rxq_info and intro
> xdp_rxq_info_is_reg
> 
> The driver code qede_free_fp_array() depend on kfree() can be called with a
> NULL pointer. This stems from the qede_alloc_fp_array() function which either
> (kz)alloc memory for fp->txq or fp->rxq.
> This also simplifies error handling code in case of memory allocation 
> failures,
> but xdp_rxq_info_unreg need to know the difference.
> 
> Introduce xdp_rxq_info_is_reg() to handle if a memory allocation fails and
> detect this is the failure path by seeing that xdp_rxq_info was not registred 
> yet,
> which first happens after successful alloaction in qede_init_fp().
> 
> Driver hook points for xdp_rxq_info:
>  * reg  : qede_init_fp
>  * unreg: qede_free_fp_array
> 
> Tested on actual hardware with samples/bpf program.
> 
> V2: Driver have no proper error path for failed XDP RX-queue info reg, as
> qede_init_fp() is a void function.
> 
> Cc: everest-linux...@cavium.com
> Cc: Ariel Elior 
> Signed-off-by: Jesper Dangaard Brouer 
> ---
>  drivers/net/ethernet/qlogic/qede/qede.h  |2 ++
>  drivers/net/ethernet/qlogic/qede/qede_fp.c   |1 +
>  drivers/net/ethernet/qlogic/qede/qede_main.c |   10 ++
>  include/net/xdp.h|1 +
>  net/core/xdp.c   |6 ++
>  5 files changed, 20 insertions(+)
> 
> diff --git a/drivers/net/ethernet/qlogic/qede/qede.h
> b/drivers/net/ethernet/qlogic/qede/qede.h
> index a3a70ade411f..62f47736511b 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede.h
> +++ b/drivers/net/ethernet/qlogic/qede/qede.h
> @@ -40,6 +40,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  #ifdef CONFIG_RFS_ACCEL
> @@ -345,6 +346,7 @@ struct qede_rx_queue {
>   u64 xdp_no_pass;
> 
>   void *handle;
> + struct xdp_rxq_info xdp_rxq;
>  };
> 
>  union db_prod {
> diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> index 48ec4c56cddf..dafc079ab6b9 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
> @@ -1006,6 +1006,7 @@ static bool qede_rx_xdp(struct qede_dev *edev,
>   xdp.data = xdp.data_hard_start + *data_offset;
>   xdp_set_data_meta_invalid();
>   xdp.data_end = xdp.data + *len;
> + xdp.rxq = >xdp_rxq;
> 
>   /* Queues always have a full reset currently, so for the time
>* being until there's atomic program replace just mark read diff --git
> a/drivers/net/ethernet/qlogic/qede/qede_main.c
> b/drivers/net/ethernet/qlogic/qede/qede_main.c
> index 57332b3e5e64..24160f1fd0e5 100644
> --- a/drivers/net/ethernet/qlogic/qede/qede_main.c
> +++ b/drivers/net/ethernet/qlogic/qede/qede_main.c
> @@ -762,6 +762,12 @@ static void qede_free_fp_array(struct qede_dev *edev)
>   fp = >fp_array[i];
> 
>   kfree(fp->sb_info);
> + /* Handle mem alloc failure case where qede_init_fp
> +  * didn't register xdp_rxq_info yet.
> +  * Implicit only (fp->type & QEDE_FASTPATH_RX)
> +  */
> + if (fp->rxq && xdp_rxq_info_is_reg(>rxq->xdp_rxq))
> + xdp_rxq_info_unreg(>rxq->xdp_rxq);

Isn't this sufficient just ?
If (fp->rxq)
xdp_rxq_info_unreg(>rxq->xdp_rxq);

xdp_rxq_info_is_reg() API seems unnecessary to me, xdp_rxq_info_unreg() seems 
to be taking care of the case
if it's not registered then WARN already, I think instead of adding these 
checks in individual drivers, better handle it properly in xdp_rxq_info_unreg() 
?

>   kfree(fp->rxq);
>   kfree(fp->xdp_tx);
>   kfree(fp->txq);
> @@ -1498,6 +1504,10 @@ static void qede_init_fp(struct qede_dev *edev)
>   else
>   fp->rxq->data_direction =
> DMA_FROM_DEVICE;
>   fp->rxq->dev = >pdev->dev;
> +
> + /* Driver have no error path from here */
> + WARN_ON(xdp_rxq_info_reg(>rxq->xdp_rxq, edev-
> >ndev,
> +  fp->rxq->rxq_id) < 0);
>   }

We will reach to this point only when fp->rxq and edev are valid
I don't think that xdp_rxq_info_reg() will fail ever in that case.