[RFC v1 0/3] Unifying fabrics drivers
James Smart
jsmart2021 at gmail.com
Fri Mar 3 15:13:53 PST 2023
On 3/1/2023 12:27 AM, Daniel Wagner wrote:
> The two fabrics rdma and tcp share a lot of common code. This here is my attempt
> to consolidate the common code.
>
> I've picked just one function (setup admin queue) for this RFC to get a feeling
> and feedback if this is a valid approach or if people hate it. I've left out fc
> for the time being because it differs too much two the other two drivers.
>
> I've tested quickly tcp, rdma is only compile tested.
>
> Daniel Wagner (3):
> nvme-rdma: stream line queue functions arguments
> nvme-rdma: factor rdma specific queue init code out
> nvme-fabrics: move configure admin queue code to fabrics.c
>
> drivers/nvme/host/fabrics.c | 52 ++++++++++++++
> drivers/nvme/host/fabrics.h | 12 ++++
> drivers/nvme/host/nvme.h | 3 +
> drivers/nvme/host/rdma.c | 132 +++++++++++++++++++-----------------
> drivers/nvme/host/tcp.c | 94 ++++++++-----------------
> 5 files changed, 165 insertions(+), 128 deletions(-)
>
Initial reaction:
The resulting nvmf_configure_admin_queue() routine does look nice.
However, I'm not sure the callback chain is all that easier to follow.
I guess I'm most disappointed that more of the logic isn't actually
being commonized.
Examples:
alloc_queue():
At least pass queue_size as an arg. I assume we should to match up
with the fabric connect routine.
init_queue():
I'd like to see setting of ctrl.max_segments/hw_sectors in the common
code. Unfortunately, likely needs to be a fops call. At least it
makes sure its done at the same point in controller creation.
start_queue():
can't common code do the nvmf_connect_admin_queue() call ?
If its too tied to the LIVE bit - then perhaps the queue LIVE
bits should actually be getting set by the common code after
the successful start_queue - or has routines transport can call
from start/stop to set/clear it.
If LIVE bit can be dealt with - this whole block in rdma/tcp moves to
common code.
Also I'd really like to get rid of the "new" argument everywhere. It
should just be a state flag set on the common ctrl.
I guess not bad, but not helping much (yet).
-- james
More information about the Linux-nvme
mailing list