Re: [PATCH bpf-next 3/6] selftests/xsk: implement get_hw_ring_size function to retrieve current and max interface size

[Stanislav Fomichev <sdf@google.com>]

On 03/15, Tushar Vyavahare wrote:
> Introduce a new function called get_hw_size that retrieves both the
> current and maximum size of the interface and stores this information in
> the 'hw_ring' structure.
> 
> Signed-off-by: Tushar Vyavahare <tushar.vyavahare@intel.com>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 32 ++++++++++++++++++++++++
>  tools/testing/selftests/bpf/xskxceiver.h |  8 ++++++
>  2 files changed, 40 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index eaa102c8098b..32005bfb9c9f 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -81,6 +81,8 @@
>  #include <linux/mman.h>
>  #include <linux/netdev.h>
>  #include <linux/bitmap.h>
> +#include <linux/sockios.h>
> +#include <linux/ethtool.h>
>  #include <arpa/inet.h>
>  #include <net/if.h>
>  #include <locale.h>
> @@ -95,6 +97,7 @@
>  #include <sys/socket.h>
>  #include <sys/time.h>
>  #include <sys/types.h>
> +#include <sys/ioctl.h>
>  #include <unistd.h>
>  
>  #include "xsk_xdp_progs.skel.h"
> @@ -409,6 +412,35 @@ static void parse_command_line(struct ifobject *ifobj_tx, struct ifobject *ifobj
>  	}
>  }
>  
> +static int get_hw_ring_size(struct ifobject *ifobj)
> +{
> +	struct ethtool_ringparam ring_param = {0};
> +	struct ifreq ifr = {0};
> +	int sockfd;
> +
> +	sockfd = socket(AF_INET, SOCK_DGRAM, 0);
> +	if (sockfd < 0)
> +		return errno;
> +
> +	memcpy(ifr.ifr_name, ifobj->ifname, sizeof(ifr.ifr_name));
> +
> +	ring_param.cmd = ETHTOOL_GRINGPARAM;
> +	ifr.ifr_data = (char *)&ring_param;
> +
> +	if (ioctl(sockfd, SIOCETHTOOL, &ifr) < 0) {
> +		close(sockfd);
> +		return errno;

close(sockfd) can potentially override the errno. Also, return -errno to
match the other cases where errors are < 0.

> +	}
> +
> +	ifobj->ring.default_tx = ring_param.tx_pending;
> +	ifobj->ring.default_rx = ring_param.rx_pending;
> +	ifobj->ring.max_tx = ring_param.tx_max_pending;
> +	ifobj->ring.max_rx = ring_param.rx_max_pending;
> +
> +	close(sockfd);
> +	return 0;
> +}
> +
>  static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
>  			     struct ifobject *ifobj_rx)
>  {
> diff --git a/tools/testing/selftests/bpf/xskxceiver.h b/tools/testing/selftests/bpf/xskxceiver.h
> index 425304e52f35..4f58b70fa781 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.h
> +++ b/tools/testing/selftests/bpf/xskxceiver.h
> @@ -114,6 +114,13 @@ struct pkt_stream {
>  	bool verbatim;
>  };
>  
> +struct hw_ring {
> +	u32 default_tx;
> +	u32 default_rx;
> +	u32 max_tx;
> +	u32 max_rx;
> +};
> +
>  struct ifobject;
>  struct test_spec;
>  typedef int (*validation_func_t)(struct ifobject *ifobj);
> @@ -130,6 +137,7 @@ struct ifobject {
>  	struct xsk_xdp_progs *xdp_progs;
>  	struct bpf_map *xskmap;
>  	struct bpf_program *xdp_prog;
> +	struct hw_ring ring;

Any reason not to store ethtool_ringparam directly here? No need to
introduce new hw_ring.