Lockless behavior for CQs in userspace

Lockless behavior for CQs in userspace

[Andrew Boyer]

Hello Leon,
I understand that we are not to create new providers that use environment variables to control locking behavior. The ‘new’ way to do it is to use thread domains and parent domains.

However, even mlx5 still uses the env var exclusively to control lockless behavior for CQs. Do you have anything in mind for how to extend thread_domains or some other part of the API to cover the CQ case?

Thank you,
Andrew

Re: Lockless behavior for CQs in userspace

[Leon Romanovsky]

On Tue, Mar 17, 2020 at 10:45:08AM -0400, Andrew Boyer wrote:
> Hello Leon,
> I understand that we are not to create new providers that use environment variables to control locking behavior. The ‘new’ way to do it is to use thread domains and parent domains.
>
> However, even mlx5 still uses the env var exclusively to control lockless behavior for CQs. Do you have anything in mind for how to extend thread_domains or some other part of the API to cover the CQ case?

Which parameter did you have in mind?
I would say that all those parameters are coming from pre-rdma-core era.

Doesn't this commit do what you are asking?
https://github.com/linux-rdma/rdma-core/commit/0dbde57c59d2983e848c3dbd9ae93eaf8e7b9405

Thanks

>
> Thank you,
> Andrew
>

Re: Lockless behavior for CQs in userspace

[Andrew Boyer]

> On Mar 17, 2020, at 11:00 AM, Leon Romanovsky <leonro@mellanox.com> wrote:
> 
> On Tue, Mar 17, 2020 at 10:45:08AM -0400, Andrew Boyer wrote:
>> Hello Leon,
>> I understand that we are not to create new providers that use environment variables to control locking behavior. The ‘new’ way to do it is to use thread domains and parent domains.
>> 
>> However, even mlx5 still uses the env var exclusively to control lockless behavior for CQs. Do you have anything in mind for how to extend thread_domains or some other part of the API to cover the CQ case?
> 
> Which parameter did you have in mind?
> I would say that all those parameters are coming from pre-rdma-core era.
> 
> Doesn't this commit do what you are asking?
> https://github.com/linux-rdma/rdma-core/commit/0dbde57c59d2983e848c3dbd9ae93eaf8e7b9405
> 
> Thanks
> 
>> 
>> Thank you,
>> Andrew
>> 

You are right - I got thrown off by this:

> 	if (mlx5_spinlock_init(&cq->lock, !mlx5_single_threaded))
>                 goto err;

If IBV_CREATE_CQ_ATTR_SINGLE_THREADED is set, it passes an argument to the polling function to skip the lock calls entirely. So it doesn’t matter that they are still enabled internally.

-Andrew

Re: Lockless behavior for CQs in userspace

[Jason Gunthorpe]

On Tue, Mar 17, 2020 at 11:10:15AM -0400, Andrew Boyer wrote:
> 
> > On Mar 17, 2020, at 11:00 AM, Leon Romanovsky <leonro@mellanox.com> wrote:
> > 
> > On Tue, Mar 17, 2020 at 10:45:08AM -0400, Andrew Boyer wrote:
> >> Hello Leon,
> >> I understand that we are not to create new providers that use environment variables to control locking behavior. The ‘new’ way to do it is to use thread domains and parent domains.
> >> 
> >> However, even mlx5 still uses the env var exclusively to control lockless behavior for CQs. Do you have anything in mind for how to extend thread_domains or some other part of the API to cover the CQ case?
> > 
> > Which parameter did you have in mind?
> > I would say that all those parameters are coming from pre-rdma-core era.
> > 
> > Doesn't this commit do what you are asking?
> > https://github.com/linux-rdma/rdma-core/commit/0dbde57c59d2983e848c3dbd9ae93eaf8e7b9405
> > 
> > Thanks
> > 
> >> 
> >> Thank you,
> >> Andrew
> >> 
> 
> You are right - I got thrown off by this:
> 
> > 	if (mlx5_spinlock_init(&cq->lock, !mlx5_single_threaded))
> >                 goto err;
> 
> If IBV_CREATE_CQ_ATTR_SINGLE_THREADED is set, it passes an argument
> to the polling function to skip the lock calls entirely. So it
> doesn’t matter that they are still enabled internally.

Hmm, a thread domain should probably force
IBV_CREATE_CQ_ATTR_SINGLE_THREADED during greation with the new API..

