| Message ID | 20210201114319.34720-1-linmiaohe@huawei.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: simplify the VM_BUG_ON condition in pmdp_huge_clear_flush() | expand |
On Mon, 1 Feb 2021 06:43:19 -0500 Miaohe Lin <linmiaohe@huawei.com> wrote: > The condition (A && !C && !D) || !A is equivalent to !A || (A && !C && !D) > and can be further simplified to !A || (!C && !D). > > .. > > --- a/mm/pgtable-generic.c > +++ b/mm/pgtable-generic.c > @@ -135,8 +135,8 @@ pmd_t pmdp_huge_clear_flush(struct vm_area_struct *vma, unsigned long address, > { > pmd_t pmd; > VM_BUG_ON(address & ~HPAGE_PMD_MASK); > - VM_BUG_ON((pmd_present(*pmdp) && !pmd_trans_huge(*pmdp) && > - !pmd_devmap(*pmdp)) || !pmd_present(*pmdp)); > + VM_BUG_ON(!pmd_present(*pmdp) || (!pmd_trans_huge(*pmdp) && > + !pmd_devmap(*pmdp))); > pmd = pmdp_huge_get_and_clear(vma->vm_mm, address, pmdp); > flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE); > return pmd; True, and the resulting code is still readable enough. But a problem with such a complex expression is that the developer will have trouble figuring out why the BUG actually triggered. If we had a VM_BUG_ON_PMD() then we could print the pmd's value and permit diagnosis from that. But we don't have such a thing. So I suggest that it would be better to have VM_BUG_ON((pmd_present(*pmdp) && !pmd_trans_huge(*pmdp) && !pmd_devmap(*pmdp))); VM_BUG_ON(!pmd_present(*pmdp)); This way, the BUG()'s file-n-line output will tell us more about why the kernel went splat. I suppose maybe this could be optimized the same way, as VM_BUG_ON(!pmd_present(*pmdp)); /* Below assumes pmd_present() is true */ VM_BUG_ON(!pmd_trans_huge(*pmdp) && !pmd_devmap(*pmdp)); Which works because VM_BUG_ON is, depending up Kconfig, either a no-op or a noreturn-if-it-triggered. I'm not sure if I like this trick much though.
Hi: On 2021/2/2 7:33, Andrew Morton wrote: > On Mon, 1 Feb 2021 06:43:19 -0500 Miaohe Lin <linmiaohe@huawei.com> wrote: > >> The condition (A && !C && !D) || !A is equivalent to !A || (A && !C && !D) >> and can be further simplified to !A || (!C && !D). >> >> .. >> >> --- a/mm/pgtable-generic.c >> +++ b/mm/pgtable-generic.c >> @@ -135,8 +135,8 @@ pmd_t pmdp_huge_clear_flush(struct vm_area_struct *vma, unsigned long address, >> { >> pmd_t pmd; >> VM_BUG_ON(address & ~HPAGE_PMD_MASK); >> - VM_BUG_ON((pmd_present(*pmdp) && !pmd_trans_huge(*pmdp) && >> - !pmd_devmap(*pmdp)) || !pmd_present(*pmdp)); >> + VM_BUG_ON(!pmd_present(*pmdp) || (!pmd_trans_huge(*pmdp) && >> + !pmd_devmap(*pmdp))); >> pmd = pmdp_huge_get_and_clear(vma->vm_mm, address, pmdp); >> flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE); >> return pmd; > > True, and the resulting code is still readable enough. > > But a problem with such a complex expression is that the developer will > have trouble figuring out why the BUG actually triggered. > Agree! We can determine which condition is failing through the line number __but__ we can't figure out exactly which one triggered BUG for a complex expression. > If we had a VM_BUG_ON_PMD() then we could print the pmd's value and > permit diagnosis from that. But we don't have such a thing. > > So I suggest that it would be better to have > > VM_BUG_ON((pmd_present(*pmdp) && !pmd_trans_huge(*pmdp) && > !pmd_devmap(*pmdp))); > VM_BUG_ON(!pmd_present(*pmdp)); > > This way, the BUG()'s file-n-line output will tell us more about why the > kernel went splat. > > > I suppose maybe this could be optimized the same way, as > > VM_BUG_ON(!pmd_present(*pmdp)); > /* Below assumes pmd_present() is true */ > VM_BUG_ON(!pmd_trans_huge(*pmdp) && !pmd_devmap(*pmdp)); This one looks good and provide more information than before. I can send another patch to do this (and feel free to merge into this one), should I ? Many thanks. > > Which works because VM_BUG_ON is, depending up Kconfig, either a no-op > or a noreturn-if-it-triggered. I'm not sure if I like this trick much though. > > . >
diff --git a/mm/pgtable-generic.c b/mm/pgtable-generic.c index 9578db83e312..fa1375f3e3b2 100644 --- a/mm/pgtable-generic.c +++ b/mm/pgtable-generic.c @@ -135,8 +135,8 @@ pmd_t pmdp_huge_clear_flush(struct vm_area_struct *vma, unsigned long address, { pmd_t pmd; VM_BUG_ON(address & ~HPAGE_PMD_MASK); - VM_BUG_ON((pmd_present(*pmdp) && !pmd_trans_huge(*pmdp) && - !pmd_devmap(*pmdp)) || !pmd_present(*pmdp)); + VM_BUG_ON(!pmd_present(*pmdp) || (!pmd_trans_huge(*pmdp) && + !pmd_devmap(*pmdp))); pmd = pmdp_huge_get_and_clear(vma->vm_mm, address, pmdp); flush_pmd_tlb_range(vma, address, address + HPAGE_PMD_SIZE); return pmd;
The condition (A && !C && !D) || !A is equivalent to !A || (A && !C && !D) and can be further simplified to !A || (!C && !D). Signed-off-by: Miaohe Lin <linmiaohe@huawei.com> --- mm/pgtable-generic.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)