[PATCH] hv_utils: drain the timesync packets on onchannelcallback

[PATCH] hv_utils: drain the timesync packets on onchannelcallback

[Vineeth Pillai]

There could be instances where a system stall prevents the timesync
packaets to be consumed. And this might lead to more than one packet
pending in the ring buffer. Current code empties one packet per callback
and it might be a stale one. So drain all the packets from ring buffer
on each callback.

Signed-off-by: Vineeth Pillai <viremana@linux.microsoft.com>
---
 drivers/hv/hv_util.c | 19 ++++++++++++++++---
 1 file changed, 16 insertions(+), 3 deletions(-)

diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
index 1357861fd8ae..c0491b727fd5 100644
--- a/drivers/hv/hv_util.c
+++ b/drivers/hv/hv_util.c
@@ -387,10 +387,23 @@ static void timesync_onchannelcallback(void *context)
 	struct ictimesync_ref_data *refdata;
 	u8 *time_txf_buf = util_timesynch.recv_buffer;
 
-	vmbus_recvpacket(channel, time_txf_buf,
-			 HV_HYP_PAGE_SIZE, &recvlen, &requestid);
+	/*
+	 * Drain the ring buffer and use the last packet to update
+	 * host_ts
+	 */
+	while (1) {
+		int ret = vmbus_recvpacket(channel, time_txf_buf,
+					   HV_HYP_PAGE_SIZE, &recvlen,
+					   &requestid);
+		if (ret) {
+			pr_warn("TimeSync IC pkt recv failed (Err: %d)\n",
+				ret);
+			break;
+		}
+
+		if (!recvlen)
+			break;
 
-	if (recvlen > 0) {
 		icmsghdrp = (struct icmsg_hdr *)&time_txf_buf[
 				sizeof(struct vmbuspipe_hdr)];
 
-- 
2.17.1

RE: [PATCH] hv_utils: drain the timesync packets on onchannelcallback

[Michael Kelley]

From: Vineeth Pillai <viremana@linux.microsoft.com> Sent: Wednesday, August 19, 2020 10:48 AM
> 
> There could be instances where a system stall prevents the timesync
> packaets to be consumed. And this might lead to more than one packet
> pending in the ring buffer. Current code empties one packet per callback
> and it might be a stale one. So drain all the packets from ring buffer
> on each callback.
> 
> Signed-off-by: Vineeth Pillai <viremana@linux.microsoft.com>
> ---
>  drivers/hv/hv_util.c | 19 ++++++++++++++++---
>  1 file changed, 16 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c
> index 1357861fd8ae..c0491b727fd5 100644
> --- a/drivers/hv/hv_util.c
> +++ b/drivers/hv/hv_util.c
> @@ -387,10 +387,23 @@ static void timesync_onchannelcallback(void *context)
>  	struct ictimesync_ref_data *refdata;
>  	u8 *time_txf_buf = util_timesynch.recv_buffer;
> 
> -	vmbus_recvpacket(channel, time_txf_buf,
> -			 HV_HYP_PAGE_SIZE, &recvlen, &requestid);
> +	/*
> +	 * Drain the ring buffer and use the last packet to update
> +	 * host_ts
> +	 */
> +	while (1) {
> +		int ret = vmbus_recvpacket(channel, time_txf_buf,
> +					   HV_HYP_PAGE_SIZE, &recvlen,
> +					   &requestid);
> +		if (ret) {
> +			pr_warn("TimeSync IC pkt recv failed (Err: %d)\n",
> +				ret);

Let's use pr_warn_once().

If there's a packet at the head of the ring buffer that specifies a bogus length,
we could take the error path.  But the bad packet stays at the head of the ring buffer,
so if we end up back here again, we'll spit out the same error message.  We
actually should not end up here again because Hyper-V shouldn't interrupt
when adding a packet to a non-empty ring buffer, but who knows what might
happen.

> +			break;
> +		}
> +
> +		if (!recvlen)
> +			break;
> 
> -	if (recvlen > 0) {
>  		icmsghdrp = (struct icmsg_hdr *)&time_txf_buf[
>  				sizeof(struct vmbuspipe_hdr)];
> 
> --
> 2.17.1

Re: [PATCH] hv_utils: drain the timesync packets on onchannelcallback

[Vineeth Pillai]

Hi Michael,

> > +			pr_warn("TimeSync IC pkt recv failed (Err: %d)\n",
> > +				ret);
> 
> Let's use pr_warn_once().
> 
> If there's a packet at the head of the ring buffer that specifies a bogus length,
> we could take the error path.  But the bad packet stays at the head of the ring buffer,
> so if we end up back here again, we'll spit out the same error message.  We
> actually should not end up here again because Hyper-V shouldn't interrupt
> when adding a packet to a non-empty ring buffer, but who knows what might
> happen.
> 
Valid point, will fix this in the next iteration.

Thanks,
Vineeth