| Message ID | 20190612155605.22450-8-maximmi@mellanox.com |
|---|---|
| State | Changes Requested |
| Delegated to: | BPF Maintainers |
| Headers | show |
| Series | AF_XDP infrastructure improvements and mlx5e support | expand |
On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: > Currently, libbpf uses the number of combined channels as the maximum > queue number. However, the kernel has a different limitation: > > - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). > > - ethtool_set_channels() checks for UMEMs in queues up to > combined_count + max(rx_count, tx_count). > > libbpf shouldn't limit applications to a lower max queue number. Account > for non-combined RX and TX channels when calculating the max queue > number. Use the same formula that is used in ethtool. > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > Reviewed-by: Tariq Toukan <tariqt@mellanox.com> > Acked-by: Saeed Mahameed <saeedm@mellanox.com> I don't think this is correct. max_tx tells you how many TX channels there can be, you can't add that to combined. Correct calculations is: max_num_chans = max(max_combined, max(max_rx, max_tx)) > tools/lib/bpf/xsk.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > index bf15a80a37c2..86107857e1f0 100644 > --- a/tools/lib/bpf/xsk.c > +++ b/tools/lib/bpf/xsk.c > @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) > goto out; > } > > - if (channels.max_combined == 0 || errno == EOPNOTSUPP) > + ret = channels.max_combined + max(channels.max_rx, channels.max_tx); > + > + if (ret == 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; > > out: > close(fd);
On Wed, 12 Jun 2019 at 22:24, Jakub Kicinski <jakub.kicinski@netronome.com> wrote: > > On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: > > Currently, libbpf uses the number of combined channels as the maximum > > queue number. However, the kernel has a different limitation: > > > > - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). > > > > - ethtool_set_channels() checks for UMEMs in queues up to > > combined_count + max(rx_count, tx_count). > > > > libbpf shouldn't limit applications to a lower max queue number. Account > > for non-combined RX and TX channels when calculating the max queue > > number. Use the same formula that is used in ethtool. > > > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > > Reviewed-by: Tariq Toukan <tariqt@mellanox.com> > > Acked-by: Saeed Mahameed <saeedm@mellanox.com> > > I don't think this is correct. max_tx tells you how many TX channels > there can be, you can't add that to combined. Correct calculations is: > > max_num_chans = max(max_combined, max(max_rx, max_tx)) > ...but the inner max should be min, right? Assuming we'd like to receive and send.
On 2019-06-12 23:23, Jakub Kicinski wrote: > On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: >> Currently, libbpf uses the number of combined channels as the maximum >> queue number. However, the kernel has a different limitation: >> >> - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). >> >> - ethtool_set_channels() checks for UMEMs in queues up to >> combined_count + max(rx_count, tx_count). >> >> libbpf shouldn't limit applications to a lower max queue number. Account >> for non-combined RX and TX channels when calculating the max queue >> number. Use the same formula that is used in ethtool. >> >> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> >> Reviewed-by: Tariq Toukan <tariqt@mellanox.com> >> Acked-by: Saeed Mahameed <saeedm@mellanox.com> > > I don't think this is correct. max_tx tells you how many TX channels > there can be, you can't add that to combined. Correct calculations is: > > max_num_chans = max(max_combined, max(max_rx, max_tx)) First of all, I'm aligning with the formula in the kernel, which is: curr.combined_count + max(curr.rx_count, curr.tx_count); (see net/core/ethtool.c, ethtool_set_channels()). The formula in libbpf should match it. Second, the existing drivers have either combined channels or separate rx and tx channels. So, for the first kind of drivers, max_tx doesn't tell how many TX channels there can be, it just says 0, and max_combined tells how many TX and RX channels are supported. As max_tx doesn't include max_combined (and vice versa), we should add them up. >> tools/lib/bpf/xsk.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c >> index bf15a80a37c2..86107857e1f0 100644 >> --- a/tools/lib/bpf/xsk.c >> +++ b/tools/lib/bpf/xsk.c >> @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) >> goto out; >> } >> >> - if (channels.max_combined == 0 || errno == EOPNOTSUPP) >> + ret = channels.max_combined + max(channels.max_rx, channels.max_tx); >> + >> + if (ret == 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; >> >> out: >> close(fd); >
On Thu, 13 Jun 2019 14:01:39 +0000 Maxim Mikityanskiy <maximmi@mellanox.com> wrote: > On 2019-06-12 23:23, Jakub Kicinski wrote: > > On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: > >> Currently, libbpf uses the number of combined channels as the maximum > >> queue number. However, the kernel has a different limitation: > >> > >> - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). > >> > >> - ethtool_set_channels() checks for UMEMs in queues up to > >> combined_count + max(rx_count, tx_count). > >> > >> libbpf shouldn't limit applications to a lower max queue number. Account > >> for non-combined RX and TX channels when calculating the max queue > >> number. Use the same formula that is used in ethtool. > >> > >> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > >> Reviewed-by: Tariq Toukan <tariqt@mellanox.com> > >> Acked-by: Saeed Mahameed <saeedm@mellanox.com> > > > > I don't think this is correct. max_tx tells you how many TX channels > > there can be, you can't add that to combined. Correct calculations is: > > > > max_num_chans = max(max_combined, max(max_rx, max_tx)) > > First of all, I'm aligning with the formula in the kernel, which is: > > curr.combined_count + max(curr.rx_count, curr.tx_count); > > (see net/core/ethtool.c, ethtool_set_channels()). > > The formula in libbpf should match it. > > Second, the existing drivers have either combined channels or separate > rx and tx channels. So, for the first kind of drivers, max_tx doesn't > tell how many TX channels there can be, it just says 0, and max_combined > tells how many TX and RX channels are supported. As max_tx doesn't > include max_combined (and vice versa), we should add them up. > > >> tools/lib/bpf/xsk.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > >> index bf15a80a37c2..86107857e1f0 100644 > >> --- a/tools/lib/bpf/xsk.c > >> +++ b/tools/lib/bpf/xsk.c > >> @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) > >> goto out; > >> } > >> > >> - if (channels.max_combined == 0 || errno == EOPNOTSUPP) > >> + ret = channels.max_combined + max(channels.max_rx, channels.max_tx); So in case of 32 HW queues you'd like to get 64 entries in xskmap? Do you still have a need for attaching the xsksocks to the RSS queues? I thought you want them to be separated. So if I'm reading this right, [0, 31] xskmap entries would be unused for the most of the time, no? > >> + > >> + if (ret == 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; > >> > >> out: > >> close(fd); > > >
On Thu, 13 Jun 2019 14:41:30 +0200, Björn Töpel wrote: > On Wed, 12 Jun 2019 at 22:24, Jakub Kicinski > <jakub.kicinski@netronome.com> wrote: > > > > On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: > > > Currently, libbpf uses the number of combined channels as the maximum > > > queue number. However, the kernel has a different limitation: > > > > > > - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). > > > > > > - ethtool_set_channels() checks for UMEMs in queues up to > > > combined_count + max(rx_count, tx_count). > > > > > > libbpf shouldn't limit applications to a lower max queue number. Account > > > for non-combined RX and TX channels when calculating the max queue > > > number. Use the same formula that is used in ethtool. > > > > > > Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > > > Reviewed-by: Tariq Toukan <tariqt@mellanox.com> > > > Acked-by: Saeed Mahameed <saeedm@mellanox.com> > > > > I don't think this is correct. max_tx tells you how many TX channels > > there can be, you can't add that to combined. Correct calculations is: > > > > max_num_chans = max(max_combined, max(max_rx, max_tx)) > > > > ...but the inner max should be min, right? > > Assuming we'd like to receive and send. That was my knee jerk reaction too, but I think this is only use to size the array (I could be wrong). In which case we need an index for unidirectional socks, too. Perhaps the helper could be named better if my understanding is correct :(
On Thu, 13 Jun 2019 14:01:39 +0000, Maxim Mikityanskiy wrote: > On 2019-06-12 23:23, Jakub Kicinski wrote: > > On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: > >> Currently, libbpf uses the number of combined channels as the maximum > >> queue number. However, the kernel has a different limitation: > >> > >> - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). > >> > >> - ethtool_set_channels() checks for UMEMs in queues up to > >> combined_count + max(rx_count, tx_count). > >> > >> libbpf shouldn't limit applications to a lower max queue number. Account > >> for non-combined RX and TX channels when calculating the max queue > >> number. Use the same formula that is used in ethtool. > >> > >> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > >> Reviewed-by: Tariq Toukan <tariqt@mellanox.com> > >> Acked-by: Saeed Mahameed <saeedm@mellanox.com> > > > > I don't think this is correct. max_tx tells you how many TX channels > > there can be, you can't add that to combined. Correct calculations is: > > > > max_num_chans = max(max_combined, max(max_rx, max_tx)) > > First of all, I'm aligning with the formula in the kernel, which is: > > curr.combined_count + max(curr.rx_count, curr.tx_count); > > (see net/core/ethtool.c, ethtool_set_channels()). curr != max. ethtool code you're pointing me to (and which I wrote) uses the current allocation, not the max values. > The formula in libbpf should match it. The formula should be based on understanding what we're doing, not copying some not-really-equivalent code from somewhere :) Combined is a basically a queue pair, RX is an RX ring with a dedicated IRQ, and TX is a TX ring with a dedicated IRQ. If driver supports both combined and single purpose interrupt vectors it will most likely set max_rx = num_hw_rx max_tx = num_hw_tx max_combined = min(rx, tx) Like with most ethtool APIs there are some variations to this. > Second, the existing drivers have either combined channels or separate > rx and tx channels. So, for the first kind of drivers, max_tx doesn't > tell how many TX channels there can be, it just says 0, and max_combined > tells how many TX and RX channels are supported. As max_tx doesn't > include max_combined (and vice versa), we should add them up. By existing drivers you mean Intel drivers which implement AF_XDP, and your driver? Both Intel and MLX drivers seem to only set max_combined. If you mean all drivers across the kernel, then I believe the best formula is what I gave you. > >> tools/lib/bpf/xsk.c | 6 +++--- > >> 1 file changed, 3 insertions(+), 3 deletions(-) > >> > >> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > >> index bf15a80a37c2..86107857e1f0 100644 > >> --- a/tools/lib/bpf/xsk.c > >> +++ b/tools/lib/bpf/xsk.c > >> @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) > >> goto out; > >> } > >> > >> - if (channels.max_combined == 0 || errno == EOPNOTSUPP) > >> + ret = channels.max_combined + max(channels.max_rx, channels.max_tx); > >> + > >> + if (ret == 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; > >> > >> out: > >> close(fd);
On 2019-06-13 21:09, Jakub Kicinski wrote: > On Thu, 13 Jun 2019 14:01:39 +0000, Maxim Mikityanskiy wrote: >> On 2019-06-12 23:23, Jakub Kicinski wrote: >>> On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: >>>> Currently, libbpf uses the number of combined channels as the maximum >>>> queue number. However, the kernel has a different limitation: >>>> >>>> - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). >>>> >>>> - ethtool_set_channels() checks for UMEMs in queues up to >>>> combined_count + max(rx_count, tx_count). >>>> >>>> libbpf shouldn't limit applications to a lower max queue number. Account >>>> for non-combined RX and TX channels when calculating the max queue >>>> number. Use the same formula that is used in ethtool. >>>> >>>> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> >>>> Reviewed-by: Tariq Toukan <tariqt@mellanox.com> >>>> Acked-by: Saeed Mahameed <saeedm@mellanox.com> >>> >>> I don't think this is correct. max_tx tells you how many TX channels >>> there can be, you can't add that to combined. Correct calculations is: >>> >>> max_num_chans = max(max_combined, max(max_rx, max_tx)) >> >> First of all, I'm aligning with the formula in the kernel, which is: >> >> curr.combined_count + max(curr.rx_count, curr.tx_count); >> >> (see net/core/ethtool.c, ethtool_set_channels()). > > curr != max. ethtool code you're pointing me to (and which I wrote) > uses the current allocation, not the max values. The ethtool code uses curr, because it wants to calculate the amount of queues currently in use. libbpf uses max, because it wants to calculate the maximum amount of queues that can be in use. That's the only difference, so the formula should be the same, and this calculation can be applied either to curr or to max. Imagine you have configured the NIC to have the maximum supported amount of channels. Then your formula in ethtool.c returns some value. Exactly the same value should also be returned from libbpf's xsk_get_max_queues(). It's achieved by applying your formula directly to max. >> The formula in libbpf should match it. > > The formula should be based on understanding what we're doing, > not copying some not-really-equivalent code from somewhere :) I have understanding of the code I write. > Combined is a basically a queue pair, RX is an RX ring with a dedicated > IRQ, and TX is a TX ring with a dedicated IRQ. If driver supports both > combined and single purpose interrupt vectors it will most likely set > > max_rx = num_hw_rx > max_tx = num_hw_tx > max_combined = min(rx, tx) OK, I got your example. The driver you are talking about won't support setting rx_count = max_rx, tx_count = max_tx and combined_count = max_combined simultaneously. However, xsk_get_max_queues has to return the maximum number of queues theoretically possible with this device, to create xsks_map of a sufficient size. Currently, it totally ignores devices without combined channels, so max_rx and max_tx have to be considered in the calculation. Next thing is that ethtool API doesn't really tell you whether the device can create up to max_rx RX channels, max_tx TX channels and max_combined combined channels simultaneously, or there are some additional limitations. Your example displays such a limitation, but it's not the only possible one, and this limitation is not even mandatory for all drivers. As ethtool doesn't expose the information about additional limitations imposed by the driver, and as it won't hurt if xsks_map will be bigger than necessary, my vision is that we shouldn't assume any limitations we are not sure about, so max_combined + max(max_rx, max_tx) is the right thing to do. > Like with most ethtool APIs there are some variations to this. > >> Second, the existing drivers have either combined channels or separate >> rx and tx channels. So, for the first kind of drivers, max_tx doesn't >> tell how many TX channels there can be, it just says 0, and max_combined >> tells how many TX and RX channels are supported. As max_tx doesn't >> include max_combined (and vice versa), we should add them up. > > By existing drivers you mean Intel drivers which implement AF_XDP, > and your driver? No, I meant all drivers, not only AF_XDP-enabled ones. I wasn't aware that some of them support the choice between a combined channel and a unidirectional channel, however, I still find my formula correct (see the explanation above). > Both Intel and MLX drivers seem to only set > max_combined. mlx4 doesn't support combined channels, but it's out of scope of this patchset. > If you mean all drivers across the kernel, then I believe the best > formula is what I gave you. > >>>> tools/lib/bpf/xsk.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c >>>> index bf15a80a37c2..86107857e1f0 100644 >>>> --- a/tools/lib/bpf/xsk.c >>>> +++ b/tools/lib/bpf/xsk.c >>>> @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) >>>> goto out; >>>> } >>>> >>>> - if (channels.max_combined == 0 || errno == EOPNOTSUPP) >>>> + ret = channels.max_combined + max(channels.max_rx, channels.max_tx); >>>> + >>>> + if (ret == 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; >>>> >>>> out: >>>> close(fd);
On 2019-06-13 17:45, Maciej Fijalkowski wrote: > On Thu, 13 Jun 2019 14:01:39 +0000 > Maxim Mikityanskiy <maximmi@mellanox.com> wrote: > >> On 2019-06-12 23:23, Jakub Kicinski wrote: >>> On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: >>>> Currently, libbpf uses the number of combined channels as the maximum >>>> queue number. However, the kernel has a different limitation: >>>> >>>> - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). >>>> >>>> - ethtool_set_channels() checks for UMEMs in queues up to >>>> combined_count + max(rx_count, tx_count). >>>> >>>> libbpf shouldn't limit applications to a lower max queue number. Account >>>> for non-combined RX and TX channels when calculating the max queue >>>> number. Use the same formula that is used in ethtool. >>>> >>>> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> >>>> Reviewed-by: Tariq Toukan <tariqt@mellanox.com> >>>> Acked-by: Saeed Mahameed <saeedm@mellanox.com> >>> >>> I don't think this is correct. max_tx tells you how many TX channels >>> there can be, you can't add that to combined. Correct calculations is: >>> >>> max_num_chans = max(max_combined, max(max_rx, max_tx)) >> >> First of all, I'm aligning with the formula in the kernel, which is: >> >> curr.combined_count + max(curr.rx_count, curr.tx_count); >> >> (see net/core/ethtool.c, ethtool_set_channels()). >> >> The formula in libbpf should match it. >> >> Second, the existing drivers have either combined channels or separate >> rx and tx channels. So, for the first kind of drivers, max_tx doesn't >> tell how many TX channels there can be, it just says 0, and max_combined >> tells how many TX and RX channels are supported. As max_tx doesn't >> include max_combined (and vice versa), we should add them up. >> >>>> tools/lib/bpf/xsk.c | 6 +++--- >>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c >>>> index bf15a80a37c2..86107857e1f0 100644 >>>> --- a/tools/lib/bpf/xsk.c >>>> +++ b/tools/lib/bpf/xsk.c >>>> @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) >>>> goto out; >>>> } >>>> >>>> - if (channels.max_combined == 0 || errno == EOPNOTSUPP) >>>> + ret = channels.max_combined + max(channels.max_rx, channels.max_tx); > > So in case of 32 HW queues you'd like to get 64 entries in xskmap? "32 HW queues" is not quite correct. It will be 32 combined channels, each with one regular RX queue and one XSK RX queue (regular RX queues are part of RSS). In this case, I'll have 64 XSKMAP entries. > Do you still > have a need for attaching the xsksocks to the RSS queues? You can attach an XSK to a regular RX queue, but not in zero-copy mode. The intended use is, of course, to attach XSKs to XSK RX queues in zero-copy mode. > I thought you want > them to be separated. So if I'm reading this right, [0, 31] xskmap entries > would be unused for the most of the time, no? This is correct, but these entries are still needed if one decides to run compatibility mode without zero-copy on queues 0..31. > >>>> + >>>> + if (ret == 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; >>>> >>>> out: >>>> close(fd); >>> >> >
On Fri, 14 Jun 2019 13:25:24 +0000 Maxim Mikityanskiy <maximmi@mellanox.com> wrote: > On 2019-06-13 17:45, Maciej Fijalkowski wrote: > > On Thu, 13 Jun 2019 14:01:39 +0000 > > Maxim Mikityanskiy <maximmi@mellanox.com> wrote: > > > >> On 2019-06-12 23:23, Jakub Kicinski wrote: > >>> On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: > >>>> Currently, libbpf uses the number of combined channels as the maximum > >>>> queue number. However, the kernel has a different limitation: > >>>> > >>>> - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). > >>>> > >>>> - ethtool_set_channels() checks for UMEMs in queues up to > >>>> combined_count + max(rx_count, tx_count). > >>>> > >>>> libbpf shouldn't limit applications to a lower max queue number. Account > >>>> for non-combined RX and TX channels when calculating the max queue > >>>> number. Use the same formula that is used in ethtool. > >>>> > >>>> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> > >>>> Reviewed-by: Tariq Toukan <tariqt@mellanox.com> > >>>> Acked-by: Saeed Mahameed <saeedm@mellanox.com> > >>> > >>> I don't think this is correct. max_tx tells you how many TX channels > >>> there can be, you can't add that to combined. Correct calculations is: > >>> > >>> max_num_chans = max(max_combined, max(max_rx, max_tx)) > >> > >> First of all, I'm aligning with the formula in the kernel, which is: > >> > >> curr.combined_count + max(curr.rx_count, curr.tx_count); > >> > >> (see net/core/ethtool.c, ethtool_set_channels()). > >> > >> The formula in libbpf should match it. > >> > >> Second, the existing drivers have either combined channels or separate > >> rx and tx channels. So, for the first kind of drivers, max_tx doesn't > >> tell how many TX channels there can be, it just says 0, and max_combined > >> tells how many TX and RX channels are supported. As max_tx doesn't > >> include max_combined (and vice versa), we should add them up. > >> > >>>> tools/lib/bpf/xsk.c | 6 +++--- > >>>> 1 file changed, 3 insertions(+), 3 deletions(-) > >>>> > >>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c > >>>> index bf15a80a37c2..86107857e1f0 100644 > >>>> --- a/tools/lib/bpf/xsk.c > >>>> +++ b/tools/lib/bpf/xsk.c > >>>> @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) > >>>> goto out; > >>>> } > >>>> > >>>> - if (channels.max_combined == 0 || errno == EOPNOTSUPP) > >>>> + ret = channels.max_combined + max(channels.max_rx, channels.max_tx); > > > > So in case of 32 HW queues you'd like to get 64 entries in xskmap? > > "32 HW queues" is not quite correct. It will be 32 combined channels, > each with one regular RX queue and one XSK RX queue (regular RX queues > are part of RSS). In this case, I'll have 64 XSKMAP entries. > > > Do you still > > have a need for attaching the xsksocks to the RSS queues? > > You can attach an XSK to a regular RX queue, but not in zero-copy mode. > The intended use is, of course, to attach XSKs to XSK RX queues in > zero-copy mode. > > > I thought you want > > them to be separated. So if I'm reading this right, [0, 31] xskmap entries > > would be unused for the most of the time, no? > > This is correct, but these entries are still needed if one decides to > run compatibility mode without zero-copy on queues 0..31. Why would I want to run AF_XDP without ZC? The main reason for having AF_XDP support in drivers is the zero copy, right? Besides that, are you educating the user in some way which queue ids should be used so there's ZC in picture? If that was already asked/answered, then sorry about that. > > > > >>>> + > >>>> + if (ret == 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; > >>>> > >>>> out: > >>>> close(fd); > >>> > >> > > >
On 2019-06-14 19:15, Maciej Fijalkowski wrote: > Why would I want to run AF_XDP without ZC? The main reason for having AF_XDP > support in drivers is the zero copy, right? In general I agree with you on this point. Short-term, I see copy-mode useful for API adoption reasons (as XDP_SKB), so from that perspecitve it's important. Longer term I'd like to explore AF_XDP as a faster AF_PACKET for pcap functionality. Björn
On Fri, 14 Jun 2019 13:25:05 +0000, Maxim Mikityanskiy wrote: > Imagine you have configured the NIC to have the maximum supported amount > of channels. Then your formula in ethtool.c returns some value. Exactly > the same value should also be returned from libbpf's > xsk_get_max_queues(). It's achieved by applying your formula directly to > max. I'm just trying to limit people inventing their own interpretations of this API. Broadcom for instance does something dumb with current counts I think they return curr.combined = curr.rx, even though there is only curr.combined rings... You will over allocate space for all NICs with return both combined and non-combined counts. But that's not a huge deal, not worth arguing about. Moving on..
On 2019-06-14 20:15, Maciej Fijalkowski wrote: > On Fri, 14 Jun 2019 13:25:24 +0000 > Maxim Mikityanskiy <maximmi@mellanox.com> wrote: > >> On 2019-06-13 17:45, Maciej Fijalkowski wrote: >>> On Thu, 13 Jun 2019 14:01:39 +0000 >>> Maxim Mikityanskiy <maximmi@mellanox.com> wrote: >>> >>>> On 2019-06-12 23:23, Jakub Kicinski wrote: >>>>> On Wed, 12 Jun 2019 15:56:48 +0000, Maxim Mikityanskiy wrote: >>>>>> Currently, libbpf uses the number of combined channels as the maximum >>>>>> queue number. However, the kernel has a different limitation: >>>>>> >>>>>> - xdp_reg_umem_at_qid() allows up to max(RX queues, TX queues). >>>>>> >>>>>> - ethtool_set_channels() checks for UMEMs in queues up to >>>>>> combined_count + max(rx_count, tx_count). >>>>>> >>>>>> libbpf shouldn't limit applications to a lower max queue number. Account >>>>>> for non-combined RX and TX channels when calculating the max queue >>>>>> number. Use the same formula that is used in ethtool. >>>>>> >>>>>> Signed-off-by: Maxim Mikityanskiy <maximmi@mellanox.com> >>>>>> Reviewed-by: Tariq Toukan <tariqt@mellanox.com> >>>>>> Acked-by: Saeed Mahameed <saeedm@mellanox.com> >>>>> >>>>> I don't think this is correct. max_tx tells you how many TX channels >>>>> there can be, you can't add that to combined. Correct calculations is: >>>>> >>>>> max_num_chans = max(max_combined, max(max_rx, max_tx)) >>>> >>>> First of all, I'm aligning with the formula in the kernel, which is: >>>> >>>> curr.combined_count + max(curr.rx_count, curr.tx_count); >>>> >>>> (see net/core/ethtool.c, ethtool_set_channels()). >>>> >>>> The formula in libbpf should match it. >>>> >>>> Second, the existing drivers have either combined channels or separate >>>> rx and tx channels. So, for the first kind of drivers, max_tx doesn't >>>> tell how many TX channels there can be, it just says 0, and max_combined >>>> tells how many TX and RX channels are supported. As max_tx doesn't >>>> include max_combined (and vice versa), we should add them up. >>>> >>>>>> tools/lib/bpf/xsk.c | 6 +++--- >>>>>> 1 file changed, 3 insertions(+), 3 deletions(-) >>>>>> >>>>>> diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c >>>>>> index bf15a80a37c2..86107857e1f0 100644 >>>>>> --- a/tools/lib/bpf/xsk.c >>>>>> +++ b/tools/lib/bpf/xsk.c >>>>>> @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) >>>>>> goto out; >>>>>> } >>>>>> >>>>>> - if (channels.max_combined == 0 || errno == EOPNOTSUPP) >>>>>> + ret = channels.max_combined + max(channels.max_rx, channels.max_tx); >>> >>> So in case of 32 HW queues you'd like to get 64 entries in xskmap? >> >> "32 HW queues" is not quite correct. It will be 32 combined channels, >> each with one regular RX queue and one XSK RX queue (regular RX queues >> are part of RSS). In this case, I'll have 64 XSKMAP entries. >> >>> Do you still >>> have a need for attaching the xsksocks to the RSS queues? >> >> You can attach an XSK to a regular RX queue, but not in zero-copy mode. >> The intended use is, of course, to attach XSKs to XSK RX queues in >> zero-copy mode. >> >>> I thought you want >>> them to be separated. So if I'm reading this right, [0, 31] xskmap entries >>> would be unused for the most of the time, no? >> >> This is correct, but these entries are still needed if one decides to >> run compatibility mode without zero-copy on queues 0..31. > > Why would I want to run AF_XDP without ZC? The main reason for having AF_XDP > support in drivers is the zero copy, right? Yes, AF_XDP is intended to be used with zero copy when the driver implements it. I'm not breaking the compatibility mode if I can keep it supported. > Besides that, are you educating the user in some way which queue ids should be > used so there's ZC in picture? If that was already asked/answered, then sorry > about that. The details about queue IDs are in the commit message for the final patch. >> >>> >>>>>> + >>>>>> + if (ret == 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; >>>>>> >>>>>> out: >>>>>> close(fd); >>>>> >>>> >>> >> >
diff --git a/tools/lib/bpf/xsk.c b/tools/lib/bpf/xsk.c index bf15a80a37c2..86107857e1f0 100644 --- a/tools/lib/bpf/xsk.c +++ b/tools/lib/bpf/xsk.c @@ -334,13 +334,13 @@ static int xsk_get_max_queues(struct xsk_socket *xsk) goto out; } - if (channels.max_combined == 0 || errno == EOPNOTSUPP) + ret = channels.max_combined + max(channels.max_rx, channels.max_tx); + + if (ret == 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; out: close(fd);