[intel-platform-dev] [intel-platform-discuss] Code review for several MONITOR/MWAIT idle loop CRs

[Bill Holler]

 > This assumes that all mwait_info structs will need
 > to be aligned to the same boundaries. (I don't know if
 > that's a safe assumption in this case.

The code does assume all cpus have the same mwait buffer alignment
requirement and size.  The boot cpu allocates the mcpu_mwait buffer
for other cpus using its own cpuid_pass1 mwait size info.  See
mp_startup_init() line 252 which executed before the other cpu is
online.  This is done because the starting cpu cannot idle until its idle
data structures are initialized (for example if the starting cpu tries to
acquire a held lock).

Is it possible for a system to have cpus with different mwait buffer
alignment requirements?  I assume this would require the cpus have
different cache line sizes.  Several other parts of the kernel assume
all cpus have the same cache line size.  Humm.

At worst on a hypothetical mixed-mwait-buffer-size-machine the
starting cpu could suffer from false sharing of its mcpu_mwait buffer
if it requires a larger mwait buffer with different alignment.  This
could possibly cause false idle wakeups.  Due to the way the idle loop
works, false wakeup will cause an increased power consumption but
does not hurt performance (actually increases performance slightly).

Regards,
Bill


Joe Bonasera wrote:
>
> Creating a kmem cache for these would waste lots of memory.
> Distinct caches have lots of overhead, especially given we
> allocate just one of these per processor and don't ever free
> or re-allocate them.
>
> johansen-osdev at sun.com wrote:
>> Bill,
>> These changes generally quite good.  The only thing that I would
>> consider changing would be the use of kmem_alloc() and size_actual,
>> buf_actual in the struct mwait_info.
>>
>> If you instead use kmem_cache_alloc() for these structures, you can
>> specify the required alignment when you create the cache as part of
>> kmem_cache_create().  This assumes that all mwait_info structs will need
>> to be aligned to the same boundaries.  (I don't know if that's a safe
>> assumption in this case.)
>>
>> Hope that helps,
>>
>> -j
>>
>>
>> On Tue, Aug 28, 2007 at 04:14:45PM -0700, Bill Holler wrote:
>>> http://cr.opensolaris.org/~bholler/mwait_fixes
>>>
>>> Here is a webrev code review for the following idle loop MONITOR/MWAIT
>>> CRs (Change Requests):
>>>
>>> 6577948 <http://monaco.sfbay.sun.com/detail.jsp?cr=6577948> 
>>> mach_alloc_mwait leaks memory when a CPU fails to start
>>> 6588054 <http://monaco.sfbay.sun.com/detail.jsp?cr=6588054> panic() 
>>> in mach_alloc_mwait() should be changed to degraded operation...
>>> 6596141 <http://monaco.sfbay.sun.com/detail.jsp?cr=6596141> Solaris 
>>> should not use an unmodified MWAIT idle loop on AMD 10h due to 
>>> increased power consumption
>>>
>>>
>>> While these are relatively low priority, I want to get these in soon 
>>> for
>>> maximum S10U5 soak time.
>>>
>>> Testing completed:
>>> 1) The x86 build of this kernel has passed ON PIT DIY.  The SPARC build
>>>      is currently in ON PIT DIY and looks ok.  (There are no common 
>>> changes
>>>      that should effect SPARC.)
>>> 2) libmicro did not regress.
>>>     PERF PIT is not planned as these are enable/disable changes 
>>> which should
>>>     not change performance in either state.
>>> 3) The memory leak is fiked in unit testing with all non-boot cpus 
>>> forced to
>>>     fail to online using cpufailset debug hook.
>>> 4) My desktop has been running this for 2 weeks without incident.
>>>
>>> Ongoing testing:
>>> 1) Barcelona testing is ongoing.
>>> 2) 6588054 error injection testing is ongoing.
>>>
>>> Thank you,
>>> Bill Holler
>>>
>>> _______________________________________________
>>> intel-platform-discuss mailing list
>>> intel-platform-discuss at opensolaris.org
>>> http://mail.opensolaris.org/mailman/listinfo/intel-platform-discuss
>> _______________________________________________
>> intel-platform-discuss mailing list
>> intel-platform-discuss at opensolaris.org
>> http://mail.opensolaris.org/mailman/listinfo/intel-platform-discuss
>