bpf.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Magnus Karlsson <magnus.karlsson@gmail.com>
To: Maciej Fijalkowski <maciej.fijalkowski@intel.com>
Cc: "Karlsson, Magnus" <magnus.karlsson@intel.com>,
	"Björn Töpel" <bjorn@kernel.org>,
	"Alexei Starovoitov" <ast@kernel.org>,
	"Daniel Borkmann" <daniel@iogearbox.net>,
	"Network Development" <netdev@vger.kernel.org>,
	"Jonathan Lemon" <jonathan.lemon@gmail.com>,
	"Ciara Loftus" <ciara.loftus@intel.com>,
	bpf <bpf@vger.kernel.org>, "Yonghong Song" <yhs@fb.com>,
	"Andrii Nakryiko" <andrii@kernel.org>
Subject: Re: [PATCH bpf-next v2 10/16] selftests: xsk: validate tx stats on tx thread
Date: Thu, 19 Aug 2021 11:52:20 +0200	[thread overview]
Message-ID: <CAJ8uoz2aUnsnN5TQ5JstfbzByq80yX9utzbt3THwMKFHpYs4zA@mail.gmail.com> (raw)
In-Reply-To: <20210819092634.GA32204@ranger.igk.intel.com>

On Thu, Aug 19, 2021 at 11:41 AM Maciej Fijalkowski
<maciej.fijalkowski@intel.com> wrote:
>
> On Tue, Aug 17, 2021 at 11:27:23AM +0200, Magnus Karlsson wrote:
> > From: Magnus Karlsson <magnus.karlsson@intel.com>
> >
> > Validate the tx stats on the Tx thread instead of the Rx
> > tread. Depending on your settings, you might not be allowed to query
> > the statistics of a socket you do not own, so better to do this on the
> > correct thread to start with.
> >
> > Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> > ---
> >  tools/testing/selftests/bpf/xdpxceiver.c | 55 ++++++++++++++++++------
> >  1 file changed, 41 insertions(+), 14 deletions(-)
> >
> > diff --git a/tools/testing/selftests/bpf/xdpxceiver.c b/tools/testing/selftests/bpf/xdpxceiver.c
> > index fe3d281a0575..8ff24472ef1e 100644
> > --- a/tools/testing/selftests/bpf/xdpxceiver.c
> > +++ b/tools/testing/selftests/bpf/xdpxceiver.c
> > @@ -642,23 +642,22 @@ static void tx_only_all(struct ifobject *ifobject)
> >       complete_tx_only_all(ifobject);
> >  }
> >
> > -static void stats_validate(struct ifobject *ifobject)
> > +static bool rx_stats_are_valid(struct ifobject *ifobject)
> >  {
> > +     u32 xsk_stat = 0, expected_stat = opt_pkt_count;
> > +     struct xsk_socket *xsk = ifobject->xsk->xsk;
> > +     int fd = xsk_socket__fd(xsk);
> >       struct xdp_statistics stats;
> >       socklen_t optlen;
> >       int err;
> > -     struct xsk_socket *xsk = stat_test_type == STAT_TEST_TX_INVALID ?
> > -                                                     ifdict[!ifobject->ifdict_index]->xsk->xsk :
> > -                                                     ifobject->xsk->xsk;
> > -     int fd = xsk_socket__fd(xsk);
> > -     unsigned long xsk_stat = 0, expected_stat = opt_pkt_count;
> > -
> > -     sigvar = 0;
> >
> >       optlen = sizeof(stats);
> >       err = getsockopt(fd, SOL_XDP, XDP_STATISTICS, &stats, &optlen);
> > -     if (err)
> > -             return;
> > +     if (err) {
> > +             ksft_test_result_fail("ERROR: [%s] getsockopt(XDP_STATISTICS) error %u %s\n",
> > +                                   __func__, -err, strerror(-err));
> > +             return true;
>
> Can we invert the logic or change the name of the func?
> Returning 'true' for error case is a bit confusing given the name of func
> is blah_are_valid, no? If there was an error then I'd return false.
>
> OTOH we're testing faulty socket situations in here, but error from
> getsockopt does not mean that stats were valid.

Yes, this is not that clear. We want the loop on the higher level to
quit, therefore we return true when there is an error. A problem with
the stats tests is that they do not terminate when the stats are
wrong, only when they pass. Once I get the second patch set accepted
(in some form), we should rewrite these stats tests in that new
framework so that they will terminate even when the stats are wrong.
This whole problem will likely disappear at that point. But I will
scratch my head and try to make it better in this patch.

> > +     }
> >
> >       if (optlen == sizeof(struct xdp_statistics)) {
> >               switch (stat_test_type) {
> > @@ -666,8 +665,7 @@ static void stats_validate(struct ifobject *ifobject)
> >                       xsk_stat = stats.rx_dropped;
> >                       break;
> >               case STAT_TEST_TX_INVALID:
> > -                     xsk_stat = stats.tx_invalid_descs;
> > -                     break;
> > +                     return true;
> >               case STAT_TEST_RX_FULL:
> >                       xsk_stat = stats.rx_ring_full;
> >                       expected_stat -= RX_FULL_RXQSIZE;
> > @@ -680,8 +678,33 @@ static void stats_validate(struct ifobject *ifobject)
> >               }
> >
> >               if (xsk_stat == expected_stat)
> > -                     sigvar = 1;
> > +                     return true;
> > +     }
> > +
> > +     return false;
> > +}
> > +
> > +static void tx_stats_validate(struct ifobject *ifobject)
> > +{
> > +     struct xsk_socket *xsk = ifobject->xsk->xsk;
> > +     int fd = xsk_socket__fd(xsk);
> > +     struct xdp_statistics stats;
> > +     socklen_t optlen;
> > +     int err;
> > +
> > +     optlen = sizeof(stats);
> > +     err = getsockopt(fd, SOL_XDP, XDP_STATISTICS, &stats, &optlen);
> > +     if (err) {
> > +             ksft_test_result_fail("ERROR: [%s] getsockopt(XDP_STATISTICS) error %u %s\n",
> > +                                   __func__, -err, strerror(-err));
> > +             return;
> >       }
> > +
> > +     if (stats.tx_invalid_descs == opt_pkt_count)
> > +             return;
> > +
> > +     ksft_test_result_fail("ERROR: [%s] tx_invalid_descs incorrect. Got [%u] expected [%u]\n",
> > +                           __func__, stats.tx_invalid_descs, opt_pkt_count);
> >  }
> >
> >  static void thread_common_ops(struct ifobject *ifobject, void *bufs)
> > @@ -767,6 +790,9 @@ static void *worker_testapp_validate_tx(void *arg)
> >       print_verbose("Sending %d packets on interface %s\n", opt_pkt_count, ifobject->ifname);
> >       tx_only_all(ifobject);
> >
> > +     if (stat_test_type == STAT_TEST_TX_INVALID)
> > +             tx_stats_validate(ifobject);
> > +
> >       testapp_cleanup_xsk_res(ifobject);
> >       pthread_exit(NULL);
> >  }
> > @@ -792,7 +818,8 @@ static void *worker_testapp_validate_rx(void *arg)
> >               if (test_type != TEST_TYPE_STATS) {
> >                       rx_pkt(ifobject->xsk, fds);
> >               } else {
> > -                     stats_validate(ifobject);
> > +                     if (rx_stats_are_valid(ifobject))
> > +                             break;
> >               }
> >               if (sigvar)
> >                       break;
> > --
> > 2.29.0
> >

  reply	other threads:[~2021-08-19  9:52 UTC|newest]

