[bpf-next,v4,07/17] libbpf: Support drivers with non-combined channels
diff mbox series

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
Related show

Commit Message

Maxim Mikityanskiy June 12, 2019, 3:56 p.m. UTC
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>
---
 tools/lib/bpf/xsk.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

Comments

Jakub Kicinski June 12, 2019, 8:23 p.m. UTC | #1
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);
Björn Töpel June 13, 2019, 12:41 p.m. UTC | #2
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.
Maxim Mikityanskiy June 13, 2019, 2:01 p.m. UTC | #3
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);
>
Maciej Fijalkowski June 13, 2019, 2:45 p.m. UTC | #4
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);  
> >   
>
Jakub Kicinski June 13, 2019, 5:34 p.m. UTC | #5
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 :(
Jakub Kicinski June 13, 2019, 6:09 p.m. UTC | #6
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);
Maxim Mikityanskiy June 14, 2019, 1:25 p.m. UTC | #7
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);
Maxim Mikityanskiy June 14, 2019, 1:25 p.m. UTC | #8
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);
>>>    
>>
>
Maciej Fijalkowski June 14, 2019, 5:15 p.m. UTC | #9
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);  
> >>>      
> >>  
> >   
>
Björn Töpel June 14, 2019, 7:50 p.m. UTC | #10
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
Jakub Kicinski June 15, 2019, 2:12 a.m. UTC | #11
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..
Maxim Mikityanskiy June 18, 2019, noon UTC | #12
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);
>>>>>       
>>>>   
>>>    
>>
>

Patch
diff mbox series

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);