| Message ID | 20191003185103.26347-2-jarkko.sakkinen@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tpm: Detach page allocation from tpm_buf | expand |
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 <jejb@linux.ibm.com> > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > --- > 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)
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 <jejb@linux.ibm.com> > > Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> > > --- > > 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
From: James Bottomley <James.Bottomley@HansenPartnership.com> 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 <James.Bottomley@HansenPartnership.com> --- 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)
On Mon Oct 07 19, James Bottomley wrote: >From: James Bottomley <James.Bottomley@HansenPartnership.com> >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 <James.Bottomley@HansenPartnership.com> >--- Reviewed-by: Jerry Snitselaar <jsnitsel@redhat.com>
On Mon, Oct 07, 2019 at 12:12:08PM -0700, James Bottomley wrote: > From: James Bottomley <James.Bottomley@HansenPartnership.com> > 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 <James.Bottomley@HansenPartnership.com> Thanks a lot. Makes a lot more sense than the patch that I sent. Reviewed-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> /Jarkko
On Mon, Oct 07, 2019 at 12:12:08PM -0700, James Bottomley wrote: > From: James Bottomley <James.Bottomley@HansenPartnership.com> > 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 <James.Bottomley@HansenPartnership.com> Pushed to master branch. /Jarkko
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 <James.Bottomley@HansenPartnership.com> > > 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 <James.Bottomley@HansenPartnership.c > > om> > > Pushed to master branch. Thanks. 0day spotted a problem with the use of free_page() so I've sent a v2. James
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;
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 <jejb@linux.ibm.com> Signed-off-by: Jarkko Sakkinen <jarkko.sakkinen@linux.intel.com> --- drivers/char/tpm/tpm.h | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)