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/...

[Attilio Rao]

On Thu, Oct 25, 2012 at 5:15 PM, Andre Oppermann <andre at freebsd.org> wrote:
> 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.

Did you see my (and also Jeff) objection to your proposal about this?
You are deliberating ignoring this?

I think that mutexes being part of structures usually don't want that
and it is really overkill. It is not the amount of wasted memory the
problem (which is also important, anyway) but the fact that
not-really-contentended sleep mutexes, will interfree with normal
structure members caching.

I think it is a very bad idea.

Attilio


-- 
Peace can only be achieved by understanding - A. Einstein