> -----Original Message-----
> From: Stanislav Fomichev <sdf@xxxxxxxxxx>
> Sent: Friday, March 15, 2024 11:14 PM
> To: Vyavahare, Tushar <tushar.vyavahare@xxxxxxxxx>
> Cc: bpf@xxxxxxxxxxxxxxx; netdev@xxxxxxxxxxxxxxx; bjorn@xxxxxxxxxx; Karlsson,
> Magnus <magnus.karlsson@xxxxxxxxx>; Fijalkowski, Maciej
> <maciej.fijalkowski@xxxxxxxxx>; jonathan.lemon@xxxxxxxxx;
> davem@xxxxxxxxxxxxx; kuba@xxxxxxxxxx; pabeni@xxxxxxxxxx;
> ast@xxxxxxxxxx; daniel@xxxxxxxxxxxxx; Sarkar, Tirthendu
> <tirthendu.sarkar@xxxxxxxxx>
> 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 <tushar.vyavahare@xxxxxxxxx>
> > ---
> > 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.
