From: Michael Kelley <mikelley@microsoft.com>
To: "Andrea Parri (Microsoft)" <parri.andrea@gmail.com>,
"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Cc: KY Srinivasan <kys@microsoft.com>,
Haiyang Zhang <haiyangz@microsoft.com>,
Stephen Hemminger <sthemmin@microsoft.com>,
"wei.liu@kernel.org" <wei.liu@kernel.org>,
"davem@davemloft.net" <davem@davemloft.net>,
"kuba@kernel.org" <kuba@kernel.org>,
"jejb@linux.ibm.com" <jejb@linux.ibm.com>,
"martin.petersen@oracle.com" <martin.petersen@oracle.com>,
"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>
Subject: RE: [RFC PATCH hyperv-next] scsi: storvsc: Use blk_mq_unique_tag() to generate requestIDs
Date: Fri, 9 Apr 2021 15:38:14 +0000 [thread overview]
Message-ID: <MWHPR21MB15934EAD302E27983E891CF2D7739@MWHPR21MB1593.namprd21.prod.outlook.com> (raw)
In-Reply-To: <20210408161315.341888-1-parri.andrea@gmail.com>
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Thursday, April 8, 2021 9:13 AM
>
> Use blk_mq_unique_tag() to generate requestIDs for StorVSC, avoiding
> all issues with allocating enough entries in the VMbus requestor.
This looks good to me! I'm glad to see that the idea worked without
too much complexity.
See a few comments inline below.
>
> Suggested-by: Michael Kelley <mikelley@microsoft.com>
> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com>
> ---
> drivers/hv/channel.c | 14 +++---
> drivers/hv/ring_buffer.c | 12 ++---
> drivers/net/hyperv/netvsc.c | 8 ++--
> drivers/net/hyperv/rndis_filter.c | 2 +
> drivers/scsi/storvsc_drv.c | 73 ++++++++++++++++++++++++++-----
> include/linux/hyperv.h | 13 +++++-
> 6 files changed, 92 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c
> index db30be8f9ccea..f78e02ace51e8 100644
> --- a/drivers/hv/channel.c
> +++ b/drivers/hv/channel.c
> @@ -1121,15 +1121,14 @@ EXPORT_SYMBOL_GPL(vmbus_recvpacket_raw);
> * vmbus_next_request_id - Returns a new request id. It is also
> * the index at which the guest memory address is stored.
> * Uses a spin lock to avoid race conditions.
> - * @rqstor: Pointer to the requestor struct
> + * @channel: Pointer to the VMbus channel struct
> * @rqst_add: Guest memory address to be stored in the array
> */
> -u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr)
> +u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr)
> {
> + struct vmbus_requestor *rqstor = &channel->requestor;
> unsigned long flags;
> u64 current_id;
> - const struct vmbus_channel *channel =
> - container_of(rqstor, const struct vmbus_channel, requestor);
>
> /* Check rqstor has been initialized */
> if (!channel->rqstor_size)
> @@ -1163,16 +1162,15 @@ EXPORT_SYMBOL_GPL(vmbus_next_request_id);
> /*
> * vmbus_request_addr - Returns the memory address stored at @trans_id
> * in @rqstor. Uses a spin lock to avoid race conditions.
> - * @rqstor: Pointer to the requestor struct
> + * @channel: Pointer to the VMbus channel struct
> * @trans_id: Request id sent back from Hyper-V. Becomes the requestor's
> * next request id.
> */
> -u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id)
> +u64 vmbus_request_addr(struct vmbus_channel *channel, u64 trans_id)
> {
> + struct vmbus_requestor *rqstor = &channel->requestor;
> unsigned long flags;
> u64 req_addr;
> - const struct vmbus_channel *channel =
> - container_of(rqstor, const struct vmbus_channel, requestor);
>
> /* Check rqstor has been initialized */
> if (!channel->rqstor_size)
> diff --git a/drivers/hv/ring_buffer.c b/drivers/hv/ring_buffer.c
> index ecd82ebfd5bc4..46d8e038e4ee1 100644
> --- a/drivers/hv/ring_buffer.c
> +++ b/drivers/hv/ring_buffer.c
> @@ -310,10 +310,12 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
> */
>
> if (desc->flags == VMBUS_DATA_PACKET_FLAG_COMPLETION_REQUESTED) {
> - rqst_id = vmbus_next_request_id(&channel->requestor, requestid);
> - if (rqst_id == VMBUS_RQST_ERROR) {
> - spin_unlock_irqrestore(&outring_info->ring_lock, flags);
> - return -EAGAIN;
> + if (channel->next_request_id_callback != NULL) {
> + rqst_id = channel->next_request_id_callback(channel, requestid);
> + if (rqst_id == VMBUS_RQST_ERROR) {
> + spin_unlock_irqrestore(&outring_info->ring_lock, flags);
> + return -EAGAIN;
> + }
> }
> }
> desc = hv_get_ring_buffer(outring_info) + old_write;
> @@ -341,7 +343,7 @@ int hv_ringbuffer_write(struct vmbus_channel *channel,
> if (channel->rescind) {
> if (rqst_id != VMBUS_NO_RQSTOR) {
> /* Reclaim request ID to avoid leak of IDs */
> - vmbus_request_addr(&channel->requestor, rqst_id);
> + channel->request_addr_callback(channel, rqst_id);
> }
> return -ENODEV;
> }
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> index c64cc7639c39c..1a221ce2d6fdc 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -730,7 +730,7 @@ static void netvsc_send_tx_complete(struct net_device *ndev,
> int queue_sends;
> u64 cmd_rqst;
>
> - cmd_rqst = vmbus_request_addr(&channel->requestor, (u64)desc->trans_id);
> + cmd_rqst = channel->request_addr_callback(channel, (u64)desc->trans_id);
> if (cmd_rqst == VMBUS_RQST_ERROR) {
> netdev_err(ndev, "Incorrect transaction id\n");
> return;
> @@ -790,8 +790,8 @@ static void netvsc_send_completion(struct net_device *ndev,
>
> /* First check if this is a VMBUS completion without data payload */
> if (!msglen) {
> - cmd_rqst = vmbus_request_addr(&incoming_channel->requestor,
> - (u64)desc->trans_id);
> + cmd_rqst = incoming_channel->request_addr_callback(incoming_channel,
> + (u64)desc->trans_id);
> if (cmd_rqst == VMBUS_RQST_ERROR) {
> netdev_err(ndev, "Invalid transaction id\n");
> return;
> @@ -1602,6 +1602,8 @@ struct netvsc_device *netvsc_device_add(struct hv_device
> *device,
> netvsc_poll, NAPI_POLL_WEIGHT);
>
> /* Open the channel */
> + device->channel->next_request_id_callback = vmbus_next_request_id;
> + device->channel->request_addr_callback = vmbus_request_addr;
> device->channel->rqstor_size = netvsc_rqstor_size(netvsc_ring_bytes);
> ret = vmbus_open(device->channel, netvsc_ring_bytes,
> netvsc_ring_bytes, NULL, 0,
> diff --git a/drivers/net/hyperv/rndis_filter.c b/drivers/net/hyperv/rndis_filter.c
> index 123cc9d25f5ed..ebf34bf3f9075 100644
> --- a/drivers/net/hyperv/rndis_filter.c
> +++ b/drivers/net/hyperv/rndis_filter.c
> @@ -1259,6 +1259,8 @@ static void netvsc_sc_open(struct vmbus_channel *new_sc)
> /* Set the channel before opening.*/
> nvchan->channel = new_sc;
>
> + new_sc->next_request_id_callback = vmbus_next_request_id;
> + new_sc->request_addr_callback = vmbus_request_addr;
> new_sc->rqstor_size = netvsc_rqstor_size(netvsc_ring_bytes);
> ret = vmbus_open(new_sc, netvsc_ring_bytes,
> netvsc_ring_bytes, NULL, 0,
> diff --git a/drivers/scsi/storvsc_drv.c b/drivers/scsi/storvsc_drv.c
> index 6bc5453cea8a7..1c05fabc06b04 100644
> --- a/drivers/scsi/storvsc_drv.c
> +++ b/drivers/scsi/storvsc_drv.c
> @@ -684,6 +684,62 @@ static void storvsc_change_target_cpu(struct vmbus_channel
> *channel, u32 old,
> spin_unlock_irqrestore(&stor_device->lock, flags);
> }
>
> +u64 storvsc_next_request_id(struct vmbus_channel *channel, u64 rqst_addr)
> +{
> + struct storvsc_cmd_request *request =
> + (struct storvsc_cmd_request *)(unsigned long)rqst_addr;
> + struct storvsc_device *stor_device;
> + struct hv_device *device;
> +
> + device = (channel->primary_channel != NULL) ?
> + channel->primary_channel->device_obj : channel->device_obj;
> + if (device == NULL)
> + return VMBUS_RQST_ERROR;
> +
> + stor_device = get_out_stor_device(device);
> + if (stor_device == NULL)
> + return VMBUS_RQST_ERROR;
> +
> + if (request == &stor_device->init_request)
> + return VMBUS_RQST_INIT;
> + if (request == &stor_device->reset_request)
> + return VMBUS_RQST_RESET;
Having to get the device and then the stor_device in order to detect the
init_request and reset_request special cases is unfortunate. So here's
an idea: The init_request and reset_request are used in a limited number
of specific places in the storvsc driver, and there are unique invocations
of vmbus_sendpacket() in those places. So rather than pass the address
of the request as the requestID parameter to vmbus_sendpacket(), pass
the sentinel value VMBUS_RQST_INIT or VMBUS_RQST_RESET. Then this
code can just detect those sentinel values as the rqst_addr input
parameter, and return them.
> +
> + return blk_mq_unique_tag(request->cmd->request);
> +}
> +
> +u64 storvsc_request_addr(struct vmbus_channel *channel, u64 rqst_id)
> +{
> + struct storvsc_cmd_request *request;
> + struct storvsc_device *stor_device;
> + struct hv_device *device;
> + struct Scsi_Host *shost;
> + struct scsi_cmnd *scmnd;
> +
> + device = (channel->primary_channel != NULL) ?
> + channel->primary_channel->device_obj : channel->device_obj;
> + if (device == NULL)
> + return VMBUS_RQST_ERROR;
> +
> + stor_device = get_out_stor_device(device);
> + if (stor_device == NULL)
> + return VMBUS_RQST_ERROR;
> +
> + if (rqst_id == VMBUS_RQST_INIT)
> + return (unsigned long)&stor_device->init_request;
> + if (rqst_id == VMBUS_RQST_RESET)
> + return (unsigned long)&stor_device->reset_request;
Unfortunately, the same simplification doesn't work here. And you need
stor_device anyway to get the scsi_host.
> +
> + shost = stor_device->host;
> +
> + scmnd = scsi_host_find_tag(shost, rqst_id);
> + if (scmnd == NULL)
> + return VMBUS_RQST_ERROR;
> +
> + request = (struct storvsc_cmd_request *)(unsigned long)scsi_cmd_priv(scmnd);
> + return (unsigned long)request;
The casts in the above two lines seem unnecessarily complex. 'request' is never
used as a pointer. So couldn't the last two lines just be:
return (unsigned long)scsi_cmd_priv(scmnd);
> +}
> +
> static void handle_sc_creation(struct vmbus_channel *new_sc)
> {
> struct hv_device *device = new_sc->primary_channel->device_obj;
> @@ -698,11 +754,8 @@ static void handle_sc_creation(struct vmbus_channel *new_sc)
>
> memset(&props, 0, sizeof(struct vmstorage_channel_properties));
>
> - /*
> - * The size of vmbus_requestor is an upper bound on the number of requests
> - * that can be in-progress at any one time across all channels.
> - */
> - new_sc->rqstor_size = scsi_driver.can_queue;
> + new_sc->next_request_id_callback = storvsc_next_request_id;
> + new_sc->request_addr_callback = storvsc_request_addr;
>
> ret = vmbus_open(new_sc,
> storvsc_ringbuffer_size,
> @@ -1255,8 +1308,7 @@ static void storvsc_on_channel_callback(void *context)
> struct storvsc_cmd_request *request;
> u64 cmd_rqst;
>
> - cmd_rqst = vmbus_request_addr(&channel->requestor,
> - desc->trans_id);
> + cmd_rqst = channel->request_addr_callback(channel, desc->trans_id);
Here's another thought: You don't really need to set the channel request_addr_callback
function and then indirect through it here. You know the specific function that storvsc
is using, so could call it directly. The other reason to set request_addr_callback is so
that at the end of hv_ringbuffer_write() you can reclaim an allocated requestID if the
rescind flag is set. But there's nothing allocated that needs to be reclaimed in the storvsc
case, so leaving request_addr_callback as NULL is OK (but hv_ringbuffer_write would
have to check for the NULL).
Then if you do that, the logic in storvsc_request_addr() can effectively go inline in
here. And that logic can take advantage of the fact that stor_device is already determined
outside the foreach_vmbus_pkt() loop. The scsi_host could be calculated outside the loop
as well, leaving the detection of init_request and reset_request, and the call to
scsi_host_find_tag() as the only things to do.
This approach is a bit asymmetrical, but it would save some processing in this interrupt
handling code. So something to consider.
> if (cmd_rqst == VMBUS_RQST_ERROR) {
> dev_err(&device->device,
> "Incorrect transaction id\n");
> @@ -1290,11 +1342,8 @@ static int storvsc_connect_to_vsp(struct hv_device *device, u32
> ring_size,
>
> memset(&props, 0, sizeof(struct vmstorage_channel_properties));
>
> - /*
> - * The size of vmbus_requestor is an upper bound on the number of requests
> - * that can be in-progress at any one time across all channels.
> - */
> - device->channel->rqstor_size = scsi_driver.can_queue;
> + device->channel->next_request_id_callback = storvsc_next_request_id;
> + device->channel->request_addr_callback = storvsc_request_addr;
>
> ret = vmbus_open(device->channel,
> ring_size,
> diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h
> index 2c18c8e768efe..5692ffa60e022 100644
> --- a/include/linux/hyperv.h
> +++ b/include/linux/hyperv.h
> @@ -779,7 +779,11 @@ struct vmbus_requestor {
>
> #define VMBUS_NO_RQSTOR U64_MAX
> #define VMBUS_RQST_ERROR (U64_MAX - 1)
> +/* NetVSC-specific */
It is netvsc specific at the moment. But if we harden other
drivers, they are likely to use the same generic requestID
allocator, and hence need the same sentinel value.
> #define VMBUS_RQST_ID_NO_RESPONSE (U64_MAX - 2)
> +/* StorVSC-specific */
> +#define VMBUS_RQST_INIT (U64_MAX - 2)
> +#define VMBUS_RQST_RESET (U64_MAX - 3)
>
> struct vmbus_device {
> u16 dev_type;
> @@ -1007,13 +1011,18 @@ struct vmbus_channel {
> u32 fuzz_testing_interrupt_delay;
> u32 fuzz_testing_message_delay;
>
> + /* callback to generate a request ID from a request address */
> + u64 (*next_request_id_callback)(struct vmbus_channel *channel, u64 rqst_addr);
> + /* callback to retrieve a request address from a request ID */
> + u64 (*request_addr_callback)(struct vmbus_channel *channel, u64 rqst_id);
> +
> /* request/transaction ids for VMBus */
> struct vmbus_requestor requestor;
> u32 rqstor_size;
> };
>
> -u64 vmbus_next_request_id(struct vmbus_requestor *rqstor, u64 rqst_addr);
> -u64 vmbus_request_addr(struct vmbus_requestor *rqstor, u64 trans_id);
> +u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr);
> +u64 vmbus_request_addr(struct vmbus_channel *channel, u64 trans_id);
>
> static inline bool is_hvsock_channel(const struct vmbus_channel *c)
> {
> --
> 2.25.1
next prev parent reply other threads:[~2021-04-09 15:38 UTC|newest]
Thread overview: 4+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-04-08 16:13 [RFC PATCH hyperv-next] scsi: storvsc: Use blk_mq_unique_tag() to generate requestIDs Andrea Parri (Microsoft)
2021-04-08 21:36 ` kernel test robot
2021-04-09 15:38 ` Michael Kelley [this message]
2021-04-12 20:29 ` Andrea Parri
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=MWHPR21MB15934EAD302E27983E891CF2D7739@MWHPR21MB1593.namprd21.prod.outlook.com \
--to=mikelley@microsoft.com \
--cc=davem@davemloft.net \
--cc=haiyangz@microsoft.com \
--cc=jejb@linux.ibm.com \
--cc=kuba@kernel.org \
--cc=kys@microsoft.com \
--cc=linux-hyperv@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-scsi@vger.kernel.org \
--cc=martin.petersen@oracle.com \
--cc=netdev@vger.kernel.org \
--cc=parri.andrea@gmail.com \
--cc=sthemmin@microsoft.com \
--cc=wei.liu@kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.