Re: [RFC PATCH 1/2] regmap: add configurable lock class key for lockdep

[Nicolas Boichat]

On Mon, Jun 29, 2015 at 8:59 PM, Lars-Peter Clausen <lars@xxxxxxxxxx> wrote:
> On 06/29/2015 02:51 PM, Nicolas Boichat wrote:
>>
>> On Fri, Jun 26, 2015 at 11:16 AM, Nicolas Boichat <drinkcat@xxxxxxxxxxxx>
>> wrote:
>>>
>>>
>>> On Fri, Jun 26, 2015 at 12:08 AM, Mark Brown <broonie@xxxxxxxxxx> wrote:
>>> [...]
>>>>>>
>>>>>> As far as I can tell we're likely to end up needing a key per regmap
>>>>>> or
>>>>>> something similar.
>>>>
>>>>
>>>>> Since the number of lockdep classes itself is also limited we should
>>>>> avoid
>>>>> creating extra lockdep classes when we can. I think the approach which
>>>>> having the option of specifying a lockdep class in the regmap config
>>>>> will be
>>>>> ok. The only case it can't handle if we nest instances with the same
>>>>> config,
>>>>> but I don't really see valid use scases for that at the moment.
>>>>
>>>>
>>>> Oh, ffs.  This just keeps getting better.  I hadn't been aware of that
>>>> limitation.  We still have the problem that this needs to be something
>>>> users can understand rather than something that's just "define something
>>>> here in one of your drivers if you're running into problems with
>>>> spurious warnings" here.  That's always been the biggest problem here
>>>> (once we got past the "what is this supposed to do in the first place?"
>>>> issues).
>>>
>>>
>>> I found that V4L2 uses separate lockdep classes for each of their
>>> v4l2_ctrl. This was introduced in 6cd247ef22e "[media] v4l2-ctrls:
>>> eliminate lockdep false alarms for struct v4l2_ctrl_handler.lock"
>>>
>>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=6cd247ef22e),
>>> so we could possibly take that approach.
>>>
>>> On my system, I have:
>>> # cat /proc/lockdep_stats
>>>   lock-classes:                         1241 [max: 8191]
>>>   direct dependencies:                  7364 [max: 32768]
>>>   indirect dependencies:               27686
>>>   all direct dependencies:            158097
>>>   dependency chains:                   10011 [max: 65536]
>>>   dependency chain hlocks:             38887 [max: 327680]
>>>   in-hardirq chains:                      92
>>>   in-softirq chains:                     372
>>>   in-process chains:                    9547
>>>   stack-trace entries:                107703 [max: 524288]
>>>
>>> So, at least on that platform, there is some room to grow...
>>>
>>> I'm just afraid that implementing this may require creating a bunch of
>>> macros to wrap all regmap_init_[i2c/spi/...] functions, as the lockdep
>>> classes need to be statically allocated... Unless we find a different
>>> solution than what V4L2 does.
>>
>>
>> Following up on this. Lars-Peter's comments also highlights that we
>> have no good way to figure out which regmap requires a separate maps,
>> no clear hierarchy we can know about in advance, so we should put each
>> regmap in its own class.
>>
>> The main issue is that the keys need to be allocated statically. We
>> have 2 options to do this:
>>
>> 1. mutex_init and v4l2_ctrl_handler_init solve this issue by being a
>> preprocessor macro that first allocates a static lock_class_key, then
>> calls the real init function.
>> This is not so practical in the case of regmap, as we have 14
>> different init functions ([devm_]regmap_init[_bus_type]), that would
>> each require a wrapper.
>>
>> 2. Bus registration takes a different approach
>>
>> (https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/?id=be871b7e5):
>> struct bus_type (statically allocated for each bus) has a lock_key
>> member: "struct lock_class_key lock_key;".
>> In the context of regmaps, that would mean adding a "lock_key" member
>> to regmap_config. I did a quick implementation of this idea, and it
>> seems to work, without modification to the rt5677 driver. The only
>> issue with this is that regmap_config cannot be const anymore: we'd
>> need to remove the const specifier in all drivers that use regmaps.
>
>
> Yeah, I though about that as well, but the problem is the regmap_config is
> only valid during regmap_init() and can for example be placed on the stack.
> In which case it won't work anymore.

Then we might need to make it a requirement... In any case, lockdep
will throw a warning if the lock_key is allocated on the stack (or
kalloc'ed).

>>
>> Both alternatives would mean that all regmaps created from 1. the same
>> line of code, or 2. the same regmap_config, would share the same
>> class. That may not be an issue, however (do we have an example of
>> different regmaps created from the same line/config that need to call
>> each other?), and the custom mutex workaround is still available....
>>
>> Any preference between a bunch of macros, and adding a non-const
>> member to regmap_config? Or maybe someone has a better idea?
>
>
> Maybe we are just over-thinking this and should just add one key to each
> regmap instance. That solves the issue without requiring the any user
> interaction.

Yes, I agree. What I'm trying to answer above is how to do that, at
least partially. I have no idea how to allocate one class per regmap,
the above only does one class per init call or per regmap_config.

regmap instances are kalloc'ed, so they cannot contain the
lock_class_key, which needs to be statically allocated (in .data).
Another option would be to preallocate a bunch of lock_class_key in
regmap.c, and pick from that, but that's terribly hacky...
--
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/