LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
From: KY Srinivasan <kys@microsoft.com>
To: Jiri Slaby <jirislaby@gmail.com>
Cc: "gregkh@suse.de" <gregkh@suse.de>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"devel@linuxdriverproject.org" <devel@linuxdriverproject.org>,
	"virtualization@lists.osdl.org" <virtualization@lists.osdl.org>
Subject: RE: [PATCH 2/3]: Staging: hv: Use native wait primitives
Date: Tue, 15 Feb 2011 13:35:56 +0000	[thread overview]
Message-ID: <FB42D5CCD7B5934EB1827DB5ED9B850E07085CF8@TK5EX14MBXC104.redmond.corp.microsoft.com> (raw)
In-Reply-To: <4D5A4565.5030501@gmail.com>



> -----Original Message-----
> From: Jiri Slaby [mailto:jirislaby@gmail.com]
> Sent: Tuesday, February 15, 2011 4:21 AM
> To: KY Srinivasan
> Cc: gregkh@suse.de; linux-kernel@vger.kernel.org;
> devel@linuxdriverproject.org; virtualization@lists.osdl.org
> Subject: Re: [PATCH 2/3]: Staging: hv: Use native wait primitives
> 
> On 02/11/2011 06:59 PM, K. Y. Srinivasan wrote:
> > In preperation for getting rid of the osd layer; change
> > the code to use native wait interfaces. As part of this,
> > fixed the buggy implementation in the osd_wait_primitive
> > where the condition was cleared potentially after the
> > condition was signalled.
> ...
> > @@ -566,7 +567,11 @@ int vmbus_establish_gpadl(struct vmbus_channel
> *channel, void *kbuffer,
> >
> >  		}
> >  	}
> > -	osd_waitevent_wait(msginfo->waitevent);
> > +	wait_event_timeout(msginfo->waitevent,
> > +				msginfo->wait_condition,
> > +				msecs_to_jiffies(1000));
> > +	BUG_ON(msginfo->wait_condition == 0);
> 
> The added BUG_ONs all over the code look scary. These shouldn't be
> BUG_ONs at all. You should maybe warn and bail out, but not kill the
> whole machine.

This is Linux code running as a guest on a Windows host; and so the guest cannot
tolerate a failure of the host. In the cases where I have chosen to BUG_ON, there 
is no reasonable recovery possible when the host is non-functional (as determined 
by a non-responsive host). 

> 
> And looking at the code, more appropriate would be completion instead of
> wait events.
> 
> And msecs_to_jiffies(1000) == HZ.

Agreed. In this first round of cleanup, I chose to keep the primitives as they were in osd.c. Greg, if it is ok with you, I will send you a patch that fixes these issues on top of the patches I have already sent.

Regards,

K. Y
> 
> > @@ -689,7 +693,8 @@ static void vmbus_ongpadl_torndown(
> >  				memcpy(&msginfo->response.gpadl_torndown,
> >  				       gpadl_torndown,
> >  				       sizeof(struct
> vmbus_channel_gpadl_torndown));
> > -				osd_waitevent_set(msginfo->waitevent);
> > +				msginfo->wait_condition = 1;
> > +				wake_up(&msginfo->waitevent);
> >  				break;
> >  			}
> >  		}
> > @@ -730,7 +735,8 @@ static void vmbus_onversion_response(
> >  			memcpy(&msginfo->response.version_response,
> >  			      version_response,
> >  			      sizeof(struct vmbus_channel_version_response));
> > -			osd_waitevent_set(msginfo->waitevent);
> > +			msginfo->wait_condition = 1;
> > +			wake_up(&msginfo->waitevent);
> >  		}
> >  	}
> >  	spin_unlock_irqrestore(&vmbus_connection.channelmsg_lock, flags);
> 
> regards,
> --
> js


  reply	other threads:[~2011-02-15 13:35 UTC|newest]

Thread overview: 8+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2011-02-11 17:59 K. Y. Srinivasan
2011-02-15  9:20 ` Jiri Slaby
2011-02-15 13:35   ` KY Srinivasan [this message]
2011-02-15 14:03     ` Greg KH
2011-02-15 16:22       ` KY Srinivasan
2011-02-15 16:29         ` Greg KH
2011-02-15 17:52           ` KY Srinivasan
2011-02-15 18:44             ` Dmitry Torokhov

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=FB42D5CCD7B5934EB1827DB5ED9B850E07085CF8@TK5EX14MBXC104.redmond.corp.microsoft.com \
    --to=kys@microsoft.com \
    --cc=devel@linuxdriverproject.org \
    --cc=gregkh@suse.de \
    --cc=jirislaby@gmail.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=virtualization@lists.osdl.org \
    --subject='RE: [PATCH 2/3]: Staging: hv: Use native wait primitives' \
    /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

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).