| Message ID | 20210926161259.238054-3-namit@vmware.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/madvise: support process_madvise(MADV_DONTNEED) | expand |
On Sun, Sep 26, 2021 at 09:12:53AM -0700, Nadav Amit wrote: > From: Nadav Amit <namit@vmware.com> > > madvise_dontneed_free() is called only from madvise_vma() and the > behavior is always either MADV_FREE or MADV_DONTNEED. There is no need > to check again in madvise_dontneed_free() if the behavior is any > different. So what. The check is free. Compiler should be clever enough to eliminate the additional check. If there's a new MADV_DONTNEED flavour, the change would have to be effectively reverted. NAK.
> On Sep 27, 2021, at 2:11 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > > On Sun, Sep 26, 2021 at 09:12:53AM -0700, Nadav Amit wrote: >> From: Nadav Amit <namit@vmware.com> >> >> madvise_dontneed_free() is called only from madvise_vma() and the >> behavior is always either MADV_FREE or MADV_DONTNEED. There is no need >> to check again in madvise_dontneed_free() if the behavior is any >> different. > > So what. The check is free. Compiler should be clever enough to eliminate > the additional check. If there's a new MADV_DONTNEED flavour, the change > would have to be effectively reverted. > > NAK. I hate bikeshedding, but I will take the bait, since I see no reason for this NAK. I do not know what future change you have in mind in which quietly failing in madvise_dontneed_free() would be the right behavior. If the current code is presumed to be more “robust” against future changes since there is an additional check, I would argue that this is not the case: failing silently on a code-path that should never run is not the right thing to do. Having redundant checks that are not documented as such do not make the code more readable or maintainable. Having said that, if you want, I can turn this condition into WARN_ON_ONCE() or VM_BUG_ON(), although I really see no reason to do so. [ You might just as well add a default statement to the switch in madvise_behavior(), which BTW would have been much more reasonable, but only if it does not fail silently as the one we discuss. ] Note that I made this change not out of boredom, but because I needed to change this piece of code later for TLB batching. I did not want to sneak this change in another patch or to leave this confusing code. Anyhow, I wasted enough time on this trivial patch.
On Mon, Sep 27, 2021 at 04:05:47AM -0700, Nadav Amit wrote: > Having said that, if you want, I can turn this condition into > WARN_ON_ONCE() or VM_BUG_ON(), although I really see no reason to > do so. BUILD_BUG() should be fine here.
> On Sep 27, 2021, at 5:19 AM, Kirill A. Shutemov <kirill@shutemov.name> wrote: > > On Mon, Sep 27, 2021 at 04:05:47AM -0700, Nadav Amit wrote: >> Having said that, if you want, I can turn this condition into >> WARN_ON_ONCE() or VM_BUG_ON(), although I really see no reason to >> do so. > > BUILD_BUG() should be fine here. It does not work. At least my gcc is not smart enough to figure it out in build time. I can put instead: BUILD_BUG_ON(__builtin_constant_p(behavior)); for potentially smarter compilers (clang?), but I doubt it would work.
diff --git a/mm/madvise.c b/mm/madvise.c index a2b05352ebfe..fe843513a4e8 100644 --- a/mm/madvise.c +++ b/mm/madvise.c @@ -820,10 +820,8 @@ static long madvise_dontneed_free(struct vm_area_struct *vma, if (behavior == MADV_DONTNEED) return madvise_dontneed_single_vma(vma, start, end); - else if (behavior == MADV_FREE) + else /* behavior == MADV_FREE */ return madvise_free_single_vma(vma, start, end); - else - return -EINVAL; } static long madvise_populate(struct vm_area_struct *vma,