Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1720424 > unrolled thread

Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag Pavel Machek <pavel@ucw.cz> - 2017-08-25 23:40 +0200
    Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag NeilBrown <neilb@suse.com> - 2017-08-26 06:20 +0200
      Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag Michal Hocko <mhocko@kernel.org> - 2017-08-28 14:40 +0200
        Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag Pavel Machek <pavel@ucw.cz> - 2017-08-31 11:10 +0200
          Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag Mel Gorman <mgorman@suse.de> - 2017-08-31 11:30 +0200
    Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag Michal Hocko <mhocko@kernel.org> - 2017-08-28 14:40 +0200
      Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag Pavel Machek <pavel@ucw.cz> - 2017-08-31 11:20 +0200

#1720424 — Re: [RFC PATCH] treewide: remove GFP_TEMPORARY allocation flag

[Multipart message — attachments visible in raw view] — view raw

On Fri 2017-08-25 10:04:42, Michal Hocko wrote:
> On Fri 25-08-17 09:28:19, Pavel Machek wrote:
> > On Fri 2017-08-25 08:35:46, Michal Hocko wrote:
> > > On Wed 23-08-17 19:57:09, Pavel Machek wrote:
> [...]
> > > > Dunno. < 1msec probably is temporary, 1 hour probably is not. If it causes
> > > > problems, can you just #define GFP_TEMPORARY GFP_KERNEL ? Treewide replace,
> > > > and then starting again goes not look attractive to me.
> > > 
> > > I do not think we want a highlevel GFP_TEMPORARY without any meaning.
> > > This just supports spreading the flag usage without a clear semantic
> > > and it will lead to even bigger mess. Once we can actually define what
> > > the flag means we can also add its users based on that new semantic.
> > 
> > It has real meaning.
> 
> Which is?

"This allocation is temporary. It lasts milliseconds, not hours."

> > You can define more exact meaning, and then adjust the usage. But
> > there's no need to do treewide replacement...
> 
> I have checked most of them and except for the initially added onces the
> large portion where added without a good reasons or even break an
> intuitive meaning by taking locks.

I don't see it. kmalloc() itself takes locks. Of course everyone takes
locks. I don't think that's intuitive meaning.

> Seriously, if we need a short term semantic it should be clearly defined
> first.

"milliseconds, not hours."

> Is there any specific case why you think this patch is in a wrong
> direction? E.g. a measurable regression?

Not playing that game. You should argue why it is improvement. And I
don't believe you did.

Best regards,

								Pavel

-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [next] | [standalone]

#1720533

[Multipart message — attachments visible in raw view] — view raw

On Fri, Aug 25 2017, Pavel Machek wrote:

> On Fri 2017-08-25 10:04:42, Michal Hocko wrote:
>> On Fri 25-08-17 09:28:19, Pavel Machek wrote:
>> > On Fri 2017-08-25 08:35:46, Michal Hocko wrote:
>> > > On Wed 23-08-17 19:57:09, Pavel Machek wrote:
>> [...]
>> > > > Dunno. < 1msec probably is temporary, 1 hour probably is not. If it causes
>> > > > problems, can you just #define GFP_TEMPORARY GFP_KERNEL ? Treewide replace,
>> > > > and then starting again goes not look attractive to me.
>> > > 
>> > > I do not think we want a highlevel GFP_TEMPORARY without any meaning.
>> > > This just supports spreading the flag usage without a clear semantic
>> > > and it will lead to even bigger mess. Once we can actually define what
>> > > the flag means we can also add its users based on that new semantic.
>> > 
>> > It has real meaning.
>> 
>> Which is?
>
> "This allocation is temporary. It lasts milliseconds, not hours."

It isn't sufficient to give a rule for when GFP_TEMPORARY will be used,
you also need to explain (at least in general terms) how the information
will be used.  Also you need to give guidelines on whether the flag
should be set for allocation that will last seconds or minutes.

If we have a flag that doesn't have a well defined meaning that actually
affects behavior, it will not be used consistently, and if we ever
change exactly how it behaves we can expect things to break.  So it is
better not to have a flag, than to have a poorly defined flag.

My current thoughts is that the important criteria is not how long the
allocation will be used for, but whether it is reclaimable.  Allocations
that will only last 5 msecs are reclaimable by calling "usleep(5000)".
Other allocations might be reclaimable in other ways.  Allocations that
are not reclaimable may well be directed to a more restricted pool of
memory, and might be more likely to fail.  If we grew a strong
"reclaimable" concept, this 'temporary' concept that you want to hold on
to would become a burden.

NeilBrown

[toc] | [prev] | [next] | [standalone]

#1721576

