There''s no need for a lock here, elimination of which makes the
function a leaf one, thus allowing for better (and smaller) code.
Further, use the variable next_channel according to its name - so far
it represented the most recently used channel rather than the next one
to use.
Signed-off-by: Jan Beulich <jbeulich@suse.com>
--- a/xen/arch/x86/hpet.c
+++ b/xen/arch/x86/hpet.c
@@ -402,30 +402,35 @@ static void __init hpet_fsb_cap_lookup(v
static struct hpet_event_channel *hpet_get_channel(unsigned int cpu)
{
static unsigned int next_channel;
- static spinlock_t next_lock = SPIN_LOCK_UNLOCKED;
unsigned int i, next;
struct hpet_event_channel *ch;
if ( num_hpets_used == 0 )
return hpet_events;
- spin_lock(&next_lock);
- next = next_channel = (next_channel + 1) % num_hpets_used;
- spin_unlock(&next_lock);
+ do {
+ next = next_channel;
+ if ( (i = next + 1) == num_hpets_used )
+ i = 0;
+ } while ( cmpxchg(&next_channel, next, i) != next );
/* try unused channel first */
- for ( i = next; i < next + num_hpets_used; i++ )
- {
- ch = &hpet_events[i % num_hpets_used];
+ ch = &hpet_events[i = next];
+ do {
if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) )
{
ch->cpu = cpu;
return ch;
}
- }
+ ++ch;
+ if ( ++i == num_hpets_used )
+ {
+ i = 0;
+ ch = hpet_events;
+ }
+ } while ( i != next );
- /* share a in-use channel */
- ch = &hpet_events[next];
+ /* Share an in-use channel. */
if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) )
ch->cpu = cpu;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xen.org
http://lists.xen.org/xen-devel
On 23/03/2012 09:20, "Jan Beulich" <JBeulich@suse.com> wrote:> There''s no need for a lock here, elimination of which makes the > function a leaf one, thus allowing for better (and smaller) code. > > Further, use the variable next_channel according to its name - so far > it represented the most recently used channel rather than the next one > to use.Why is the following for-loop changed into do-while? It looks like it just makes the code longer, especially as you replace a % operator with open-coded equivalent for no reason I can see. -- Keir> Signed-off-by: Jan Beulich <jbeulich@suse.com> > > --- a/xen/arch/x86/hpet.c > +++ b/xen/arch/x86/hpet.c > @@ -402,30 +402,35 @@ static void __init hpet_fsb_cap_lookup(v > static struct hpet_event_channel *hpet_get_channel(unsigned int cpu) > { > static unsigned int next_channel; > - static spinlock_t next_lock = SPIN_LOCK_UNLOCKED; > unsigned int i, next; > struct hpet_event_channel *ch; > > if ( num_hpets_used == 0 ) > return hpet_events; > > - spin_lock(&next_lock); > - next = next_channel = (next_channel + 1) % num_hpets_used; > - spin_unlock(&next_lock); > + do { > + next = next_channel; > + if ( (i = next + 1) == num_hpets_used ) > + i = 0; > + } while ( cmpxchg(&next_channel, next, i) != next ); > > /* try unused channel first */ > - for ( i = next; i < next + num_hpets_used; i++ ) > - { > - ch = &hpet_events[i % num_hpets_used]; > + ch = &hpet_events[i = next]; > + do { > if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) ) > { > ch->cpu = cpu; > return ch; > } > - } > + ++ch; > + if ( ++i == num_hpets_used ) > + { > + i = 0; > + ch = hpet_events; > + } > + } while ( i != next ); > > - /* share a in-use channel */ > - ch = &hpet_events[next]; > + /* Share an in-use channel. */ > if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) ) > ch->cpu = cpu; > > > > > _______________________________________________ > Xen-devel mailing list > Xen-devel@lists.xen.org > http://lists.xen.org/xen-devel
>>> On 23.03.12 at 11:24, Keir Fraser <keir@xen.org> wrote: > On 23/03/2012 09:20, "Jan Beulich" <JBeulich@suse.com> wrote: > >> There''s no need for a lock here, elimination of which makes the >> function a leaf one, thus allowing for better (and smaller) code. >> >> Further, use the variable next_channel according to its name - so far >> it represented the most recently used channel rather than the next one >> to use. > > Why is the following for-loop changed into do-while? It looks like it just > makes the code longer, especially as you replace a % operator with > open-coded equivalent for no reason I can see.Simply because for ( i = next; i != next; ++i ) makes no sense. Removing the % is an efficiency thing - the resulting code is not only faster, but also smaller (presumably because of the special register limitations div has). Jan>> Signed-off-by: Jan Beulich <jbeulich@suse.com> >> >> --- a/xen/arch/x86/hpet.c >> +++ b/xen/arch/x86/hpet.c >> @@ -402,30 +402,35 @@ static void __init hpet_fsb_cap_lookup(v >> static struct hpet_event_channel *hpet_get_channel(unsigned int cpu) >> { >> static unsigned int next_channel; >> - static spinlock_t next_lock = SPIN_LOCK_UNLOCKED; >> unsigned int i, next; >> struct hpet_event_channel *ch; >> >> if ( num_hpets_used == 0 ) >> return hpet_events; >> >> - spin_lock(&next_lock); >> - next = next_channel = (next_channel + 1) % num_hpets_used; >> - spin_unlock(&next_lock); >> + do { >> + next = next_channel; >> + if ( (i = next + 1) == num_hpets_used ) >> + i = 0; >> + } while ( cmpxchg(&next_channel, next, i) != next ); >> >> /* try unused channel first */ >> - for ( i = next; i < next + num_hpets_used; i++ ) >> - { >> - ch = &hpet_events[i % num_hpets_used]; >> + ch = &hpet_events[i = next]; >> + do { >> if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) ) >> { >> ch->cpu = cpu; >> return ch; >> } >> - } >> + ++ch; >> + if ( ++i == num_hpets_used ) >> + { >> + i = 0; >> + ch = hpet_events; >> + } >> + } while ( i != next ); >> >> - /* share a in-use channel */ >> - ch = &hpet_events[next]; >> + /* Share an in-use channel. */ >> if ( !test_and_set_bit(HPET_EVT_USED_BIT, &ch->flags) ) >> ch->cpu = cpu; >> >> >> >> >> _______________________________________________ >> Xen-devel mailing list >> Xen-devel@lists.xen.org >> http://lists.xen.org/xen-devel
On 23/03/2012 10:46, "Jan Beulich" <JBeulich@suse.com> wrote:>> Why is the following for-loop changed into do-while? It looks like it just >> makes the code longer, especially as you replace a % operator with >> open-coded equivalent for no reason I can see. > > Simply because > > for ( i = next; i != next; ++i ) > > makes no sense. > > Removing the % is an efficiency thing - the resulting code is not only > faster, but also smaller (presumably because of the special register > limitations div has).Does that matter? It''s the last thing done before going into deep sleep. I suppose I don''t mind that much, the whole thing just seems to make the C code longer. -- Keir
>>> On 23.03.12 at 14:52, Keir Fraser <keir@xen.org> wrote: > On 23/03/2012 10:46, "Jan Beulich" <JBeulich@suse.com> wrote: > >>> Why is the following for-loop changed into do-while? It looks like it just >>> makes the code longer, especially as you replace a % operator with >>> open-coded equivalent for no reason I can see. >> >> Simply because >> >> for ( i = next; i != next; ++i ) >> >> makes no sense. >> >> Removing the % is an efficiency thing - the resulting code is not only >> faster, but also smaller (presumably because of the special register >> limitations div has). > > Does that matter? It''s the last thing done before going into deep sleep. > > I suppose I don''t mind that much, the whole thing just seems to make the C > code longer.Okay, let''s forget about it then. (I could certainly make the code shorter using a few comma expressions, but I don''t think you''d like that any better.) Jan
On 23/03/2012 14:40, "Jan Beulich" <JBeulich@suse.com> wrote:>> Does that matter? It''s the last thing done before going into deep sleep. >> >> I suppose I don''t mind that much, the whole thing just seems to make the C >> code longer. > > Okay, let''s forget about it then. (I could certainly make the code > shorter using a few comma expressions, but I don''t think you''d > like that any better.)Not really! Getting rid of the lock and using cmpxchg, and making next_channel have the right meaning all makes sense. So a patch that just replaced the locked region, but didn''t touch the following for-loop, would make sense to me. You can consider that Acked if you want to apply it. -- Keir