[PATCH 206/206] Staging: hv: Get rid of the function count_hv_channel()

[KY Srinivasan]

> -----Original Message-----
> From: KY Srinivasan
> Sent: Wednesday, May 11, 2011 6:56 PM
> To: 'Greg KH'
> Cc: gregkh at suse.de; devel at linuxdriverproject.org; Haiyang Zhang; Abhishek
> Kane (Mindtree Consulting PVT LTD); Hank Janssen
> Subject: RE: [PATCH 206/206] Staging: hv: Get rid of the function
> count_hv_channel()
> 
> 
> 
> > -----Original Message-----
> > From: Greg KH [mailto:greg at kroah.com]
> > Sent: Wednesday, May 11, 2011 4:57 PM
> > To: KY Srinivasan
> > Cc: gregkh at suse.de; devel at linuxdriverproject.org; Haiyang Zhang; Abhishek
> > Kane (Mindtree Consulting PVT LTD); Hank Janssen
> > Subject: Re: [PATCH 206/206] Staging: hv: Get rid of the function
> > count_hv_channel()
> >
> > On Tue, May 10, 2011 at 07:57:12AM -0700, K. Y. Srinivasan wrote:
> > > Get rid of the function count_hv_channel()
> > > by inlining the code.
> >
> > Close, but not quite:
> >
> > > +static atomic_t num_channels;
> >
> > Why is this atomic?
> 
> The original code had a spin lock protecting this counter; so I made the
> variable atomic and eliminated the locking around the increment. Now
> looking at the code more carefully, this code itself is single threaded and
> so no locking should be necessary (I will check with the windows guys if this
> is the case).
> 
> >
> > >  struct vmbus_channel_message_table_entry {
> > >  	enum vmbus_channel_message_type message_type;
> > >  	void (*message_handler)(struct vmbus_channel_message_header
> > *msg);
> > > @@ -316,21 +318,6 @@ void free_channel(struct vmbus_channel *channel)
> > >  DECLARE_COMPLETION(hv_channel_ready);
> > >
> > >  /*
> > > - * Count initialized channels, and ensure all channels are ready when
> > hv_vmbus
> > > - * module loading completes.
> > > - */
> > > -static void count_hv_channel(void)
> > > -{
> > > -	static int counter;
> > > -	unsigned long flags;
> > > -
> > > -	spin_lock_irqsave(&vmbus_connection.channel_lock, flags);
> > > -	if (++counter == MAX_MSG_TYPES)
> > > -		complete(&hv_channel_ready);
> > > -	spin_unlock_irqrestore(&vmbus_connection.channel_lock, flags);
> > > -}
> > > -
> > > -/*
> > >   * vmbus_process_rescind_offer -
> > >   * Rescind the offer by initiating a device removal
> > >   */
> > > @@ -433,7 +420,16 @@ static void vmbus_process_offer(struct work_struct
> > *work)
> > >
> > >  				pr_info("%s\n", hv_cb_utils[cnt].log_msg);
> > >
> > > -				count_hv_channel();
> > > +				/*
> > > +				 * Count initialized channels, and ensure
> > > +				 * all channels are ready when hv_vmbus
> > > +				 * module loading completes.
> > > +				 */
> > > +
> > > +				atomic_inc(&num_channels);
> > > +				if (atomic_read(&num_channels)
> > > +					 == MAX_MSG_TYPES)
> > > +					complete(&hv_channel_ready);
> >
> > I don't see why this needs to be atomic, if you properly lock things it
> > can just be a variable, right?
> >
> > And what's copying this from ever terminating if the count never
> > increments?  This logic is very strange and should be reworked to not do
> > this kind of "loop until something else counts high enough" code.
> 
> I need to check with Haiyang who I think introduced this. If I recall correctly
> this was to fix a bug and the patch was accepted only a few months ago.
> I will check and see if some other solution is possible.

Greg, I have completely re-worked this patch and I will send it to you ASAP. I have 
circulated the patches internally and I hope to send it to you soon. With this, I have 
addressed all the review comments you had given me earlier. I know you are very busy
but if you let me know what else needs to be fixed, I can work on that.

Regards,

K. Y