Re: [PATCH] nvmet: dynamically allocate nvmet_ns->nguid

[Jens Axboe <axboe@kernel.dk>]

On 5/1/23 11:30?PM, Chaitanya Kulkarni wrote:
> The nvmet_ns struct is critical to I/O operations in each backend bdev
> and file, but its static nguid array is not accessed in the fast path.
> This means that pulling all the memory for the array on each access
> is inefficient.
> 
> This patch dynamically allocates the nvmet_ns->nguid array, reducing the
> size of the nvmet_ns struct. This optimization should reduce unnecessary
> memory access in the fast path that is required for the array vs pointer.
> For allocation of nguid with kzalloc() use same policy GFP_KERNEL that is
> used to allocate nvmet_ns struct iself.

Replacing 16 bytes of embedded memory with 8 bytes and then alloc+free
seems like a poor tradeoff.

Why not just arrange it a bit more sanely, and also push the config
stuff out-of-line as that would not be used in the fast path. The below
30 second job takes it from 456 -> 440 bytes for me, and has a better
layout imho.
 

diff --git a/drivers/nvme/target/nvmet.h b/drivers/nvme/target/nvmet.h
index dc60a22646f7..790d7513e442 100644
--- a/drivers/nvme/target/nvmet.h
+++ b/drivers/nvme/target/nvmet.h
@@ -61,29 +61,31 @@ struct nvmet_ns {
 	struct block_device	*bdev;
 	struct file		*file;
 	bool			readonly;
+	bool			buffered_io;
+	bool			enabled;
+	u8			csi;
 	u32			nsid;
 	u32			blksize_shift;
+	u32			anagrpid;
 	loff_t			size;
 	u8			nguid[16];
 	uuid_t			uuid;
-	u32			anagrpid;
 
-	bool			buffered_io;
-	bool			enabled;
 	struct nvmet_subsys	*subsys;
 	const char		*device_path;
 
-	struct config_group	device_group;
-	struct config_group	group;
-
 	struct completion	disable_done;
 	mempool_t		*bvec_pool;
 
-	int			use_p2pmem;
 	struct pci_dev		*p2p_dev;
+	int			use_p2pmem;
 	int			pi_type;
 	int			metadata_size;
-	u8			csi;
+
+	struct config_group	device_group;
+	struct config_group	group;
+
+
 };
 
 static inline struct nvmet_ns *to_nvmet_ns(struct config_item *item)

-- 
Jens Axboe