diff mbox series

[bpf-next,v2,10/16] selftests: xsk: validate tx stats on tx thread

Message ID 20210817092729.433-11-magnus.karlsson@gmail.com (mailing list archive)
State Superseded
Delegated to: BPF
Headers show
Series selftests: xsk: various simplifications | expand

Checks

Context Check Description
netdev/cover_letter success Link
netdev/fixes_present success Link
netdev/patch_count fail Series longer than 15 patches
netdev/tree_selection success Clearly marked for bpf-next
netdev/subject_prefix success Link
netdev/cc_maintainers warning 9 maintainers not CCed: davem@davemloft.net songliubraving@fb.com shuah@kernel.org kafai@fb.com hawk@kernel.org kuba@kernel.org john.fastabend@gmail.com linux-kselftest@vger.kernel.org kpsingh@kernel.org
netdev/source_inline success Was 0 now: 0
netdev/verify_signedoff success Link
netdev/module_param success Was 0 now: 0
netdev/build_32bit success Errors and warnings before: 0 this patch: 0
netdev/kdoc success Errors and warnings before: 0 this patch: 0
netdev/verify_fixes success Link
netdev/checkpatch warning + ksft_test_result_fail("ERROR: [%s] getsockopt(XDP_STATISTICS) error %u %s\n", + ksft_test_result_fail("ERROR: [%s] tx_invalid_descs incorrect. Got [%u] expected [%u]\n", WARNING: line length of 93 exceeds 80 columns WARNING: line length of 97 exceeds 80 columns
netdev/build_allmodconfig_warn success Errors and warnings before: 0 this patch: 0
netdev/header_inline success Link

Commit Message

Magnus Karlsson Aug. 17, 2021, 9:27 a.m. UTC 
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(-)

Comments

Maciej Fijalkowski Aug. 19, 2021, 9:26 a.m. UTC | #1 
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.

> +	}
>  
>  	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
>
Magnus Karlsson Aug. 19, 2021, 9:52 a.m. UTC | #2 
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
> >
diff mbox series

Patch

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;
+	}
 
 	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;