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

[Daniel Vetter]

On Fri, Feb 10, 2017 at 8:29 AM, Jani Nikula
<jani.nikula at linux.intel.com> wrote:
> 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.

+1 on begin/end for the iters, if someone gets around to it.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch