Question/Bug about AF_XDP idx_cq from xsk_ring_cons__peek?

Question/Bug about AF_XDP idx_cq from xsk_ring_cons__peek?

[William Tu]

Hi,

In my implementation[1], I'm doing s.t like
    tx_done = xsk_ring_cons__peek(&xsk->umem->cq, BATCH_SIZE, &idx_cq);
    if (tx_done > 0) {
        xsk_ring_cons__release(&xsk->umem->cq, tx_done);
        xsk->outstanding_tx -= tx_done;
    }

I expect if tx_done returns 32, then after calling xsk_ring_cons__release,
the next time I call xsk_ring_cons__peek, I should get idx_cq + 32.
However, sometimes I see the same value of idx_cq is returned as previous, even
when tx_done > 0. Is this the expected behavior?

I experiment with xdpsock_user.c with the patch below and run
~/bpf-next/samples/bpf# ./xdpsock -l -N -z -i eth3
using bpf-next commit 5e2ac390fbd08b2a462db66cef2663e4db0d5191

--- a/samples/bpf/xdpsock_user.c
+++ b/samples/bpf/xdpsock_user.c
@@ -444,6 +443,8 @@ static void kick_tx(struct xsk_socket_info *xsk)
        exit_with_error(errno);
 }

+static int prev_idx_cq;
+
 static inline void complete_tx_l2fwd(struct xsk_socket_info *xsk)
 {
        u32 idx_cq = 0, idx_fq = 0;
@@ -463,6 +464,15 @@ static inline void complete_tx_l2fwd(struct
xsk_socket_info *xsk)
                unsigned int i;
                int ret;

+        if (idx_cq == prev_idx_cq) {
+            printf("\n\n ERROR \n\n");
+        }
+        if (idx_cq - prev_idx_cq != rcvd) {
+            printf("\n\n ERROR request %d return %d idx_cq %x prev_cq
%x diff %d\n",
+                   ndescs, rcvd, idx_cq, prev_idx_cq, idx_cq - prev_idx_cq);
+        }
+        prev_idx_cq = idx_cq;
+
                ret = xsk_ring_prod__reserve(&xsk->umem->fq, rcvd, &idx_fq);
                while (ret != rcvd) {
                        if (ret < 0)
@@ -599,7 +609,7 @@ static void tx_only(struct xsk_socket_info *xsk)
        }
 }
Most of the time idx_cq - prev_idx_cq == rcvd, but sometimes I got:
 ERROR request 64 return 64 idx_cq 107f54 prev_cq 107f50 diff 4
 ERROR request 64 return 4 idx_cq 2df794 prev_cq 2df754 diff 64
 ERROR request 60 return 60 idx_cq 2df798 prev_cq 2df794 diff 4
 ERROR request 64 return 64 idx_cq 2df7d4 prev_cq 2df798 diff 60
I wonder if it is a bug?
[1]https://github.com/williamtu/ovs-ebpf/blob/afxdp-v11/lib/netdev-afxdp.c#L719

Thanks
William

Re: Question/Bug about AF_XDP idx_cq from xsk_ring_cons__peek?

[Björn Töpel]

On Sat, 15 Jun 2019 at 01:25, William Tu <u9012063@gmail.com> wrote:
>
> Hi,
>
> In my implementation[1], I'm doing s.t like
>     tx_done = xsk_ring_cons__peek(&xsk->umem->cq, BATCH_SIZE, &idx_cq);
>     if (tx_done > 0) {
>         xsk_ring_cons__release(&xsk->umem->cq, tx_done);
>         xsk->outstanding_tx -= tx_done;
>     }
>
> I expect if tx_done returns 32, then after calling xsk_ring_cons__release,
> the next time I call xsk_ring_cons__peek, I should get idx_cq + 32.
> However, sometimes I see the same value of idx_cq is returned as previous, even
> when tx_done > 0. Is this the expected behavior?
>
> I experiment with xdpsock_user.c with the patch below and run
> ~/bpf-next/samples/bpf# ./xdpsock -l -N -z -i eth3
> using bpf-next commit 5e2ac390fbd08b2a462db66cef2663e4db0d5191
>

Will, apologies for the slow response.

Hmm, it looks like a bug. We'll take a look!


Thanks,
Björn