Yishai?

Jason

Re: Lockless behavior for CQs in userspace

[Yishai Hadas]

On 3/17/2020 9:51 PM, Jason Gunthorpe wrote:
> On Tue, Mar 17, 2020 at 11:10:15AM -0400, Andrew Boyer wrote:
>>
>>> On Mar 17, 2020, at 11:00 AM, Leon Romanovsky <leonro@mellanox.com> wrote:
>>>
>>> On Tue, Mar 17, 2020 at 10:45:08AM -0400, Andrew Boyer wrote:
>>>> Hello Leon,
>>>> I understand that we are not to create new providers that use environment variables to control locking behavior. The ‘new’ way to do it is to use thread domains and parent domains.
>>>>
>>>> However, even mlx5 still uses the env var exclusively to control lockless behavior for CQs. Do you have anything in mind for how to extend thread_domains or some other part of the API to cover the CQ case?
>>>
>>> Which parameter did you have in mind?
>>> I would say that all those parameters are coming from pre-rdma-core era.
>>>
>>> Doesn't this commit do what you are asking?
>>> https://github.com/linux-rdma/rdma-core/commit/0dbde57c59d2983e848c3dbd9ae93eaf8e7b9405
>>>
>>> Thanks
>>>
>>>>
>>>> Thank you,
>>>> Andrew
>>>>
>>
>> You are right - I got thrown off by this:
>>
>>> 	if (mlx5_spinlock_init(&cq->lock, !mlx5_single_threaded))
>>>                  goto err;
>>
>> If IBV_CREATE_CQ_ATTR_SINGLE_THREADED is set, it passes an argument
>> to the polling function to skip the lock calls entirely. So it
>> doesn’t matter that they are still enabled internally.
> 
> Hmm, a thread domain should probably force
> IBV_CREATE_CQ_ATTR_SINGLE_THREADED during greation with the new API..
> 
> Yishai?
> 

We can really consider extending the functionality of a parent domain 
that was just added for CQ in case it holds a thread domain for this 
purpose. However, current code enforces creating a dedicated BF as part 
of thread domain unless we extend ibv_alloc_td() to ask only for marking 
the single thread functionality.
The existing alternative is or to use the legacy ENV that mentioned 
above or to use the ibv_cq_ex functionality which upon its creation gets 
an explicit flag for single threaded one (i.e. 
IBV_CREATE_CQ_ATTR_SINGLE_THREADED).

Yishai

Re: Lockless behavior for CQs in userspace

[Leon Romanovsky]

On Wed, Mar 18, 2020 at 09:52:27AM +0200, Yishai Hadas wrote:
> On 3/17/2020 9:51 PM, Jason Gunthorpe wrote:
> > On Tue, Mar 17, 2020 at 11:10:15AM -0400, Andrew Boyer wrote:
> > >
> > > > On Mar 17, 2020, at 11:00 AM, Leon Romanovsky <leonro@mellanox.com> wrote:
> > > >
> > > > On Tue, Mar 17, 2020 at 10:45:08AM -0400, Andrew Boyer wrote:
> > > > > Hello Leon,
> > > > > I understand that we are not to create new providers that use environment variables to control locking behavior. The ‘new’ way to do it is to use thread domains and parent domains.
> > > > >
> > > > > However, even mlx5 still uses the env var exclusively to control lockless behavior for CQs. Do you have anything in mind for how to extend thread_domains or some other part of the API to cover the CQ case?
> > > >
> > > > Which parameter did you have in mind?
> > > > I would say that all those parameters are coming from pre-rdma-core era.
> > > >
> > > > Doesn't this commit do what you are asking?
> > > > https://github.com/linux-rdma/rdma-core/commit/0dbde57c59d2983e848c3dbd9ae93eaf8e7b9405
> > > >
> > > > Thanks
> > > >
> > > > >
> > > > > Thank you,
> > > > > Andrew
> > > > >
> > >
> > > You are right - I got thrown off by this:
> > >
> > > > 	if (mlx5_spinlock_init(&cq->lock, !mlx5_single_threaded))
> > > >                  goto err;
> > >
> > > If IBV_CREATE_CQ_ATTR_SINGLE_THREADED is set, it passes an argument
> > > to the polling function to skip the lock calls entirely. So it
> > > doesn’t matter that they are still enabled internally.
> >
> > Hmm, a thread domain should probably force
> > IBV_CREATE_CQ_ATTR_SINGLE_THREADED during greation with the new API..
> >
> > Yishai?
> >
>
> We can really consider extending the functionality of a parent domain that
> was just added for CQ in case it holds a thread domain for this purpose.
> However, current code enforces creating a dedicated BF as part of thread
> domain unless we extend ibv_alloc_td() to ask only for marking the single
> thread functionality.
> The existing alternative is or to use the legacy ENV that mentioned above or
> to use the ibv_cq_ex functionality which upon its creation gets an explicit
> flag for single threaded one (i.e. IBV_CREATE_CQ_ATTR_SINGLE_THREADED).

