Re: [PATCH v3 1/2] tpm: Use GFP_KERNEL for allocating struct tpm_buf
On Thu, 2019-10-10 at 00:36 +0300, Jarkko Sakkinen wrote: > On Mon, Oct 07, 2019 at 12:12:08PM -0700, James Bottomley wrote: > > From: James Bottomley > > Subject: [PATCH] tpm: use GFP kernel for tpm_buf allocations > > > > The current code uses GFP_HIGHMEM, which is wrong because > > GFP_HIGHMEM (on 32 bit systems) is memory ordinarily inaccessible > > to the kernel and should only be used for allocations affecting > > userspace. In order to make highmem visible to the kernel on 32 > > bit it has to be kmapped, which consumes valuable entries in the > > kmap region. Since the tpm_buf is only ever used in the kernel, > > switch to using a GFP_KERNEL allocation so as not to waste kmap > > space on 32 bits. > > > > Fixes: a74f8b36352e (tpm: introduce tpm_buf) > > Signed-off-by: James Bottomley > om> > > Pushed to master branch. Thanks. 0day spotted a problem with the use of free_page() so I've sent a v2. James
Re: [PATCH v3 1/2] tpm: Use GFP_KERNEL for allocating struct tpm_buf
On Mon, Oct 07, 2019 at 12:12:08PM -0700, James Bottomley wrote: > From: James Bottomley > Subject: [PATCH] tpm: use GFP kernel for tpm_buf allocations > > The current code uses GFP_HIGHMEM, which is wrong because GFP_HIGHMEM > (on 32 bit systems) is memory ordinarily inaccessible to the kernel > and should only be used for allocations affecting userspace. In order > to make highmem visible to the kernel on 32 bit it has to be kmapped, > which consumes valuable entries in the kmap region. Since the tpm_buf > is only ever used in the kernel, switch to using a GFP_KERNEL > allocation so as not to waste kmap space on 32 bits. > > Fixes: a74f8b36352e (tpm: introduce tpm_buf) > Signed-off-by: James Bottomley Pushed to master branch. /Jarkko
Re: [PATCH v3 1/2] tpm: Use GFP_KERNEL for allocating struct tpm_buf
On Mon, Oct 07, 2019 at 12:12:08PM -0700, James Bottomley wrote: > From: James Bottomley > Subject: [PATCH] tpm: use GFP kernel for tpm_buf allocations > > The current code uses GFP_HIGHMEM, which is wrong because GFP_HIGHMEM > (on 32 bit systems) is memory ordinarily inaccessible to the kernel > and should only be used for allocations affecting userspace. In order > to make highmem visible to the kernel on 32 bit it has to be kmapped, > which consumes valuable entries in the kmap region. Since the tpm_buf > is only ever used in the kernel, switch to using a GFP_KERNEL > allocation so as not to waste kmap space on 32 bits. > > Fixes: a74f8b36352e (tpm: introduce tpm_buf) > Signed-off-by: James Bottomley Thanks a lot. Makes a lot more sense than the patch that I sent. Reviewed-by: Jarkko Sakkinen /Jarkko
Re: [PATCH v3 1/2] tpm: Use GFP_KERNEL for allocating struct tpm_buf
On Mon Oct 07 19, James Bottomley wrote: From: James Bottomley Subject: [PATCH] tpm: use GFP kernel for tpm_buf allocations The current code uses GFP_HIGHMEM, which is wrong because GFP_HIGHMEM (on 32 bit systems) is memory ordinarily inaccessible to the kernel and should only be used for allocations affecting userspace. In order to make highmem visible to the kernel on 32 bit it has to be kmapped, which consumes valuable entries in the kmap region. Since the tpm_buf is only ever used in the kernel, switch to using a GFP_KERNEL allocation so as not to waste kmap space on 32 bits. Fixes: a74f8b36352e (tpm: introduce tpm_buf) Signed-off-by: James Bottomley --- Reviewed-by: Jerry Snitselaar
Re: [PATCH v3 1/2] tpm: Use GFP_KERNEL for allocating struct tpm_buf
From: James Bottomley
Subject: [PATCH] tpm: use GFP kernel for tpm_buf allocations
The current code uses GFP_HIGHMEM, which is wrong because GFP_HIGHMEM
(on 32 bit systems) is memory ordinarily inaccessible to the kernel
and should only be used for allocations affecting userspace. In order
to make highmem visible to the kernel on 32 bit it has to be kmapped,
which consumes valuable entries in the kmap region. Since the tpm_buf
is only ever used in the kernel, switch to using a GFP_KERNEL
allocation so as not to waste kmap space on 32 bits.
Fixes: a74f8b36352e (tpm: introduce tpm_buf)
Signed-off-by: James Bottomley
---
drivers/char/tpm/tpm.h | 9 +++--
1 file changed, 3 insertions(+), 6 deletions(-)
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..b4f1cbf344b6 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -284,7 +284,6 @@ enum tpm_buf_flags {
};
struct tpm_buf {
- struct page *data_page;
unsigned int flags;
u8 *data;
};
@@ -300,20 +299,18 @@ static inline void tpm_buf_reset(struct tpm_buf *buf, u16
tag, u32 ordinal)
static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
{
- buf->data_page = alloc_page(GFP_HIGHUSER);
- if (!buf->data_page)
+ buf->data = (u8 *)__get_free_page(GFP_KERNEL);
+ if (!buf->data)
return -ENOMEM;
buf->flags = 0;
- buf->data = kmap(buf->data_page);
tpm_buf_reset(buf, tag, ordinal);
return 0;
}
static inline void tpm_buf_destroy(struct tpm_buf *buf)
{
- kunmap(buf->data_page);
- __free_page(buf->data_page);
+ free_page(buf->data);
}
static inline u32 tpm_buf_length(struct tpm_buf *buf)
--
2.16.4
Re: [PATCH v3 1/2] tpm: Use GFP_KERNEL for allocating struct tpm_buf
On Thu, Oct 03, 2019 at 05:25:16PM -0700, James Bottomley wrote:
> On Thu, 2019-10-03 at 21:51 +0300, Jarkko Sakkinen wrote:
> > Switch from GFP_HIGHUSER to GFP_KERNEL. On 32-bit platforms kmap()
> > space
> > could be unnecessarily wasted because of using GFP_HIGHUSER by taking
> > a
> > page of from the highmem.
> >
> > Suggested-by: James Bottomley
> > Signed-off-by: Jarkko Sakkinen
> > ---
> > drivers/char/tpm/tpm.h | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> > index a4f74dd02a35..d20745965350 100644
> > --- a/drivers/char/tpm/tpm.h
> > +++ b/drivers/char/tpm/tpm.h
> > @@ -297,7 +297,7 @@ static inline void tpm_buf_reset(struct tpm_buf
> > *buf, u16 tag, u32 ordinal)
> >
> > static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32
> > ordinal)
> > {
> > - buf->data_page = alloc_page(GFP_HIGHUSER);
> > + buf->data_page = alloc_page(GFP_KERNEL);
> > if (!buf->data_page)
> > return -ENOMEM;
>
> The kmap/kunmap needs removing as well, and now the data_page field
> isn't necessary, so it can go. I think the result should be something
> like the below (uncompiled and untested).
>
> James
>
> ---
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index a7fea3e0ca86..b4f1cbf344b6 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -284,7 +284,6 @@ enum tpm_buf_flags {
> };
>
> struct tpm_buf {
> - struct page *data_page;
> unsigned int flags;
> u8 *data;
> };
> @@ -300,20 +299,18 @@ static inline void tpm_buf_reset(struct tpm_buf *buf,
> u16 tag, u32 ordinal)
>
> static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
> {
> - buf->data_page = alloc_page(GFP_HIGHUSER);
> - if (!buf->data_page)
> + buf->data = (u8 *)__get_free_page(GFP_KERNEL);
> + if (!buf->data)
> return -ENOMEM;
>
> buf->flags = 0;
> - buf->data = kmap(buf->data_page);
> tpm_buf_reset(buf, tag, ordinal);
> return 0;
> }
>
> static inline void tpm_buf_destroy(struct tpm_buf *buf)
> {
> - kunmap(buf->data_page);
> - __free_page(buf->data_page);
> + free_page(buf->data);
> }
>
> static inline u32 tpm_buf_length(struct tpm_buf *buf)
>
>
Care to make this a proper patch? Much better idea to do it this
way, agreed.
/Jarkko
Re: [PATCH v3 1/2] tpm: Use GFP_KERNEL for allocating struct tpm_buf
On Thu, 2019-10-03 at 21:51 +0300, Jarkko Sakkinen wrote:
> Switch from GFP_HIGHUSER to GFP_KERNEL. On 32-bit platforms kmap()
> space
> could be unnecessarily wasted because of using GFP_HIGHUSER by taking
> a
> page of from the highmem.
>
> Suggested-by: James Bottomley
> Signed-off-by: Jarkko Sakkinen
> ---
> drivers/char/tpm/tpm.h | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
> index a4f74dd02a35..d20745965350 100644
> --- a/drivers/char/tpm/tpm.h
> +++ b/drivers/char/tpm/tpm.h
> @@ -297,7 +297,7 @@ static inline void tpm_buf_reset(struct tpm_buf
> *buf, u16 tag, u32 ordinal)
>
> static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32
> ordinal)
> {
> - buf->data_page = alloc_page(GFP_HIGHUSER);
> + buf->data_page = alloc_page(GFP_KERNEL);
> if (!buf->data_page)
> return -ENOMEM;
The kmap/kunmap needs removing as well, and now the data_page field
isn't necessary, so it can go. I think the result should be something
like the below (uncompiled and untested).
James
---
diff --git a/drivers/char/tpm/tpm.h b/drivers/char/tpm/tpm.h
index a7fea3e0ca86..b4f1cbf344b6 100644
--- a/drivers/char/tpm/tpm.h
+++ b/drivers/char/tpm/tpm.h
@@ -284,7 +284,6 @@ enum tpm_buf_flags {
};
struct tpm_buf {
- struct page *data_page;
unsigned int flags;
u8 *data;
};
@@ -300,20 +299,18 @@ static inline void tpm_buf_reset(struct tpm_buf *buf, u16
tag, u32 ordinal)
static inline int tpm_buf_init(struct tpm_buf *buf, u16 tag, u32 ordinal)
{
- buf->data_page = alloc_page(GFP_HIGHUSER);
- if (!buf->data_page)
+ buf->data = (u8 *)__get_free_page(GFP_KERNEL);
+ if (!buf->data)
return -ENOMEM;
buf->flags = 0;
- buf->data = kmap(buf->data_page);
tpm_buf_reset(buf, tag, ordinal);
return 0;
}
static inline void tpm_buf_destroy(struct tpm_buf *buf)
{
- kunmap(buf->data_page);
- __free_page(buf->data_page);
+ free_page(buf->data);
}
static inline u32 tpm_buf_length(struct tpm_buf *buf)
