On Tue, Oct 03, 2023 at 12:44:45AM -0700, Vivek Kasireddy wrote:

> +/**
> + * pin_user_pages_fd() - pin user pages associated with a file
> + * @fd:         the fd whose pages are to be pinned
> + * @start:      starting file offset
> + * @nr_pages:   number of pages from start to pin
> + * @gup_flags:  flags modifying pin behaviour
> + * @pages:      array that receives pointers to the pages pinned.
> + *              Should be at least nr_pages long.
> + *
> + * Attempt to pin (and migrate) pages associated with a file belonging to
> + * either shmem or hugetlbfs. An error is returned if pages associated with
> + * hugetlbfs files are not present in the page cache. However, shmem pages
> + * are swapped in or allocated if they are not present in the page cache.
> + *
> + * Returns number of pages pinned. This would be equal to the number of
> + * pages requested.
> + * If nr_pages is 0 or negative, returns 0. If no pages were pinned, returns
> + * -errno.
> + */
> +long pin_user_pages_fd(int fd, pgoff_t start, unsigned long nr_pages,
> +                    unsigned int gup_flags, struct page **pages)
> +{
> +     struct page *page;
> +     struct file *filep;
> +     unsigned int flags, i;
> +     long ret;
> +
> +     if (nr_pages <= 0)
> +             return 0;
> +     if (!is_valid_gup_args(pages, NULL, &gup_flags, FOLL_PIN))
> +             return 0;
> +
> +     if (start < 0)
> +             return -EINVAL;
> +
> +     filep = fget(fd);
> +     if (!filep)
> +         return -EINVAL;

I think the caller should pass in the file *

In some cases we will need to hold a reference on it for a long time.

> +     if (!shmem_file(filep) && !is_file_hugepages(filep))
> +         return -EINVAL;
> +
> +     flags = memalloc_pin_save();
> +     do {
> +             for (i = 0; i < nr_pages; i++) {
> +                     if (shmem_mapping(filep->f_mapping)) {
> +                             page = shmem_read_mapping_page(filep->f_mapping,
> +                                                            start + i);
> +                             if (IS_ERR(page)) {
> +                                     ret = PTR_ERR(page);
> +                                     goto err;
> +                             }
> +                     } else {
> +                             page = find_get_page_flags(filep->f_mapping,
> +                                                        start + i,
> +                                                        FGP_ACCESSED);
> +                             if (!page) {
> +                                     ret = -EINVAL;
> +                                     goto err;
> +                             }

I don't know these APIs at all, but I admit to being surprised we need
the special case for shmem ?

> +                     ret = try_grab_page(page, FOLL_PIN);
> +                     if (unlikely(ret))
> +                             goto err;
> +
> +                     pages[i] = page;
> +                     put_page(pages[i]);
> +             }
> +
> +             ret = check_and_migrate_movable_pages(nr_pages, pages);
> +     } while (ret == -EAGAIN);

It seems OK, but I do wish it was faster :) Maybe for another day.

Jason

Reply via email to