On Mon, Mar 11, 2024 at 3:59 PM Andrii Nakryiko <andrii.nakryiko@xxxxxxxxx> wrote: > > > no, I get that, my point was a bit different and purely pedantic. It > doesn't overflow 32-bit only when viewed from user-space addresses > POV. > > It seems like it can overflow when we translate it into kernel address > by adding kernel_vm_start (`kern_vm = get_vm_area(KERN_VM_SZ, > VM_SPARSE | VM_USERMAP)` doesn't guarantee 4GB alignment, IIUC). But I > don't see what kernel-side overflow matters (yet the comment is next > to the code that does kernel-side range mapping, which is why I > commented, the placement of the comment is what makes it a bit more > confusing). Got it. Ok. Will rephrase the comment. > But I was pointing out that if the user-requested area is exactly 4GB > and user_vm_start is aligned at the 4GB boundary, then user_vm_start + > 4GB, technically is incrementing the upper 32 bits of user_vm_start. > Which I don't think matters because it's the exclusive end of range. yes. should be fine. I didn't add a selftest for 4Gb, because not every CI has runners with this much memory. I guess selftest can try to allocate and skip the test if enomem without failing it. Will add it in the follow up. > > > The way you split these zap_pages for page_cnt == 1 and page_cnt > 1 > > > is quite confusing. Why can't you just unconditionally zap_pages() > > > regardless of page_cnt before this loop? And why for page_cnt == 1 we > > > have `page_mapped(page)` check, but it's ok to not check this for > > > page_cnt>1 case? > > > > > > This asymmetric handling is confusing and suggests something more is > > > going on here. Or am I overthinking it? > > > > It's an important optimization for the common case of page_cnt==1. > > If page wasn't mapped into some user vma there is no need to call zap_pages > > which is slow. > > But when page_cnt is big it's much faster to do the batched zap > > which is what this code does. > > For the case of page_cnt=2 or small number there is no good optimization > > to do other than try to count whether all pages in this range are > > not page_mapped() and omit zap_page(). > > I don't think it's worth doing such optimization at this point, > > since page_cnt=1 is likely the most common case. > > If it changes, it can be optimized later. > > yep, makes sense, and a small comment stating that would be useful, IMO :) Ok. Will add that too.
