Re: [PATCH bpf-next v4 07/17] libbpf: Support drivers with non-combined channels

[Maciej Fijalkowski <maciejromanfijalkowski@gmail.com>]

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