Re: [PATCH bpf-next 02/15] xsk: add user memory registration support sockopt

[Willem de Bruijn <willemdebruijn.kernel@gmail.com>]

On Mon, Apr 23, 2018 at 9:56 AM, Björn Töpel <bjorn.topel@gmail.com> wrote:
> From: Björn Töpel <bjorn.topel@intel.com>
>
> In this commit the base structure of the AF_XDP address family is set
> up. Further, we introduce the abilty register a window of user memory
> to the kernel via the XDP_UMEM_REG setsockopt syscall. The memory
> window is viewed by an AF_XDP socket as a set of equally large
> frames. After a user memory registration all frames are "owned" by the
> user application, and not the kernel.
>
> Co-authored-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Signed-off-by: Magnus Karlsson <magnus.karlsson@intel.com>
> Signed-off-by: Björn Töpel <bjorn.topel@intel.com>

> +static void xdp_umem_release(struct xdp_umem *umem)
> +{
> +       struct task_struct *task;
> +       struct mm_struct *mm;
> +       unsigned long diff;
> +
> +       if (umem->pgs) {
> +               xdp_umem_unpin_pages(umem);
> +
> +               task = get_pid_task(umem->pid, PIDTYPE_PID);
> +               put_pid(umem->pid);
> +               if (!task)
> +                       goto out;
> +               mm = get_task_mm(task);
> +               put_task_struct(task);
> +               if (!mm)
> +                       goto out;
> +
> +               diff = umem->size >> PAGE_SHIFT;

Need to round up or size must always be a multiple of PAGE_SIZE.

> +
> +               down_write(&mm->mmap_sem);
> +               mm->pinned_vm -= diff;
> +               up_write(&mm->mmap_sem);

When using user->locked_vm for resource limit checks, no need
to also update mm->pinned_vm?

> +static int __xdp_umem_reg(struct xdp_umem *umem, struct xdp_umem_reg *mr)
> +{
> +       u32 frame_size = mr->frame_size, frame_headroom = mr->frame_headroom;
> +       u64 addr = mr->addr, size = mr->len;
> +       unsigned int nframes;
> +       int size_chk, err;
> +
> +       if (frame_size < XDP_UMEM_MIN_FRAME_SIZE || frame_size > PAGE_SIZE) {
> +               /* Strictly speaking we could support this, if:
> +                * - huge pages, or*
> +                * - using an IOMMU, or
> +                * - making sure the memory area is consecutive
> +                * but for now, we simply say "computer says no".
> +                */
> +               return -EINVAL;
> +       }

Ideally, AF_XDP subsumes all packet socket use cases. It does not
have packet v3's small packet optimizations of variable sized frames
and block signaling.

I don't suggest adding that now. But for the non-zerocopy case, it may
make sense to ensure that nothing is blocking a later addition of these
features. Especially for header-only (snaplen) workloads. So far, I don't
see any issues.

> +       if (!is_power_of_2(frame_size))
> +               return -EINVAL;
> +
> +       if (!PAGE_ALIGNED(addr)) {
> +               /* Memory area has to be page size aligned. For
> +                * simplicity, this might change.
> +                */
> +               return -EINVAL;
> +       }
> +
> +       if ((addr + size) < addr)
> +               return -EINVAL;
> +
> +       nframes = size / frame_size;
> +       if (nframes == 0 || nframes > UINT_MAX)
> +               return -EINVAL;

You may also want a check here that nframes * frame_size is at least
PAGE_SIZE and probably a multiple of that.

> +       frame_headroom = ALIGN(frame_headroom, 64);
> +
> +       size_chk = frame_size - frame_headroom - XDP_PACKET_HEADROOM;
> +       if (size_chk < 0)
> +               return -EINVAL;
> +
> +       umem->pid = get_task_pid(current, PIDTYPE_PID);
> +       umem->size = (size_t)size;
> +       umem->address = (unsigned long)addr;
> +       umem->props.frame_size = frame_size;
> +       umem->props.nframes = nframes;
> +       umem->frame_headroom = frame_headroom;
> +       umem->npgs = size / PAGE_SIZE;
> +       umem->pgs = NULL;
> +       umem->user = NULL;
> +
> +       umem->frame_size_log2 = ilog2(frame_size);
> +       umem->nfpp_mask = (PAGE_SIZE / frame_size) - 1;
> +       umem->nfpplog2 = ilog2(PAGE_SIZE / frame_size);
> +       atomic_set(&umem->users, 1);
> +
> +       err = xdp_umem_account_pages(umem);
> +       if (err)
> +               goto out;
> +
> +       err = xdp_umem_pin_pages(umem);
> +       if (err)

need to call xdp_umem_unaccount_pages on error
> +               goto out;
> +       return 0;
> +
> +out:
> +       put_pid(umem->pid);
> +       return err;
> +}