Re: [PATCH v3 bpf-next 01/14] bpf: Introduce bpf_arena.

[Alexei Starovoitov <alexei.starovoitov@gmail.com>]

On Mon, Mar 11, 2024 at 3:59 PM Andrii Nakryiko
<andrii.nakryiko@gmail.com> 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.