[bpf-next,4/6] selftests/xsk: implement set_hw_ring_size function to configure interface ring size

Commit Message

Tushar Vyavahare March 15, 2024, 2:07 p.m. UTC

Introduce a new function called set_hw_ring_size that allows for the
dynamic configuration of the ring size within the interface.

Signed-off-by: Tushar Vyavahare <[e-mail]>
---
 tools/testing/selftests/bpf/xskxceiver.c | 35 ++++++++++++++++++++++++
 1 file changed, 35 insertions(+)

Comments

Alexei Starovoitov March 15, 2024, 3:47 p.m. UTC | #1

On Fri, Mar 15, 2024 at 7:23 AM Tushar Vyavahare
<[e-mail]> wrote:
>
> Introduce a new function called set_hw_ring_size that allows for the
> dynamic configuration of the ring size within the interface.
>
> Signed-off-by: Tushar Vyavahare <[e-mail]>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 35 ++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
>
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 32005bfb9c9f..aafa78307586 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -441,6 +441,41 @@ static int get_hw_ring_size(struct ifobject *ifobj)
>         return 0;
>  }
>
> +static int set_hw_ring_size(struct ifobject *ifobj, u32 tx, u32 rx)
> +{
> +       struct ethtool_ringparam ring_param = {0};
> +       struct ifreq ifr = {0};
> +       int sockfd, ret;
> +       u32 ctr = 0;
> +
> +       sockfd = socket(AF_INET, SOCK_DGRAM, 0);
> +       if (sockfd < 0)
> +               return errno;
> +
> +       memcpy(ifr.ifr_name, ifobj->ifname, sizeof(ifr.ifr_name));
> +
> +       ring_param.tx_pending = tx;
> +       ring_param.rx_pending = rx;
> +
> +       ring_param.cmd = ETHTOOL_SRINGPARAM;
> +       ifr.ifr_data = (char *)&ring_param;
> +
> +       while (ctr++ < SOCK_RECONF_CTR) {
> +               ret = ioctl(sockfd, SIOCETHTOOL, &ifr);
> +               if (!ret)
> +                       break;
> +               /* Retry if it fails */
> +               if (ctr >= SOCK_RECONF_CTR) {
> +                       close(sockfd);
> +                       return errno;
> +               }
> +               usleep(USLEEP_MAX);

Does it really have to sleep or copy paste from other places?
This ioctl() is supposed to do synchronous config, no?

Stanislav Fomichev March 15, 2024, 5:44 p.m. UTC | #2

On 03/15, Tushar Vyavahare wrote:
> Introduce a new function called set_hw_ring_size that allows for the
> dynamic configuration of the ring size within the interface.
> 
> Signed-off-by: Tushar Vyavahare <[e-mail]>
> ---
>  tools/testing/selftests/bpf/xskxceiver.c | 35 ++++++++++++++++++++++++
>  1 file changed, 35 insertions(+)
> 
> diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
> index 32005bfb9c9f..aafa78307586 100644
> --- a/tools/testing/selftests/bpf/xskxceiver.c
> +++ b/tools/testing/selftests/bpf/xskxceiver.c
> @@ -441,6 +441,41 @@ static int get_hw_ring_size(struct ifobject *ifobj)
>  	return 0;
>  }
>  
> +static int set_hw_ring_size(struct ifobject *ifobj, u32 tx, u32 rx)

Hmm, now that we have set/get, should we put them into
network_helpers.c? These seem pretty generic if you accept
iface_name + ethtool_ringparam in the api.

> +{
> +	struct ethtool_ringparam ring_param = {0};
> +	struct ifreq ifr = {0};
> +	int sockfd, ret;
> +	u32 ctr = 0;
> +
> +	sockfd = socket(AF_INET, SOCK_DGRAM, 0);
> +	if (sockfd < 0)
> +		return errno;
> +
> +	memcpy(ifr.ifr_name, ifobj->ifname, sizeof(ifr.ifr_name));
> +
> +	ring_param.tx_pending = tx;
> +	ring_param.rx_pending = rx;
> +
> +	ring_param.cmd = ETHTOOL_SRINGPARAM;
> +	ifr.ifr_data = (char *)&ring_param;
> +

[..]

> +	while (ctr++ < SOCK_RECONF_CTR) {

Is it to retry EINTR? Retrying something else doesn't make sense
probably? So maybe do only errno==EINTR cases? Will make it more
generic and not dependent on SOCK_RECONF_CTR.


> +		ret = ioctl(sockfd, SIOCETHTOOL, &ifr);
> +		if (!ret)
> +			break;
> +		/* Retry if it fails */
> +		if (ctr >= SOCK_RECONF_CTR) {
> +			close(sockfd);
> +			return errno;
> +		}

[..]

> +		usleep(USLEEP_MAX);

Same here. Not sure what's the purpose of sleep? Alternatively, maybe
clarify in the commit description what particular error case we want
to retry.

Tushar Vyavahare March 19, 2024, 6:52 a.m. UTC | #3

> -----Original Message-----
> From: Stanislav Fomichev <[e-mail]>
> Sent: Friday, March 15, 2024 11:14 PM
> To: Vyavahare, Tushar <[e-mail]>
> Cc: [e-mail]; [e-mail]; [e-mail]; Karlsson,
> Magnus <[e-mail]>; Fijalkowski, Maciej
> <[e-mail]>; [e-mail];
> [e-mail]; [e-mail]; [e-mail];
> [e-mail]; [e-mail]; Sarkar, Tirthendu
> <[e-mail]>
> Subject: Re: [PATCH bpf-next 4/6] selftests/xsk: implement set_hw_ring_size
> function to configure interface ring size
> 
> On 03/15, Tushar Vyavahare wrote:
> > Introduce a new function called set_hw_ring_size that allows for the
> > dynamic configuration of the ring size within the interface.
> >
> > Signed-off-by: Tushar Vyavahare <[e-mail]>
> > ---
> >  tools/testing/selftests/bpf/xskxceiver.c | 35
> > ++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > b/tools/testing/selftests/bpf/xskxceiver.c
> > index 32005bfb9c9f..aafa78307586 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -441,6 +441,41 @@ static int get_hw_ring_size(struct ifobject *ifobj)
> >  	return 0;
> >  }
> >
> > +static int set_hw_ring_size(struct ifobject *ifobj, u32 tx, u32 rx)
> 
> Hmm, now that we have set/get, should we put them into network_helpers.c?
> These seem pretty generic if you accept iface_name + ethtool_ringparam in
> the api.
> 

Clean version of get_hw_ring_size() and set_hw_ring_size() both are moved to network_helpers.c

> > +{
> > +	struct ethtool_ringparam ring_param = {0};
> > +	struct ifreq ifr = {0};
> > +	int sockfd, ret;
> > +	u32 ctr = 0;
> > +
> > +	sockfd = socket(AF_INET, SOCK_DGRAM, 0);
> > +	if (sockfd < 0)
> > +		return errno;
> > +
> > +	memcpy(ifr.ifr_name, ifobj->ifname, sizeof(ifr.ifr_name));
> > +
> > +	ring_param.tx_pending = tx;
> > +	ring_param.rx_pending = rx;
> > +
> > +	ring_param.cmd = ETHTOOL_SRINGPARAM;
> > +	ifr.ifr_data = (char *)&ring_param;
> > +
> 
> [..]
> 
> > +	while (ctr++ < SOCK_RECONF_CTR) {
> 
> Is it to retry EINTR? Retrying something else doesn't make sense probably? So
> maybe do only errno==EINTR cases? Will make it more generic and not
> dependent on SOCK_RECONF_CTR.
> 
> 

The close of an AF_XDP socket is an asynchronous operation. When an AF_XDP socket is active, changing the ring size is forbidden. Therefore, when we call set_hw_ring_size(), a previous AF_XDP socket might still be in the process of being closed and the ioct() will then fail, as it is forbidden to change the ring size when there is an active AF_XDP socket.

When the AF_XDP socket is active, we are getting an EBUSY error. I will handle the retry logic for this in a separate patch for xskxceiver.c.

> > +		ret = ioctl(sockfd, SIOCETHTOOL, &ifr);
> > +		if (!ret)
> > +			break;
> > +		/* Retry if it fails */
> > +		if (ctr >= SOCK_RECONF_CTR) {
> > +			close(sockfd);
> > +			return errno;
> > +		}
> 
> [..]
> 
> > +		usleep(USLEEP_MAX);
> 
> Same here. Not sure what's the purpose of sleep? Alternatively, maybe clarify
> in the commit description what particular error case we want to retry.

Removed this retry logic from set_hw_ring_size() , which moved to networks_helpers.c.
I will handle the retry logic with sleep for this in a separate patch for xskxceiver.c and I will put this in the commit message.

Tushar Vyavahare March 19, 2024, 6:54 a.m. UTC | #4

> -----Original Message-----
> From: Alexei Starovoitov <[e-mail]>
> Sent: Friday, March 15, 2024 9:18 PM
> To: Vyavahare, Tushar <[e-mail]>
> Cc: bpf <[e-mail]>; Network Development
> <[e-mail]>; Björn Töpel <[e-mail]>; Karlsson, Magnus
> <[e-mail]>; Fijalkowski, Maciej
> <[e-mail]>; Jonathan Lemon
> <[e-mail]>; David S. Miller <[e-mail]>; Jakub
> Kicinski <[e-mail]>; Paolo Abeni <[e-mail]>; Alexei
> Starovoitov <[e-mail]>; Daniel Borkmann <[e-mail]>;
> Sarkar, Tirthendu <[e-mail]>
> Subject: Re: [PATCH bpf-next 4/6] selftests/xsk: implement set_hw_ring_size
> function to configure interface ring size
> 
> On Fri, Mar 15, 2024 at 7:23 AM Tushar Vyavahare
> <[e-mail]> wrote:
> >
> > Introduce a new function called set_hw_ring_size that allows for the
> > dynamic configuration of the ring size within the interface.
> >
> > Signed-off-by: Tushar Vyavahare <[e-mail]>
> > ---
> >  tools/testing/selftests/bpf/xskxceiver.c | 35
> > ++++++++++++++++++++++++
> >  1 file changed, 35 insertions(+)
> >
> > diff --git a/tools/testing/selftests/bpf/xskxceiver.c
> > b/tools/testing/selftests/bpf/xskxceiver.c
> > index 32005bfb9c9f..aafa78307586 100644
> > --- a/tools/testing/selftests/bpf/xskxceiver.c
> > +++ b/tools/testing/selftests/bpf/xskxceiver.c
> > @@ -441,6 +441,41 @@ static int get_hw_ring_size(struct ifobject *ifobj)
> >         return 0;
> >  }
> >
> > +static int set_hw_ring_size(struct ifobject *ifobj, u32 tx, u32 rx) {
> > +       struct ethtool_ringparam ring_param = {0};
> > +       struct ifreq ifr = {0};
> > +       int sockfd, ret;
> > +       u32 ctr = 0;
> > +
> > +       sockfd = socket(AF_INET, SOCK_DGRAM, 0);
> > +       if (sockfd < 0)
> > +               return errno;
> > +
> > +       memcpy(ifr.ifr_name, ifobj->ifname, sizeof(ifr.ifr_name));
> > +
> > +       ring_param.tx_pending = tx;
> > +       ring_param.rx_pending = rx;
> > +
> > +       ring_param.cmd = ETHTOOL_SRINGPARAM;
> > +       ifr.ifr_data = (char *)&ring_param;
> > +
> > +       while (ctr++ < SOCK_RECONF_CTR) {
> > +               ret = ioctl(sockfd, SIOCETHTOOL, &ifr);
> > +               if (!ret)
> > +                       break;
> > +               /* Retry if it fails */
> > +               if (ctr >= SOCK_RECONF_CTR) {
> > +                       close(sockfd);
> > +                       return errno;
> > +               }
> > +               usleep(USLEEP_MAX);
> 
> Does it really have to sleep or copy paste from other places?
> This ioctl() is supposed to do synchronous config, no?

Response in the previous mail  to Stanislav Fomichev <[e-mail]>

Patch

diff --git a/tools/testing/selftests/bpf/xskxceiver.c b/tools/testing/selftests/bpf/xskxceiver.c
index 32005bfb9c9f..aafa78307586 100644
--- a/tools/testing/selftests/bpf/xskxceiver.c
+++ b/tools/testing/selftests/bpf/xskxceiver.c
@@ -441,6 +441,41 @@  static int get_hw_ring_size(struct ifobject *ifobj)
 	return 0;
 }
 
+static int set_hw_ring_size(struct ifobject *ifobj, u32 tx, u32 rx)
+{
+	struct ethtool_ringparam ring_param = {0};
+	struct ifreq ifr = {0};
+	int sockfd, ret;
+	u32 ctr = 0;
+
+	sockfd = socket(AF_INET, SOCK_DGRAM, 0);
+	if (sockfd < 0)
+		return errno;
+
+	memcpy(ifr.ifr_name, ifobj->ifname, sizeof(ifr.ifr_name));
+
+	ring_param.tx_pending = tx;
+	ring_param.rx_pending = rx;
+
+	ring_param.cmd = ETHTOOL_SRINGPARAM;
+	ifr.ifr_data = (char *)&ring_param;
+
+	while (ctr++ < SOCK_RECONF_CTR) {
+		ret = ioctl(sockfd, SIOCETHTOOL, &ifr);
+		if (!ret)
+			break;
+		/* Retry if it fails */
+		if (ctr >= SOCK_RECONF_CTR) {
+			close(sockfd);
+			return errno;
+		}
+		usleep(USLEEP_MAX);
+	}
+
+	close(sockfd);
+	return 0;
+}
+
 static void __test_spec_init(struct test_spec *test, struct ifobject *ifobj_tx,
 			     struct ifobject *ifobj_rx)
 {