"dedicated BF", isn't this Mellanox internal implementation?

Thanks

>
> Yishai
>
>

Re: Lockless behavior for CQs in userspace

[Yishai Hadas]

On 3/18/2020 10:03 AM, Leon Romanovsky wrote:
> On Wed, Mar 18, 2020 at 09:52:27AM +0200, Yishai Hadas wrote:
>> On 3/17/2020 9:51 PM, Jason Gunthorpe wrote:
>>> On Tue, Mar 17, 2020 at 11:10:15AM -0400, Andrew Boyer wrote:
>>>>
>>>>> On Mar 17, 2020, at 11:00 AM, Leon Romanovsky <leonro@mellanox.com> wrote:
>>>>>
>>>>> On Tue, Mar 17, 2020 at 10:45:08AM -0400, Andrew Boyer wrote:
>>>>>> Hello Leon,
>>>>>> I understand that we are not to create new providers that use environment variables to control locking behavior. The ‘new’ way to do it is to use thread domains and parent domains.
>>>>>>
>>>>>> However, even mlx5 still uses the env var exclusively to control lockless behavior for CQs. Do you have anything in mind for how to extend thread_domains or some other part of the API to cover the CQ case?
>>>>>
>>>>> Which parameter did you have in mind?
>>>>> I would say that all those parameters are coming from pre-rdma-core era.
>>>>>
>>>>> Doesn't this commit do what you are asking?
>>>>> https://github.com/linux-rdma/rdma-core/commit/0dbde57c59d2983e848c3dbd9ae93eaf8e7b9405
>>>>>
>>>>> Thanks
>>>>>
>>>>>>
>>>>>> Thank you,
>>>>>> Andrew
>>>>>>
>>>>
>>>> You are right - I got thrown off by this:
>>>>
>>>>> 	if (mlx5_spinlock_init(&cq->lock, !mlx5_single_threaded))
>>>>>                   goto err;
>>>>
>>>> If IBV_CREATE_CQ_ATTR_SINGLE_THREADED is set, it passes an argument
>>>> to the polling function to skip the lock calls entirely. So it
>>>> doesn’t matter that they are still enabled internally.
>>>
>>> Hmm, a thread domain should probably force
>>> IBV_CREATE_CQ_ATTR_SINGLE_THREADED during greation with the new API..
>>>
>>> Yishai?
>>>
>>
>> We can really consider extending the functionality of a parent domain that
>> was just added for CQ in case it holds a thread domain for this purpose.
>> However, current code enforces creating a dedicated BF as part of thread
>> domain unless we extend ibv_alloc_td() to ask only for marking the single
>> thread functionality.
>> The existing alternative is or to use the legacy ENV that mentioned above or
>> to use the ibv_cq_ex functionality which upon its creation gets an explicit
>> flag for single threaded one (i.e. IBV_CREATE_CQ_ATTR_SINGLE_THREADED).
> 
> "dedicated BF", isn't this Mellanox internal implementation?
> 

Yes, however from API point of view we can consider extending 
ibv_alloc_td() to ask for this specific usage that may prevent this 
resource allocation, unless we are fine with having it always which may 
give a benefit also for a QP that will use it down the road.

As pointed above, there are already few alternatives that may be used in 
the meantime.

Yishai

Re: Lockless behavior for CQs in userspace

[Jason Gunthorpe]

On Wed, Mar 18, 2020 at 09:52:27AM +0200, Yishai Hadas wrote:

