Received-SPF: pass (google.com: domain of linux-kernel+bounces-151660-linux.lists.archive=gmail.com@vger.kernel.org designates 139.178.88.99 as permitted sender) client-ip=139.178.88.99;
Date: Fri, 19 Apr 2024 11:21:58 -0400
From: Peter Xu <peterx@redhat.com>
To: Mina Almasry <almasrymina@google.com>
Cc: linux-mm@kvack.org, linux-kernel@vger.kernel.org,
	David Hildenbrand <david@redhat.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Muchun Song <muchun.song@linux.dev>,
	David Rientjes <rientjes@google.com>
Subject: Re: [PATCH 3/3] mm/hugetlb: Assert hugetlb_lock in
 __hugetlb_cgroup_commit_charge
Message-ID: <ZiKMFiEyjz_A-hkj@x1n>
References: <20240417211836.2742593-1-peterx@redhat.com>
 <20240417211836.2742593-4-peterx@redhat.com>
 <CAHS8izPMjBMNUStsUjobbo4rUXirFtkOZVvJTFFqD4SUafQZaQ@mail.gmail.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAHS8izPMjBMNUStsUjobbo4rUXirFtkOZVvJTFFqD4SUafQZaQ@mail.gmail.com>

On Fri, Apr 19, 2024 at 08:03:08AM -0700, Mina Almasry wrote:
> On Wed, Apr 17, 2024 at 2:18 PM Peter Xu <peterx@redhat.com> wrote:
> >
> > This is similar to __hugetlb_cgroup_uncharge_folio() where it relies on
> > holding hugetlb_lock.  Add the similar assertion like the other one, since
> > it looks like such things may help some day.
> >
> > Signed-off-by: Peter Xu <peterx@redhat.com>
> 
> Reviewed-by: Mina Almasry <almasrymina@google.com>

Thanks.

> 
> > ---
> >  mm/hugetlb_cgroup.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/mm/hugetlb_cgroup.c b/mm/hugetlb_cgroup.c
> > index aa4486bd3904..e20339a346b9 100644
> > --- a/mm/hugetlb_cgroup.c
> > +++ b/mm/hugetlb_cgroup.c
> > @@ -308,7 +308,7 @@ static void __hugetlb_cgroup_commit_charge(int idx, unsigned long nr_pages,
> >  {
> >         if (hugetlb_cgroup_disabled() || !h_cg)
> >                 return;
> > -
> > +       lockdep_assert_held(&hugetlb_lock);
> 
> Maybe also remove the comment on the top of the function:
> 
> /* Should be called with hugetlb_lock held */
> 
> Now that the function asserts, the comment seems redundant, but up to you.

IMHO there's no harm to be verbose in this case, so I'll just keep it
simple to be as-is.

Thanks,

-- 
Peter Xu