| Message ID | 20190603131907.13395-3-maciej.fijalkowski@intel.com |
|---|---|
| State | RFC |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | [RFC,bpf-next,1/4] libbpf: fill the AF_XDP fill queue before bind() call | expand |
On 2019-06-03 15:19, Maciej Fijalkowski wrote: > When it comes down to ethtool's get channels API, various drivers are > reporting the queue count in two ways - they are setting max_combined or > max_tx/max_rx fields. When creating the eBPF maps for xsk socket, this > API is used so that we have an entries in maps per each queue. > In case where driver (mlx4, ice) reports queues in max_tx/max_rx, we end > up with eBPF maps with single entries, so it's not possible to attach an > AF_XDP socket onto queue other than 0 - xsk_set_bpf_maps() would try to > call bpf_map_update_elem() with key set to xsk->queue_id. > > To fix this, let's look for channels.max_{t,r}x as well in > xsk_get_max_queues. > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > --- > tools/lib/bpf/xsk.c | 18 ++++++++++-------- > 1 file changed, 10 insertions(+), 8 deletions(-) > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index 57dda1389870..514ab3fb06f4 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -339,21 +339,23 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) > ifr.ifr_data = (void *)&channels; > strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ); > err = ioctl(fd, SIOCETHTOOL, &ifr); > - if (err && errno != EOPNOTSUPP) { > - ret = -errno; > - goto out; > - } > + close(fd); > + > + if (err && errno != EOPNOTSUPP) > + return -errno; > > - if (channels.max_combined == 0 || errno == EOPNOTSUPP) > + if (channels.max_combined) > + ret = channels.max_combined; > + else if (channels.max_rx && channels.max_tx) > + ret = min(channels.max_rx, channels.max_tx); Hmm, do we really need to look at max_tx? For each Rx, there's (usually) an XDP ring. OTOH, when AF_XDP ZC is not implemented, it uses the skb path... > + else if (channels.max_combined == 0 || errno == EOPNOTSUPP) > /* If the device says it has no channels, then all traffic > * is sent to a single stream, so max queues = 1. > */ > ret = 1; > else > - ret = channels.max_combined; > + ret = -1; > > -out: > - close(fd); > return ret; > } > >
On Tue, 4 Jun 2019 10:06:57 +0200 Björn Töpel <bjorn.topel@intel.com> wrote: > On 2019-06-03 15:19, Maciej Fijalkowski wrote: > > When it comes down to ethtool's get channels API, various drivers are > > reporting the queue count in two ways - they are setting max_combined or > > max_tx/max_rx fields. When creating the eBPF maps for xsk socket, this > > API is used so that we have an entries in maps per each queue. > > In case where driver (mlx4, ice) reports queues in max_tx/max_rx, we end > > up with eBPF maps with single entries, so it's not possible to attach an > > AF_XDP socket onto queue other than 0 - xsk_set_bpf_maps() would try to > > call bpf_map_update_elem() with key set to xsk->queue_id. > > > > To fix this, let's look for channels.max_{t,r}x as well in > > xsk_get_max_queues. > > > > Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> > > --- > > tools/lib/bpf/xsk.c | 18 ++++++++++-------- > > 1 file changed, 10 insertions(+), 8 deletions(-) > > > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > > index 57dda1389870..514ab3fb06f4 100644 > > --- a/tools/lib/bpf/xsk.c > > +++ b/tools/lib/bpf/xsk.c > > @@ -339,21 +339,23 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) > > ifr.ifr_data = (void *)&channels; > > strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ); > > err = ioctl(fd, SIOCETHTOOL, &ifr); > > - if (err && errno != EOPNOTSUPP) { > > - ret = -errno; > > - goto out; > > - } > > + close(fd); > > + > > + if (err && errno != EOPNOTSUPP) > > + return -errno; > > > > - if (channels.max_combined == 0 || errno == EOPNOTSUPP) > > + if (channels.max_combined) > > + ret = channels.max_combined; > > + else if (channels.max_rx && channels.max_tx) > > + ret = min(channels.max_rx, channels.max_tx); > > Hmm, do we really need to look at max_tx? For each Rx, there's (usually) > an XDP ring. Probably we would be good to go with only max_rx, but in drivers during the umem setup we also are comparing the queue id provided by user against the num tx queues...so in theory, we could allocate the max_rx entries, but if the current txq count is lower than reported max_rx, a bunch of map entries would never be used, no? > > OTOH, when AF_XDP ZC is not implemented, it uses the skb path... > > > + else if (channels.max_combined == 0 || errno == EOPNOTSUPP) > > /* If the device says it has no channels, then all traffic > > * is sent to a single stream, so max queues = 1. > > */ > > ret = 1; > > else > > - ret = channels.max_combined; > > + ret = -1; > > > > -out: > > - close(fd); > > return ret; > > } > > > >
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index 57dda1389870..514ab3fb06f4 100644 --- a/tools/lib/bpf/xsk.c +++ b/tools/lib/bpf/xsk.c @@ -339,21 +339,23 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) ifr.ifr_data = (void *)&channels; strncpy(ifr.ifr_name, xsk->ifname, IFNAMSIZ); err = ioctl(fd, SIOCETHTOOL, &ifr); - if (err && errno != EOPNOTSUPP) { - ret = -errno; - goto out; - } + close(fd); + + if (err && errno != EOPNOTSUPP) + return -errno; - if (channels.max_combined == 0 || errno == EOPNOTSUPP) + if (channels.max_combined) + ret = channels.max_combined; + else if (channels.max_rx && channels.max_tx) + ret = min(channels.max_rx, channels.max_tx); + else if (channels.max_combined == 0 || errno == EOPNOTSUPP) /* If the device says it has no channels, then all traffic * is sent to a single stream, so max queues = 1. */ ret = 1; else - ret = channels.max_combined; + ret = -1; -out: - close(fd); return ret; }
When it comes down to ethtool's get channels API, various drivers are reporting the queue count in two ways - they are setting max_combined or max_tx/max_rx fields. When creating the eBPF maps for xsk socket, this API is used so that we have an entries in maps per each queue. In case where driver (mlx4, ice) reports queues in max_tx/max_rx, we end up with eBPF maps with single entries, so it's not possible to attach an AF_XDP socket onto queue other than 0 - xsk_set_bpf_maps() would try to call bpf_map_update_elem() with key set to xsk->queue_id. To fix this, let's look for channels.max_{t,r}x as well in xsk_get_max_queues. Signed-off-by: Maciej Fijalkowski <maciej.fijalkowski@intel.com> --- tools/lib/bpf/xsk.c | 18 ++++++++++-------- 1 file changed, 10 insertions(+), 8 deletions(-)