From: John Hubbard <jhubbard@nvidia.com>
To: <christian.koenig@amd.com>, <linux-mm@kvack.org>,
<linux-kernel@vger.kernel.org>, <linaro-mm-sig@lists.linaro.org>,
<dri-devel@lists.freedesktop.org>, <linux-media@vger.kernel.org>,
<chris@chris-wilson.co.uk>, <airlied@redhat.com>,
<akpm@linux-foundation.org>, <daniel@ffwll.ch>,
<sumit.semwal@linaro.org>
Subject: Re: [PATCH 1/4] mm: introduce vma_set_file function v2
Date: Fri, 9 Oct 2020 00:36:24 -0700 [thread overview]
Message-ID: <0e3a8483-2b51-b9a4-30b9-360e761c95e4@nvidia.com> (raw)
In-Reply-To: <747e1832-0341-9029-95f6-638f0f1a6f76@gmail.com>
On 10/9/20 12:33 AM, Christian König wrote:
> Am 08.10.20 um 23:49 schrieb John Hubbard:
>> On 10/8/20 4:23 AM, Christian König wrote:
>> ...
>>
>>> diff --git a/drivers/gpu/drm/i915/gem/i915_gem_mman.c b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>> index 3d69e51f3e4d..c9d5f1a38af3 100644
>>> --- a/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>> +++ b/drivers/gpu/drm/i915/gem/i915_gem_mman.c
>>> @@ -893,8 +893,8 @@ int i915_gem_mmap(struct file *filp, struct vm_area_struct *vma)
>>> * requires avoiding extraneous references to their filp, hence why
>>> * we prefer to use an anonymous file for their mmaps.
>>> */
>>> - fput(vma->vm_file);
>>> - vma->vm_file = anon;
>>> + vma_set_file(vma, anon);
>>> + fput(anon);
>>
>> That's one fput() too many, isn't it?
>
> No, the other cases were replacing the vm_file with something pre-allocated and also grabbed a new
> reference.
>
> But this case here uses the freshly allocated anon file and so vma_set_file() grabs another extra
> reference which we need to drop.
>
> The alternative is to just keep it as it is. Opinions?
>
I think just a small comment for these cases, is probably about right.
>> ...
>>
>>> diff --git a/drivers/staging/android/ashmem.c b/drivers/staging/android/ashmem.c
>>> index 10b4be1f3e78..a51dc089896e 100644
>>> --- a/drivers/staging/android/ashmem.c
>>> +++ b/drivers/staging/android/ashmem.c
>>> @@ -450,9 +450,8 @@ static int ashmem_mmap(struct file *file, struct vm_area_struct *vma)
>>> vma_set_anonymous(vma);
>>> }
>>> - if (vma->vm_file)
>>> - fput(vma->vm_file);
>>> - vma->vm_file = asma->file;
>>> + vma_set_file(vma, asma->file);
>>> + fput(asma->file);
>>
>> Same here: that fput() seems wrong, as it was already done within vma_set_file().
>
> No, that case is correct as well. The Android code here has the matching get_file() a few lines up,
> see the surrounding code.
>
> I didn't wanted to replace that since it does some strange error handling here, so the result is
> that we need to drop the extra reference as again.
>
> We could also keep it like it is or maybe better put a TODO comment on it.
>
Yeah, I think a comment is a good way to go.
thanks,
--
John Hubbard
NVIDIA
prev parent reply other threads:[~2020-10-09 7:36 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-08 11:23 [PATCH 1/4] mm: introduce vma_set_file function v2 Christian König
2020-10-08 11:23 ` [PATCH 2/4] drm/prime: document that use the page array is deprecated Christian König
2020-10-08 14:09 ` Daniel Vetter
2020-10-08 14:14 ` Daniel Vetter
2020-10-09 7:36 ` Christian König
2020-10-09 7:40 ` Daniel Vetter
2020-10-08 11:23 ` [PATCH 3/4] drm/radeon: stop using pages with drm_prime_sg_to_page_addr_arrays Christian König
2020-10-08 11:23 ` [PATCH 4/4] drm/amdgpu: " Christian König
2020-10-08 11:39 ` [PATCH 1/4] mm: introduce vma_set_file function v2 Matthew Wilcox
2020-10-08 12:06 ` Christian König
2020-10-08 14:12 ` Daniel Vetter
2020-10-09 7:16 ` Christian König
2020-10-09 7:39 ` Daniel Vetter
2020-10-09 12:12 ` Jason Gunthorpe
2020-10-09 12:15 ` Christian König
2020-10-08 15:35 ` kernel test robot
2020-10-08 16:19 ` kernel test robot
2020-10-08 21:49 ` John Hubbard
2020-10-09 7:33 ` Christian König
2020-10-09 7:36 ` John Hubbard [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=0e3a8483-2b51-b9a4-30b9-360e761c95e4@nvidia.com \
--to=jhubbard@nvidia.com \
--cc=airlied@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=chris@chris-wilson.co.uk \
--cc=christian.koenig@amd.com \
--cc=daniel@ffwll.ch \
--cc=dri-devel@lists.freedesktop.org \
--cc=linaro-mm-sig@lists.linaro.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-media@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=sumit.semwal@linaro.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).