Re: [patch 36/61] lock validator: special locking: serial

[Ingo Molnar]

* Andrew Morton <akpm@xxxxxxxx> wrote:

> btw, I was looking at this change:

> @@ -1003,6 +1003,7 @@ unsigned ata_exec_internal(struct ata_de
>  	unsigned int err_mask;
>  	int rc;
>  
> +	init_completion(&wait);
>  	spin_lock_irqsave(ap->lock, flags);
>  
>  	/* no internal command while frozen */
> 
> That local was already initialised with DEFINE_COMPLETION().  Am 
> surprised that an init_completion() also was needed?

That's a fundamental problem of DECLARE_COMPLETION() done on the kernel 
stack - it does build-time initialization with no opportunity to inject 
any runtime logic. (which lockdep would need. Maybe i missed some clever 
way to add a runtime callback into the initialization? [*])

Btw., there is no danger from missing the initialization of a wait 
structure: lockdep will detect "uninitialized" on-stack locks and will 
complain about it and turn itself off. [this happened a few times during 
development - that's how those init_completion() calls got added]

But at a minimum these initializations need to become lockdep-specific 
key-reinits - otherwise there will be impact to non-lockdep kernels too.

	Ingo

[*] the only solution i can see is to introduce 
DECLARE_COMPLETION_ONSTACK(), which could call a function with &wait 
passed in, where that function would return with a structure. The macro 
magic would resolve to something like:

  struct completion wait = lockdep_init_completion(&wait);

and thus the structure would be initialized. But this method cannot be 
used for static scope uses of DECLARE_COMPLETION, because it's not a 
constant initializer. So we'd definitely have to make a distinction in 
terms of _ONSTACK(). Is there really no compiler feature that could help 
us out here?
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/