> We can really consider extending the functionality of a parent domain that
> was just added for CQ in case it holds a thread domain for this purpose.
> However, current code enforces creating a dedicated BF as part of thread
> domain unless we extend ibv_alloc_td() to ask only for marking the single
> thread functionality.

Doesn't the CQ need a BF too?

In any event, I don't think it matters, any real application is likely
to use the thread domain with a QP as well so the BF will not be
wasted

> The existing alternative is or to use the legacy ENV that mentioned above or
> to use the ibv_cq_ex functionality which upon its creation gets an explicit
> flag for single threaded one (i.e. IBV_CREATE_CQ_ATTR_SINGLE_THREADED).

An apps that don't want the BF can always use
IBV_CREATE_CQ_ATTR_SINGLE_THREADED

I think consistency says that if a thread domain is passed into CQ it
should trigger IBV_CREATE_CQ_ATTR_SINGLE_THREADED

Jason

Re: Lockless behavior for CQs in userspace

[Yishai Hadas]

On 3/18/2020 1:43 PM, Jason Gunthorpe wrote:
> On Wed, Mar 18, 2020 at 09:52:27AM +0200, Yishai Hadas wrote:
> 
>> We can really consider extending the functionality of a parent domain that
>> was just added for CQ in case it holds a thread domain for this purpose.
>> However, current code enforces creating a dedicated BF as part of thread
>> domain unless we extend ibv_alloc_td() to ask only for marking the single
>> thread functionality.
> 
> Doesn't the CQ need a BF too?
> 

Not really, it uses a per-allocated device one for writing 8 bytes for 
doorbell, applicable only in the arm/events mode.

> In any event, I don't think it matters, any real application is likely
> to use the thread domain with a QP as well so the BF will not be
> wasted
> 

Agree.

>> The existing alternative is or to use the legacy ENV that mentioned above or
>> to use the ibv_cq_ex functionality which upon its creation gets an explicit
>> flag for single threaded one (i.e. IBV_CREATE_CQ_ATTR_SINGLE_THREADED).
> 
> An apps that don't want the BF can always use
> IBV_CREATE_CQ_ATTR_SINGLE_THREADED
>

This requires moving to the new polling mechanism where this option is 
really applicable (see a3a31e8dc6a3fbf577dd8b12742d0c70cf63bd29).

> I think consistency says that if a thread domain is passed into CQ it
> should trigger IBV_CREATE_CQ_ATTR_SINGLE_THREADED
> 

I would say, as of turning the general ENV of MLX5_SINGLE_THREADED but 
only for this single CQ to support the legacy polling mode, the new one 
has its own flag/flow in any case.

Yishai

Re: Lockless behavior for CQs in userspace

[Jason Gunthorpe]

On Wed, Mar 18, 2020 at 02:36:35PM +0200, Yishai Hadas wrote:
> On 3/18/2020 1:43 PM, Jason Gunthorpe wrote:
> > On Wed, Mar 18, 2020 at 09:52:27AM +0200, Yishai Hadas wrote:
> > 
> > > We can really consider extending the functionality of a parent domain that
> > > was just added for CQ in case it holds a thread domain for this purpose.
> > > However, current code enforces creating a dedicated BF as part of thread
> > > domain unless we extend ibv_alloc_td() to ask only for marking the single
> > > thread functionality.
> > 
> > Doesn't the CQ need a BF too?
> 
> Not really, it uses a per-allocated device one for writing 8 bytes for
> doorbell, applicable only in the arm/events mode.

But this also wastes a BF, if the app is only running a single
lockless TD then the CQ could use the BF from the TD, shared with the
QP and not allocate another one

> > > The existing alternative is or to use the legacy ENV that mentioned above or
> > > to use the ibv_cq_ex functionality which upon its creation gets an explicit
> > > flag for single threaded one (i.e. IBV_CREATE_CQ_ATTR_SINGLE_THREADED).
> > 
> > An apps that don't want the BF can always use
> > IBV_CREATE_CQ_ATTR_SINGLE_THREADED
> > 
> 
> This requires moving to the new polling mechanism where this option is
> really applicable (see a3a31e8dc6a3fbf577dd8b12742d0c70cf63bd29).

cq_ex is pollable the normal way if converted with ibv_cq_ex_to_cq()

It is also a bug if the old env driven lock elimination doesn't trigger
for normal CQ poll if IBV_CREATE_CQ_ATTR_SINGLE_THREADED is specified.

Jason