> -----Original Message----- > From: KY Srinivasan > Sent: Wednesday, May 11, 2011 6:56 PM > To: 'Greg KH' > 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() > > > > > -----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. 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 _______________________________________________ devel mailing list devel@xxxxxxxxxxxxxxxxxxxxxx http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