> --- a/samples/bpf/xdpsock_user.c
> +++ b/samples/bpf/xdpsock_user.c
> @@ -444,6 +443,8 @@ static void kick_tx(struct xsk_socket_info *xsk)
>         exit_with_error(errno);
>  }
>
> +static int prev_idx_cq;
> +
>  static inline void complete_tx_l2fwd(struct xsk_socket_info *xsk)
>  {
>         u32 idx_cq = 0, idx_fq = 0;
> @@ -463,6 +464,15 @@ static inline void complete_tx_l2fwd(struct
> xsk_socket_info *xsk)
>                 unsigned int i;
>                 int ret;
>
> +        if (idx_cq == prev_idx_cq) {
> +            printf("\n\n ERROR \n\n");
> +        }
> +        if (idx_cq - prev_idx_cq != rcvd) {
> +            printf("\n\n ERROR request %d return %d idx_cq %x prev_cq
> %x diff %d\n",
> +                   ndescs, rcvd, idx_cq, prev_idx_cq, idx_cq - prev_idx_cq);
> +        }
> +        prev_idx_cq = idx_cq;
> +
>                 ret = xsk_ring_prod__reserve(&xsk->umem->fq, rcvd, &idx_fq);
>                 while (ret != rcvd) {
>                         if (ret < 0)
> @@ -599,7 +609,7 @@ static void tx_only(struct xsk_socket_info *xsk)
>         }
>  }
> Most of the time idx_cq - prev_idx_cq == rcvd, but sometimes I got:
>  ERROR request 64 return 64 idx_cq 107f54 prev_cq 107f50 diff 4
>  ERROR request 64 return 4 idx_cq 2df794 prev_cq 2df754 diff 64
>  ERROR request 60 return 60 idx_cq 2df798 prev_cq 2df794 diff 4
>  ERROR request 64 return 64 idx_cq 2df7d4 prev_cq 2df798 diff 60
> I wonder if it is a bug?
> [1]https://github.com/williamtu/ovs-ebpf/blob/afxdp-v11/lib/netdev-afxdp.c#L719
>
> Thanks
> William

Re: Question/Bug about AF_XDP idx_cq from xsk_ring_cons__peek?

[William Tu]

On Tue, Jun 18, 2019 at 5:51 AM Björn Töpel <bjorn.topel@gmail.com> wrote:
>
> On Sat, 15 Jun 2019 at 01:25, William Tu <u9012063@gmail.com> wrote:
> >
> > Hi,
> >
> > In my implementation[1], I'm doing s.t like
> >     tx_done = xsk_ring_cons__peek(&xsk->umem->cq, BATCH_SIZE, &idx_cq);
> >     if (tx_done > 0) {
> >         xsk_ring_cons__release(&xsk->umem->cq, tx_done);
> >         xsk->outstanding_tx -= tx_done;
> >     }
> >
> > I expect if tx_done returns 32, then after calling xsk_ring_cons__release,
> > the next time I call xsk_ring_cons__peek, I should get idx_cq + 32.
> > However, sometimes I see the same value of idx_cq is returned as previous, even
> > when tx_done > 0. Is this the expected behavior?
> >
> > I experiment with xdpsock_user.c with the patch below and run
> > ~/bpf-next/samples/bpf# ./xdpsock -l -N -z -i eth3
> > using bpf-next commit 5e2ac390fbd08b2a462db66cef2663e4db0d5191
> >
>
> Will, apologies for the slow response.
>
> Hmm, it looks like a bug. We'll take a look!
>
No worries, thanks for the update!
Or let me know if I can try something to get more info.

the xsk_ring_cons__peek just keeps doing
        *idx = cons->cached_cons;
        cons->cached_cons += entries;

It's weird to see idx value not advancing 'entries' occasionally.

Regards,
William

Re: Question/Bug about AF_XDP idx_cq from xsk_ring_cons__peek?

[William Tu]

