On Wed, May 11, 2011 at 10:55:52PM +0000, KY Srinivasan wrote:
>
>
> > -----Original Message-----
> > From: Greg KH [mailto:[email protected]]
> > Sent: Wednesday, May 11, 2011 4:57 PM
> > To: KY Srinivasan
> > Cc: [email protected]; [email protected]; 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).
Yeah, I thought so...
> > > 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.
Ok, that would be great.
greg k-h
_______________________________________________
devel mailing list
[email protected]
http://driverdev.linuxdriverproject.org/mailman/listinfo/devel
