Re: [PATCH V2 for-next 7/7] IB/core: Change completion channel to use the reworked objects schema

[Matan Barak <matanb-LDSdmyG8hGV8YrgS2mwiifqBs+8SCbDb@public.gmane.org>]

On Thu, Apr 6, 2017 at 7:42 PM, Jason Gunthorpe
<jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org> wrote:
> On Thu, Apr 06, 2017 at 05:10:51PM +0300, Matan Barak wrote:
>> Effectively this kref is used to capture the dependencies between
>> various objects.
>
> No, that is what the usecnt is for, the kref is just to make sure we
> can still *access* the usecnt without derefing freed memory.
>

There are three usecnt(s) actually. One is intended to determine how
many readers
(or if there's a writer) currently accessing this uobject. Zero here
means no one
accessing this uobject and we could lock it for either read/write.
This is uverbs only.

Another one is to protect the lifetime of the uobject.

The last usecnt is intended to capture dependencies, meaning when use_cnt > 1
we can't free that object as there are dependent objects. Nowadays,
this use_cnt is
in the verbs layer and semi-protects (when things are doing serially)
kernel layer as well.

>> This won't solve the "inc_uobject_kref; destroy_uobject" schema, as
>> in this case we really want to destroy the object, but we want to
>> keep the uobject alive in order to get information about the
>> object's state at destruction.
>
> Right, this is why I said you need a destory_uobject varient that
> retains the kref with the caller.
>

So currently we have three states in uverbs (when an object is destroyed):
(a) uobject is alive, object is alive
(b) uobject is alive, object isn't
(c) both are destroyed

You need to somehow capture the second state. This is used in order to
write the updated last
information when an object is destroyed (so no new events could be
generated on this object).

>> If we use kref on the uobject only, we lose the kref count in ULPs.
>> Moreover, we'll need to somehow
>
> No, we can't really do that obviously..
>
> I'd rather see the usecnt hoisted entirely into the uverbs layer where
> it can work sanely with proper locking and reserve a second
> for-debugging-only WARN_ON scheme in the verbs layer that checks
> cleanup ordering for the kapi.
 >
> The kapi returning error codes on destroy is insane, I cleaned up PD
> at one point, but they all need fixing...
>

The current solution in the verbs layer is really half baked. It works
well as long
as you don't try to use an object while destroying it. If you do such
a non-sense action,
you should do that proper locking and checking yourself.

So, if we move that to a uobject, we need three reference counts:
(a) read_write_count
(b) dependencies_count (or object_refcount)
(c) uobject_refcount

Anyway, this is really unrelated to the ABI work. In any solution we
choose right now, we could
move that reference count from objects to uobjects later on. The
purpose here isn't doing a cleanup.
The locking change was done as the new ABI relies on that new behavior
to be dead-lock free, but the
refcount change change could be delayed without risking anything.

> Jason

Matan
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html