On Sat 26-08-17 14:11:33, NeilBrown wrote:
> On Fri, Aug 25 2017, Pavel Machek wrote:
> 
> > On Fri 2017-08-25 10:04:42, Michal Hocko wrote:
> >> On Fri 25-08-17 09:28:19, Pavel Machek wrote:
> >> > On Fri 2017-08-25 08:35:46, Michal Hocko wrote:
> >> > > On Wed 23-08-17 19:57:09, Pavel Machek wrote:
> >> [...]
> >> > > > Dunno. < 1msec probably is temporary, 1 hour probably is not. If it causes
> >> > > > problems, can you just #define GFP_TEMPORARY GFP_KERNEL ? Treewide replace,
> >> > > > and then starting again goes not look attractive to me.
> >> > > 
> >> > > I do not think we want a highlevel GFP_TEMPORARY without any meaning.
> >> > > This just supports spreading the flag usage without a clear semantic
> >> > > and it will lead to even bigger mess. Once we can actually define what
> >> > > the flag means we can also add its users based on that new semantic.
> >> > 
> >> > It has real meaning.
> >> 
> >> Which is?
> >
> > "This allocation is temporary. It lasts milliseconds, not hours."
> 
> It isn't sufficient to give a rule for when GFP_TEMPORARY will be used,
> you also need to explain (at least in general terms) how the information
> will be used.  Also you need to give guidelines on whether the flag
> should be set for allocation that will last seconds or minutes.
> 
> If we have a flag that doesn't have a well defined meaning that actually
> affects behavior, it will not be used consistently, and if we ever
> change exactly how it behaves we can expect things to break.  So it is
> better not to have a flag, than to have a poorly defined flag.

Absolutely agreed!

> My current thoughts is that the important criteria is not how long the
> allocation will be used for, but whether it is reclaimable.  Allocations
> that will only last 5 msecs are reclaimable by calling "usleep(5000)".
> Other allocations might be reclaimable in other ways.  Allocations that
> are not reclaimable may well be directed to a more restricted pool of
> memory, and might be more likely to fail.  If we grew a strong
> "reclaimable" concept, this 'temporary' concept that you want to hold on
> to would become a burden.

... and here again. The whole motivation for the flag was to gather
these objects together and reduce chances of internal fragmentation
due to long lived objects mixed with short term ones. Without an
explicit way to reclaim those objects or having a clear checkpoint to
wait for it is not really helping us to reach desired outcome (less
fragmented memory).

-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1724019

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > > "This allocation is temporary. It lasts milliseconds, not hours."
> > 
> > It isn't sufficient to give a rule for when GFP_TEMPORARY will be used,
> > you also need to explain (at least in general terms) how the information
> > will be used.  Also you need to give guidelines on whether the flag
> > should be set for allocation that will last seconds or minutes.
> > 
> > If we have a flag that doesn't have a well defined meaning that actually
> > affects behavior, it will not be used consistently, and if we ever
> > change exactly how it behaves we can expect things to break.  So it is
> > better not to have a flag, than to have a poorly defined flag.
> 
> Absolutely agreed!
> 
> > My current thoughts is that the important criteria is not how long the
> > allocation will be used for, but whether it is reclaimable.  Allocations
> > that will only last 5 msecs are reclaimable by calling "usleep(5000)".
> > Other allocations might be reclaimable in other ways.  Allocations that
> > are not reclaimable may well be directed to a more restricted pool of
> > memory, and might be more likely to fail.  If we grew a strong
> > "reclaimable" concept, this 'temporary' concept that you want to hold on
> > to would become a burden.
> 
> ... and here again. The whole motivation for the flag was to gather
> these objects together and reduce chances of internal fragmentation
> due to long lived objects mixed with short term ones. Without an
> explicit way to reclaim those objects or having a clear checkpoint to
> wait for it is not really helping us to reach desired outcome (less
> fragmented memory).

Really?

If you group allocations that last << 1 second, and ones that last >>
1 second, I'm pretty sure it reduces fragmentation... "reclaimable" or
not.

Fragmentation is just statistical property, so getting it "mostly
right" helps...
									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [next] | [standalone]

#1724026

On Thu, Aug 31, 2017 at 11:07:22AM +0200, Pavel Machek wrote:
> > > > "This allocation is temporary. It lasts milliseconds, not hours."
> > > 
> > > It isn't sufficient to give a rule for when GFP_TEMPORARY will be used,
> > > you also need to explain (at least in general terms) how the information
> > > will be used.  Also you need to give guidelines on whether the flag
> > > should be set for allocation that will last seconds or minutes.
> > > 
> > > If we have a flag that doesn't have a well defined meaning that actually
> > > affects behavior, it will not be used consistently, and if we ever
> > > change exactly how it behaves we can expect things to break.  So it is
> > > better not to have a flag, than to have a poorly defined flag.
> > 
> > Absolutely agreed!
> > 
> > > My current thoughts is that the important criteria is not how long the
> > > allocation will be used for, but whether it is reclaimable.  Allocations
> > > that will only last 5 msecs are reclaimable by calling "usleep(5000)".
> > > Other allocations might be reclaimable in other ways.  Allocations that
> > > are not reclaimable may well be directed to a more restricted pool of
> > > memory, and might be more likely to fail.  If we grew a strong
> > > "reclaimable" concept, this 'temporary' concept that you want to hold on
> > > to would become a burden.
> > 
> > ... and here again. The whole motivation for the flag was to gather
> > these objects together and reduce chances of internal fragmentation
> > due to long lived objects mixed with short term ones. Without an
> > explicit way to reclaim those objects or having a clear checkpoint to
> > wait for it is not really helping us to reach desired outcome (less
> > fragmented memory).
> 
> Really?
> 
> If you group allocations that last << 1 second, and ones that last >>
> 1 second, I'm pretty sure it reduces fragmentation... "reclaimable" or
> not.
> 

