linux-kernel - Re: [PATCH v3 09/15] drm/gem: Add LRU/shrinker helper

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6ceabe5a-a1ff-0717-96f1-0d28167ef649@collabora.com>
Date:   Fri, 29 Jul 2022 20:01:02 +0300
From:   Dmitry Osipenko <dmitry.osipenko@...labora.com>
To:     Rob Clark <robdclark@...il.com>
Cc:     dri-devel@...ts.freedesktop.org, freedreno@...ts.freedesktop.org,
        linux-arm-msm@...r.kernel.org, Rob Clark <robdclark@...omium.org>,
        Daniel Vetter <daniel@...ll.ch>,
        Thomas Zimmermann <tzimmermann@...e.de>,
        Maarten Lankhorst <maarten.lankhorst@...ux.intel.com>,
        Maxime Ripard <mripard@...nel.org>,
        David Airlie <airlied@...ux.ie>,
        open list <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v3 09/15] drm/gem: Add LRU/shrinker helper

On 7/29/22 18:40, Rob Clark wrote:
> On Fri, Jul 29, 2022 at 8:27 AM Dmitry Osipenko
> <dmitry.osipenko@...labora.com> wrote:
>>
>> On 7/26/22 20:50, Rob Clark wrote:
>>> +/**
>>> + * drm_gem_lru_move_tail_locked - move the object to the tail of the LRU
>>> + *
>>> + * If the object is already in this LRU it will be moved to the
>>> + * tail.  Otherwise it will be removed from whichever other LRU
>>> + * it is in (if any) and moved into this LRU.
>>> + *
>>> + * Call with LRU lock held.
>>> + *
>>> + * @lru: The LRU to move the object into.
>>> + * @obj: The GEM object to move into this LRU
>>> + */
>>> +void
>>> +drm_gem_lru_move_tail_locked(struct drm_gem_lru *lru, struct drm_gem_object *obj)
>>> +{
>>> +     lockdep_assert_held_once(lru->lock);
>>> +
>>> +     if (obj->lru)
>>> +             lru_remove(obj);
>>
>> The obj->lru also needs to be locked if lru != obj->lru, isn't it? And
>> then we should add lockdep_assert_held_once(obj->lru->lock).
>>
> 
> It is expected (mentioned in comment on drm_gem_lru::lock) that all
> lru's are sharing the same lock.  Possibly that could be made more
> obvious?  Having per-lru locks wouldn't really work for accessing the
> single drm_gem_object::lru_node.

Right, my bad. I began to update the DRM-SHMEM shrinker patches on top
of the shrinker helper, but missed that the lock is shared when was
looking at this patch again today.

Adding comment to the code about the shared lock may help a tad, but
it's not really necessary. It was my fault that I forgot about it.

Thank you!

-- 
Best regards,
Dmitry