Re: [PATCH] hugetlb_cgroup: fix imbalanced css_get and css_put pair for shared mappings

[Miaohe Lin <linmiaohe@huawei.com>]

On 2021/2/10 23:32, kernel test robot wrote:
> Hi Miaohe,
> 
> Thank you for the patch! Yet something to improve:
> 
> [auto build test ERROR on next-20210125]
> [also build test ERROR on v5.11-rc7]
> [cannot apply to linux/master linus/master hnaz-linux-mm/master v5.11-rc7 v5.11-rc6 v5.11-rc5]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch]
> 
> url:    https://github.com/0day-ci/linux/commits/Miaohe-Lin/hugetlb_cgroup-fix-imbalanced-css_get-and-css_put-pair-for-shared-mappings/20210210-171736
> base:    59fa6a163ffabc1bf25c5e0e33899e268a96d3cc
> config: arm64-randconfig-r025-20210209 (attached as .config)
> compiler: clang version 12.0.0 (https://github.com/llvm/llvm-project c9439ca36342fb6013187d0a69aef92736951476)
> reproduce (this is a W=1 build):
>         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>         chmod +x ~/bin/make.cross
>         # install arm64 cross compiling tool for clang build
>         # apt-get install binutils-aarch64-linux-gnu
>         # https://github.com/0day-ci/linux/commit/68f4ed1b80aa7c51a921c3ad913ee7e6e00618d0
>         git remote add linux-review https://github.com/0day-ci/linux
>         git fetch --no-tags linux-review Miaohe-Lin/hugetlb_cgroup-fix-imbalanced-css_get-and-css_put-pair-for-shared-mappings/20210210-171736
>         git checkout 68f4ed1b80aa7c51a921c3ad913ee7e6e00618d0
>         # save the attached .config to linux build tree
>         COMPILER_INSTALL_PATH=$HOME/0day COMPILER=clang make.cross ARCH=arm64 
> 
> If you fix the issue, kindly add following tag as appropriate
> Reported-by: kernel test robot <lkp@intel.com>
> 
> All errors (new ones prefixed by >>):
> 
>>> mm/hugetlb.c:5222:17: error: incomplete definition of type 'struct hugetlb_cgroup'
>                            css_put(&h_cg->css);
>                                     ~~~~^
>    include/linux/hugetlb_cgroup.h:20:8: note: forward declaration of 'struct hugetlb_cgroup'
>    struct hugetlb_cgroup;
>           ^
>    1 error generated.
> 

Sorry for late respond. Will fix this. Thanks!

> 
> vim +5222 mm/hugetlb.c
> 
>   5091	
>   5092	/* Return true if reservation was successful, false otherwise.  */
>   5093	bool hugetlb_reserve_pages(struct inode *inode,
>   5094						long from, long to,
>   5095						struct vm_area_struct *vma,
>   5096						vm_flags_t vm_flags)
>   5097	{
>   5098		long chg, add = -1;
>   5099		struct hstate *h = hstate_inode(inode);
>   5100		struct hugepage_subpool *spool = subpool_inode(inode);
>   5101		struct resv_map *resv_map;
>   5102		struct hugetlb_cgroup *h_cg = NULL;
>   5103		long gbl_reserve, regions_needed = 0;
>   5104	
>   5105		/* This should never happen */
>   5106		if (from > to) {
>   5107			VM_WARN(1, "%s called with a negative range\n", __func__);
>   5108			return false;
>   5109		}
>   5110	
>   5111		/*
>   5112		 * Only apply hugepage reservation if asked. At fault time, an
>   5113		 * attempt will be made for VM_NORESERVE to allocate a page
>   5114		 * without using reserves
>   5115		 */
>   5116		if (vm_flags & VM_NORESERVE)
>   5117			return true;
>   5118	
>   5119		/*
>   5120		 * Shared mappings base their reservation on the number of pages that
>   5121		 * are already allocated on behalf of the file. Private mappings need
>   5122		 * to reserve the full area even if read-only as mprotect() may be
>   5123		 * called to make the mapping read-write. Assume !vma is a shm mapping
>   5124		 */
>   5125		if (!vma || vma->vm_flags & VM_MAYSHARE) {
>   5126			/*
>   5127			 * resv_map can not be NULL as hugetlb_reserve_pages is only
>   5128			 * called for inodes for which resv_maps were created (see
>   5129			 * hugetlbfs_get_inode).
>   5130			 */
>   5131			resv_map = inode_resv_map(inode);
>   5132	
>   5133			chg = region_chg(resv_map, from, to, &regions_needed);
>   5134	
>   5135		} else {
>   5136			/* Private mapping. */
>   5137			resv_map = resv_map_alloc();
>   5138			if (!resv_map)
>   5139				return false;
>   5140	
>   5141			chg = to - from;
>   5142	
>   5143			set_vma_resv_map(vma, resv_map);
>   5144			set_vma_resv_flags(vma, HPAGE_RESV_OWNER);
>   5145		}
>   5146	
>   5147		if (chg < 0)
>   5148			goto out_err;
>   5149	
>   5150		if (hugetlb_cgroup_charge_cgroup_rsvd(hstate_index(h),
>   5151					chg * pages_per_huge_page(h), &h_cg) < 0)
>   5152			goto out_err;
>   5153	
>   5154		if (vma && !(vma->vm_flags & VM_MAYSHARE) && h_cg) {
>   5155			/* For private mappings, the hugetlb_cgroup uncharge info hangs
>   5156			 * of the resv_map.
>   5157			 */
>   5158			resv_map_set_hugetlb_cgroup_uncharge_info(resv_map, h_cg, h);
>   5159		}
>   5160	
>   5161		/*
>   5162		 * There must be enough pages in the subpool for the mapping. If
>   5163		 * the subpool has a minimum size, there may be some global
>   5164		 * reservations already in place (gbl_reserve).
>   5165		 */
>   5166		gbl_reserve = hugepage_subpool_get_pages(spool, chg);
>   5167		if (gbl_reserve < 0)
>   5168			goto out_uncharge_cgroup;
>   5169	
>   5170		/*
>   5171		 * Check enough hugepages are available for the reservation.
>   5172		 * Hand the pages back to the subpool if there are not
>   5173		 */
>   5174		if (hugetlb_acct_memory(h, gbl_reserve) < 0)
>   5175			goto out_put_pages;
>   5176	
>   5177		/*
>   5178		 * Account for the reservations made. Shared mappings record regions
>   5179		 * that have reservations as they are shared by multiple VMAs.
>   5180		 * When the last VMA disappears, the region map says how much
>   5181		 * the reservation was and the page cache tells how much of
>   5182		 * the reservation was consumed. Private mappings are per-VMA and
>   5183		 * only the consumed reservations are tracked. When the VMA
>   5184		 * disappears, the original reservation is the VMA size and the
>   5185		 * consumed reservations are stored in the map. Hence, nothing
>   5186		 * else has to be done for private mappings here
>   5187		 */
>   5188		if (!vma || vma->vm_flags & VM_MAYSHARE) {
>   5189			add = region_add(resv_map, from, to, regions_needed, h, h_cg);
>   5190	
>   5191			if (unlikely(add < 0)) {
>   5192				hugetlb_acct_memory(h, -gbl_reserve);
>   5193				goto out_put_pages;
>   5194			} else if (unlikely(chg > add)) {
>   5195				/*
>   5196				 * pages in this range were added to the reserve
>   5197				 * map between region_chg and region_add.  This
>   5198				 * indicates a race with alloc_huge_page.  Adjust
>   5199				 * the subpool and reserve counts modified above
>   5200				 * based on the difference.
>   5201				 */
>   5202				long rsv_adjust;
>   5203	
>   5204				/*
>   5205				 * hugetlb_cgroup_uncharge_cgroup_rsvd() will put the
>   5206				 * reference to h_cg->css. See comment below for detail.
>   5207				 */
>   5208				hugetlb_cgroup_uncharge_cgroup_rsvd(
>   5209					hstate_index(h),
>   5210					(chg - add) * pages_per_huge_page(h), h_cg);
>   5211	
>   5212				rsv_adjust = hugepage_subpool_put_pages(spool,
>   5213									chg - add);
>   5214				hugetlb_acct_memory(h, -rsv_adjust);
>   5215			} else if (h_cg) {
>   5216				/*
>   5217				 * The file_regions will hold their own reference to
>   5218				 * h_cg->css. So we should release the reference held
>   5219				 * via hugetlb_cgroup_charge_cgroup_rsvd() when we are
>   5220				 * done.
>   5221				 */
>> 5222				css_put(&h_cg->css);
>   5223			}
>   5224		}
>   5225		return true;
>   5226	
>   5227	out_put_pages:
>   5228		/* put back original number of pages, chg */
>   5229		(void)hugepage_subpool_put_pages(spool, chg);
>   5230	out_uncharge_cgroup:
>   5231		hugetlb_cgroup_uncharge_cgroup_rsvd(hstate_index(h),
>   5232						    chg * pages_per_huge_page(h), h_cg);
>   5233	out_err:
>   5234		if (!vma || vma->vm_flags & VM_MAYSHARE)
>   5235			/* Only call region_abort if the region_chg succeeded but the
>   5236			 * region_add failed or didn't run.
>   5237			 */
>   5238			if (chg >= 0 && add < 0)
>   5239				region_abort(resv_map, from, to, regions_needed);
>   5240		if (vma && is_vma_resv_set(vma, HPAGE_RESV_OWNER))
>   5241			kref_put(&resv_map->refs, resv_map_release);
>   5242		return false;
>   5243	}
>   5244	
> 
> ---
> 0-DAY CI Kernel Test Service, Intel Corporation
> https://lists.01.org/hyperkitty/list/kbuild-all@lists.01.org
>