svn commit: r241889 - in user/andre/tcp_workqueue/sys: arm/arm cddl/compat/opensolaris/kern cddl/contrib/opensolaris/uts/common/dtrace cddl/contrib/opensolaris/uts/common/fs/zfs ddb dev/acpica dev/...

[Andre Oppermann]

On 23.10.2012 14:53, John Baldwin wrote:
> On Tuesday, October 23, 2012 5:35:03 am Andre Oppermann wrote:
>> On 22.10.2012 19:10, Bruce Evans wrote:
>>> On Mon, 22 Oct 2012, Andre Oppermann wrote:
>>>
>>>> Log:
>>>>   Second pass at making global mutexes use MTX_DEF_SYSINIT().
>>>
>>>> Modified: user/andre/tcp_workqueue/sys/arm/arm/busdma_machdep.c
>>>> ==============================================================================
>>>> --- user/andre/tcp_workqueue/sys/arm/arm/busdma_machdep.c    Mon Oct 22 14:10:17 2012    (r241888)
>>>> +++ user/andre/tcp_workqueue/sys/arm/arm/busdma_machdep.c    Mon Oct 22 14:18:22 2012    (r241889)
>>>> @@ -163,9 +163,7 @@ static TAILQ_HEAD(,bus_dmamap) dmamap_fr
>>>> #define BUSDMA_STATIC_MAPS    500
>>>> static struct bus_dmamap map_pool[BUSDMA_STATIC_MAPS];
>>>>
>>>> -static struct mtx busdma_mtx;
>>>> -
>>>> -MTX_SYSINIT(busdma_mtx, &busdma_mtx, "busdma lock", MTX_DEF);
>>>> +static MTX_DEF_SYSINIT(busdma_mtx, "busdma lock", MTX_DEF);
>>>
>>> The verboser (sic) name is uglier.
>>>
>>> But once it says DEF, the arg list shouldn't say DEF.  Either this is
>>> DEF for something unrelated to MTX_DEF, in which case it is confusing
>>> and the confusion is made worse by having MTX_ precede DEF in both,
>>> or it is the same DEF and the arg is redundant, or it implies MTX_DEF
>>> by default but this can be changed, in which case it is confusing.
>>
>> You're right, it is a bit confusing.  However the DEF in MTX_DEF_SYSINIT
>> stands for DEFining the global variable whereas MTX_DEF stands for a
>> default type mutex.
>>
>> Unfortunately I couldn't come up with a bette short name for MTX_DEF_SYSINIT.
>> I'm open for suggestions though.
>
> It's not super clear to me that having one macro to replace
> 'struct mtx and MTX_SYSINIT' is substantially more readable compared to just
> adding '__aligned(CACHE_LINE_SIZE)' at the end of global locks.

Struct mtx and MTX_SYSINIT always occur as pair next to each other.
It's simpler to have merged into one macro.

Instead of adding the __aligned() to ever instance it simpler to have
it in one place.  That way it also can be globally disabled if undesired.

> Note, btw, that I have local patches to make mtx_pool's use aligned mutexes,
> but they have never shown a performance improvement in real-word benchmarks
> (granted, the last set of benchmarks were run several years ago (the patches
> are old) by kris@).  OTOH, alc@ found a measurable boost by aligning a few
> global mutexes in the VM system.

Ideally the alignment doesn't show any difference to today's baseline.

There have been reports (in private) that random changes to the .bss
ordering have caused large drops in measured network performance.

With the alignment we gain the confidence that important mutexes do not
end up on the same cacheline either due to placement in the source code
nor through changes in .bss ordering.

-- 
Andre