Commit 7dff719f authored by Daniel Verkamp's avatar Daniel Verkamp
Browse files

nvme: optimize layout of struct nvme_payload 



Rather than storing nvme_payload::type explicitly, use the SGL reset
function pointer as an indicator: if reset_sgl_fn is non-NULL, then the
payload is an SGL type; otherwise it is a contiguous buffer type.

This eliminates the one-byte type member from struct nvme_payload,
making it an even 32 bytes instead of 33, allowing the removal of the
awkward packing inside struct nvme_request.

Change-Id: If2a32437a23fe14eb5287e096ac060067296f1dd
Signed-off-by: default avatarDaniel Verkamp <daniel.verkamp@intel.com>
Reviewed-on: https://review.gerrithub.io/413175


Tested-by: default avatarSPDK Automated Test System <sys_sgsw@intel.com>
Reviewed-by: default avatarJim Harris <james.r.harris@intel.com>
Reviewed-by: default avatarBen Walker <benjamin.walker@intel.com>
parent caf85d8f

lib/nvme/nvme.c

+2 −2
Original line number Diff line number Diff line
@@ -206,10 +206,10 @@ nvme_user_copy_cmd_complete(void *arg, const struct spdk_nvme_cpl *cpl) 
		if (xfer == SPDK_NVME_DATA_CONTROLLER_TO_HOST ||

		    xfer == SPDK_NVME_DATA_BIDIRECTIONAL) {

			assert(req->pid == getpid());

			memcpy(req->user_buffer, req->payload.u.contig, req->payload_size);

			memcpy(req->user_buffer, req->payload.contig_or_cb_arg, req->payload_size);

		}



		spdk_dma_free(req->payload.u.contig);

		spdk_dma_free(req->payload.contig_or_cb_arg);

	}



	/* Call the user's original callback now that the buffer has been copied */

lib/nvme/nvme_internal.h

