> -----Original Message----- > From: Vitaly Kuznetsov [mailto:vkuznets@xxxxxxxxxx] > Sent: Thursday, September 29, 2016 2:22 AM > To: KY Srinivasan <kys@xxxxxxxxxxxxx>; Long Li <longli@xxxxxxxxxxxxx> > Cc: Haiyang Zhang <haiyangz@xxxxxxxxxxxxx>; > devel@xxxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx > Subject: Re: [PATCH] hv: do not lose pending heartbeat vmbus packets > > Long Li <longli@xxxxxxxxxxxxxxxxxxxxxx> writes: > > > From: Long Li <longli@xxxxxxxxxxxxx> > > > > The host keeps sending heartbeat packets independent of guest > responding to them. In some situations, there might be multiple heartbeat > packets pending in the ring buffer. Don't lose them, read them all. > > > > Signed-off-by: Long Li <longli@xxxxxxxxxxxxx> > > Long, K. Y., > > it seems this patch didn't make it to char-misc tree and it looks like an > important fix. A couple of nitpicks below, > > > --- > > drivers/hv/hv_util.c | 10 +++++++--- > > 1 file changed, 7 insertions(+), 3 deletions(-) > > > > diff --git a/drivers/hv/hv_util.c b/drivers/hv/hv_util.c index > > d5acaa2..9dc6372 100644 > > --- a/drivers/hv/hv_util.c > > +++ b/drivers/hv/hv_util.c > > @@ -283,10 +283,14 @@ static void heartbeat_onchannelcallback(void > *context) > > u8 *hbeat_txf_buf = util_heartbeat.recv_buffer; > > struct icmsg_negotiate *negop = NULL; > > > > - vmbus_recvpacket(channel, hbeat_txf_buf, > > - PAGE_SIZE, &recvlen, &requestid); > > + while (1) { > > + > > + vmbus_recvpacket(channel, hbeat_txf_buf, > > + PAGE_SIZE, &recvlen, &requestid); > > We should check vmbus_recvpacket() return value as well. E.g. > hv_ringbuffer_read() may return -EAGAIN in case we didn't receive the > whole packet (and we do this check in other drivers, see > storvsc_on_channel_callback() for example). I agree with you, we should check for -EAGAIN. This should also be done in storvsc_on_channel_callback. I think the chance of hv_ringbuffer_read() returning -EAGAIN is almost zero. Because read_index and write_index are updated after the whole packet is written to the ring buffer, and protected by memory barriers. So getting a partial read is impossible, unless the host is doing something wrong. Checking for recvlen is safe, because it's always set to 0 at the beginning of hv_ringbuffer_read(). Anyway, we should check for -EAGAIN for all hyperv drivers on read. I think this is a separate issue on how we deal with a buggy host. Will send another set of patches . > > > + > > + if (!recvlen) > > so this should be 'if (ret || !recvlen)' > > > + break; > > > > - if (recvlen > 0) { > > icmsghdrp = (struct icmsg_hdr *)&hbeat_txf_buf[ > > sizeof(struct vmbuspipe_hdr)]; > > -- > Vitaly _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/driverdev-devel
