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 26.10.2012 08:27, Attilio Rao wrote:
> On Thu, Oct 25, 2012 at 10:03 PM, Andre Oppermann <andre at freebsd.org> wrote:
>> On 25.10.2012 18:21, Attilio Rao wrote:
>>> Did you see my (and also Jeff) objection to your proposal about this?
>>> You are deliberating ignoring this?
>>
>>
>> Well, I'm allowed to have a different opinion, am I?  I'm not ignoring
>> your objection in the sense as I'm not trying to commit any of this to
>> HEAD while it is disputed.
>>
>> Mind you this whole conversation was started because I was trying to solve
>> a problem with unfortunate cache line sharing for global mutexes in the
>> kernel .bss section on my *personal* svn branch.
>
> Andre,
> I'm sorry if you felt I was being harsh or confrontative. This was
> really not my intention and I apologize.

I apologize too for being a bit difficult and taking some time understand
the differences in __aligned() regarding padding behavior.

> Said that, I fail in seeing a proper technical discussion on your side
> on how what I propose is "overdoing it" or how do you plan to address
> the concerns people are raising with your proposal of bumping all lock
> sizes indiscriminately.

I'm wary of micro-optimizing and generally prefer clean and for a
reader obvious approaches.  That said my assumption on the distribution
of mutex use cases in the kernel was wrong.  By counting from a grep
it seems that about half of the mutexes could possibly benefit from
being padded and the other half doesn't because it is in structures
and next to its data.

> However, here is the first half of the patch I'd like to see in:
> http:///www.freebsd.org/~attilio/mtx_decoupled.patch
 >
> This is just the part to give the ability to crunch different
> structures to the mtx KPI. Please note that from the users perspective
> the mtx KPI remains absolutely the same, so there is theoretically no
> KPI discontinuity, the support is absolutely transparent.

This seems rather complicated.  Instead of mtxlock2mtx() wouldn't
__containerof() work just as well?  The __DEVOLATILE() looks a bit
dangerous.  Are you sure the compiler won't reorder things it should
not?

> It is not yet throughfully tested, because I first want to hear
> opinions also on nits.

Different though: Since cases where an aligned and padded mutex is
wanted it has to be specified explicitly anyway.

Can't we simplify and have the aligned one be a container of the
normal one and do some union magic?

That way the lock_* functions can stay the same and current mutex
size doesn't change.

> For example:
> - Do we need to implement mtxlock2mtx() only in kern_mutex.c? I'd say
> yes, as a general rule, but as long as this functionality relies on
> the ABI maybe it is better to leave it in _mutex.h as done in the
> patch.
> - MTX_SYSINIT() doesn't work for hypotetical mtx_unshare right now so
> it will need to have its own version. Should I add it now? Should I
> add it if and only if there is a real need? (I suspect this is likely
> because several external mutexes, that we want to convert, may use it)
> - others that came to your mind

-- 
Andre