Thread overview: 24+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-17  9:27 [PATCH bpf-next v2 00/16] selftests: xsk: various simplifications Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 01/16] selftests: xsk: remove color mode Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 02/16] selftests: xsk: remove the num_tx_packets option Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 03/16] selftests: xsk: remove unused variables Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 04/16] selftests: xsk: return correct error codes Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 05/16] selftests: xsk: simplify the retry code Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 06/16] selftests: xsk: remove end-of-test packet Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 07/16] selftests: xsk: disassociate umem size with packets sent Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 08/16] selftests: xsk: rename worker_* functions that are not thread entry points Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 09/16] selftests: xsk: simplify packet validation in xsk tests Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 10/16] selftests: xsk: validate tx stats on tx thread Magnus Karlsson
2021-08-19  9:26   ` Maciej Fijalkowski
2021-08-19  9:52     ` Magnus Karlsson [this message]
2021-08-17  9:27 ` [PATCH bpf-next v2 11/16] selftests: xsk: decrease batch size Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 12/16] selftests: xsk: remove cleanup at end of program Magnus Karlsson
2021-08-19  9:28   ` Maciej Fijalkowski
2021-08-19 10:02     ` Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 13/16] selftests: xsk: generate packet directly in umem Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 14/16] selftests: xsk: generate packets from specification Magnus Karlsson
2021-08-19 10:10   ` Maciej Fijalkowski
2021-08-19 10:38     ` Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 15/16] selftests: xsk: make enums lower case Magnus Karlsson
2021-08-17  9:27 ` [PATCH bpf-next v2 16/16] selftests: xsk: preface options with opt Magnus Karlsson
2021-08-19 10:13 ` [PATCH bpf-next v2 00/16] selftests: xsk: various simplifications Maciej Fijalkowski

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CAJ8uoz2aUnsnN5TQ5JstfbzByq80yX9utzbt3THwMKFHpYs4zA@mail.gmail.com \
    --to=magnus.karlsson@gmail.com \
    --cc=andrii@kernel.org \
    --cc=ast@kernel.org \
    --cc=bjorn@kernel.org \
    --cc=bpf@vger.kernel.org \
    --cc=ciara.loftus@intel.com \
    --cc=daniel@iogearbox.net \
    --cc=jonathan.lemon@gmail.com \
    --cc=maciej.fijalkowski@intel.com \
    --cc=magnus.karlsson@intel.com \
    --cc=netdev@vger.kernel.org \
    --cc=yhs@fb.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).