> -----Original Message----- > From: Greg KH [mailto:greg@xxxxxxxxx] > Sent: Wednesday, May 11, 2011 4:57 PM > To: KY Srinivasan > Cc: gregkh@xxxxxxx; devel@xxxxxxxxxxxxxxxxxxxxxx; 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. Regards, K. Y _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
