Re: [PATCH for-next 3/7] IB/core: Add support for fd objects

[Jason Gunthorpe <jgunthorpe-ePGOBjL8dl3ta4EC/59zMFaTQe2KTcn/@public.gmane.org>]

On Tue, Jan 17, 2017 at 07:24:59PM +0200, Matan Barak wrote:
> >> +void uverbs_cleanup_fd(void *private_data)
> >> +{
> >> +     struct ib_uobject *uobject = uverbs_priv_fd_to_uobject(private_data);
> >> +
> >> +     kfree(uobject);
> >> +}
> >
> > Woah, this is not OK at this point in the series. There is really too
> > much stuff bundled into patch 7 to make proper sense of this.
> >
> 
> This is a standard structure of a series - you first build up the
> infrastructure and then use it to change everything.

Well, again, no, this is not normal. At this point in the series the
lifetime model for uboject is totally screwed up between the new/old
code. That is not OK

Do not get confused with how you write *new code* vs how you
*transform* old code. This is the latter and I expect each patch in
the series to globally follow or change the overal invarients. Do not
introduce two incompatible schemes.

> worried that embedding the actual changes with the infrastructural
> changes will require massive amounts of testing to verify it's
> bisect-able.

If each patch makes internal self consisent sense and compiles I'm
happy enough... Not asking that every patch be exhaustively tested.

As it stands this series is pretty useless for bisection because
everything hinges on the final patch, so having at least the ideas
properly broken up is a win from that standpoint.

> > Do not drop the kref out of ib_uobject, that should be the master
> > kref, drop the kref out of ib_uverbs_event_file instead.
> 
> I didn't really follow, could you please clarify?

In patch 7 you delete the kref from ib_uobject, but keep a kref in
ib_uverbs_event_file.

Instead you should keep the kref in ib_uobject, make it work properly,
and discard the kref in ib_uverbs_event_file when you add in
ib_uobject as a member.

The uobject kref semantics around the IDR should be pretty simple: The
IDR holds a kref on all the members of the IDR. get on add, put on
remove.

The per-uobject rwlock prevents removing the object from the IDR so
anything operating within the read side of the rwlock does not need to
kref get. Only when the uobject is transfered outside that rwlock is a
get required (eg for fops private_data)

Not mangling the kref model of uobject should make the two APIs more
compatible.

> It can't be moved to finalize, as fdallocate could fail and we assume nothing
> in the finalize stage could fail. However, I'll zero this value.

Okay, sure, -1 or something.

Jason
--
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