Re: [PATCH RFC 0/9] A rendezvous module

From: Jason Gunthorpe <jgg@nvidia.com>
To: "Hefty, Sean" <sean.hefty@intel.com>
Cc: Dennis Dalessandro <dennis.dalessandro@cornelisnetworks.com>,
	Leon Romanovsky <leon@kernel.org>,
	"Rimmer, Todd" <todd.rimmer@intel.com>,
	"Wan, Kaike" <kaike.wan@intel.com>,
	"dledford@redhat.com" <dledford@redhat.com>,
	"linux-rdma@vger.kernel.org" <linux-rdma@vger.kernel.org>
Subject: Re: [PATCH RFC 0/9] A rendezvous module
Date: Tue, 23 Mar 2021 19:56:38 -0300	[thread overview]
Message-ID: <20210323225638.GP2356281@nvidia.com> (raw)
In-Reply-To: <DM6PR11MB4609382524AE6EA1EAFB15B29E659@DM6PR11MB4609.namprd11.prod.outlook.com>

On Mon, Mar 22, 2021 at 05:31:07PM +0000, Hefty, Sean wrote:
> > > To be fair it is sent as RFC. So to me that says they know it's a ways off
> > > from being ready to be included and are starting the process early.
> > 
> > I'm not sure why it is RFC. It sounds like it is much more than this
> > if someone has already made a version with links to the NVIDIA GPU
> > driver and has reached a point where they care about ABI stablility?
> 
> I can take some blame here.  A couple of us were asked to look at
> this module.  Because the functionality is intended to be device
> agnostic, we assumed there would be more scrutiny, and we weren't
> sure how acceptable some aspects would be (e.g. mr cache, ib cm data
> format).  Rather than debate this internally for months, rework the
> code, and still miss, we asked Kaike to post an RFC to get community
> feedback.  For *upstreaming* purposes it was intended as an RFC to
> gather feedback on the overall approach.  That should have been made
> clearer.

Well, it is hard to even have that kind of conversation when all the
details are wrong. The way this interfaces with uverbs is *just
wrong*, it completely ignores the locking model for how disassociation
works.

Something like this, that is trying to closely integrate with uverbs,
really cannot exist without major surgery to uverbs. If you want to do
something along these lines the uverbs parts cannot be in a ULP.

The mr cache could be moved into some kind of new uverb, that could be
interesting if it is carefully designed.

The actual transport code.. That is going to be really hard. RDS
doesn't integrate with uverbs for a reason, the kernel side owns the
QPs and PDs.

How you create a QP owned by the kernel but linked to a PD owned by
uverbs is going to need very delicate and careful work to be somehow
compatible with our disassociation model.

Are you *sure* this needs to be in the kernel? You can't take a
context switch to a user process and use the shared verbs FD stuff
to create the de-duplicated QPs instead? It is a much simpler design.

Have you thought about an actual *shared verbs QP* ? We have a lot of
other shared objects right now, it is not such a big step. It does
require inter-process locking though - and that is non-trivial.

A shared QP with a kernel owned send/recv to avoid the locking issue
could also be a very interesting solution.

Jason