| Message ID | 20200807091251.12129-3-richard.weiyang@linux.alibaba.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm/hugetlb: code refine and simplification | expand |
On 08/07/20 at 05:12pm, Wei Yang wrote: > list_first_entry() may not return NULL even when the list is empty. > > Let's make sure the behavior by using list_first_entry_or_null(), > otherwise it would corrupt the list. > > Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> > --- > mm/hugetlb.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/mm/hugetlb.c b/mm/hugetlb.c > index 62ec74f6d03f..0a2f3851b828 100644 > --- a/mm/hugetlb.c > +++ b/mm/hugetlb.c > @@ -237,7 +237,8 @@ get_file_region_entry_from_cache(struct resv_map *resv, long from, long to) > VM_BUG_ON(resv->region_cache_count <= 0); We have had above line, is it possible to be NULL from list_first_entry? > > resv->region_cache_count--; > - nrg = list_first_entry(&resv->region_cache, struct file_region, link); > + nrg = list_first_entry_or_null(&resv->region_cache, > + struct file_region, link); > VM_BUG_ON(!nrg); > list_del(&nrg->link); > > -- > 2.20.1 (Apple Git-117) > >
On Fri, Aug 07, 2020 at 08:49:51PM +0800, Baoquan He wrote: >On 08/07/20 at 05:12pm, Wei Yang wrote: >> list_first_entry() may not return NULL even when the list is empty. >> >> Let's make sure the behavior by using list_first_entry_or_null(), >> otherwise it would corrupt the list. >> >> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >> --- >> mm/hugetlb.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >> index 62ec74f6d03f..0a2f3851b828 100644 >> --- a/mm/hugetlb.c >> +++ b/mm/hugetlb.c >> @@ -237,7 +237,8 @@ get_file_region_entry_from_cache(struct resv_map *resv, long from, long to) >> VM_BUG_ON(resv->region_cache_count <= 0); > > >We have had above line, is it possible to be NULL from list_first_entry? > >> >> resv->region_cache_count--; >> - nrg = list_first_entry(&resv->region_cache, struct file_region, link); >> + nrg = list_first_entry_or_null(&resv->region_cache, >> + struct file_region, link); >> VM_BUG_ON(!nrg); Or we can remove this VM_BUG_ON()? >> list_del(&nrg->link); >> >> -- >> 2.20.1 (Apple Git-117) >> >>
On 08/07/20 at 10:28pm, Wei Yang wrote: > On Fri, Aug 07, 2020 at 08:49:51PM +0800, Baoquan He wrote: > >On 08/07/20 at 05:12pm, Wei Yang wrote: > >> list_first_entry() may not return NULL even when the list is empty. > >> > >> Let's make sure the behavior by using list_first_entry_or_null(), > >> otherwise it would corrupt the list. > >> > >> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> > >> --- > >> mm/hugetlb.c | 3 ++- > >> 1 file changed, 2 insertions(+), 1 deletion(-) > >> > >> diff --git a/mm/hugetlb.c b/mm/hugetlb.c > >> index 62ec74f6d03f..0a2f3851b828 100644 > >> --- a/mm/hugetlb.c > >> +++ b/mm/hugetlb.c > >> @@ -237,7 +237,8 @@ get_file_region_entry_from_cache(struct resv_map *resv, long from, long to) > >> VM_BUG_ON(resv->region_cache_count <= 0); > > > > > >We have had above line, is it possible to be NULL from list_first_entry? > > > >> > >> resv->region_cache_count--; > >> - nrg = list_first_entry(&resv->region_cache, struct file_region, link); > >> + nrg = list_first_entry_or_null(&resv->region_cache, > >> + struct file_region, link); > >> VM_BUG_ON(!nrg); > > Or we can remove this VM_BUG_ON()? Yeah, it's fine to me. > > >> list_del(&nrg->link); > >> > >> -- > >> 2.20.1 (Apple Git-117) > >> > >> > > -- > Wei Yang > Help you, Help me >
On 8/7/20 7:28 AM, Wei Yang wrote: > On Fri, Aug 07, 2020 at 08:49:51PM +0800, Baoquan He wrote: >> On 08/07/20 at 05:12pm, Wei Yang wrote: >>> list_first_entry() may not return NULL even when the list is empty. >>> >>> Let's make sure the behavior by using list_first_entry_or_null(), >>> otherwise it would corrupt the list. >>> >>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >>> --- >>> mm/hugetlb.c | 3 ++- >>> 1 file changed, 2 insertions(+), 1 deletion(-) >>> >>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>> index 62ec74f6d03f..0a2f3851b828 100644 >>> --- a/mm/hugetlb.c >>> +++ b/mm/hugetlb.c >>> @@ -237,7 +237,8 @@ get_file_region_entry_from_cache(struct resv_map *resv, long from, long to) >>> VM_BUG_ON(resv->region_cache_count <= 0); >> >> >> We have had above line, is it possible to be NULL from list_first_entry? >> >>> >>> resv->region_cache_count--; >>> - nrg = list_first_entry(&resv->region_cache, struct file_region, link); >>> + nrg = list_first_entry_or_null(&resv->region_cache, >>> + struct file_region, link); >>> VM_BUG_ON(!nrg); > > Or we can remove this VM_BUG_ON()? > I would prefer that we just remove the 'VM_BUG_ON(!nrg)'. Code elsewhere is responsible for making sure there is ALWAYS an entry in the cache. That is why the 'VM_BUG_ON(resv->region_cache_count <= 0)' is at the beginning of the routine.
On Mon, Aug 10, 2020 at 01:28:46PM -0700, Mike Kravetz wrote: >On 8/7/20 7:28 AM, Wei Yang wrote: >> On Fri, Aug 07, 2020 at 08:49:51PM +0800, Baoquan He wrote: >>> On 08/07/20 at 05:12pm, Wei Yang wrote: >>>> list_first_entry() may not return NULL even when the list is empty. >>>> >>>> Let's make sure the behavior by using list_first_entry_or_null(), >>>> otherwise it would corrupt the list. >>>> >>>> Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> >>>> --- >>>> mm/hugetlb.c | 3 ++- >>>> 1 file changed, 2 insertions(+), 1 deletion(-) >>>> >>>> diff --git a/mm/hugetlb.c b/mm/hugetlb.c >>>> index 62ec74f6d03f..0a2f3851b828 100644 >>>> --- a/mm/hugetlb.c >>>> +++ b/mm/hugetlb.c >>>> @@ -237,7 +237,8 @@ get_file_region_entry_from_cache(struct resv_map *resv, long from, long to) >>>> VM_BUG_ON(resv->region_cache_count <= 0); >>> >>> >>> We have had above line, is it possible to be NULL from list_first_entry? >>> >>>> >>>> resv->region_cache_count--; >>>> - nrg = list_first_entry(&resv->region_cache, struct file_region, link); >>>> + nrg = list_first_entry_or_null(&resv->region_cache, >>>> + struct file_region, link); >>>> VM_BUG_ON(!nrg); >> >> Or we can remove this VM_BUG_ON()? >> > >I would prefer that we just remove the 'VM_BUG_ON(!nrg)'. Code elsewhere >is responsible for making sure there is ALWAYS an entry in the cache. That >is why the 'VM_BUG_ON(resv->region_cache_count <= 0)' is at the beginning >of the routine. Sure, will change to this. > >-- >Mike Kravetz
diff --git a/mm/hugetlb.c b/mm/hugetlb.c index 62ec74f6d03f..0a2f3851b828 100644 --- a/mm/hugetlb.c +++ b/mm/hugetlb.c @@ -237,7 +237,8 @@ get_file_region_entry_from_cache(struct resv_map *resv, long from, long to) VM_BUG_ON(resv->region_cache_count <= 0); resv->region_cache_count--; - nrg = list_first_entry(&resv->region_cache, struct file_region, link); + nrg = list_first_entry_or_null(&resv->region_cache, + struct file_region, link); VM_BUG_ON(!nrg); list_del(&nrg->link);
list_first_entry() may not return NULL even when the list is empty. Let's make sure the behavior by using list_first_entry_or_null(), otherwise it would corrupt the list. Signed-off-by: Wei Yang <richard.weiyang@linux.alibaba.com> --- mm/hugetlb.c | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)