[PATCH net 0/4] Add missing xdp_do_flush() invocations.

[PATCH net 0/4] Add missing xdp_do_flush() invocations.

[Sebastian Andrzej Siewior]

Hi,

I've been looking at the drivers/ XDP users and noticed that some
XDP_REDIRECT user don't invoke xdp_do_flush() at the end.

Sebastian

[PATCH net 1/4] net: ena: Flush XDP packets on error.

[Sebastian Andrzej Siewior]

xdp_do_flush() should be invoked before leaving the NAPI poll function
after a XDP-redirect. This is not the case if the driver leaves via
the error path (after having a redirect in one of its previous
iterations).

Invoke xdp_do_flush() also in the error path.

Cc: Arthur Kiyanovski <akiyano@amazon.com>
Cc: David Arinzon <darinzon@amazon.com>
Cc: Noam Dagan <ndagan@amazon.com>
Cc: Saeed Bishara <saeedb@amazon.com>
Cc: Shay Agroskin <shayagr@amazon.com>
Fixes: a318c70ad152b ("net: ena: introduce XDP redirect implementation")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/amazon/ena/ena_netdev.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c b/drivers/net/ethernet/amazon/ena/ena_netdev.c
index ad32ca81f7ef4..f955bde10cf90 100644
--- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
+++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
@@ -1833,6 +1833,9 @@ static int ena_clean_rx_irq(struct ena_ring *rx_ring, struct napi_struct *napi,
 	return work_done;
 
 error:
+	if (xdp_flags & ENA_XDP_REDIRECT)
+		xdp_do_flush();
+
 	adapter = netdev_priv(rx_ring->netdev);
 
 	if (rc == -ENOSPC) {
-- 
2.40.1

RE: [PATCH net 1/4] net: ena: Flush XDP packets on error.

[Kiyanovski, Arthur]

> -----Original Message-----
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Sent: Friday, September 8, 2023 4:58 PM
> To: netdev@vger.kernel.org; bpf@vger.kernel.org
> Cc: David S. Miller <davem@davemloft.net>; Alexei Starovoitov
> <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Jesper
> Dangaard Brouer <hawk@kernel.org>; John Fastabend
> <john.fastabend@gmail.com>; Paolo Abeni <pabeni@redhat.com>; Thomas
> Gleixner <tglx@linutronix.de>; Sebastian Andrzej Siewior
> <bigeasy@linutronix.de>; Kiyanovski, Arthur <akiyano@amazon.com>;
> Arinzon, David <darinzon@amazon.com>; Dagan, Noam
> <ndagan@amazon.com>; Bshara, Saeed <saeedb@amazon.com>; Agroskin,
> Shay <shayagr@amazon.com>
> Subject: [EXTERNAL] [PATCH net 1/4] net: ena: Flush XDP packets on error.
> 
> CAUTION: This email originated from outside of the organization. Do not click
> links or open attachments unless you can confirm the sender and know the
> content is safe.
> 
> 
> 
> xdp_do_flush() should be invoked before leaving the NAPI poll function after
> a XDP-redirect. This is not the case if the driver leaves via the error path
> (after having a redirect in one of its previous iterations).
> 
> Invoke xdp_do_flush() also in the error path.
> 
> Cc: Arthur Kiyanovski <akiyano@amazon.com>
> Cc: David Arinzon <darinzon@amazon.com>
> Cc: Noam Dagan <ndagan@amazon.com>
> Cc: Saeed Bishara <saeedb@amazon.com>
> Cc: Shay Agroskin <shayagr@amazon.com>
> Fixes: a318c70ad152b ("net: ena: introduce XDP redirect implementation")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/net/ethernet/amazon/ena/ena_netdev.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> index ad32ca81f7ef4..f955bde10cf90 100644
> --- a/drivers/net/ethernet/amazon/ena/ena_netdev.c
> +++ b/drivers/net/ethernet/amazon/ena/ena_netdev.c
> @@ -1833,6 +1833,9 @@ static int ena_clean_rx_irq(struct ena_ring
> *rx_ring, struct napi_struct *napi,
>         return work_done;
> 
>  error:
> +       if (xdp_flags & ENA_XDP_REDIRECT)
> +               xdp_do_flush();
> +
>         adapter = netdev_priv(rx_ring->netdev);
> 
>         if (rc == -ENOSPC) {
> --
> 2.40.1
> 

Thanks for submitting this change.

Acked-by: Arthur Kiyanovski <akiyano@amazon.com>

[PATCH net 2/4] bnxt_en: Flush XDP for bnxt_poll_nitroa0()'s NAPI

[Sebastian Andrzej Siewior]

bnxt_poll_nitroa0() invokes bnxt_rx_pkt() which can run a XDP program
which in turn can return XDP_REDIRECT. bnxt_rx_pkt() is also used by
__bnxt_poll_work() which flushes (xdp_do_flush()) the packets after each
round. bnxt_poll_nitroa0() lacks this feature.
xdp_do_flush() should be invoked before leaving the NAPI callback.

Invoke xdp_do_flush() after a redirect in bnxt_poll_nitroa0() NAPI.

Cc: Michael Chan <michael.chan@broadcom.com>
Fixes: f18c2b77b2e4e ("bnxt_en: optimized XDP_REDIRECT support")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
index 5cc0dbe121327..7551aa8068f8f 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
@@ -2614,6 +2614,7 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
 	struct rx_cmp_ext *rxcmp1;
 	u32 cp_cons, tmp_raw_cons;
 	u32 raw_cons = cpr->cp_raw_cons;
+	bool flush_xdp = false;
 	u32 rx_pkts = 0;
 	u8 event = 0;
 
@@ -2648,6 +2649,8 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
 				rx_pkts++;
 			else if (rc == -EBUSY)	/* partial completion */
 				break;
+			if (event & BNXT_REDIRECT_EVENT)
+				flush_xdp = true;
 		} else if (unlikely(TX_CMP_TYPE(txcmp) ==
 				    CMPL_BASE_TYPE_HWRM_DONE)) {
 			bnxt_hwrm_handler(bp, txcmp);
@@ -2667,6 +2670,8 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
 
 	if (event & BNXT_AGG_EVENT)
 		bnxt_db_write(bp, &rxr->rx_agg_db, rxr->rx_agg_prod);
+	if (flush_xdp)
+		xdp_do_flush();
 
 	if (!bnxt_has_work(bp, cpr) && rx_pkts < budget) {
 		napi_complete_done(napi, rx_pkts);
-- 
2.40.1

Re: [PATCH net 2/4] bnxt_en: Flush XDP for bnxt_poll_nitroa0()'s NAPI

[Pavan Chebbi]

[-- Attachment #1: Type: text/plain, Size: 2524 bytes --]

On Fri, Sep 8, 2023 at 7:29 PM Sebastian Andrzej Siewior
<bigeasy@linutronix.de> wrote:
>
> bnxt_poll_nitroa0() invokes bnxt_rx_pkt() which can run a XDP program
> which in turn can return XDP_REDIRECT. bnxt_rx_pkt() is also used by
> __bnxt_poll_work() which flushes (xdp_do_flush()) the packets after each
> round. bnxt_poll_nitroa0() lacks this feature.
> xdp_do_flush() should be invoked before leaving the NAPI callback.
>
> Invoke xdp_do_flush() after a redirect in bnxt_poll_nitroa0() NAPI.
>
> Cc: Michael Chan <michael.chan@broadcom.com>
> Fixes: f18c2b77b2e4e ("bnxt_en: optimized XDP_REDIRECT support")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 +++++
>  1 file changed, 5 insertions(+)
>
> diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> index 5cc0dbe121327..7551aa8068f8f 100644
> --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> @@ -2614,6 +2614,7 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
>         struct rx_cmp_ext *rxcmp1;
>         u32 cp_cons, tmp_raw_cons;
>         u32 raw_cons = cpr->cp_raw_cons;
> +       bool flush_xdp = false;

Michael can confirm but I don't think we need this additional variable.
Since the event is always ORed, we could directly check if (event &
BNXT_REDIRECT_EVENT) just like is done in __bnxt_poll_work().

>         u32 rx_pkts = 0;
>         u8 event = 0;
>
> @@ -2648,6 +2649,8 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
>                                 rx_pkts++;
>                         else if (rc == -EBUSY)  /* partial completion */
>                                 break;
> +                       if (event & BNXT_REDIRECT_EVENT)
> +                               flush_xdp = true;
>                 } else if (unlikely(TX_CMP_TYPE(txcmp) ==
>                                     CMPL_BASE_TYPE_HWRM_DONE)) {
>                         bnxt_hwrm_handler(bp, txcmp);
> @@ -2667,6 +2670,8 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
>
>         if (event & BNXT_AGG_EVENT)
>                 bnxt_db_write(bp, &rxr->rx_agg_db, rxr->rx_agg_prod);
> +       if (flush_xdp)
> +               xdp_do_flush();
>
>         if (!bnxt_has_work(bp, cpr) && rx_pkts < budget) {
>                 napi_complete_done(napi, rx_pkts);
> --
> 2.40.1
>
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net 2/4] bnxt_en: Flush XDP for bnxt_poll_nitroa0()'s NAPI

[Michael Chan]

[-- Attachment #1: Type: text/plain, Size: 3195 bytes --]

On Fri, Sep 8, 2023 at 9:30 AM Pavan Chebbi <pavan.chebbi@broadcom.com> wrote:
>
> On Fri, Sep 8, 2023 at 7:29 PM Sebastian Andrzej Siewior
> <bigeasy@linutronix.de> wrote:
> >
> > bnxt_poll_nitroa0() invokes bnxt_rx_pkt() which can run a XDP program
> > which in turn can return XDP_REDIRECT. bnxt_rx_pkt() is also used by
> > __bnxt_poll_work() which flushes (xdp_do_flush()) the packets after each
> > round. bnxt_poll_nitroa0() lacks this feature.
> > xdp_do_flush() should be invoked before leaving the NAPI callback.
> >
> > Invoke xdp_do_flush() after a redirect in bnxt_poll_nitroa0() NAPI.
> >
> > Cc: Michael Chan <michael.chan@broadcom.com>
> > Fixes: f18c2b77b2e4e ("bnxt_en: optimized XDP_REDIRECT support")
> > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > ---
> >  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 +++++
> >  1 file changed, 5 insertions(+)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > index 5cc0dbe121327..7551aa8068f8f 100644
> > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > @@ -2614,6 +2614,7 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
> >         struct rx_cmp_ext *rxcmp1;
> >         u32 cp_cons, tmp_raw_cons;
> >         u32 raw_cons = cpr->cp_raw_cons;
> > +       bool flush_xdp = false;
>
> Michael can confirm but I don't think we need this additional variable.
> Since the event is always ORed, we could directly check if (event &
> BNXT_REDIRECT_EVENT) just like is done in __bnxt_poll_work().

If we have a mix of XDP_TX and XDP_REDIRECT during NAPI, event can be
cleared by XDP_TX.  So this patch looks correct to me because of that.
Or we can make it consistent with __bnxt_poll_work() and assume that
XDP_TX won't mix with XDP_REDIRECT.

Handling a mix of XDP actions needs to be looked at separately.  The
driver currently won't work well when that happens.  I am working on
an internal patch to address that and will post it when it's ready.
Thanks.

>
> >         u32 rx_pkts = 0;
> >         u8 event = 0;
> >
> > @@ -2648,6 +2649,8 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
> >                                 rx_pkts++;
> >                         else if (rc == -EBUSY)  /* partial completion */
> >                                 break;
> > +                       if (event & BNXT_REDIRECT_EVENT)
> > +                               flush_xdp = true;
> >                 } else if (unlikely(TX_CMP_TYPE(txcmp) ==
> >                                     CMPL_BASE_TYPE_HWRM_DONE)) {
> >                         bnxt_hwrm_handler(bp, txcmp);
> > @@ -2667,6 +2670,8 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
> >
> >         if (event & BNXT_AGG_EVENT)
> >                 bnxt_db_write(bp, &rxr->rx_agg_db, rxr->rx_agg_prod);
> > +       if (flush_xdp)
> > +               xdp_do_flush();
> >
> >         if (!bnxt_has_work(bp, cpr) && rx_pkts < budget) {
> >                 napi_complete_done(napi, rx_pkts);
> > --
> > 2.40.1
> >
> >

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4209 bytes --]

Re: [PATCH net 2/4] bnxt_en: Flush XDP for bnxt_poll_nitroa0()'s NAPI

[Andy Gospodarek]

On Fri, Sep 08, 2023 at 10:57:13AM -0700, Michael Chan wrote:
> On Fri, Sep 8, 2023 at 9:30 AM Pavan Chebbi <pavan.chebbi@broadcom.com> wrote:
> >
> > On Fri, Sep 8, 2023 at 7:29 PM Sebastian Andrzej Siewior
> > <bigeasy@linutronix.de> wrote:
> > >
> > > bnxt_poll_nitroa0() invokes bnxt_rx_pkt() which can run a XDP program
> > > which in turn can return XDP_REDIRECT. bnxt_rx_pkt() is also used by
> > > __bnxt_poll_work() which flushes (xdp_do_flush()) the packets after each
> > > round. bnxt_poll_nitroa0() lacks this feature.
> > > xdp_do_flush() should be invoked before leaving the NAPI callback.
> > >
> > > Invoke xdp_do_flush() after a redirect in bnxt_poll_nitroa0() NAPI.
> > >
> > > Cc: Michael Chan <michael.chan@broadcom.com>
> > > Fixes: f18c2b77b2e4e ("bnxt_en: optimized XDP_REDIRECT support")
> > > Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> > > ---
> > >  drivers/net/ethernet/broadcom/bnxt/bnxt.c | 5 +++++
> > >  1 file changed, 5 insertions(+)
> > >
> > > diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt.c b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > index 5cc0dbe121327..7551aa8068f8f 100644
> > > --- a/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > +++ b/drivers/net/ethernet/broadcom/bnxt/bnxt.c
> > > @@ -2614,6 +2614,7 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
> > >         struct rx_cmp_ext *rxcmp1;
> > >         u32 cp_cons, tmp_raw_cons;
> > >         u32 raw_cons = cpr->cp_raw_cons;
> > > +       bool flush_xdp = false;
> >
> > Michael can confirm but I don't think we need this additional variable.
> > Since the event is always ORed, we could directly check if (event &
> > BNXT_REDIRECT_EVENT) just like is done in __bnxt_poll_work().
> 
> If we have a mix of XDP_TX and XDP_REDIRECT during NAPI, event can be
> cleared by XDP_TX.  So this patch looks correct to me because of that.

Agreed

> Or we can make it consistent with __bnxt_poll_work() and assume that
> XDP_TX won't mix with XDP_REDIRECT.

Unfortunately we probably cannot guarantee that or maybe more to point
we do not want to guarantee that.

Thanks for this patch.

Reviewed-by: Andy Gospodarek <gospo@broadcom.com>


> Handling a mix of XDP actions needs to be looked at separately.  The
> driver currently won't work well when that happens.  I am working on
> an internal patch to address that and will post it when it's ready.
> Thanks.
> 
> >
> > >         u32 rx_pkts = 0;
> > >         u8 event = 0;
> > >
> > > @@ -2648,6 +2649,8 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
> > >                                 rx_pkts++;
> > >                         else if (rc == -EBUSY)  /* partial completion */
> > >                                 break;
> > > +                       if (event & BNXT_REDIRECT_EVENT)
> > > +                               flush_xdp = true;
> > >                 } else if (unlikely(TX_CMP_TYPE(txcmp) ==
> > >                                     CMPL_BASE_TYPE_HWRM_DONE)) {
> > >                         bnxt_hwrm_handler(bp, txcmp);
> > > @@ -2667,6 +2670,8 @@ static int bnxt_poll_nitroa0(struct napi_struct *napi, int budget)
> > >
> > >         if (event & BNXT_AGG_EVENT)
> > >                 bnxt_db_write(bp, &rxr->rx_agg_db, rxr->rx_agg_prod);
> > > +       if (flush_xdp)
> > > +               xdp_do_flush();
> > >
> > >         if (!bnxt_has_work(bp, cpr) && rx_pkts < budget) {
> > >                 napi_complete_done(napi, rx_pkts);
> > > --
> > > 2.40.1
> > >
> > >

[PATCH net 3/4] octeontx2-pf: Do xdp_do_flush() after redirects.

[Sebastian Andrzej Siewior]

xdp_do_flush() should be invoked before leaving the NAPI poll function
if XDP-redirect has been performed.

Invoke xdp_do_flush() before leaving NAPI.

Cc: Geetha sowjanya <gakula@marvell.com>
Cc: Subbaraya Sundeep <sbhatta@marvell.com>
Cc: Sunil Goutham <sgoutham@marvell.com>
Cc: hariprasad <hkelam@marvell.com>
Fixes: 06059a1a9a4a5 ("octeontx2-pf: Add XDP support to netdev PF")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 .../marvell/octeontx2/nic/otx2_txrx.c         | 19 +++++++++++++------
 1 file changed, 13 insertions(+), 6 deletions(-)

diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
index e369baf115301..6c02eaa460277 100644
--- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
+++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
@@ -29,7 +29,8 @@
 static bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
 				     struct bpf_prog *prog,
 				     struct nix_cqe_rx_s *cqe,
-				     struct otx2_cq_queue *cq);
+				     struct otx2_cq_queue *cq,
+				     bool *need_xdp_flush);
 
 static int otx2_nix_cq_op_status(struct otx2_nic *pfvf,
 				 struct otx2_cq_queue *cq)
@@ -337,7 +338,7 @@ static bool otx2_check_rcv_errors(struct otx2_nic *pfvf,
 static void otx2_rcv_pkt_handler(struct otx2_nic *pfvf,
 				 struct napi_struct *napi,
 				 struct otx2_cq_queue *cq,
-				 struct nix_cqe_rx_s *cqe)
+				 struct nix_cqe_rx_s *cqe, bool *need_xdp_flush)
 {
 	struct nix_rx_parse_s *parse = &cqe->parse;
 	struct nix_rx_sg_s *sg = &cqe->sg;
@@ -353,7 +354,7 @@ static void otx2_rcv_pkt_handler(struct otx2_nic *pfvf,
 	}
 
 	if (pfvf->xdp_prog)
-		if (otx2_xdp_rcv_pkt_handler(pfvf, pfvf->xdp_prog, cqe, cq))
+		if (otx2_xdp_rcv_pkt_handler(pfvf, pfvf->xdp_prog, cqe, cq, need_xdp_flush))
 			return;
 
 	skb = napi_get_frags(napi);
@@ -388,6 +389,7 @@ static int otx2_rx_napi_handler(struct otx2_nic *pfvf,
 				struct napi_struct *napi,
 				struct otx2_cq_queue *cq, int budget)
 {
+	bool need_xdp_flush = false;
 	struct nix_cqe_rx_s *cqe;
 	int processed_cqe = 0;
 
@@ -409,13 +411,15 @@ static int otx2_rx_napi_handler(struct otx2_nic *pfvf,
 		cq->cq_head++;
 		cq->cq_head &= (cq->cqe_cnt - 1);
 
-		otx2_rcv_pkt_handler(pfvf, napi, cq, cqe);
+		otx2_rcv_pkt_handler(pfvf, napi, cq, cqe, &need_xdp_flush);
 
 		cqe->hdr.cqe_type = NIX_XQE_TYPE_INVALID;
 		cqe->sg.seg_addr = 0x00;
 		processed_cqe++;
 		cq->pend_cqe--;
 	}
+	if (need_xdp_flush)
+		xdp_do_flush();
 
 	/* Free CQEs to HW */
 	otx2_write64(pfvf, NIX_LF_CQ_OP_DOOR,
@@ -1334,7 +1338,8 @@ bool otx2_xdp_sq_append_pkt(struct otx2_nic *pfvf, u64 iova, int len, u16 qidx)
 static bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
 				     struct bpf_prog *prog,
 				     struct nix_cqe_rx_s *cqe,
-				     struct otx2_cq_queue *cq)
+				     struct otx2_cq_queue *cq,
+				     bool *need_xdp_flush)
 {
 	unsigned char *hard_start, *data;
 	int qidx = cq->cq_idx;
@@ -1371,8 +1376,10 @@ static bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
 
 		otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
 				    DMA_FROM_DEVICE);
-		if (!err)
+		if (!err) {
+			*need_xdp_flush = true;
 			return true;
+		}
 		put_page(page);
 		break;
 	default:
-- 
2.40.1

RE: [EXT] [PATCH net 3/4] octeontx2-pf: Do xdp_do_flush() after redirects.

[Geethasowjanya Akula]

Thanks for the patch.

ACK. 

> -----Original Message-----
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Sent: Friday, September 8, 2023 7:28 PM
> To: netdev@vger.kernel.org; bpf@vger.kernel.org
> Cc: David S. Miller <davem@davemloft.net>; Alexei Starovoitov
> <ast@kernel.org>; Daniel Borkmann <daniel@iogearbox.net>; Eric Dumazet
> <edumazet@google.com>; Jakub Kicinski <kuba@kernel.org>; Jesper
> Dangaard Brouer <hawk@kernel.org>; John Fastabend
> <john.fastabend@gmail.com>; Paolo Abeni <pabeni@redhat.com>; Thomas
> Gleixner <tglx@linutronix.de>; Sebastian Andrzej Siewior
> <bigeasy@linutronix.de>; Geethasowjanya Akula <gakula@marvell.com>;
> Subbaraya Sundeep Bhatta <sbhatta@marvell.com>; Sunil Kovvuri Goutham
> <sgoutham@marvell.com>; Hariprasad Kelam <hkelam@marvell.com>
> Subject: [EXT] [PATCH net 3/4] octeontx2-pf: Do xdp_do_flush() after
> redirects.
> 
> External Email
> 
> ----------------------------------------------------------------------
> xdp_do_flush() should be invoked before leaving the NAPI poll function if
> XDP-redirect has been performed.
> 
> Invoke xdp_do_flush() before leaving NAPI.
> 
> Cc: Geetha sowjanya <gakula@marvell.com>
> Cc: Subbaraya Sundeep <sbhatta@marvell.com>
> Cc: Sunil Goutham <sgoutham@marvell.com>
> Cc: hariprasad <hkelam@marvell.com>
> Fixes: 06059a1a9a4a5 ("octeontx2-pf: Add XDP support to netdev PF")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  .../marvell/octeontx2/nic/otx2_txrx.c         | 19 +++++++++++++------
>  1 file changed, 13 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> index e369baf115301..6c02eaa460277 100644
> --- a/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> +++ b/drivers/net/ethernet/marvell/octeontx2/nic/otx2_txrx.c
> @@ -29,7 +29,8 @@
>  static bool otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
>  				     struct bpf_prog *prog,
>  				     struct nix_cqe_rx_s *cqe,
> -				     struct otx2_cq_queue *cq);
> +				     struct otx2_cq_queue *cq,
> +				     bool *need_xdp_flush);
> 
>  static int otx2_nix_cq_op_status(struct otx2_nic *pfvf,
>  				 struct otx2_cq_queue *cq)
> @@ -337,7 +338,7 @@ static bool otx2_check_rcv_errors(struct otx2_nic
> *pfvf,  static void otx2_rcv_pkt_handler(struct otx2_nic *pfvf,
>  				 struct napi_struct *napi,
>  				 struct otx2_cq_queue *cq,
> -				 struct nix_cqe_rx_s *cqe)
> +				 struct nix_cqe_rx_s *cqe, bool
> *need_xdp_flush)
>  {
>  	struct nix_rx_parse_s *parse = &cqe->parse;
>  	struct nix_rx_sg_s *sg = &cqe->sg;
> @@ -353,7 +354,7 @@ static void otx2_rcv_pkt_handler(struct otx2_nic
> *pfvf,
>  	}
> 
>  	if (pfvf->xdp_prog)
> -		if (otx2_xdp_rcv_pkt_handler(pfvf, pfvf->xdp_prog, cqe, cq))
> +		if (otx2_xdp_rcv_pkt_handler(pfvf, pfvf->xdp_prog, cqe, cq,
> +need_xdp_flush))
>  			return;
> 
>  	skb = napi_get_frags(napi);
> @@ -388,6 +389,7 @@ static int otx2_rx_napi_handler(struct otx2_nic *pfvf,
>  				struct napi_struct *napi,
>  				struct otx2_cq_queue *cq, int budget)  {
> +	bool need_xdp_flush = false;
>  	struct nix_cqe_rx_s *cqe;
>  	int processed_cqe = 0;
> 
> @@ -409,13 +411,15 @@ static int otx2_rx_napi_handler(struct otx2_nic
> *pfvf,
>  		cq->cq_head++;
>  		cq->cq_head &= (cq->cqe_cnt - 1);
> 
> -		otx2_rcv_pkt_handler(pfvf, napi, cq, cqe);
> +		otx2_rcv_pkt_handler(pfvf, napi, cq, cqe, &need_xdp_flush);
> 
>  		cqe->hdr.cqe_type = NIX_XQE_TYPE_INVALID;
>  		cqe->sg.seg_addr = 0x00;
>  		processed_cqe++;
>  		cq->pend_cqe--;
>  	}
> +	if (need_xdp_flush)
> +		xdp_do_flush();
> 
>  	/* Free CQEs to HW */
>  	otx2_write64(pfvf, NIX_LF_CQ_OP_DOOR,
> @@ -1334,7 +1338,8 @@ bool otx2_xdp_sq_append_pkt(struct otx2_nic
> *pfvf, u64 iova, int len, u16 qidx)  static bool
> otx2_xdp_rcv_pkt_handler(struct otx2_nic *pfvf,
>  				     struct bpf_prog *prog,
>  				     struct nix_cqe_rx_s *cqe,
> -				     struct otx2_cq_queue *cq)
> +				     struct otx2_cq_queue *cq,
> +				     bool *need_xdp_flush)
>  {
>  	unsigned char *hard_start, *data;
>  	int qidx = cq->cq_idx;
> @@ -1371,8 +1376,10 @@ static bool otx2_xdp_rcv_pkt_handler(struct
> otx2_nic *pfvf,
> 
>  		otx2_dma_unmap_page(pfvf, iova, pfvf->rbsize,
>  				    DMA_FROM_DEVICE);
> -		if (!err)
> +		if (!err) {
> +			*need_xdp_flush = true;
>  			return true;
> +		}
>  		put_page(page);
>  		break;
>  	default:
> --
> 2.40.1

[PATCH net 4/4] bpf, cpumap: Flush xdp after cpu_map_bpf_prog_run_skb().

[Sebastian Andrzej Siewior]

xdp_do_flush() should be invoked before leaving the NAPI poll function
if XDP-redirect has been performed.

There are two possible XDP invocations in cpu_map_bpf_prog_run():
- cpu_map_bpf_prog_run_xdp()
- cpu_map_bpf_prog_run_skb()

Both functions update stats->redirect if a redirect is performed but
xdp_do_flush() is only invoked after the former.

Invoke xdp_do_flush() after both functions run and a redirect was
performed.

Cc: Andrii Nakryiko <andrii@kernel.org>
Cc: Hao Luo <haoluo@google.com>
Cc: Jiri Olsa <jolsa@kernel.org>
Cc: KP Singh <kpsingh@kernel.org>
Cc: Martin KaFai Lau <martin.lau@linux.dev>
Cc: Song Liu <song@kernel.org>
Cc: Stanislav Fomichev <sdf@google.com>
Cc: Yonghong Song <yonghong.song@linux.dev>
Fixes: 11941f8a85362 ("bpf: cpumap: Implement generic cpumap")
Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
---
 kernel/bpf/cpumap.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
index e42a1bdb7f536..f3ba11b4a732b 100644
--- a/kernel/bpf/cpumap.c
+++ b/kernel/bpf/cpumap.c
@@ -248,12 +248,12 @@ static int cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames,
 
 	nframes = cpu_map_bpf_prog_run_xdp(rcpu, frames, xdp_n, stats);
 
-	if (stats->redirect)
-		xdp_do_flush();
-
 	if (unlikely(!list_empty(list)))
 		cpu_map_bpf_prog_run_skb(rcpu, list, stats);
 
+	if (stats->redirect)
+		xdp_do_flush();
+
 	rcu_read_unlock_bh(); /* resched point, may call do_softirq() */
 
 	return nframes;
-- 
2.40.1

Re: [PATCH net 4/4] bpf, cpumap: Flush xdp after cpu_map_bpf_prog_run_skb().

[Hou Tao]

Hi,

On 9/8/2023 9:57 PM, Sebastian Andrzej Siewior wrote:
> xdp_do_flush() should be invoked before leaving the NAPI poll function
> if XDP-redirect has been performed.
>
> There are two possible XDP invocations in cpu_map_bpf_prog_run():
> - cpu_map_bpf_prog_run_xdp()
> - cpu_map_bpf_prog_run_skb()
>
> Both functions update stats->redirect if a redirect is performed but
> xdp_do_flush() is only invoked after the former.
>
> Invoke xdp_do_flush() after both functions run and a redirect was
> performed.
>
> Cc: Andrii Nakryiko <andrii@kernel.org>
> Cc: Hao Luo <haoluo@google.com>
> Cc: Jiri Olsa <jolsa@kernel.org>
> Cc: KP Singh <kpsingh@kernel.org>
> Cc: Martin KaFai Lau <martin.lau@linux.dev>
> Cc: Song Liu <song@kernel.org>
> Cc: Stanislav Fomichev <sdf@google.com>
> Cc: Yonghong Song <yonghong.song@linux.dev>
> Fixes: 11941f8a85362 ("bpf: cpumap: Implement generic cpumap")
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>  kernel/bpf/cpumap.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/bpf/cpumap.c b/kernel/bpf/cpumap.c
> index e42a1bdb7f536..f3ba11b4a732b 100644
> --- a/kernel/bpf/cpumap.c
> +++ b/kernel/bpf/cpumap.c
> @@ -248,12 +248,12 @@ static int cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames,
>  
>  	nframes = cpu_map_bpf_prog_run_xdp(rcpu, frames, xdp_n, stats);
>  
> -	if (stats->redirect)
> -		xdp_do_flush();
> -
>  	if (unlikely(!list_empty(list)))
>  		cpu_map_bpf_prog_run_skb(rcpu, list, stats);
>  
> +	if (stats->redirect)
> +		xdp_do_flush();
> +

The purpose of xdp_do_flush() is to flush xdp frames stashed in per-cpu
cpu_map_flush list into xdp_bulk_queue. But for redirected skbs, these
skbs will be directly added into xdp_bulk_queue() in
cpu_map_generic_redirect(), so I think xdp_do_flush() is not needed for
redirected skbs.
>  	rcu_read_unlock_bh(); /* resched point, may call do_softirq() */
>  
>  	return nframes;

Re: [PATCH net 4/4] bpf, cpumap: Flush xdp after cpu_map_bpf_prog_run_skb().

[Sebastian Andrzej Siewior]

On 2023-09-09 10:49:09 [+0800], Hou Tao wrote:
> Hi,
Hi,

> > --- a/kernel/bpf/cpumap.c
> > +++ b/kernel/bpf/cpumap.c
> > @@ -248,12 +248,12 @@ static int cpu_map_bpf_prog_run(struct bpf_cpu_map_entry *rcpu, void **frames,
> >  
> >  	nframes = cpu_map_bpf_prog_run_xdp(rcpu, frames, xdp_n, stats);
> >  
> > -	if (stats->redirect)
> > -		xdp_do_flush();
> > -
> >  	if (unlikely(!list_empty(list)))
> >  		cpu_map_bpf_prog_run_skb(rcpu, list, stats);
> >  
> > +	if (stats->redirect)
> > +		xdp_do_flush();
> > +
> 
> The purpose of xdp_do_flush() is to flush xdp frames stashed in per-cpu
> cpu_map_flush list into xdp_bulk_queue. But for redirected skbs, these
> skbs will be directly added into xdp_bulk_queue() in
> cpu_map_generic_redirect(), so I think xdp_do_flush() is not needed for
> redirected skbs.

Now that I checked the down streams of cpu_map_bpf_prog_run_skb() it
does not queue anything to the per-CPU lists like I assumed.  You are
right, it is no needed.

Sorry for the confusion.

> >  	rcu_read_unlock_bh(); /* resched point, may call do_softirq() */
> >  
> >  	return nframes;
> 

Sebastian