If this was always done reliably then sure, it makes sense. At the time it
was introduced by me, proc was used to relay large amounts of information
to userspace. The patch had a noticable impact but on limited memory,
32-bit and this proc relay was in use. In retrospect, it's possible that
it was the monitoring itself that showed a "benefit" for the patch.

If the flag is used incorrectly even once then the value is diminished
and it can even cause harm (not as severe as misusing __GFP_MOVABLE but
harmful nonetheless). It only has a benefit if there is a large source of
temporary allocations that are long-lived enough to cause fragmentation
during small intervals and to be honest, directly measuring that is
extremely difficult. The benefit is too marginal, the potential for harm
is high and Michal is right to remove it.

-- 
Mel Gorman
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1721573

On Fri 25-08-17 23:39:36, Pavel Machek wrote:
> On Fri 2017-08-25 10:04:42, Michal Hocko wrote:
> > On Fri 25-08-17 09:28:19, Pavel Machek wrote:
> > > On Fri 2017-08-25 08:35:46, Michal Hocko wrote:
> > > > On Wed 23-08-17 19:57:09, Pavel Machek wrote:
> > [...]
> > > > > Dunno. < 1msec probably is temporary, 1 hour probably is not. If it causes
> > > > > problems, can you just #define GFP_TEMPORARY GFP_KERNEL ? Treewide replace,
> > > > > and then starting again goes not look attractive to me.
> > > > 
> > > > I do not think we want a highlevel GFP_TEMPORARY without any meaning.
> > > > This just supports spreading the flag usage without a clear semantic
> > > > and it will lead to even bigger mess. Once we can actually define what
> > > > the flag means we can also add its users based on that new semantic.
> > > 
> > > It has real meaning.
> > 
> > Which is?
> 
> "This allocation is temporary. It lasts milliseconds, not hours."

And why would such a semantic make any sense what so ever? We certainly
do not try to wait for a pinned memory for $TIMEOUT when somebody really
needs a larger memory block and there is a temporary allocation standing
in the way. We simply do not know that an object is a temporary one.

> > > You can define more exact meaning, and then adjust the usage. But
> > > there's no need to do treewide replacement...
> > 
> > I have checked most of them and except for the initially added onces the
> > large portion where added without a good reasons or even break an
> > intuitive meaning by taking locks.
> 
> I don't see it. kmalloc() itself takes locks. Of course everyone takes
> locks. I don't think that's intuitive meaning.

I was talking about users of the flag. I have seen some to take a lock
right after they allocated GFP_TEMPORARY object.

> > Seriously, if we need a short term semantic it should be clearly defined
> > first.
> 
> "milliseconds, not hours."
> 
> > Is there any specific case why you think this patch is in a wrong
> > direction? E.g. a measurable regression?
> 
> Not playing that game. You should argue why it is improvement. And I
> don't believe you did.

Please read the whole changelog where I was quite verbose about how the
current flag is abused and how its semantic is weak and encourages a
wrong usage pattern. Moreover it is not even clear whether it helps
anything. I haven't seen any actual counter argument from you other than
"milliseconds not hours" without actually explaining how that would be
useful for any decisions done in the core MM layer.
-- 
Michal Hocko
SUSE Labs

[toc] | [prev] | [next] | [standalone]

#1724024

[Multipart message — attachments visible in raw view] — view raw

Hi!

> > > > You can define more exact meaning, and then adjust the usage. But
> > > > there's no need to do treewide replacement...
> > > 
> > > I have checked most of them and except for the initially added onces the
> > > large portion where added without a good reasons or even break an
> > > intuitive meaning by taking locks.
> > 
> > I don't see it. kmalloc() itself takes locks. Of course everyone takes
> > locks. I don't think that's intuitive meaning.
> 
> I was talking about users of the flag. I have seen some to take a lock
> right after they allocated GFP_TEMPORARY object.

Yes, I'd expect people to take locks after allocating temporary
objects. kmalloc itself takes locks. If the allocation is "usually"
freed within miliseconds, that should be enough.

> > > Seriously, if we need a short term semantic it should be clearly defined
> > > first.
> > 
> > "milliseconds, not hours."
> > 
> > > Is there any specific case why you think this patch is in a wrong
> > > direction? E.g. a measurable regression?
> > 
> > Not playing that game. You should argue why it is improvement. And I
> > don't believe you did.
> 
> Please read the whole changelog where I was quite verbose about how the
> current flag is abused and how its semantic is weak and encourages a
> wrong usage pattern. Moreover it is not even clear whether it helps
> anything. I haven't seen any actual counter argument from you other than
> "milliseconds not hours" without actually explaining how that would be
> useful for any decisions done in the core MM layer.

Well, I find that argumentation insufficient for global
search&replace.

									Pavel
-- 
(english) http://www.livejournal.com/~pavelmachek
(cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web