Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Thu, 20 Apr 2023 05:29:18 -0700
From:   Breno Leitao <leitao@debian.org>
To:     Christoph Hellwig <hch@lst.de>
Cc:     io-uring@vger.kernel.org, linux-nvme@lists.infradead.org,
        asml.silence@gmail.com, axboe@kernel.dk, leit@fb.com,
        linux-kernel@vger.kernel.org, linux-block@vger.kernel.org,
        sagi@grimberg.me, kbusch@kernel.org, ming.lei@redhat.com
Subject: Re: [PATCH 1/2] io_uring: Pass whole sqe to commands
Message-ID: <ZEEwHk32Y8IcT20n@gmail.com>
References: <20230419102930.2979231-1-leitao@debian.org>
 <20230419102930.2979231-2-leitao@debian.org>
 <20230420045712.GA4239@lst.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20230420045712.GA4239@lst.de>
Precedence: bulk

Hello Christoph,

On Thu, Apr 20, 2023 at 06:57:12AM +0200, Christoph Hellwig wrote:
> On Wed, Apr 19, 2023 at 03:29:29AM -0700, Breno Leitao wrote:
> >  	struct nvme_uring_cmd_pdu *pdu = nvme_uring_cmd_pdu(ioucmd);
> > -	const struct nvme_uring_cmd *cmd = ioucmd->cmd;
> > +	const struct nvme_uring_cmd *cmd = (struct nvme_uring_cmd *)ioucmd->sqe->cmd;
> 
> Please don't add the pointless cast.  And in general avoid the overly
> long lines.

Ack!

> 
> I suspect most other users should just also defined their structures
> const instead of adding all theses casts thare are a sign of problems,
> but that's a pre-existing issue.
> >  	struct io_uring_cmd *ioucmd = io_kiocb_to_cmd(req, struct io_uring_cmd);
> > -	size_t cmd_size;
> > +	size_t size = sizeof(struct io_uring_sqe);
> >  
> >  	BUILD_BUG_ON(uring_cmd_pdu_size(0) != 16);
> >  	BUILD_BUG_ON(uring_cmd_pdu_size(1) != 80);
> >  
> > -	cmd_size = uring_cmd_pdu_size(req->ctx->flags & IORING_SETUP_SQE128);
> > +	if (req->ctx->flags & IORING_SETUP_SQE128)
> > +		size <<= 1;
> 
> 
> Why does this stop using uring_cmd_pdu_size()?

Before, only the cmd payload (sqe->cmd) was being copied to the async
structure. We are copying over the whole sqe now, since we can use SQE
fields inside the ioctl callbacks (instead of only cmd fields). So, the
copy now is 64 bytes for single SQE or 128 for double SQEs.

This has two major advantages:
 * It is not necessary to create a cmd structure for every command
   operations (which will be mapped in sqe->cmd) to pass arguments. The
   arguments could be passed as fields in SQE.

 * sqe->cmd is 16 bytes on single SQEs. Passing the whole SQE to cmd
   will reduce the necessity to use double SQE for operations that
   require large fields, such as {g,s}etsockopt().

There are some discussions about it also at
https://lkml.org/lkml/2023/4/6/786

Thanks for the review!