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 andre at freebsd.org
Thu Oct 25 16:16:01 UTC 2012


On 25.10.2012 05:14, Attilio Rao wrote:
> On Wed, Oct 24, 2012 at 9:13 PM, Attilio Rao <attilio at freebsd.org> wrote:
>> On Wed, Oct 24, 2012 at 7:14 PM, John Baldwin <jhb at freebsd.org> wrote:
>>> On Wednesday, October 24, 2012 11:41:24 am Attilio Rao wrote:
>>>> On Wed, Oct 24, 2012 at 4:36 PM, John Baldwin <jhb at freebsd.org> wrote:
>>>>> On Wednesday, October 24, 2012 11:24:22 am Attilio Rao wrote:
>>>>>> On Wed, Oct 24, 2012 at 4:09 PM, Attilio Rao <attilio at freebsd.org> wrote:
>>>>> union mtx_foo {
>>>>>          struct mtx lock;
>>>>>          char junk[roundup2(sizeof(struct mtx), CACHE_LINE_SIZE)];
>>>>> } __aligned_CACHE_LINE_SIZE;
>>>>>
>>>>>> then let mtx_* functions to accept void ptrs and cast them to struct
>>>>>> mtx as long as the functions enter.
>>>>>
>>>>> Eh, that removes all compile time type checks.  That seems very dubious to me.
>>>>
>>>> Well right now fast path already has a fair amount of macros wrapping
>>>> the operations, which don't really enforce any type checks.
>>>
>>> Sure they do.  They still call a function that takes a 'struct mtx *' even
>>> if it isn't called in the fast path.  If you pass a 'struct sx *' to
>>> mtx_lock() it will fail to compile.  That needs to stay that way.
>>
>> I think that with some trickery using CTASSERT() and typeof() we may
>> be able to enforce sanity even with void * arguments.
>
> I think I've had a better idea for this.
> In our locking scheme we already rely on the fact that lock_object
> will always be present in all our locking primitives and that it will
> be the first object of every locking primitives. This is an assumption
> we must live with in order to correctly implement lock classes. I
> think we can use the same concept in order to use the same KPI for the
> 2 different structures (struct mtx and struct mtx_unshare) and keep
> the compile time ability to find stupid bugs.

I think we're completely overdoing it.  On amd64 the size difference
of 64B cache line aligning and padding all mutex, sx and rw_lock
structures adds the tiny amount of 16K on a GENERIC kernel of 19MB.
That is a 0.009% increase in size.  Of course dynamically allocated
memory that includes a mutex grows a tiny bit at well.

Hence I propose to unconditionally slap __aligned(CACHE_LINE_SIZE) into
all locking structures and be done with it.  As an added benefit we
don't have to worry about individual micro-optimizations on a case by
case basis.

> What I propose is that we assume mtx_lock remains always the second
> member of the struct mtx/mtx_unshare and no other lock is allowed to
> use such member name. This happens natually nowadays so there is no
> problem in having such a rule. What does that allows to do is to pass
> the address of the mtx_lock member to the underlying functions and
> from there we can get back the address of the mutex (because we assume
> that mtx_lock will be just after the first mandatory member
> lock_object).
>
> Here is a patch that implements mtx_init() in the way I think about:
> http://people.freebsd.org/~attilio/mtx_unshare_poc.patch
>
> this should give us all the desired effects. In this patch I've used
> volatile uintptr_t * but it can certainly be void * too, if you prefer
> less verbose.
> If you agree with this idea I can hack a patch right away.

-- 
Andre



More information about the svn-src-user mailing list