Commit 194983ee authored by John Levon's avatar John Levon Committed by Tomasz Zawadzki
Browse files

harmonize spdk_*_get/set_opts() 



spdk_bdev_get/set_opts() is careful to check its size argument, so that
it can add options in a backwards-compatible manner. However,
spdk_iobuf_get/set_opts() and spdk_accel_get/set_opts() both have
slightly different interfaces to the bdev variant, and are less careful.

Make all three variants operate in the same manner instead.

For spdk_iobuf_set_opts(), make all validation consistently return an
error instead of trying to adjust automatically.

Signed-off-by: default avatarJohn Levon <john.levon@nutanix.com>
Change-Id: I4077a5f1df7039992a556544acdcb1ef379887ae
Reviewed-on: https://review.spdk.io/gerrit/c/spdk/spdk/+/22093


Reviewed-by: default avatarJim Harris <jim.harris@samsung.com>
Reviewed-by: default avatarShuhei Matsumoto <smatsumoto@nvidia.com>
Community-CI: Mellanox Build Bot
Tested-by: default avatarSPDK CI Jenkins <sys_sgci@intel.com>
parent 2e497261

CHANGELOG.md

+8 −0
Original line number Diff line number Diff line
@@ -2,6 +2,10 @@ 


## v24.05: (Upcoming Release)



### accel



spdk_accel_get/set_opts() has changed to act more like spdk_bdev's variant.



### bdev



Added `spdk_bdev_get_nvme_ctratt()` API to get controller attributes of bdev.
@@ -35,6 +39,10 @@ Added `iscsi_enable_histogram` RPC method to enable or disable histogram for spe 


Added `iscsi_get_histogram` RPC method to get histogram for specified iscsi target.



### thread



spdk_iobuf_get/set_opts() has changed to act more like spdk_bdev's variant.



### trace



Merged `struct spdk_trace_flags` and `struct spdk_trace_histories` into

include/spdk/accel.h

+12 −4
Original line number Diff line number Diff line
@@ -740,8 +740,14 @@ int spdk_accel_set_driver(const char *name); 
struct spdk_memory_domain *spdk_accel_get_memory_domain(void);



struct spdk_accel_opts {

	/** Size of this structure */

	size_t		size;

	/**

	 * The size of spdk_accel_opts according to the caller of this library is used for ABI

	 * compatibility.  The library uses this field to know how many fields in this

	 * structure are valid. And the library will populate any remaining fields with default values.

	 * New added fields should be put at the end of the struct.

	 */

	size_t		opts_size;



	/** Size of the small iobuf cache */

	uint32_t	small_cache_size;

	/** Size of the large iobuf cache */
@@ -752,6 +758,7 @@ struct spdk_accel_opts { 
	uint32_t	sequence_count;

	/** Maximum number of accel buffers per IO channel */

	uint32_t	buf_count;



} __attribute__((packed));



/**
@@ -766,9 +773,10 @@ int spdk_accel_set_opts(const struct spdk_accel_opts *opts); 
/**

 * Get the options for the accel framework.

 *

 * \param opts Accel options.

 * \param opts Output parameter for options.

 * \param opts_size sizeof(*opts)

 */

void spdk_accel_get_opts(struct spdk_accel_opts *opts);

void spdk_accel_get_opts(struct spdk_accel_opts *opts, size_t opts_size);



struct spdk_accel_opcode_stats {

