On 6/7/19 5:34 AM, Jason Gunthorpe wrote:
> On Thu, Jun 06, 2019 at 07:29:08PM -0700, John Hubbard wrote:
>> On 6/6/19 11:44 AM, Jason Gunthorpe wrote:
>>> From: Jason Gunthorpe <jgg@xxxxxxxxxxxx>
>> ...
>>> @@ -153,10 +158,14 @@ void hmm_mm_destroy(struct mm_struct *mm)
>>>
>>> static void hmm_release(struct mmu_notifier *mn, struct mm_struct *mm)
>>> {
>>> - struct hmm *hmm = mm_get_hmm(mm);
>>> + struct hmm *hmm = container_of(mn, struct hmm, mmu_notifier);
>>> struct hmm_mirror *mirror;
>>> struct hmm_range *range;
>>>
>>> + /* hmm is in progress to free */
>>
>> Well, sometimes, yes. :)
>
> It think it is in all cases actually.. The only way we see a 0 kref
> and still reach this code path is if another thread has alreay setup
> the hmm_free in the call_srcu..
>
>> Maybe this wording is clearer (if we need any comment at all):
>
> I always find this hard.. This is a very standard pattern when working
> with RCU - however in my experience few people actually know the RCU
> patterns, and missing the _unless_zero is a common bug I find when
> looking at code.
>
> This is mm/ so I can drop it, what do you think?
>
I forgot to respond to this section, so catching up now:
I think we're talking about slightly different things. I was just
noting that the comment above the "if" statement was only accurate
if the branch is taken, which is why I recommended this combination
of comment and code:
/* Bail out if hmm is in the process of being freed */
if (!kref_get_unless_zero(&hmm->kref))
return;
As for the actual _unless_zero part, I think that's good to have.
And it's a good reminder if nothing else, even in mm/ code.
thanks,
--
John Hubbard
NVIDIA