On Mon, Jun 24, 2019 at 2:51 AM Magnus Karlsson
<magnus.karlsson@gmail.com> wrote:
>
> On Sat, 15 Jun 2019 at 01:25, William Tu <u9012063@xxxxxxxxx> wrote:
> >
> > Hi,
> >
> > In my implementation[1], I'm doing s.t like
> >     tx_done = xsk_ring_cons__peek(&xsk->umem->cq, BATCH_SIZE, &idx_cq);
> >     if (tx_done > 0) {
> >         xsk_ring_cons__release(&xsk->umem->cq, tx_done);
> >         xsk->outstanding_tx -= tx_done;
> >     }
> >
> > I expect if tx_done returns 32, then after calling xsk_ring_cons__release,
> > the next time I call xsk_ring_cons__peek, I should get idx_cq + 32.
> > However, sometimes I see the same value of idx_cq is returned as previous, even
> > when tx_done > 0. Is this the expected behavior?
> >
> > I experiment with xdpsock_user.c with the patch below and run
> > ~/bpf-next/samples/bpf# ./xdpsock -l -N -z -i eth3
> > using bpf-next commit 5e2ac390fbd08b2a462db66cef2663e4db0d5191
> >
> > --- a/samples/bpf/xdpsock_user.c
> > +++ b/samples/bpf/xdpsock_user.c
> > @@ -444,6 +443,8 @@ static void kick_tx(struct xsk_socket_info *xsk)
> >         exit_with_error(errno);
> >  }
> >
> > +static int prev_idx_cq;
> > +
> >  static inline void complete_tx_l2fwd(struct xsk_socket_info *xsk)
> >  {
> >         u32 idx_cq = 0, idx_fq = 0;
> > @@ -463,6 +464,15 @@ static inline void complete_tx_l2fwd(struct
> > xsk_socket_info *xsk)
> >                 unsigned int i;
> >                 int ret;
> >
> > +        if (idx_cq == prev_idx_cq) {
> > +            printf("\n\n ERROR \n\n");
> > +        }
> > +        if (idx_cq - prev_idx_cq != rcvd) {
>
> William,
>
> You need to compare with prev_rcvd (and introduce that variable) here
> as the difference in index should be the same as the amount you
> received last time. If you do this change, you will get no errors. You
> can also see this pattern in your printouts. The diff is always equal
> to rcvd during the previous iteration.
>
> /Magnus
>

Hi Magnus,

Thanks for the clarification! Now I understand it's my mistake.
So:
 rcvd1 = xsk_ring_cons__peek(&xsk->umem->cq, ndescs, &idx1_cq);
 ...
 rcvd2 = xsk_ring_cons__peek(&xsk->umem->cq, ndescs, &idx2_cq);

and idx2_cq - idx1_cq == rcvd1.

--William

Re: Question/Bug about AF_XDP idx_cq from xsk_ring_cons__peek?

[Magnus Karlsson]

On Sat, 15 Jun 2019 at 01:25, William Tu <u9012063@xxxxxxxxx> wrote:
>
> Hi,
>
> In my implementation[1], I'm doing s.t like
>     tx_done = xsk_ring_cons__peek(&xsk->umem->cq, BATCH_SIZE, &idx_cq);
>     if (tx_done > 0) {
>         xsk_ring_cons__release(&xsk->umem->cq, tx_done);
>         xsk->outstanding_tx -= tx_done;
>     }
>
> I expect if tx_done returns 32, then after calling xsk_ring_cons__release,
> the next time I call xsk_ring_cons__peek, I should get idx_cq + 32.
> However, sometimes I see the same value of idx_cq is returned as previous, even
> when tx_done > 0. Is this the expected behavior?
>
> I experiment with xdpsock_user.c with the patch below and run
> ~/bpf-next/samples/bpf# ./xdpsock -l -N -z -i eth3
> using bpf-next commit 5e2ac390fbd08b2a462db66cef2663e4db0d5191
>
> --- a/samples/bpf/xdpsock_user.c
> +++ b/samples/bpf/xdpsock_user.c
> @@ -444,6 +443,8 @@ static void kick_tx(struct xsk_socket_info *xsk)
>         exit_with_error(errno);
>  }
>
> +static int prev_idx_cq;
> +
>  static inline void complete_tx_l2fwd(struct xsk_socket_info *xsk)
>  {
>         u32 idx_cq = 0, idx_fq = 0;
> @@ -463,6 +464,15 @@ static inline void complete_tx_l2fwd(struct
> xsk_socket_info *xsk)
>                 unsigned int i;
>                 int ret;
>
> +        if (idx_cq == prev_idx_cq) {
> +            printf("\n\n ERROR \n\n");
> +        }
> +        if (idx_cq - prev_idx_cq != rcvd) {

William,

You need to compare with prev_rcvd (and introduce that variable) here
as the difference in index should be the same as the amount you
received last time. If you do this change, you will get no errors. You
can also see this pattern in your printouts. The diff is always equal
to rcvd during the previous iteration.

/Magnus

> +            printf("\n\n ERROR request %d return %d idx_cq %x prev_cq
> %x diff %d\n",
> +                   ndescs, rcvd, idx_cq, prev_idx_cq, idx_cq - prev_idx_cq);
> +        }
> +        prev_idx_cq = idx_cq;
> +
>                 ret = xsk_ring_prod__reserve(&xsk->umem->fq, rcvd, &idx_fq);
>                 while (ret != rcvd) {
>                         if (ret < 0)
> @@ -599,7 +609,7 @@ static void tx_only(struct xsk_socket_info *xsk)
>         }
>  }
> Most of the time idx_cq - prev_idx_cq == rcvd, but sometimes I got:
>  ERROR request 64 return 64 idx_cq 107f54 prev_cq 107f50 diff 4
>  ERROR request 64 return 4 idx_cq 2df794 prev_cq 2df754 diff 64
>  ERROR request 60 return 60 idx_cq 2df798 prev_cq 2df794 diff 4
>  ERROR request 64 return 64 idx_cq 2df7d4 prev_cq 2df798 diff 60
> I wonder if it is a bug?
> [1]https://github.com/williamtu/ovs-ebpf/blob/afxdp-v11/lib/netdev-afxdp.c#L719
>
> Thanks
> William