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

From: Daniel Vetter <daniel@ffwll.ch>
To: Jani Nikula <jani.nikula@linux.intel.com>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>,
	Intel Graphics Development <intel-gfx@lists.freedesktop.org>,
	DRI Development <dri-devel@lists.freedesktop.org>,
	Daniel Vetter <daniel.vetter@intel.com>
Subject: Re: [PATCH 05/13] drm: locking&new iterators for connector_list
Date: Wed, 14 Dec 2016 13:26:06 +0100	[thread overview]
Message-ID: <20161214122606.sifkk7qnb7wupwvl@phenom.ffwll.local> (raw)
In-Reply-To: <874m26sql4.fsf@intel.com>

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.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx