On Wed, 2019-09-25 at 16:48 +0300, Jarkko Sakkinen wrote:
[...]
> + data_page = alloc_page(GFP_HIGHUSER);
> + if (!data_page)
> + return -ENOMEM;
> +
> + data_ptr = kmap(data_page);
I don't think this is such a good idea. On 64 bit it's no different
from GFP_KERNEL and on 32 bit where we do have highmem, kmap space is
at a premium, so doing a highmem allocation + kmap is more wasteful of
resources than simply doing GFP_KERNEL. In general, you should only do
GFP_HIGHMEM if the page is going to be mostly used by userspace, which
really isn't the case here.
James
On Wed, Sep 25, 2019 at 10:03:46AM -0400, James Bottomley wrote:
> On Wed, 2019-09-25 at 16:48 +0300, Jarkko Sakkinen wrote:
> [...]
> > + data_page = alloc_page(GFP_HIGHUSER);
> > + if (!data_page)
> > + return -ENOMEM;
> > +
> > + data_ptr = kmap(data_page);
>
> I don't think this is such a good idea. On 64 bit it's no different
> from GFP_KERNEL and on 32 bit where we do have highmem, kmap space is
> at a premium, so doing a highmem allocation + kmap is more wasteful of
> resources than simply doing GFP_KERNEL. In general, you should only do
> GFP_HIGHMEM if the page is going to be mostly used by userspace, which
> really isn't the case here.
Changing that in this commit would be wrong even if you are right.
After this commit has been applied it is somewhat easier to make
best choices for allocation in each call site (probably most will
end up using stack).
/Jarkko
On Fri, 2019-09-27 at 16:06 +0300, Jarkko Sakkinen wrote:
> On Wed, Sep 25, 2019 at 10:03:46AM -0400, James Bottomley wrote:
> > On Wed, 2019-09-25 at 16:48 +0300, Jarkko Sakkinen wrote:
> > [...]
> > > + data_page = alloc_page(GFP_HIGHUSER);
> > > + if (!data_page)
> > > + return -ENOMEM;
> > > +
> > > + data_ptr = kmap(data_page);
> >
> > I don't think this is such a good idea. On 64 bit it's no different
> > from GFP_KERNEL and on 32 bit where we do have highmem, kmap space is
> > at a premium, so doing a highmem allocation + kmap is more wasteful of
> > resources than simply doing GFP_KERNEL. In general, you should only do
> > GFP_HIGHMEM if the page is going to be mostly used by userspace, which
> > really isn't the case here.
>
> Changing that in this commit would be wrong even if you are right.
> After this commit has been applied it is somewhat easier to make
> best choices for allocation in each call site (probably most will
> end up using stack).
Agreed, but it could be a separate patch, prior to this one. Why
duplicate the problem all over only to change it later?
Mimi
On Wed, Oct 02, 2019 at 08:41:45AM -0400, Mimi Zohar wrote:
> On Fri, 2019-09-27 at 16:06 +0300, Jarkko Sakkinen wrote:
> > On Wed, Sep 25, 2019 at 10:03:46AM -0400, James Bottomley wrote:
> > > On Wed, 2019-09-25 at 16:48 +0300, Jarkko Sakkinen wrote:
> > > [...]
> > > > + data_page = alloc_page(GFP_HIGHUSER);
> > > > + if (!data_page)
> > > > + return -ENOMEM;
> > > > +
> > > > + data_ptr = kmap(data_page);
> > >
> > > I don't think this is such a good idea. On 64 bit it's no different
> > > from GFP_KERNEL and on 32 bit where we do have highmem, kmap space is
> > > at a premium, so doing a highmem allocation + kmap is more wasteful of
> > > resources than simply doing GFP_KERNEL. In general, you should only do
> > > GFP_HIGHMEM if the page is going to be mostly used by userspace, which
> > > really isn't the case here.
> >
> > Changing that in this commit would be wrong even if you are right.
> > After this commit has been applied it is somewhat easier to make
> > best choices for allocation in each call site (probably most will
> > end up using stack).
>
> Agreed, but it could be a separate patch, prior to this one. ?Why
> duplicate the problem all over only to change it later?
What problem exactly it is duplicating? The existing allocation
scheme here works correctly.
/Jarkko
On Thu, 2019-10-03 at 14:35 +0300, Jarkko Sakkinen wrote:
> On Wed, Oct 02, 2019 at 08:41:45AM -0400, Mimi Zohar wrote:
> > On Fri, 2019-09-27 at 16:06 +0300, Jarkko Sakkinen wrote:
> > > On Wed, Sep 25, 2019 at 10:03:46AM -0400, James Bottomley wrote:
> > > > On Wed, 2019-09-25 at 16:48 +0300, Jarkko Sakkinen wrote:
> > > > [...]
> > > > > + data_page = alloc_page(GFP_HIGHUSER);
> > > > > + if (!data_page)
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + data_ptr = kmap(data_page);
> > > >
> > > > I don't think this is such a good idea. On 64 bit it's no different
> > > > from GFP_KERNEL and on 32 bit where we do have highmem, kmap space is
> > > > at a premium, so doing a highmem allocation + kmap is more wasteful of
> > > > resources than simply doing GFP_KERNEL. In general, you should only do
> > > > GFP_HIGHMEM if the page is going to be mostly used by userspace, which
> > > > really isn't the case here.
> > >
> > > Changing that in this commit would be wrong even if you are right.
> > > After this commit has been applied it is somewhat easier to make
> > > best choices for allocation in each call site (probably most will
> > > end up using stack).
> >
> > Agreed, but it could be a separate patch, prior to this one. Why
> > duplicate the problem all over only to change it later?
>
> What problem exactly it is duplicating? The existing allocation
> scheme here works correctly.
In the current code "alloc_page(GFP_HIGHUSER)" exists in a single
function. With this patch, "alloc_page(GFP_HIGHUSER)" is duplicated
24 times. If it is incorrect and we shouldn't be using GFP_HIGHUSER,
as James said, then why duplicate it 24 times? Fix it as a separate
patch first, that could be backported if needed, and then make the
change.
Mimi
On Thu, Oct 03, 2019 at 08:50:54AM -0400, Mimi Zohar wrote:
> On Thu, 2019-10-03 at 14:35 +0300, Jarkko Sakkinen wrote:
> > On Wed, Oct 02, 2019 at 08:41:45AM -0400, Mimi Zohar wrote:
> > > On Fri, 2019-09-27 at 16:06 +0300, Jarkko Sakkinen wrote:
> > > > On Wed, Sep 25, 2019 at 10:03:46AM -0400, James Bottomley wrote:
> > > > > On Wed, 2019-09-25 at 16:48 +0300, Jarkko Sakkinen wrote:
> > > > > [...]
> > > > > > + data_page = alloc_page(GFP_HIGHUSER);
> > > > > > + if (!data_page)
> > > > > > + return -ENOMEM;
> > > > > > +
> > > > > > + data_ptr = kmap(data_page);
> > > > >
> > > > > I don't think this is such a good idea. On 64 bit it's no different
> > > > > from GFP_KERNEL and on 32 bit where we do have highmem, kmap space is
> > > > > at a premium, so doing a highmem allocation + kmap is more wasteful of
> > > > > resources than simply doing GFP_KERNEL. In general, you should only do
> > > > > GFP_HIGHMEM if the page is going to be mostly used by userspace, which
> > > > > really isn't the case here.
> > > >
> > > > Changing that in this commit would be wrong even if you are right.
> > > > After this commit has been applied it is somewhat easier to make
> > > > best choices for allocation in each call site (probably most will
> > > > end up using stack).
> > >
> > > Agreed, but it could be a separate patch, prior to this one. ?Why
> > > duplicate the problem all over only to change it later?
> >
> > What problem exactly it is duplicating? The existing allocation
> > scheme here works correctly.
>
> In the current code "alloc_page(GFP_HIGHUSER)" exists in a single
> function. ?With this patch, "alloc_page(GFP_HIGHUSER)" is duplicated
> 24 times. ?If it is incorrect and we shouldn't be using GFP_HIGHUSER,
> as James said, then why duplicate it 24 times? ?Fix it as a separate
> patch first, that could be backported if needed, and then make the
> change.
Sorry I mixed this with Jerry's proposal :-) I can do that of course.
/Jarkko