+22 −26
Original line number Diff line number Diff line
@@ -135,50 +135,46 @@ enum spdk_nvme_ctrlr_flags { 


/**

 * Descriptor for a request data payload.

 *

 * This struct is arranged so that it fits nicely in struct nvme_request.

 */

struct __attribute__((packed)) nvme_payload {

	union {

		/** Virtual memory address of a single physically contiguous buffer */

		void *contig;



struct nvme_payload {

	/**

	 * Functions for retrieving physical addresses for scattered payloads.

	 */

		struct nvme_sgl_args {

	spdk_nvme_req_reset_sgl_cb reset_sgl_fn;

	spdk_nvme_req_next_sge_cb next_sge_fn;

			void *cb_arg;

		} sgl;

	} u;



	/**

	 * If reset_sgl_fn == NULL, this is a contig payload, and contig_or_cb_arg contains the

	 * virtual memory address of a single physically contiguous buffer.

	 *

	 * If reset_sgl_fn != NULL, this is a SGL payload, and contig_or_cb_arg contains the

	 * cb_arg that will be passed to the SGL callback functions.

	 */

	void *contig_or_cb_arg;



	/** Virtual memory address of a single physically contiguous metadata buffer */

	void *md;



	/** \ref nvme_payload_type */

	uint8_t type;

};



#define NVME_PAYLOAD_CONTIG(contig_, md_) \

	(struct nvme_payload) { \

		.u.contig = (contig_), \

		.reset_sgl_fn = NULL, \

		.next_sge_fn = NULL, \

		.contig_or_cb_arg = (contig_), \

		.md = (md_), \

		.type = NVME_PAYLOAD_TYPE_CONTIG, \

	}



#define NVME_PAYLOAD_SGL(reset_sgl_fn_, next_sge_fn_, cb_arg_, md_) \

	(struct nvme_payload) { \

		.u.sgl.reset_sgl_fn = (reset_sgl_fn_), \

		.u.sgl.next_sge_fn = (next_sge_fn_), \

		.u.sgl.cb_arg = (cb_arg_), \

		.reset_sgl_fn = (reset_sgl_fn_), \

		.next_sge_fn = (next_sge_fn_), \

		.contig_or_cb_arg = (cb_arg_), \

		.md = (md_), \

		.type = NVME_PAYLOAD_TYPE_SGL, \

	}



static inline enum nvme_payload_type

nvme_payload_type(const struct nvme_payload *payload) {

	return payload->type;

	return payload->reset_sgl_fn ? NVME_PAYLOAD_TYPE_SGL : NVME_PAYLOAD_TYPE_CONTIG;

}



struct nvme_request {

lib/nvme/nvme_ns_cmd.c

+11 −10
Original line number Diff line number Diff line
@@ -234,7 +234,9 @@ _nvme_ns_cmd_split_request_prp(struct spdk_nvme_ns *ns, 
			       uint32_t io_flags, struct nvme_request *req,

			       uint16_t apptag_mask, uint16_t apptag)

{

	struct nvme_sgl_args *args;

	spdk_nvme_req_reset_sgl_cb reset_sgl_fn = req->payload.reset_sgl_fn;

	spdk_nvme_req_next_sge_cb next_sge_fn = req->payload.next_sge_fn;

	void *sgl_cb_arg = req->payload.contig_or_cb_arg;

	bool start_valid, end_valid, last_sge, child_equals_parent;

	uint64_t child_lba = lba;

	uint32_t req_current_length = 0;
@@ -243,10 +245,8 @@ _nvme_ns_cmd_split_request_prp(struct spdk_nvme_ns *ns, 
	uint32_t page_size = qpair->ctrlr->page_size;

	uintptr_t address;



	args = &req->payload.u.sgl;



	args->reset_sgl_fn(args->cb_arg, payload_offset);

	args->next_sge_fn(args->cb_arg, (void **)&address, &sge_length);

	reset_sgl_fn(sgl_cb_arg, payload_offset);

	next_sge_fn(sgl_cb_arg, (void **)&address, &sge_length);

	while (req_current_length < req->payload_size) {



		if (sge_length == 0) {
@@ -289,7 +289,7 @@ _nvme_ns_cmd_split_request_prp(struct spdk_nvme_ns *ns, 
			child_length += sge_length;

			req_current_length += sge_length;

			if (req_current_length < req->payload_size) {

				args->next_sge_fn(args->cb_arg, (void **)&address, &sge_length);

				next_sge_fn(sgl_cb_arg, (void **)&address, &sge_length);

			}

			/*

			 * If the next SGE is not page aligned, we will need to create a child
@@ -356,7 +356,9 @@ _nvme_ns_cmd_split_request_sgl(struct spdk_nvme_ns *ns, 
			       uint32_t io_flags, struct nvme_request *req,

			       uint16_t apptag_mask, uint16_t apptag)

{

	struct nvme_sgl_args *args;

	spdk_nvme_req_reset_sgl_cb reset_sgl_fn = req->payload.reset_sgl_fn;

	spdk_nvme_req_next_sge_cb next_sge_fn = req->payload.next_sge_fn;

	void *sgl_cb_arg = req->payload.contig_or_cb_arg;

	uint64_t child_lba = lba;

	uint32_t req_current_length = 0;

	uint32_t child_length = 0;
@@ -364,14 +366,13 @@ _nvme_ns_cmd_split_request_sgl(struct spdk_nvme_ns *ns, 
	uint16_t max_sges, num_sges;

	uintptr_t address;



	args = &req->payload.u.sgl;

	max_sges = ns->ctrlr->max_sges;



	args->reset_sgl_fn(args->cb_arg, payload_offset);

	reset_sgl_fn(sgl_cb_arg, payload_offset);

	num_sges = 0;



	while (req_current_length < req->payload_size) {

		args->next_sge_fn(args->cb_arg, (void **)&address, &sge_length);

		next_sge_fn(sgl_cb_arg, (void **)&address, &sge_length);



		if (req_current_length + sge_length > req->payload_size) {

			sge_length = req->payload_size - req_current_length;

lib/nvme/nvme_pcie.c

+9 −9
Original line number Diff line number Diff line
@@ -1743,7 +1743,7 @@ nvme_pcie_qpair_build_contig_request(struct spdk_nvme_qpair *qpair, struct nvme_ 
	uint32_t prp_index = 0;

	int rc;



	rc = nvme_pcie_prp_list_append(tr, &prp_index, req->payload.u.contig + req->payload_offset,

	rc = nvme_pcie_prp_list_append(tr, &prp_index, req->payload.contig_or_cb_arg + req->payload_offset,

				       req->payload_size, qpair->ctrlr->page_size);

	if (rc) {

		nvme_pcie_fail_request_bad_vtophys(qpair, tr);
@@ -1772,9 +1772,9 @@ nvme_pcie_qpair_build_hw_sgl_request(struct spdk_nvme_qpair *qpair, struct nvme_ 
	 */

	assert(req->payload_size != 0);

	assert(nvme_payload_type(&req->payload) == NVME_PAYLOAD_TYPE_SGL);

	assert(req->payload.u.sgl.reset_sgl_fn != NULL);

	assert(req->payload.u.sgl.next_sge_fn != NULL);

	req->payload.u.sgl.reset_sgl_fn(req->payload.u.sgl.cb_arg, req->payload_offset);

	assert(req->payload.reset_sgl_fn != NULL);

	assert(req->payload.next_sge_fn != NULL);

	req->payload.reset_sgl_fn(req->payload.contig_or_cb_arg, req->payload_offset);



	sgl = tr->u.sgl;

	req->cmd.psdt = SPDK_NVME_PSDT_SGL_MPTR_CONTIG;
@@ -1788,7 +1788,7 @@ nvme_pcie_qpair_build_hw_sgl_request(struct spdk_nvme_qpair *qpair, struct nvme_ 
			return -1;

		}



		rc = req->payload.u.sgl.next_sge_fn(req->payload.u.sgl.cb_arg, &virt_addr, &length);

		rc = req->payload.next_sge_fn(req->payload.contig_or_cb_arg, &virt_addr, &length);

		if (rc) {

			nvme_pcie_fail_request_bad_vtophys(qpair, tr);

			return -1;
@@ -1849,13 +1849,13 @@ nvme_pcie_qpair_build_prps_sgl_request(struct spdk_nvme_qpair *qpair, struct nvm 
	 * Build scattered payloads.

	 */

	assert(nvme_payload_type(&req->payload) == NVME_PAYLOAD_TYPE_SGL);

	assert(req->payload.u.sgl.reset_sgl_fn != NULL);

	req->payload.u.sgl.reset_sgl_fn(req->payload.u.sgl.cb_arg, req->payload_offset);

	assert(req->payload.reset_sgl_fn != NULL);

	req->payload.reset_sgl_fn(req->payload.contig_or_cb_arg, req->payload_offset);



	remaining_transfer_len = req->payload_size;

	while (remaining_transfer_len > 0) {

		assert(req->payload.u.sgl.next_sge_fn != NULL);

		rc = req->payload.u.sgl.next_sge_fn(req->payload.u.sgl.cb_arg, &virt_addr, &length);

		assert(req->payload.next_sge_fn != NULL);

		rc = req->payload.next_sge_fn(req->payload.contig_or_cb_arg, &virt_addr, &length);

		if (rc) {

			nvme_pcie_fail_request_bad_vtophys(qpair, tr);

			return -1;

lib/nvme/nvme_rdma.c

+5 −5
Original line number Diff line number Diff line
@@ -905,7 +905,7 @@ nvme_rdma_build_null_request(struct nvme_request *req) 
static int

nvme_rdma_build_contig_request(struct nvme_rdma_qpair *rqpair, struct nvme_request *req)

{

	void *payload = req->payload.u.contig + req->payload_offset;

	void *payload = req->payload.contig_or_cb_arg + req->payload_offset;

	struct ibv_mr *mr;



	assert(req->payload_size != 0);
@@ -939,12 +939,12 @@ nvme_rdma_build_sgl_request(struct nvme_rdma_qpair *rqpair, struct nvme_request 


	assert(req->payload_size != 0);

	assert(nvme_payload_type(&req->payload) == NVME_PAYLOAD_TYPE_SGL);

	assert(req->payload.u.sgl.reset_sgl_fn != NULL);

	assert(req->payload.u.sgl.next_sge_fn != NULL);

	req->payload.u.sgl.reset_sgl_fn(req->payload.u.sgl.cb_arg, req->payload_offset);

	assert(req->payload.reset_sgl_fn != NULL);

	assert(req->payload.next_sge_fn != NULL);

	req->payload.reset_sgl_fn(req->payload.contig_or_cb_arg, req->payload_offset);



	/* TODO: for now, we only support a single SGL entry */

	rc = req->payload.u.sgl.next_sge_fn(req->payload.u.sgl.cb_arg, &virt_addr, &length);

	rc = req->payload.next_sge_fn(req->payload.contig_or_cb_arg, &virt_addr, &length);

	if (rc) {

		return -1;

	}