Re: [Intel-gfx] [PATCH 05/13] drm: locking&new iterators for connector_list

[Jani Nikula <jani.nikula@linux.intel.com>]

On Wed, 14 Dec 2016, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Dec 14, 2016 at 01:22:15PM +0200, Jani Nikula wrote:
>> On Wed, 14 Dec 2016, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>> > +/**
>> > + * drm_for_each_connector_iter - connector_list iterator macro
>> > + * @connector: struct &drm_connector pointer used as cursor
>> > + * @iter: struct &drm_connector_list_iter
>> > + *
>> > + * Note that @connector is only valid within the list body, if you want to use
>> > + * @connector after calling drm_connector_list_iter_put() then you need to grab
>> > + * your own reference first using drm_connector_reference().
>> > + */
>> > +#define drm_for_each_connector_iter(connector, iter) \
>> > +	while ((connector = drm_connector_list_iter_next(iter)))
>> > +
>> 
>> Observe that in most, if not all, cases you lock over the for loop, but
>> not more. That means you always get/put right around the loop.
>> 
>> You could have a variant of get() that returns the first item, and a
>> variant of next() that does put() automatically when it's about to
>> return NULL, and implement most of the loops like this:
>> 
>> #define drm_for_each_connector_simple(dev, iter, connector) \
>> 	for (connector = drm_connector_list_iter_get_first(dev, iter); \
>> 	     connector != NULL; \
>> 	     connector = drm_connector_list_iter_next_put(iter))
>> 
>> In the long run, that should be called just drm_for_each_connector.
>> 
>> The only case where you'd have to call put() explicitly is when you
>> break out of the loop early. Otherwise all looping would be dead simple,
>> without all the gets and puts, just like they are now. Perhaps the
>> naming of the functions should be such that this is the most common
>> case. Perhaps you don't actually need the versions with "manual" locking
>> at all.
>
> I had this in an earlier iteration of this patch series. The implemenation
> was somewhat misguided (as in it used srcu and some other wizzardry that I
> now managed to remove), but otherwise was exactly what you've asking for
> here.
>
> The amount of leaking was mindboggling.
>
> And that was only me being sloppyin in converting the piles of existing
> loop, not even ongoing maintenance of new loops additions done by people
> who're not well versed in the finer details of connector_list walking and
> the refcounting dance involved.
>
> Given that I've concluded that hiding those details is a bad choice, and
> to top it off the new code enforces matching get/put using lockdep. We do
> pay a price in that simple loops become a bit more verbose, but
> unfortunately there's no way to create something which is guarnateed to
> get destructed when leaving a code block (unlike in C++). And without that
> guarantee I don't think it'll be maintainable long-term.
>
> I expect that drm_for_each_connector will stay around for a long time
> (maybe even forever). As long as your driver doesn't hotplug connectors,
> it's perfectly fine. Only core + helpers + any driver supporting mst
> really need to switch over.
>
> Overall not the prettiest thing, but still an acceptable tradeoff imo.

Fair enough.

Jani.


-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel