[v3,bpf-next,06/10] lib/buildid: implement sleepable build_id_parse() API

Commit Message

Andrii Nakryiko July 30, 2024, 8:39 p.m. UTC

Extend freader with a flag specifying whether it's OK to cause page
fault to fetch file data that is not already physically present in
memory. With this, it's now easy to wait for data if the caller is
running in sleepable (faultable) context.

We utilize read_cache_folio() to bring the desired file page into page
cache, after which the rest of the logic works just the same at page level.

Suggested-by: Omar Sandoval <[e-mail]>
Cc: Shakeel Butt <[e-mail]>
Cc: Johannes Weiner <[e-mail]>
Signed-off-by: Andrii Nakryiko <[e-mail]>
---
 lib/buildid.c | 50 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 16 deletions(-)

Comments

Andrii Nakryiko July 31, 2024, 9:56 p.m. UTC | #1

On Tue, Jul 30, 2024 at 1:39 PM Andrii Nakryiko <[e-mail]> wrote:
>
> Extend freader with a flag specifying whether it's OK to cause page
> fault to fetch file data that is not already physically present in
> memory. With this, it's now easy to wait for data if the caller is
> running in sleepable (faultable) context.
>
> We utilize read_cache_folio() to bring the desired file page into page
> cache, after which the rest of the logic works just the same at page level.
>
> Suggested-by: Omar Sandoval <[e-mail]>
> Cc: Shakeel Butt <[e-mail]>
> Cc: Johannes Weiner <[e-mail]>
> Signed-off-by: Andrii Nakryiko <[e-mail]>
> ---
>  lib/buildid.c | 50 ++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 34 insertions(+), 16 deletions(-)
>
> diff --git a/lib/buildid.c b/lib/buildid.c
> index 5c869a2a30ab..6b5558cd95bf 100644
> --- a/lib/buildid.c
> +++ b/lib/buildid.c
> @@ -20,6 +20,7 @@ struct freader {
>                         struct page *page;
>                         void *page_addr;
>                         u64 file_off;
> +                       bool may_fault;
>                 };
>                 struct {
>                         const char *data;
> @@ -29,12 +30,13 @@ struct freader {
>  };
>
>  static void freader_init_from_file(struct freader *r, void *buf, u32 buf_sz,
> -                                  struct address_space *mapping)
> +                                  struct address_space *mapping, bool may_fault)
>  {
>         memset(r, 0, sizeof(*r));
>         r->buf = buf;
>         r->buf_sz = buf_sz;
>         r->mapping = mapping;
> +       r->may_fault = may_fault;
>  }
>
>  static void freader_init_from_mem(struct freader *r, const char *data, u64 data_sz)
> @@ -60,6 +62,17 @@ static int freader_get_page(struct freader *r, u64 file_off)
>         freader_put_page(r);
>
>         r->page = find_get_page(r->mapping, pg_off);
> +
> +       if (!r->page && r->may_fault) {
> +               struct folio *folio;
> +
> +               folio = read_cache_folio(r->mapping, pg_off, NULL, NULL);
> +               if (IS_ERR(folio))
> +                       return PTR_ERR(folio);
> +
> +               r->page = folio_file_page(folio, pg_off);
> +       }
> +

mm folks, is this the sane way to do this? Can you please take a look
and provide your ack? Thank you!

>         if (!r->page)
>                 return -EFAULT; /* page not mapped */
>
> @@ -273,18 +286,8 @@ static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
>  /* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */
>  #define MAX_FREADER_BUF_SZ 64
>
> -/*
> - * Parse build ID of ELF file mapped to vma
> - * @vma:      vma object
> - * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
> - * @size:     returns actual build id size in case of success
> - *
> - * Assumes no page fault can be taken, so if relevant portions of ELF file are
> - * not already paged in, fetching of build ID fails.
> - *
> - * Return: 0 on success; negative error, otherwise
> - */
> -int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
> +static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
> +                           __u32 *size, bool may_fault)
>  {
>         const Elf32_Ehdr *ehdr;
>         struct freader r;
> @@ -295,7 +298,7 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
>         if (!vma->vm_file)
>                 return -EINVAL;
>
> -       freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file->f_mapping);
> +       freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file->f_mapping, may_fault);
>
>         /* fetch first 18 bytes of ELF header for checks */
>         ehdr = freader_fetch(&r, 0, offsetofend(Elf32_Ehdr, e_type));
> @@ -323,6 +326,22 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
>         return ret;
>  }
>
> +/*
> + * Parse build ID of ELF file mapped to vma
> + * @vma:      vma object
> + * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
> + * @size:     returns actual build id size in case of success
> + *
> + * Assumes no page fault can be taken, so if relevant portions of ELF file are
> + * not already paged in, fetching of build ID fails.
> + *
> + * Return: 0 on success; negative error, otherwise
> + */
> +int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
> +{
> +       return __build_id_parse(vma, build_id, size, false /* !may_fault */);
> +}
> +
>  /*
>   * Parse build ID of ELF file mapped to VMA
>   * @vma:      vma object
> @@ -336,8 +355,7 @@ int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
>   */
>  int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
>  {
> -       /* fallback to non-faultable version for now */
> -       return build_id_parse_nofault(vma, build_id, size);
> +       return __build_id_parse(vma, build_id, size, true /* may_fault */);
>  }
>
>  /**
> --
> 2.43.0
>
>

Shakeel Butt Aug. 5, 2024, 6:30 p.m. UTC | #2

On Wed, Jul 31, 2024 at 02:56:00PM GMT, Andrii Nakryiko wrote:
> On Tue, Jul 30, 2024 at 1:39 PM Andrii Nakryiko <[e-mail]> wrote:
> >
> > Extend freader with a flag specifying whether it's OK to cause page
> > fault to fetch file data that is not already physically present in
> > memory. With this, it's now easy to wait for data if the caller is
> > running in sleepable (faultable) context.
> >
> > We utilize read_cache_folio() to bring the desired file page into page
> > cache, after which the rest of the logic works just the same at page level.
> >
> > Suggested-by: Omar Sandoval <[e-mail]>
> > Cc: Shakeel Butt <[e-mail]>
> > Cc: Johannes Weiner <[e-mail]>
> > Signed-off-by: Andrii Nakryiko <[e-mail]>
> > ---
> >  lib/buildid.c | 50 ++++++++++++++++++++++++++++++++++----------------
> >  1 file changed, 34 insertions(+), 16 deletions(-)
> >
> > diff --git a/lib/buildid.c b/lib/buildid.c
> > index 5c869a2a30ab..6b5558cd95bf 100644
> > --- a/lib/buildid.c
> > +++ b/lib/buildid.c
> > @@ -20,6 +20,7 @@ struct freader {
> >                         struct page *page;
> >                         void *page_addr;
> >                         u64 file_off;
> > +                       bool may_fault;
> >                 };
> >                 struct {
> >                         const char *data;
> > @@ -29,12 +30,13 @@ struct freader {
> >  };
> >
> >  static void freader_init_from_file(struct freader *r, void *buf, u32 buf_sz,
> > -                                  struct address_space *mapping)
> > +                                  struct address_space *mapping, bool may_fault)
> >  {
> >         memset(r, 0, sizeof(*r));
> >         r->buf = buf;
> >         r->buf_sz = buf_sz;
> >         r->mapping = mapping;
> > +       r->may_fault = may_fault;
> >  }
> >
> >  static void freader_init_from_mem(struct freader *r, const char *data, u64 data_sz)
> > @@ -60,6 +62,17 @@ static int freader_get_page(struct freader *r, u64 file_off)
> >         freader_put_page(r);
> >
> >         r->page = find_get_page(r->mapping, pg_off);
> > +
> > +       if (!r->page && r->may_fault) {
> > +               struct folio *folio;
> > +
> > +               folio = read_cache_folio(r->mapping, pg_off, NULL, NULL);
> > +               if (IS_ERR(folio))
> > +                       return PTR_ERR(folio);
> > +
> > +               r->page = folio_file_page(folio, pg_off);
> > +       }
> > +
> 
> mm folks, is this the sane way to do this? Can you please take a look
> and provide your ack? Thank you!

Yes I think this is sane. I just had to check if read_cache_folio() is
also elevating a reference to the folio similar to find_get_page() and I
think it is doing so. I will go through the series soon.

Patch

diff --git a/lib/buildid.c b/lib/buildid.c
index 5c869a2a30ab..6b5558cd95bf 100644
--- a/lib/buildid.c
+++ b/lib/buildid.c
@@ -20,6 +20,7 @@  struct freader {
 			struct page *page;
 			void *page_addr;
 			u64 file_off;
+			bool may_fault;
 		};
 		struct {
 			const char *data;
@@ -29,12 +30,13 @@  struct freader {
 };
 
 static void freader_init_from_file(struct freader *r, void *buf, u32 buf_sz,
-				   struct address_space *mapping)
+				   struct address_space *mapping, bool may_fault)
 {
 	memset(r, 0, sizeof(*r));
 	r->buf = buf;
 	r->buf_sz = buf_sz;
 	r->mapping = mapping;
+	r->may_fault = may_fault;
 }
 
 static void freader_init_from_mem(struct freader *r, const char *data, u64 data_sz)
@@ -60,6 +62,17 @@  static int freader_get_page(struct freader *r, u64 file_off)
 	freader_put_page(r);
 
 	r->page = find_get_page(r->mapping, pg_off);
+
+	if (!r->page && r->may_fault) {
+		struct folio *folio;
+
+		folio = read_cache_folio(r->mapping, pg_off, NULL, NULL);
+		if (IS_ERR(folio))
+			return PTR_ERR(folio);
+
+		r->page = folio_file_page(folio, pg_off);
+	}
+
 	if (!r->page)
 		return -EFAULT;	/* page not mapped */
 