	/** Number of executed operations */

include/spdk/thread.h

+12 −2
Original line number Diff line number Diff line
@@ -991,6 +991,15 @@ struct spdk_iobuf_opts { 
	uint32_t small_bufsize;

	/** Size of a single large buffer */

	uint32_t large_bufsize;



	/**

	 * The size of spdk_iobuf_opts according to the caller of this library is used for ABI

	 * compatibility.  The library uses this field to know how many fields in this

	 * structure are valid. And the library will populate any remaining fields with default values.

	 * New added fields should be put at the end of the struct.

	 */

	size_t opts_size;



};



struct spdk_iobuf_pool_stats {
@@ -1084,9 +1093,10 @@ int spdk_iobuf_set_opts(const struct spdk_iobuf_opts *opts); 
/**

 * Get iobuf options.

 *

 * \param opts Options to fill in.

 * \param opts Output parameter for options.

 * \param opts_size sizeof(*opts)

 */

void spdk_iobuf_get_opts(struct spdk_iobuf_opts *opts);

void spdk_iobuf_get_opts(struct spdk_iobuf_opts *opts, size_t opts_size);



/**

 * Register a module as an iobuf pool user.  Only registered users can request buffers from the

lib/accel/accel.c

+49 −8
Original line number Diff line number Diff line
@@ -3045,24 +3045,65 @@ spdk_accel_driver_register(struct spdk_accel_driver *driver) 
int

spdk_accel_set_opts(const struct spdk_accel_opts *opts)

{

	if (opts->size > sizeof(*opts)) {

		return -EINVAL;

	if (!opts) {

		SPDK_ERRLOG("opts cannot be NULL\n");

		return -1;

	}



	if (!opts->opts_size) {

		SPDK_ERRLOG("opts_size inside opts cannot be zero value\n");

		return -1;

	}



	memcpy(&g_opts, opts, opts->size);

#define SET_FIELD(field) \

        if (offsetof(struct spdk_accel_opts, field) + sizeof(opts->field) <= opts->opts_size) { \

                g_opts.field = opts->field; \

        } \



	SET_FIELD(small_cache_size);

	SET_FIELD(large_cache_size);

	SET_FIELD(task_count);

	SET_FIELD(sequence_count);

	SET_FIELD(buf_count);



	g_opts.opts_size = opts->opts_size;



#undef SET_FIELD



	return 0;

}



void

spdk_accel_get_opts(struct spdk_accel_opts *opts)

spdk_accel_get_opts(struct spdk_accel_opts *opts, size_t opts_size)

{

	size_t size = opts->size;

	if (!opts) {

		SPDK_ERRLOG("opts should not be NULL\n");

		return;

	}



	if (!opts_size) {

		SPDK_ERRLOG("opts_size should not be zero value\n");

		return;

	}



	opts->opts_size = opts_size;



	assert(size <= sizeof(*opts));

#define SET_FIELD(field) \

	if (offsetof(struct spdk_accel_opts, field) + sizeof(opts->field) <= opts_size) { \

		opts->field = g_opts.field; \

	} \



	SET_FIELD(small_cache_size);

	SET_FIELD(large_cache_size);

	SET_FIELD(task_count);

	SET_FIELD(sequence_count);

	SET_FIELD(buf_count);



#undef SET_FIELD



	memcpy(opts, &g_opts, spdk_min(sizeof(*opts), size));

	opts->size = size;

	/* Do not remove this statement, you should always update this statement when you adding a new field,

	 * and do not forget to add the SET_FIELD statement for your added field. */

	SPDK_STATIC_ASSERT(sizeof(struct spdk_accel_opts) == 28, "Incorrect size");

}



struct accel_get_stats_ctx {

lib/accel/accel_rpc.c

+3 −3
Original line number Diff line number Diff line
@@ -374,13 +374,13 @@ static const struct spdk_json_object_decoder rpc_accel_set_options_decoders[] = 
static void

rpc_accel_set_options(struct spdk_jsonrpc_request *request, const struct spdk_json_val *params)

{

	struct spdk_accel_opts opts = { .size = sizeof(opts) };

	struct rpc_accel_opts rpc_opts;

	struct spdk_accel_opts opts;

	int rc;



	/* We can't pass spdk_accel_opts directly to spdk_json_decode_object(), because that

	 * structure is packed, leading undefined behavior due to misaligned pointer access */

	spdk_accel_get_opts(&opts);

	 * structure is packed, leading to undefined behavior due to misaligned pointer access */

	spdk_accel_get_opts(&opts, sizeof(opts));

	rpc_opts.small_cache_size = opts.small_cache_size;

	rpc_opts.large_cache_size = opts.large_cache_size;

	rpc_opts.task_count = opts.task_count;