| Message ID | 20220407043028.379534-5-parri.andrea@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Lorenzo Pieralisi |
| Headers | show |
| Series | PCI: hv: VMbus requestor and related fixes | expand |
From: Andrea Parri (Microsoft) <parri.andrea@gmail.com> Sent: Wednesday, April 6, 2022 9:30 PM > > The function can be used to retrieve and clear/remove a transation ID > from a channel requestor, provided the memory address corresponding to > the ID equals a specified address. The function, and its 'lockless' > variant __vmbus_request_addr_match(), will be used by hv_pci. > > Refactor vmbus_request_addr() to reuse the 'newly' introduced code. > > No functional change. > > Suggested-by: Michael Kelley <mikelley@microsoft.com> > Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> > --- > drivers/hv/channel.c | 65 ++++++++++++++++++++++++++++++------------ > include/linux/hyperv.h | 5 ++++ > 2 files changed, 52 insertions(+), 18 deletions(-) > > diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c > index 585a8084848bf..49f10a603a091 100644 > --- a/drivers/hv/channel.c > +++ b/drivers/hv/channel.c > @@ -1279,17 +1279,11 @@ u64 vmbus_next_request_id(struct vmbus_channel > *channel, u64 rqst_addr) > } > 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. > - * @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_channel *channel, u64 trans_id) > +/* As in vmbus_request_addr_match() but without the requestor lock */ > +u64 __vmbus_request_addr_match(struct vmbus_channel *channel, u64 trans_id, > + u64 rqst_addr) > { > struct vmbus_requestor *rqstor = &channel->requestor; > - unsigned long flags; > u64 req_addr; > > /* Check rqstor has been initialized */ > @@ -1300,25 +1294,60 @@ u64 vmbus_request_addr(struct vmbus_channel > *channel, u64 trans_id) > if (!trans_id) > return VMBUS_RQST_ERROR; > > - spin_lock_irqsave(&rqstor->req_lock, flags); > - > /* Data corresponding to trans_id is stored at trans_id - 1 */ > trans_id--; > > /* Invalid trans_id */ > - if (trans_id >= rqstor->size || !test_bit(trans_id, rqstor->req_bitmap)) { > - spin_unlock_irqrestore(&rqstor->req_lock, flags); > + if (trans_id >= rqstor->size || !test_bit(trans_id, rqstor->req_bitmap)) > return VMBUS_RQST_ERROR; > - } > > req_addr = rqstor->req_arr[trans_id]; > - rqstor->req_arr[trans_id] = rqstor->next_request_id; > - rqstor->next_request_id = trans_id; > + if (rqst_addr == VMBUS_RQST_ADDR_ANY || req_addr == rqst_addr) { > + rqstor->req_arr[trans_id] = rqstor->next_request_id; > + rqstor->next_request_id = trans_id; > > - /* The already held spin lock provides atomicity */ > - bitmap_clear(rqstor->req_bitmap, trans_id, 1); > + /* The already held spin lock provides atomicity */ > + bitmap_clear(rqstor->req_bitmap, trans_id, 1); > + } In the case where a specific match is required, and trans_id is valid but the addr's do not match, it looks like this function will return the addr that didn't match, without removing the entry. Shouldn't it return VMBUS_RQST_ERROR in that case? > + > + return req_addr; > +} > +EXPORT_SYMBOL_GPL(__vmbus_request_addr_match); > + > +/* > + * vmbus_request_addr_match - Clears/removes @trans_id from the @channel's > + * requestor, provided the memory address stored at @trans_id equals @rqst_addr > + * (or provided @rqst_addr matches the sentinel value VMBUS_RQST_ADDR_ANY). > + * > + * Returns the memory address stored at @trans_id, or VMBUS_RQST_ERROR if > + * @trans_id is not contained in the requestor. > + * > + * Acquires and releases the requestor spin lock. > + */ > +u64 vmbus_request_addr_match(struct vmbus_channel *channel, u64 trans_id, > + u64 rqst_addr) > +{ > + struct vmbus_requestor *rqstor = &channel->requestor; > + unsigned long flags; > + u64 req_addr; > > + spin_lock_irqsave(&rqstor->req_lock, flags); > + req_addr = __vmbus_request_addr_match(channel, trans_id, rqst_addr); > spin_unlock_irqrestore(&rqstor->req_lock, flags); > + > return req_addr; > } > +EXPORT_SYMBOL_GPL(vmbus_request_addr_match); > + > +/* > + * vmbus_request_addr - Returns the memory address stored at @trans_id > + * in @rqstor. Uses a spin lock to avoid race conditions. > + * @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_channel *channel, u64 trans_id) > +{ > + return vmbus_request_addr_match(channel, trans_id, > VMBUS_RQST_ADDR_ANY); > +} > EXPORT_SYMBOL_GPL(vmbus_request_addr); > diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h > index a7cb596d893b1..c77d78f34b96a 100644 > --- a/include/linux/hyperv.h > +++ b/include/linux/hyperv.h > @@ -788,6 +788,7 @@ struct vmbus_requestor { > > #define VMBUS_NO_RQSTOR U64_MAX > #define VMBUS_RQST_ERROR (U64_MAX - 1) > +#define VMBUS_RQST_ADDR_ANY U64_MAX > /* NetVSC-specific */ > #define VMBUS_RQST_ID_NO_RESPONSE (U64_MAX - 2) > /* StorVSC-specific */ > @@ -1042,6 +1043,10 @@ struct vmbus_channel { > }; > > u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr); > +u64 __vmbus_request_addr_match(struct vmbus_channel *channel, u64 trans_id, > + u64 rqst_addr); > +u64 vmbus_request_addr_match(struct vmbus_channel *channel, u64 trans_id, > + 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
> > @@ -1300,25 +1294,60 @@ u64 vmbus_request_addr(struct vmbus_channel > > *channel, u64 trans_id) > > if (!trans_id) > > return VMBUS_RQST_ERROR; > > > > - spin_lock_irqsave(&rqstor->req_lock, flags); > > - > > /* Data corresponding to trans_id is stored at trans_id - 1 */ > > trans_id--; > > > > /* Invalid trans_id */ > > - if (trans_id >= rqstor->size || !test_bit(trans_id, rqstor->req_bitmap)) { > > - spin_unlock_irqrestore(&rqstor->req_lock, flags); > > + if (trans_id >= rqstor->size || !test_bit(trans_id, rqstor->req_bitmap)) > > return VMBUS_RQST_ERROR; > > - } > > > > req_addr = rqstor->req_arr[trans_id]; > > - rqstor->req_arr[trans_id] = rqstor->next_request_id; > > - rqstor->next_request_id = trans_id; > > + if (rqst_addr == VMBUS_RQST_ADDR_ANY || req_addr == rqst_addr) { > > + rqstor->req_arr[trans_id] = rqstor->next_request_id; > > + rqstor->next_request_id = trans_id; > > > > - /* The already held spin lock provides atomicity */ > > - bitmap_clear(rqstor->req_bitmap, trans_id, 1); > > + /* The already held spin lock provides atomicity */ > > + bitmap_clear(rqstor->req_bitmap, trans_id, 1); > > + } > > In the case where a specific match is required, and trans_id is > valid but the addr's do not match, it looks like this function will > return the addr that didn't match, without removing the entry. Yes, that is consistent with the description on vmbus_request_addr_match(): Returns the memory address stored at @trans_id, or VMBUS_RQST_ERROR if @trans_id is not contained in the requestor. > Shouldn't it return VMBUS_RQST_ERROR in that case? Can certainly be done, although I'm not sure to follow your concerns. Can you elaborate? Thanks, Andrea
From: Andrea Parri <parri.andrea@gmail.com> Sent: Friday, April 8, 2022 9:47 AM > > > > @@ -1300,25 +1294,60 @@ u64 vmbus_request_addr(struct vmbus_channel > > > *channel, u64 trans_id) > > > if (!trans_id) > > > return VMBUS_RQST_ERROR; > > > > > > - spin_lock_irqsave(&rqstor->req_lock, flags); > > > - > > > /* Data corresponding to trans_id is stored at trans_id - 1 */ > > > trans_id--; > > > > > > /* Invalid trans_id */ > > > - if (trans_id >= rqstor->size || !test_bit(trans_id, rqstor->req_bitmap)) { > > > - spin_unlock_irqrestore(&rqstor->req_lock, flags); > > > + if (trans_id >= rqstor->size || !test_bit(trans_id, rqstor->req_bitmap)) > > > return VMBUS_RQST_ERROR; > > > - } > > > > > > req_addr = rqstor->req_arr[trans_id]; > > > - rqstor->req_arr[trans_id] = rqstor->next_request_id; > > > - rqstor->next_request_id = trans_id; > > > + if (rqst_addr == VMBUS_RQST_ADDR_ANY || req_addr == rqst_addr) { > > > + rqstor->req_arr[trans_id] = rqstor->next_request_id; > > > + rqstor->next_request_id = trans_id; > > > > > > - /* The already held spin lock provides atomicity */ > > > - bitmap_clear(rqstor->req_bitmap, trans_id, 1); > > > + /* The already held spin lock provides atomicity */ > > > + bitmap_clear(rqstor->req_bitmap, trans_id, 1); > > > + } > > > > In the case where a specific match is required, and trans_id is > > valid but the addr's do not match, it looks like this function will > > return the addr that didn't match, without removing the entry. > > Yes, that is consistent with the description on vmbus_request_addr_match(): > > Returns the memory address stored at @trans_id, or VMBUS_RQST_ERROR if > @trans_id is not contained in the requestor. > > > > Shouldn't it return VMBUS_RQST_ERROR in that case? > > Can certainly be done, although I'm not sure to follow your concerns. Can > you elaborate? > Having the function return "success" when it failed to match is unexpected for me. There's only one invocation where we care about matching (in hv_compose_msi_msg). In that invocation the purpose for matching is to not remove the wrong entry, and the return value is ignored. So I think it all works correctly. Just thinking out loud, maybe vmbus_request_addr_match() should be renamed to vmbus_request_addr_remove(), and not have a return value? That would be a bit more consistent with the actual purpose. Michael
> > > In the case where a specific match is required, and trans_id is > > > valid but the addr's do not match, it looks like this function will > > > return the addr that didn't match, without removing the entry. > > > > Yes, that is consistent with the description on vmbus_request_addr_match(): > > > > Returns the memory address stored at @trans_id, or VMBUS_RQST_ERROR if > > @trans_id is not contained in the requestor. > > > > > > > Shouldn't it return VMBUS_RQST_ERROR in that case? > > > > Can certainly be done, although I'm not sure to follow your concerns. Can > > you elaborate? > > > > Having the function return "success" when it failed to match is unexpected > for me. There's only one invocation where we care about matching > (in hv_compose_msi_msg). In that invocation the purpose for matching is to > not remove the wrong entry, and the return value is ignored. So I think > it all works correctly. You're reading it wrongly: the point is that there's nothing wrong in *not removing the "wrong entry" (or in failing to match). In the mentioned use, that means the channel callback has already processed "our" request, and that we don't have to worry about the ID. (Otherwise, i.e. if we do match, the callback will eventually scream "Invalid transaction".) > Just thinking out loud, maybe vmbus_request_addr_match() should be > renamed to vmbus_request_addr_remove(), and not have a return value? Mmh. We have vmbus_request_addr() that (always) removes the ID; it seems a _remove() would just add to the confusion. And removing the return value would mean duplicating most of vmbus_request_addr() in the "new" function. So, I'm not convinced that's the right thing to do. I'm inclined to leave this patch as is (and, as usual, happy to be proven wrong). Thanks, Andrea
From: Andrea Parri <parri.andrea@gmail.com> Sent: Friday, April 8, 2022 1:38 PM > > > > > In the case where a specific match is required, and trans_id is > > > > valid but the addr's do not match, it looks like this function will > > > > return the addr that didn't match, without removing the entry. > > > > > > Yes, that is consistent with the description on vmbus_request_addr_match(): > > > > > > Returns the memory address stored at @trans_id, or VMBUS_RQST_ERROR if > > > @trans_id is not contained in the requestor. > > > > > > > > > > Shouldn't it return VMBUS_RQST_ERROR in that case? > > > > > > Can certainly be done, although I'm not sure to follow your concerns. Can > > > you elaborate? > > > > > > > Having the function return "success" when it failed to match is unexpected > > for me. There's only one invocation where we care about matching > > (in hv_compose_msi_msg). In that invocation the purpose for matching is to > > not remove the wrong entry, and the return value is ignored. So I think > > it all works correctly. > > You're reading it wrongly: the point is that there's nothing wrong in *not > removing the "wrong entry" (or in failing to match). In the mentioned use, > that means the channel callback has already processed "our" request, and > that we don't have to worry about the ID. (Otherwise, i.e. if we do match, > the callback will eventually scream "Invalid transaction".) > > > > Just thinking out loud, maybe vmbus_request_addr_match() should be > > renamed to vmbus_request_addr_remove(), and not have a return value? > > Mmh. We have vmbus_request_addr() that (always) removes the ID; it seems > a _remove() would just add to the confusion. And removing the return value > would mean duplicating most of vmbus_request_addr() in the "new" function. > So, I'm not convinced that's the right thing to do. I'm inclined to leave > this patch as is (and, as usual, happy to be proven wrong). > I'll defer to your judgment. I don't see anything that broken with the patch as written, so I can live with it as is. Michael
diff --git a/drivers/hv/channel.c b/drivers/hv/channel.c index 585a8084848bf..49f10a603a091 100644 --- a/drivers/hv/channel.c +++ b/drivers/hv/channel.c @@ -1279,17 +1279,11 @@ u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr) } 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. - * @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_channel *channel, u64 trans_id) +/* As in vmbus_request_addr_match() but without the requestor lock */ +u64 __vmbus_request_addr_match(struct vmbus_channel *channel, u64 trans_id, + u64 rqst_addr) { struct vmbus_requestor *rqstor = &channel->requestor; - unsigned long flags; u64 req_addr; /* Check rqstor has been initialized */ @@ -1300,25 +1294,60 @@ u64 vmbus_request_addr(struct vmbus_channel *channel, u64 trans_id) if (!trans_id) return VMBUS_RQST_ERROR; - spin_lock_irqsave(&rqstor->req_lock, flags); - /* Data corresponding to trans_id is stored at trans_id - 1 */ trans_id--; /* Invalid trans_id */ - if (trans_id >= rqstor->size || !test_bit(trans_id, rqstor->req_bitmap)) { - spin_unlock_irqrestore(&rqstor->req_lock, flags); + if (trans_id >= rqstor->size || !test_bit(trans_id, rqstor->req_bitmap)) return VMBUS_RQST_ERROR; - } req_addr = rqstor->req_arr[trans_id]; - rqstor->req_arr[trans_id] = rqstor->next_request_id; - rqstor->next_request_id = trans_id; + if (rqst_addr == VMBUS_RQST_ADDR_ANY || req_addr == rqst_addr) { + rqstor->req_arr[trans_id] = rqstor->next_request_id; + rqstor->next_request_id = trans_id; - /* The already held spin lock provides atomicity */ - bitmap_clear(rqstor->req_bitmap, trans_id, 1); + /* The already held spin lock provides atomicity */ + bitmap_clear(rqstor->req_bitmap, trans_id, 1); + } + + return req_addr; +} +EXPORT_SYMBOL_GPL(__vmbus_request_addr_match); + +/* + * vmbus_request_addr_match - Clears/removes @trans_id from the @channel's + * requestor, provided the memory address stored at @trans_id equals @rqst_addr + * (or provided @rqst_addr matches the sentinel value VMBUS_RQST_ADDR_ANY). + * + * Returns the memory address stored at @trans_id, or VMBUS_RQST_ERROR if + * @trans_id is not contained in the requestor. + * + * Acquires and releases the requestor spin lock. + */ +u64 vmbus_request_addr_match(struct vmbus_channel *channel, u64 trans_id, + u64 rqst_addr) +{ + struct vmbus_requestor *rqstor = &channel->requestor; + unsigned long flags; + u64 req_addr; + spin_lock_irqsave(&rqstor->req_lock, flags); + req_addr = __vmbus_request_addr_match(channel, trans_id, rqst_addr); spin_unlock_irqrestore(&rqstor->req_lock, flags); + return req_addr; } +EXPORT_SYMBOL_GPL(vmbus_request_addr_match); + +/* + * vmbus_request_addr - Returns the memory address stored at @trans_id + * in @rqstor. Uses a spin lock to avoid race conditions. + * @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_channel *channel, u64 trans_id) +{ + return vmbus_request_addr_match(channel, trans_id, VMBUS_RQST_ADDR_ANY); +} EXPORT_SYMBOL_GPL(vmbus_request_addr); diff --git a/include/linux/hyperv.h b/include/linux/hyperv.h index a7cb596d893b1..c77d78f34b96a 100644 --- a/include/linux/hyperv.h +++ b/include/linux/hyperv.h @@ -788,6 +788,7 @@ struct vmbus_requestor { #define VMBUS_NO_RQSTOR U64_MAX #define VMBUS_RQST_ERROR (U64_MAX - 1) +#define VMBUS_RQST_ADDR_ANY U64_MAX /* NetVSC-specific */ #define VMBUS_RQST_ID_NO_RESPONSE (U64_MAX - 2) /* StorVSC-specific */ @@ -1042,6 +1043,10 @@ struct vmbus_channel { }; u64 vmbus_next_request_id(struct vmbus_channel *channel, u64 rqst_addr); +u64 __vmbus_request_addr_match(struct vmbus_channel *channel, u64 trans_id, + u64 rqst_addr); +u64 vmbus_request_addr_match(struct vmbus_channel *channel, u64 trans_id, + 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)
The function can be used to retrieve and clear/remove a transation ID from a channel requestor, provided the memory address corresponding to the ID equals a specified address. The function, and its 'lockless' variant __vmbus_request_addr_match(), will be used by hv_pci. Refactor vmbus_request_addr() to reuse the 'newly' introduced code. No functional change. Suggested-by: Michael Kelley <mikelley@microsoft.com> Signed-off-by: Andrea Parri (Microsoft) <parri.andrea@gmail.com> --- drivers/hv/channel.c | 65 ++++++++++++++++++++++++++++++------------ include/linux/hyperv.h | 5 ++++ 2 files changed, 52 insertions(+), 18 deletions(-)