@@ -273,18 +286,8 @@  static int get_build_id_64(struct freader *r, unsigned char *build_id, __u32 *si
 /* enough for Elf64_Ehdr, Elf64_Phdr, and all the smaller requests */
 #define MAX_FREADER_BUF_SZ 64
 
-/*
- * Parse build ID of ELF file mapped to vma
- * @vma:      vma object
- * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
- * @size:     returns actual build id size in case of success
- *
- * Assumes no page fault can be taken, so if relevant portions of ELF file are
- * not already paged in, fetching of build ID fails.
- *
- * Return: 0 on success; negative error, otherwise
- */
-int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
+static int __build_id_parse(struct vm_area_struct *vma, unsigned char *build_id,
+			    __u32 *size, bool may_fault)
 {
 	const Elf32_Ehdr *ehdr;
 	struct freader r;
@@ -295,7 +298,7 @@  int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
 	if (!vma->vm_file)
 		return -EINVAL;
 
-	freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file->f_mapping);
+	freader_init_from_file(&r, buf, sizeof(buf), vma->vm_file->f_mapping, may_fault);
 
 	/* fetch first 18 bytes of ELF header for checks */
 	ehdr = freader_fetch(&r, 0, offsetofend(Elf32_Ehdr, e_type));
@@ -323,6 +326,22 @@  int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
 	return ret;
 }
 
+/*
+ * Parse build ID of ELF file mapped to vma
+ * @vma:      vma object
+ * @build_id: buffer to store build id, at least BUILD_ID_SIZE long
+ * @size:     returns actual build id size in case of success
+ *
+ * Assumes no page fault can be taken, so if relevant portions of ELF file are
+ * not already paged in, fetching of build ID fails.
+ *
+ * Return: 0 on success; negative error, otherwise
+ */
+int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
+{
+	return __build_id_parse(vma, build_id, size, false /* !may_fault */);
+}
+
 /*
  * Parse build ID of ELF file mapped to VMA
  * @vma:      vma object
@@ -336,8 +355,7 @@  int build_id_parse_nofault(struct vm_area_struct *vma, unsigned char *build_id,
  */
 int build_id_parse(struct vm_area_struct *vma, unsigned char *build_id, __u32 *size)
 {
-	/* fallback to non-faultable version for now */
-	return build_id_parse_nofault(vma, build_id, size);
+	return __build_id_parse(vma, build_id, size, true /* may_fault */);
 }
 
 /**