| Message ID | 20240417211836.2742593-4-peterx@redhat.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/hugetlb: Fix missing hugetlb_lock for memcg resv uncharge | expand |
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> > --- > 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. > __set_hugetlb_cgroup(folio, h_cg, rsvd); > if (!rsvd) { > unsigned long usage = > -- > 2.44.0 >
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,
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); __set_hugetlb_cgroup(folio, h_cg, rsvd); if (!rsvd) { unsigned long usage =
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> --- mm/hugetlb_cgroup.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)