Messages in this thread | | | From | "Yang, Bin" <> | Subject | Re: [PATCH] x86/mm: fix cpu stuck issue in __change_page_attr_set_clr | Date | Wed, 4 Jul 2018 09:16:37 +0000 |
| |
You are completely right. After considering, I think my patch is like a workaround but not real fix. I am trying to re-write a new patch without cache implementation.
Please give me one or two days to re-write this patch and discribe it more clearly in commit comment.
thanks, Bin
On Wed, 2018-07-04 at 09:40 +0200, Thomas Gleixner wrote: > On Wed, 4 Jul 2018, Yang, Bin wrote: > > thanks for reviewing my patch. I will update a new patch version > > based > > on your feedback soon > > Take your time. > > > On Tue, 2018-07-03 at 15:57 +0200, Thomas Gleixner wrote: > > Below is the new commit comment I will use in new patch version > > soon > > > Please do not use the output of git show for submitting a patch. > > > Use > > > git format-patch(1). > > > > > I use "git send-email -1" to submit patch for review. Should I run > > "git > > format-patch" first and send the patch as email? > > git send-email is fine, but it should not result in the changelog > being > indented by a bunch of spaces. That's why I assumed that you used git > show > because that's exacty the format. I'm not using it, so I can't give > you > advise there. > > > =========== > > If cpu supports "pdpe1gb", kernel will try its best to use 1G big > > page. > > When changing a 4K page attr inside 1G page range, > > __change_page_attr() > > will either consume this 4K page into the 1G page, or it splits 1G > > page > > into 2M pages and tries again. The retry will either consume the 4K > > page into a 2MB page, or it splits 2MB page into 4K pages. > > try_preserve_large_page() is called by __change_page_attr() to > > decide > > I know what calls try_preserve_large_page(), but you still fail to > explain > the full call chain including parameters which I asked for. > > > it by checking all 4K pages inside the big page one by one. > > After your change this will still happen. You just shortcut the inner > workings, but you are still not explaining why the shortcut is > necessary in > the first place. > > The try_preserve_large_page() logic should not need any of this > unless > there is a subtle implementation bug. If that's the case, then the > bug > needs to be isolated and fixed and not papered over by magic short > cuts. > > > This issue is discovered during kernel boot time optimization. > > Sometimes, free_initmem() returns after about 600ms on my x86_64 > > board > > with 4GB memory. > > > > Since it is a common issue of x86_64, it can be reproduced by qemu > > too. > > We can add some logs in try_preserve_large_page() function to > > measure > > the loop count and elapsed time. Please make sure the host CPU has > > "pdpe1gb" flag and run below qemu command to reproduce it: > > > > qemu-system-x86_64 -enable-kvm -cpu host -serial mon:stdio -m 4G > > -nographic -kernel bzImage -initrd ramdisk.img -append > > "console=ttyS0" > > > > Since default x86_64 kernel enables CONFIG_RANDOMIZE_BASE, it needs > > to > > Huch? What as this to do with randomize base? > > > try many times to let init memory be randomized in a 1G page range. > > And no, I'm not interested in random qemu commands and adding logs > into > something. You already did the investigation, but you fail to provide > the > information. And I'm not asking for random timing logs, I ask about a > proper explanation why this happens even if it's supposed not to > happen. > > > This patch try to cache the last address which had been checked > > just > > See Documentation/process/submitting-patches.rst and search for 'This > patch' .... > > > now. If the next address is in same big page with same attr, the > > cache > > will be used without full range check. > > > > @@ -552,16 +552,20 @@ static int > > > > try_preserve_large_page(pte_t *kpte, unsigned long address, > > > > struct cpa_data *cpa) > > > > { > > > > + static unsigned long address_cache; > > > > + static unsigned long do_split_cache = 1; > > > > > > What are the life time and protection rules of these static > > > variables > > > and > > > why are they static in the first place. > > > > they will be protected by pgd_lock. They only cache previous > > "do_split" > > result and will be refreshed every time. > > So why is there no comment explaining this? And I'm still not > convinced > about pgd_lock being the real protection. pgd_lock protects against > concurrent page table manipulations, but it does not protect against > concurrent calls of the change_page_attr logic at all. That's what > cpa_lock > does. > > > > > unsigned long nextpage_addr, numpages, pmask, psize, > > > > addr, > > > > pfn, old_pfn; > > > > pte_t new_pte, old_pte, *tmp; > > > > pgprot_t old_prot, new_prot, req_prot; > > > > int i, do_split = 1; > > > > enum pg_level level; > > > > > > > > - if (cpa->force_split) > > > > + spin_lock(&pgd_lock); > > > > + if (cpa->force_split) { > > > > + do_split_cache = 1; > > > > > > Returns with pgd_lock held which will immediately lockup the > > > system > > > on the > > > next spin_lock(&pgd_lock) operation. > > > > I am so sorry to make such stupid mistake. force_split was not hit > > on > > my board :( > > > > > > Also what's the point of storing the already available > > > information of > > > cpa->force_split in yet another variable? This enforces taking > > > the > > > lock on > > > every invocation for no reason. > > > > As you know, do_split is initialized as 1. If do_split_cache == 1, > > the > > cache value will not be used. If force_split == 1, cache value > > should > > be expired. Since do_split_cache is protected by this lock, it > > needs to > > task this lock here. > > No. It can be done w/o the lock and without touching the cache > variable. cpa->force_split does not need any of it. > > > > > + /* > > > > + * If an address in same range had been checked just > > > > now, > > > > re-use the > > > > + * cache value without full range check. In the worst > > > > case, it needs to > > > > + * check every 4K page in 1G range, which causes cpu > > > > stuck > > > > for long > > > > + * time. > > > > + */ > > > > + if (!do_split_cache && > > > > + address_cache >= addr && address_cache < > > > > nextpage_addr > > > > && > > > > > > On the first call, address_cache contains 0. On any subsequent > > > call > > > > on the first call, do_split_cache is 1. if do_split_cache == 1, > > address_cache will not be used. > > That's really not obvious and the whole code flow is obfuscated. > > > > > @@ -670,6 +686,8 @@ try_preserve_large_page(pte_t *kpte, > > > > unsigned > > > > long address, > > > > } > > > > > > > > out_unlock: > > > > + address_cache = address; > > > > + do_split_cache = do_split; > > > > spin_unlock(&pgd_lock); > > > > > > So here you store the 'cache' values and while this code suggests > > > that it > > > protects the 'cache' via pgd_lock (due to lack of comments), the > > > protection > > > is actually cpa_lock. > > > > > > But, that cache information stays around when cpa_lock is > > > dropped, > > > i.e. when the current (partial) operation has been done and this > > > information stays stale for the next user. That does not make > > > sense. > > > > __change_page_attr is the only function to change page attr and > > try_preserve_large_page will be called every time for big page > > check. > > If a big page had been splitted, it will not be merged again. So it > > is > > safe to cache previous result in try_preserve_large_page() > > function. > > Come on. __change_page_attr() has a single call site: > __change_page_attr_set_clr() which itself is called from a ton of > places. > And once cpa_lock is dropped in the loop, the 'cache' thing is not > longer > protected and and stale. > > Unless it's coherentely explained, this looks more like works by > chance > than by design. > > Thanks, > > tglx | |