[PATCH 0/6] drm: Introduce consistent reference counting APIs

[Jani Nikula]

On Thu, 09 Feb 2017, Thierry Reding <thierry.reding at gmail.com> wrote:
> On Thu, Feb 09, 2017 at 08:07:41PM +0100, Daniel Vetter wrote:
>> On Thu, Feb 09, 2017 at 07:39:33PM +0200, Jani Nikula wrote:
>> > On Thu, 09 Feb 2017, Daniel Vetter <daniel at ffwll.ch> wrote:
>> > > On Thu, Feb 09, 2017 at 04:10:12PM +0200, Jani Nikula wrote:
>> > >> On Wed, 08 Feb 2017, Thierry Reding <thierry.reding at gmail.com> wrote:
>> > >> > This series introduces DRM reference counting APIs that are consistent
>> > >> > with other reference counting APIs in the kernel. They are also much
>> > >> > shorter. Compatibility aliases are added to keep existing code working
>> > >> > and will stay in place until all users of the old APIs are gone.
>> > >> 
>> > >> I like it.
>> > >> 
>> > >> But it makes drm_connector_list_iter_{get,put} stick out like a sore
>> > >> thumb. Something for the TODO list I guess.
>> > >
>> > > Hm, why is that one the sore thumb now? Just because it's really long? It
>> > > does acquire/drop references behind the scenes, that's why I went with the
>> > > _get/put postfixes ...
>> > 
>> > I think the assumption is that get/put work on an object, and you can
>> > get/put as many times as you like, as long as you keep them at
>> > balance. AFAICT this doesn't hold for iter.
>> 
>> Hm right. What else should we use instead? start/stop are confusing in the
>> context of list walking, create/destroy maybe?
>
> I rather like the _begin()/_end() suffix that I think iterators have in
> the C++ STL. Well, I guess it's really .end() and .begin() methods in
> C++.
>
> But the following reads rather nicely:
>
> 	drm_connector_list_iter_begin(dev, &iter);
>
> 	drm_for_each_connector_iter(connector, &iter)
> 		...
>
> 	drm_connector_list_iter_end(&iter);
>
> Alternatively some other iterators simply use an _init() suffix to
> initialize the iterator. The kernel has a few of these, such as the
> of_phandle_iterator (see include/linux/of.h) or of_pci_range_parser (see
> include include/linux/of_address.h).
>
> EFI code uses _begin()/_end() in include/linux/efi.h, cgroups seems to
> have _start()/_end() in include/linux/cgroup.h. include/linux/klist.h
> uses klist_iter_init()/klist_iter_exit(). include/linux/rhashtable.h has
> rhashtable_walk_start()/rhashtable_walk_stop().
>
> A quick grep also shows other variants, one of them being _first(), but
> those don't have an explicit end function.
>
> So there's quite a few options to choose from, but I guess that doesn't
> make it any easier.
>
> If this was a poll, my vote would go to _begin()/_end().

I think begin/end are sensible.

An alternative is to make iter get/put work as get/put do, but I suppose
there's no real need for them to behave that way, i.e. it's
overengineering.

And if it wasn't clear, I didn't mean that this should block the patches
at hand in any way. Someone(tm) can follow up with the iter stuff later
on. It's not broken, it just feels slightly misleading after these
changes.

BR,
Jani.


-- 
Jani Nikula, Intel Open Source Technology Center