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

Groups > linux.kernel > #1209737 > unrolled thread

[PATCHv3 0/5] Fix compound_head() race

Back to article view | Back to linux.kernel

Contents

  [PATCHv3 0/5] Fix compound_head() race "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2015-08-19 11:30 +0200
    [PATCHv3 4/5] mm: make compound_head() robust "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2015-08-19 11:30 +0200
      Re: [PATCHv3 4/5] mm: make compound_head() robust Andrew Morton <akpm@linux-foundation.org> - 2015-08-21 01:40 +0200
        Re: [PATCHv3 4/5] mm: make compound_head() robust "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-08-21 14:20 +0200
          Re: [PATCHv3 4/5] mm: make compound_head() robust Christoph Lameter <cl@linux.com> - 2015-08-21 18:20 +0200
            Re: [PATCHv3 4/5] mm: make compound_head() robust "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-08-21 21:40 +0200
              Re: [PATCHv3 4/5] mm: make compound_head() robust Andrew Morton <akpm@linux-foundation.org> - 2015-08-21 21:40 +0200
                Re: [PATCHv3 4/5] mm: make compound_head() robust Christoph Lameter <cl@linux.com> - 2015-08-21 23:20 +0200
                Re: [PATCHv3 4/5] mm: make compound_head() robust Vlastimil Babka <vbabka@suse.cz> - 2015-08-24 17:50 +0200
          Re: [PATCHv3 4/5] mm: make compound_head() robust Vlastimil Babka <vbabka@suse.cz> - 2015-08-25 13:50 +0200
            Re: [PATCHv3 4/5] mm: make compound_head() robust "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-08-25 20:40 +0200
              Re: [PATCHv3 4/5] mm: make compound_head() robust "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-08-25 22:20 +0200
                Re: [PATCHv3 4/5] mm: make compound_head() robust Vlastimil Babka <vbabka@suse.cz> - 2015-08-25 22:50 +0200
                  Re: [PATCHv3 4/5] mm: make compound_head() robust "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-08-25 23:30 +0200
                    Re: [PATCHv3 4/5] mm: make compound_head() robust "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-08-26 17:10 +0200
                      Re: [PATCHv3 4/5] mm: make compound_head() robust Vlastimil Babka <vbabka@suse.cz> - 2015-08-26 17:40 +0200
                      Re: [PATCHv3 4/5] mm: make compound_head() robust "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-08-26 18:40 +0200
                    Re: [PATCHv3 4/5] mm: make compound_head() robust Hugh Dickins <hughd@google.com> - 2015-08-26 20:20 +0200
                      Re: [PATCHv3 4/5] mm: make compound_head() robust "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-08-26 23:30 +0200
                        Re: [PATCHv3 4/5] mm: make compound_head() robust Hugh Dickins <hughd@google.com> - 2015-08-27 00:30 +0200
                          Re: [PATCHv3 4/5] mm: make compound_head() robust "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-08-27 01:40 +0200
                        Re: [PATCHv3 4/5] mm: make compound_head() robust Michal Hocko <mhocko@kernel.org> - 2015-08-27 17:10 +0200
                          Re: [PATCHv3 4/5] mm: make compound_head() robust Michal Hocko <mhocko@kernel.org> - 2015-08-27 18:10 +0200
                            Re: [PATCHv3 4/5] mm: make compound_head() robust Hugh Dickins <hughd@google.com> - 2015-08-27 19:30 +0200
                              Re: [PATCHv3 4/5] mm: make compound_head() robust Michal Hocko <mhocko@kernel.org> - 2015-08-27 20:10 +0200
                          Re: [PATCHv3 4/5] mm: make compound_head() robust "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-08-27 18:40 +0200
                            Re: [PATCHv3 4/5] mm: make compound_head() robust Michal Hocko <mhocko@kernel.org> - 2015-08-27 20:20 +0200
                              Re: [PATCHv3 4/5] mm: make compound_head() robust "Paul E. McKenney" <paulmck@linux.vnet.ibm.com> - 2015-08-27 21:20 +0200
      Re: [PATCHv3 4/5] mm: make compound_head() robust Jesper Dangaard Brouer <netdev@brouer.com> - 2015-08-24 02:20 +0200
        Re: [PATCHv3 4/5] mm: make compound_head() robust "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-08-24 11:40 +0200
      Re: [PATCHv3 4/5] mm: make compound_head() robust "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-08-24 12:20 +0200
    [PATCHv3 5/5] mm: use 'unsigned int' for page order "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> - 2015-08-19 11:30 +0200
      Re: [PATCHv3 5/5] mm: use 'unsigned int' for page order Michal Hocko <mhocko@kernel.org> - 2015-08-20 10:40 +0200
    Re: [PATCHv3 0/5] Fix compound_head() race "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-08-20 14:40 +0200
      Re: [PATCHv3 0/5] Fix compound_head() race Andrew Morton <akpm@linux-foundation.org> - 2015-08-21 01:40 +0200
        Re: [PATCHv3 0/5] Fix compound_head() race Hugh Dickins <hughd@google.com> - 2015-08-22 22:20 +0200
          Re: [PATCHv3 0/5] Fix compound_head() race "Kirill A. Shutemov" <kirill@shutemov.name> - 2015-08-24 11:40 +0200

Page 2 of 2 — ← Prev page 1 [2]

#1214247 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Wed, Aug 26, 2015 at 03:28:39PM -0700, Hugh Dickins wrote:
> On Wed, 26 Aug 2015, Paul E. McKenney wrote:
> > On Wed, Aug 26, 2015 at 11:18:45AM -0700, Hugh Dickins wrote:
> > > On Tue, 25 Aug 2015, Paul E. McKenney wrote:
> > > > On Tue, Aug 25, 2015 at 10:46:44PM +0200, Vlastimil Babka wrote:
> > > > > On 25.8.2015 22:11, Paul E. McKenney wrote:
> > > > > > On Tue, Aug 25, 2015 at 09:33:54PM +0300, Kirill A. Shutemov wrote:
> > > > > >> On Tue, Aug 25, 2015 at 01:44:13PM +0200, Vlastimil Babka wrote:
> > > > > >>> On 08/21/2015 02:10 PM, Kirill A. Shutemov wrote:
> > > > > >>>> On Thu, Aug 20, 2015 at 04:36:43PM -0700, Andrew Morton wrote:
> > > > > >>>>> On Wed, 19 Aug 2015 12:21:45 +0300 "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> > > > > >>>>>
> > > > > >>>>>> The patch introduces page->compound_head into third double word block in
> > > > > >>>>>> front of compound_dtor and compound_order. That means it shares storage
> > > > > >>>>>> space with:
> > > > > >>>>>>
> > > > > >>>>>>  - page->lru.next;
> > > > > >>>>>>  - page->next;
> > > > > >>>>>>  - page->rcu_head.next;
> > > > > >>>>>>  - page->pmd_huge_pte;
> > > > > >>>>>>
> > > > > >>>
> > > > > >>> We should probably ask Paul about the chances that rcu_head.next would like
> > > > > >>> to use the bit too one day?
> > > > > >>
> > > > > >> +Paul.
> > > > > > 
> > > > > > The call_rcu() function does stomp that bit, but if you stop using that
> > > > > > bit before you invoke call_rcu(), no problem.
> > > > > 
> > > > > You mean that it sets the bit 0 of rcu_head.next during its processing?
> > > > 
> > > > Not at the moment, though RCU will splat if given a misaligned rcu_head
> > > > structure because of the possibility to use that bit to flag callbacks
> > > > that do nothing but free memory.  If RCU needs to do that (e.g., to
> > > > promote energy efficiency), then that bit might well be set during
> > > > RCU grace-period processing.
> > > 
> > > But if you do one day implement that, wouldn't sl?b.c have to use
> > > call_rcu_with_added_meaning() instead of call_rcu(), to be in danger
> > > of getting that bit set?  (No rcu_head is placed in a PageTail page.)
> > 
> > Good point, call_rcu_lazy(), but yes.
> > 
> > > So although it might be a little strange not to use a variant intended
> > > for freeing memory when indeed that's what it's doing, it would not be
> > > the end of the world for SLAB_DESTROY_BY_RCU to carry on using straight
> > > call_rcu(), in defence of the struct page safety Kirill is proposing.
> > 
> > As long as you are OK with the bottom bit being zero throughout the RCU
> > processing, yes.
> 
> That's exactly what we want: sounds like we have no problem, thanks Paul.

Whew!  ;-)

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214632 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Wed 26-08-15 14:29:16, Paul E. McKenney wrote:
> On Wed, Aug 26, 2015 at 11:18:45AM -0700, Hugh Dickins wrote:
[...]
> > But if you do one day implement that, wouldn't sl?b.c have to use
> > call_rcu_with_added_meaning() instead of call_rcu(), to be in danger
> > of getting that bit set?  (No rcu_head is placed in a PageTail page.)
> 
> Good point, call_rcu_lazy(), but yes.
> 
> > So although it might be a little strange not to use a variant intended
> > for freeing memory when indeed that's what it's doing, it would not be
> > the end of the world for SLAB_DESTROY_BY_RCU to carry on using straight
> > call_rcu(), in defence of the struct page safety Kirill is proposing.
> 
> As long as you are OK with the bottom bit being zero throughout the RCU
> processing, yes.

I am really not sure I udnerstand. What will prevent
call_rcu(&page->rcu_head, free_page_rcu) done in a random driver?

Cannot the RCU simply claim bit1? I can see 1146edcbef37 ("rcu: Loosen
__call_rcu()'s rcu_head alignment constraint") but AFAIU all it would
take to fix this would be to require struct rcu_head to be aligned to
32b no?

Btw. Do we need the same think for page::mapping and KSM?
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214671 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Thu 27-08-15 17:09:17, Michal Hocko wrote:
[...]
> Btw. Do we need the same think for page::mapping and KSM?

I guess we are safe here because the address for mappings comes from
kmalloc and that aligned properly, right?
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214733 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Thu, 27 Aug 2015, Michal Hocko wrote:
> On Thu 27-08-15 17:09:17, Michal Hocko wrote:
> [...]
> > Btw. Do we need the same think for page::mapping and KSM?
> 
> I guess we are safe here because the address for mappings comes from
> kmalloc and that aligned properly, right?

Not quite right, in fact.  Because usually the struct address_space
is embedded within the struct inode (at i_data), and the struct inode
embedded within the fs-dependent inode, and that's what's kmalloc'ed.

What makes the mapping pointer low bits safe is include/linux/fs.h:
struct address_space {
	...
} __attribute__((aligned(sizeof(long))));

Which we first had to add in for the cris architecture, which stumbled
not on a genuine allocated address_space, but on that funny statically
declared swapper_space in mm/swap_state.c.

But struct anon_vma and KSM's struct stable_node (which depend on
the same scheme for low bits of page->mapping) have no such alignment
attribute specified: those ones are indeed relying on the kmalloc
guarantee as you suppose.

Does struct rcu_head have no __attribute__((aligned(whatever)))?
Perhaps that attribute should be added when it's needed.

Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214760 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Thu 27-08-15 10:28:48, Hugh Dickins wrote:
> On Thu, 27 Aug 2015, Michal Hocko wrote:
> > On Thu 27-08-15 17:09:17, Michal Hocko wrote:
> > [...]
> > > Btw. Do we need the same think for page::mapping and KSM?
> > 
> > I guess we are safe here because the address for mappings comes from
> > kmalloc and that aligned properly, right?
> 
> Not quite right, in fact.  Because usually the struct address_space
> is embedded within the struct inode (at i_data), and the struct inode
> embedded within the fs-dependent inode, and that's what's kmalloc'ed.
> 
> What makes the mapping pointer low bits safe is include/linux/fs.h:
> struct address_space {
> 	...
> } __attribute__((aligned(sizeof(long))));

Oh, right you are.

> Which we first had to add in for the cris architecture, which stumbled
> not on a genuine allocated address_space, but on that funny statically
> declared swapper_space in mm/swap_state.c.

Thanks for the clarification.

> But struct anon_vma and KSM's struct stable_node (which depend on
> the same scheme for low bits of page->mapping) have no such alignment
> attribute specified: those ones are indeed relying on the kmalloc
> guarantee as you suppose.
> 
> Does struct rcu_head have no __attribute__((aligned(whatever)))?
> Perhaps that attribute should be added when it's needed.

That's basically what I meant in the previous email.
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214690 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Thu, Aug 27, 2015 at 05:09:17PM +0200, Michal Hocko wrote:
> On Wed 26-08-15 14:29:16, Paul E. McKenney wrote:
> > On Wed, Aug 26, 2015 at 11:18:45AM -0700, Hugh Dickins wrote:
> [...]
> > > But if you do one day implement that, wouldn't sl?b.c have to use
> > > call_rcu_with_added_meaning() instead of call_rcu(), to be in danger
> > > of getting that bit set?  (No rcu_head is placed in a PageTail page.)
> > 
> > Good point, call_rcu_lazy(), but yes.
> > 
> > > So although it might be a little strange not to use a variant intended
> > > for freeing memory when indeed that's what it's doing, it would not be
> > > the end of the world for SLAB_DESTROY_BY_RCU to carry on using straight
> > > call_rcu(), in defence of the struct page safety Kirill is proposing.
> > 
> > As long as you are OK with the bottom bit being zero throughout the RCU
> > processing, yes.
> 
> I am really not sure I udnerstand. What will prevent
> call_rcu(&page->rcu_head, free_page_rcu) done in a random driver?

As long as it uses call_rcu(), call_rcu_bh(), call_rcu_sched(),
or call_srcu() and not some future call_rcu_lazy(), no problem.

But yes, if you are going to assume that RCU leaves the bottom
bit of the rcu_head structure's ->next field zero, then everything
everywhere in the kernel might in the future need to be careful of
exactly what variant of call_rcu() is used.

> Cannot the RCU simply claim bit1? I can see 1146edcbef37 ("rcu: Loosen
> __call_rcu()'s rcu_head alignment constraint") but AFAIU all it would
> take to fix this would be to require struct rcu_head to be aligned to
> 32b no?

There are some architectures that guarantee only 16-bit alignment.
If those architectures are fixed to do 32-bit alignment, or if support
for them is dropped, then the future restrictions mentioned above could
be dropped.

							Thanx, Paul

> Btw. Do we need the same think for page::mapping and KSM?
> -- 
> Michal Hocko
> SUSE Labs
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214761 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Thu 27-08-15 09:36:34, Paul E. McKenney wrote:
> On Thu, Aug 27, 2015 at 05:09:17PM +0200, Michal Hocko wrote:
> > On Wed 26-08-15 14:29:16, Paul E. McKenney wrote:
> > > On Wed, Aug 26, 2015 at 11:18:45AM -0700, Hugh Dickins wrote:
> > [...]
> > > > But if you do one day implement that, wouldn't sl?b.c have to use
> > > > call_rcu_with_added_meaning() instead of call_rcu(), to be in danger
> > > > of getting that bit set?  (No rcu_head is placed in a PageTail page.)
> > > 
> > > Good point, call_rcu_lazy(), but yes.
> > > 
> > > > So although it might be a little strange not to use a variant intended
> > > > for freeing memory when indeed that's what it's doing, it would not be
> > > > the end of the world for SLAB_DESTROY_BY_RCU to carry on using straight
> > > > call_rcu(), in defence of the struct page safety Kirill is proposing.
> > > 
> > > As long as you are OK with the bottom bit being zero throughout the RCU
> > > processing, yes.
> > 
> > I am really not sure I udnerstand. What will prevent
> > call_rcu(&page->rcu_head, free_page_rcu) done in a random driver?
> 
> As long as it uses call_rcu(), call_rcu_bh(), call_rcu_sched(),
> or call_srcu() and not some future call_rcu_lazy(), no problem.
> 
> But yes, if you are going to assume that RCU leaves the bottom
> bit of the rcu_head structure's ->next field zero, then everything
> everywhere in the kernel might in the future need to be careful of
> exactly what variant of call_rcu() is used.

OK, so it would be call_rcu_$special to use the bit. This wasn't entirely
clear to me. I thought it would be opposite.

> > Cannot the RCU simply claim bit1? I can see 1146edcbef37 ("rcu: Loosen
> > __call_rcu()'s rcu_head alignment constraint") but AFAIU all it would
> > take to fix this would be to require struct rcu_head to be aligned to
> > 32b no?
> 
> There are some architectures that guarantee only 16-bit alignment.
> If those architectures are fixed to do 32-bit alignment, or if support
> for them is dropped, then the future restrictions mentioned above could
> be dropped.

My understanding of the discussion which led to the above patch is that
m68k allows for 32b alignment you just have to be explicit about that
(http://thread.gmane.org/gmane.linux.ports.m68k/5932/focus=5960). Which
other archs would be affected?

I mean, this patch allows for quite some simplification in the mm code.
And I think that RCU can live with mm of the low bits without any
issues. You've said that one bit should be sufficient for the RCU use
case. So having 2 bits sounds like a good thing.
-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1214795 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Thu, Aug 27, 2015 at 08:14:35PM +0200, Michal Hocko wrote:
> On Thu 27-08-15 09:36:34, Paul E. McKenney wrote:
> > On Thu, Aug 27, 2015 at 05:09:17PM +0200, Michal Hocko wrote:
> > > On Wed 26-08-15 14:29:16, Paul E. McKenney wrote:
> > > > On Wed, Aug 26, 2015 at 11:18:45AM -0700, Hugh Dickins wrote:
> > > [...]
> > > > > But if you do one day implement that, wouldn't sl?b.c have to use
> > > > > call_rcu_with_added_meaning() instead of call_rcu(), to be in danger
> > > > > of getting that bit set?  (No rcu_head is placed in a PageTail page.)
> > > > 
> > > > Good point, call_rcu_lazy(), but yes.
> > > > 
> > > > > So although it might be a little strange not to use a variant intended
> > > > > for freeing memory when indeed that's what it's doing, it would not be
> > > > > the end of the world for SLAB_DESTROY_BY_RCU to carry on using straight
> > > > > call_rcu(), in defence of the struct page safety Kirill is proposing.
> > > > 
> > > > As long as you are OK with the bottom bit being zero throughout the RCU
> > > > processing, yes.
> > > 
> > > I am really not sure I udnerstand. What will prevent
> > > call_rcu(&page->rcu_head, free_page_rcu) done in a random driver?
> > 
> > As long as it uses call_rcu(), call_rcu_bh(), call_rcu_sched(),
> > or call_srcu() and not some future call_rcu_lazy(), no problem.
> > 
> > But yes, if you are going to assume that RCU leaves the bottom
> > bit of the rcu_head structure's ->next field zero, then everything
> > everywhere in the kernel might in the future need to be careful of
> > exactly what variant of call_rcu() is used.
> 
> OK, so it would be call_rcu_$special to use the bit. This wasn't entirely
> clear to me. I thought it would be opposite.

Yes.  And I cannot resist adding that the need to avoid
call_rcu_$special() would be with respect to a given rcu_head structure,
not global.  Though I believe that you already figured that out.  ;-)

> > > Cannot the RCU simply claim bit1? I can see 1146edcbef37 ("rcu: Loosen
> > > __call_rcu()'s rcu_head alignment constraint") but AFAIU all it would
> > > take to fix this would be to require struct rcu_head to be aligned to
> > > 32b no?
> > 
> > There are some architectures that guarantee only 16-bit alignment.
> > If those architectures are fixed to do 32-bit alignment, or if support
> > for them is dropped, then the future restrictions mentioned above could
> > be dropped.
> 
> My understanding of the discussion which led to the above patch is that
> m68k allows for 32b alignment you just have to be explicit about that
> (http://thread.gmane.org/gmane.linux.ports.m68k/5932/focus=5960). Which
> other archs would be affected?
> 
> I mean, this patch allows for quite some simplification in the mm code.
> And I think that RCU can live with mm of the low bits without any
> issues. You've said that one bit should be sufficient for the RCU use
> case. So having 2 bits sounds like a good thing.

As long as MM doesn't use call_rcu_$special() for the rcu_head structure
in question, as long as MM is OK with the bottom bit of ->next always
being zero during a grace period, and as long as MM avoids writing
to ->next during a grace period, we should be good as is, even if a
call_rcu_$special() becomes necessary.

							Thanx, Paul

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1211699 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Wed, 19 Aug 2015 12:21:45 +0300
"Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:

> Hugh has pointed that compound_head() call can be unsafe in some
> context. There's one example:
> 
[...]

> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 0735bc0a351a..a4c4b7d07473 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h

[...]
> -/*
> - * If we access compound page synchronously such as access to
> - * allocated page, there is no need to handle tail flag race, so we can
> - * check tail flag directly without any synchronization primitive.
> - */
> -static inline struct page *compound_head_fast(struct page *page)
> -{
> -	if (unlikely(PageTail(page)))
> -		return page->first_page;
> -	return page;
> -}
> -
[...]

> @@ -548,13 +508,7 @@ static inline struct page *virt_to_head_page(const void *x)
>  {
>  	struct page *page = virt_to_page(x);
>  
> -	/*
> -	 * We don't need to worry about synchronization of tail flag
> -	 * when we call virt_to_head_page() since it is only called for
> -	 * already allocated page and this page won't be freed until
> -	 * this virt_to_head_page() is finished. So use _fast variant.
> -	 */
> -	return compound_head_fast(page);
> +	return compound_head(page);
>  }

I hope this does not slow down the SLAB/slub allocator?
(which calls virt_to_head_page() frequently)

-- 
Best regards,
  Jesper Dangaard Brouer
  MSc.CS, Sr. Network Kernel Developer at Red Hat
  Author of http://www.iptv-analyzer.org
  LinkedIn: http://www.linkedin.com/in/brouer
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1212011 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Mon, Aug 24, 2015 at 01:59:45AM +0200, Jesper Dangaard Brouer wrote:
> On Wed, 19 Aug 2015 12:21:45 +0300
> "Kirill A. Shutemov" <kirill.shutemov@linux.intel.com> wrote:
> 
> > Hugh has pointed that compound_head() call can be unsafe in some
> > context. There's one example:
> > 
> [...]
> 
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 0735bc0a351a..a4c4b7d07473 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> 
> [...]
> > -/*
> > - * If we access compound page synchronously such as access to
> > - * allocated page, there is no need to handle tail flag race, so we can
> > - * check tail flag directly without any synchronization primitive.
> > - */
> > -static inline struct page *compound_head_fast(struct page *page)
> > -{
> > -	if (unlikely(PageTail(page)))
> > -		return page->first_page;
> > -	return page;
> > -}
> > -
> [...]
> 
> > @@ -548,13 +508,7 @@ static inline struct page *virt_to_head_page(const void *x)
> >  {
> >  	struct page *page = virt_to_page(x);
> >  
> > -	/*
> > -	 * We don't need to worry about synchronization of tail flag
> > -	 * when we call virt_to_head_page() since it is only called for
> > -	 * already allocated page and this page won't be freed until
> > -	 * this virt_to_head_page() is finished. So use _fast variant.
> > -	 */
> > -	return compound_head_fast(page);
> > +	return compound_head(page);
> >  }
> 
> I hope this does not slow down the SLAB/slub allocator?
> (which calls virt_to_head_page() frequently)

It should be slightly faster.

Before:

00002e90 <test_virt_to_head_page>:
    2e90:	8b 15 00 00 00 00    	mov    0x0,%edx
    2e96:	05 00 00 00 40       	add    $0x40000000,%eax
    2e9b:	c1 e8 0c             	shr    $0xc,%eax
    2e9e:	c1 e0 05             	shl    $0x5,%eax
    2ea1:	01 d0                	add    %edx,%eax
    2ea3:	8b 10                	mov    (%eax),%edx
    2ea5:	f6 c6 80             	test   $0x80,%dh
    2ea8:	75 06                	jne    2eb0 <test_virt_to_head_page+0x20>
    2eaa:	c3                   	ret    
    2eab:	90                   	nop
    2eac:	8d 74 26 00          	lea    0x0(%esi,%eiz,1),%esi
    2eb0:	8b 40 1c             	mov    0x1c(%eax),%eax
    2eb3:	c3                   	ret    

After:

00003070 <test_virt_to_head_page>:
    3070:	8b 15 00 00 00 00    	mov    0x0,%edx
    3076:	05 00 00 00 40       	add    $0x40000000,%eax
    307b:	c1 e8 0c             	shr    $0xc,%eax
    307e:	c1 e0 05             	shl    $0x5,%eax
    3081:	01 d0                	add    %edx,%eax
    3083:	8b 50 14             	mov    0x14(%eax),%edx
    3086:	8d 4a ff             	lea    -0x1(%edx),%ecx
    3089:	f6 c2 01             	test   $0x1,%dl
    308c:	0f 45 c1             	cmovne %ecx,%eax
    308f:	c3                   	ret    

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1212057 — Re: [PATCHv3 4/5] mm: make compound_head() robust

On Wed, Aug 19, 2015 at 12:21:45PM +0300, Kirill A. Shutemov wrote:
> Hugh has pointed that compound_head() call can be unsafe in some
> context. There's one example:
> 
> 	CPU0					CPU1
> 
> isolate_migratepages_block()
>   page_count()
>     compound_head()
>       !!PageTail() == true
> 					put_page()
> 					  tail->first_page = NULL
>       head = tail->first_page
> 					alloc_pages(__GFP_COMP)
> 					   prep_compound_page()
> 					     tail->first_page = head
> 					     __SetPageTail(p);
>       !!PageTail() == true
>     <head == NULL dereferencing>
> 
> The race is pure theoretical. I don't it's possible to trigger it in
> practice. But who knows.
> 
> We can fix the race by changing how encode PageTail() and compound_head()
> within struct page to be able to update them in one shot.
> 
> The patch introduces page->compound_head into third double word block in
> front of compound_dtor and compound_order. That means it shares storage
> space with:
> 
>  - page->lru.next;
>  - page->next;
>  - page->rcu_head.next;
>  - page->pmd_huge_pte;
> 
> That's too long list to be absolutely sure, but looks like nobody uses
> bit 0 of the word. It can be used to encode PageTail(). And if the bit
> set, rest of the word is pointer to head page.
> 
> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
> Acked-by: Michal Hocko <mhocko@suse.com>
> Cc: Hugh Dickins <hughd@google.com>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Vlastimil Babka <vbabka@suse.cz>

If DEFERRED_STRUCT_PAGE_INIT=n, combining this patchset with my page-flags
patches causes oops in SetPageReserved() called from
reserve_bootmem_region().

It happens because we haven't yet initilized the word in struct page and
PageTail() inside SetPageReserved() can give false-positive, which leads
to bogus compound_head() result.

IIUC, we initialize the word only on first allocation of the page. It can
be too late: pfn scanner can see false-positive PageTail() from not yet
allocated pages too.

Here's fixlet for patch to address the issue.

diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 347724850665..d0e3fca830f8 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -892,6 +892,8 @@ static void init_reserved_page(unsigned long pfn)
 #else
 static inline void init_reserved_page(unsigned long pfn)
 {
+	/* Avoid false-positive PageTail() */
+	INIT_LIST_HEAD(&pfn_to_page(pfn)->lru);
 }
 #endif /* CONFIG_DEFERRED_STRUCT_PAGE_INIT */
 
-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1209740 — [PATCHv3 5/5] mm: use 'unsigned int' for page order

Let's try to be consistent about data type of page order.

Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>
---
 include/linux/mm.h |  5 +++--
 mm/hugetlb.c       | 19 ++++++++++---------
 mm/internal.h      |  4 ++--
 mm/page_alloc.c    | 27 +++++++++++++++------------
 4 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/include/linux/mm.h b/include/linux/mm.h
index a4c4b7d07473..a75bbb3f7142 100644
--- a/include/linux/mm.h
+++ b/include/linux/mm.h
@@ -557,7 +557,7 @@ static inline compound_page_dtor *get_compound_page_dtor(struct page *page)
 	return compound_page_dtors[page[1].compound_dtor];
 }
 
-static inline int compound_order(struct page *page)
+static inline unsigned int compound_order(struct page *page)
 {
 	if (!PageHead(page))
 		return 0;
@@ -1718,7 +1718,8 @@ extern void si_meminfo(struct sysinfo * val);
 extern void si_meminfo_node(struct sysinfo *val, int nid);
 
 extern __printf(3, 4)
-void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...);
+void warn_alloc_failed(gfp_t gfp_mask, unsigned int order,
+		const char *fmt, ...);
 
 extern void setup_per_cpu_pageset(void);
 
diff --git a/mm/hugetlb.c b/mm/hugetlb.c
index 53c0709fd87b..bf64bfebc473 100644
--- a/mm/hugetlb.c
+++ b/mm/hugetlb.c
@@ -817,7 +817,7 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
 
 #if defined(CONFIG_CMA) && defined(CONFIG_X86_64)
 static void destroy_compound_gigantic_page(struct page *page,
-					unsigned long order)
+					unsigned int order)
 {
 	int i;
 	int nr_pages = 1 << order;
@@ -832,7 +832,7 @@ static void destroy_compound_gigantic_page(struct page *page,
 	__ClearPageHead(page);
 }
 
-static void free_gigantic_page(struct page *page, unsigned order)
+static void free_gigantic_page(struct page *page, unsigned int order)
 {
 	free_contig_range(page_to_pfn(page), 1 << order);
 }
@@ -876,7 +876,7 @@ static bool zone_spans_last_pfn(const struct zone *zone,
 	return zone_spans_pfn(zone, last_pfn);
 }
 
-static struct page *alloc_gigantic_page(int nid, unsigned order)
+static struct page *alloc_gigantic_page(int nid, unsigned int order)
 {
 	unsigned long nr_pages = 1 << order;
 	unsigned long ret, pfn, flags;
@@ -912,7 +912,7 @@ static struct page *alloc_gigantic_page(int nid, unsigned order)
 }
 
 static void prep_new_huge_page(struct hstate *h, struct page *page, int nid);
-static void prep_compound_gigantic_page(struct page *page, unsigned long order);
+static void prep_compound_gigantic_page(struct page *page, unsigned int order);
 
 static struct page *alloc_fresh_gigantic_page_node(struct hstate *h, int nid)
 {
@@ -945,9 +945,9 @@ static int alloc_fresh_gigantic_page(struct hstate *h,
 static inline bool gigantic_page_supported(void) { return true; }
 #else
 static inline bool gigantic_page_supported(void) { return false; }
-static inline void free_gigantic_page(struct page *page, unsigned order) { }
+static inline void free_gigantic_page(struct page *page, unsigned int order) { }
 static inline void destroy_compound_gigantic_page(struct page *page,
-						unsigned long order) { }
+						unsigned int order) { }
 static inline int alloc_fresh_gigantic_page(struct hstate *h,
 					nodemask_t *nodes_allowed) { return 0; }
 #endif
@@ -1073,7 +1073,7 @@ static void prep_new_huge_page(struct hstate *h, struct page *page, int nid)
 	put_page(page); /* free it into the hugepage allocator */
 }
 
-static void prep_compound_gigantic_page(struct page *page, unsigned long order)
+static void prep_compound_gigantic_page(struct page *page, unsigned int order)
 {
 	int i;
 	int nr_pages = 1 << order;
@@ -1640,7 +1640,8 @@ found:
 	return 1;
 }
 
-static void __init prep_compound_huge_page(struct page *page, int order)
+static void __init prep_compound_huge_page(struct page *page,
+		unsigned int order)
 {
 	if (unlikely(order > (MAX_ORDER - 1)))
 		prep_compound_gigantic_page(page, order);
@@ -2351,7 +2352,7 @@ static int __init hugetlb_init(void)
 module_init(hugetlb_init);
 
 /* Should be called on processing a hugepagesz=... option */
-void __init hugetlb_add_hstate(unsigned order)
+void __init hugetlb_add_hstate(unsigned int order)
 {
 	struct hstate *h;
 	unsigned long i;
diff --git a/mm/internal.h b/mm/internal.h
index 89e21a07080a..9a9fc497593f 100644
--- a/mm/internal.h
+++ b/mm/internal.h
@@ -157,7 +157,7 @@ __find_buddy_index(unsigned long page_idx, unsigned int order)
 extern int __isolate_free_page(struct page *page, unsigned int order);
 extern void __free_pages_bootmem(struct page *page, unsigned long pfn,
 					unsigned int order);
-extern void prep_compound_page(struct page *page, unsigned long order);
+extern void prep_compound_page(struct page *page, unsigned int order);
 #ifdef CONFIG_MEMORY_FAILURE
 extern bool is_free_buddy_page(struct page *page);
 #endif
@@ -214,7 +214,7 @@ int find_suitable_fallback(struct free_area *area, unsigned int order,
  * page cannot be allocated or merged in parallel. Alternatively, it must
  * handle invalid values gracefully, and use page_order_unsafe() below.
  */
-static inline unsigned long page_order(struct page *page)
+static inline unsigned int page_order(struct page *page)
 {
 	/* PageBuddy() must be checked by the caller */
 	return page_private(page);
diff --git a/mm/page_alloc.c b/mm/page_alloc.c
index 78859d47aaf4..347724850665 100644
--- a/mm/page_alloc.c
+++ b/mm/page_alloc.c
@@ -163,7 +163,7 @@ bool pm_suspended_storage(void)
 #endif /* CONFIG_PM_SLEEP */
 
 #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE
-int pageblock_order __read_mostly;
+unsigned int pageblock_order __read_mostly;
 #endif
 
 static void __free_pages_ok(struct page *page, unsigned int order);
@@ -441,7 +441,7 @@ static void free_compound_page(struct page *page)
 	__free_pages_ok(page, compound_order(page));
 }
 
-void prep_compound_page(struct page *page, unsigned long order)
+void prep_compound_page(struct page *page, unsigned int order)
 {
 	int i;
 	int nr_pages = 1 << order;
@@ -641,7 +641,7 @@ static inline void __free_one_page(struct page *page,
 	unsigned long combined_idx;
 	unsigned long uninitialized_var(buddy_idx);
 	struct page *buddy;
-	int max_order = MAX_ORDER;
+	unsigned int max_order = MAX_ORDER;
 
 	VM_BUG_ON(!zone_is_initialized(zone));
 	VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
@@ -1436,7 +1436,7 @@ int move_freepages(struct zone *zone,
 			  int migratetype)
 {
 	struct page *page;
-	unsigned long order;
+	unsigned int order;
 	int pages_moved = 0;
 
 #ifndef CONFIG_HOLES_IN_ZONE
@@ -1550,7 +1550,7 @@ static bool can_steal_fallback(unsigned int order, int start_mt)
 static void steal_suitable_fallback(struct zone *zone, struct page *page,
 							  int start_type)
 {
-	int current_order = page_order(page);
+	unsigned int current_order = page_order(page);
 	int pages;
 
 	/* Take ownership for orders >= pageblock_order */
@@ -2657,7 +2657,7 @@ static DEFINE_RATELIMIT_STATE(nopage_rs,
 		DEFAULT_RATELIMIT_INTERVAL,
 		DEFAULT_RATELIMIT_BURST);
 
-void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...)
+void warn_alloc_failed(gfp_t gfp_mask, unsigned int order, const char *fmt, ...)
 {
 	unsigned int filter = SHOW_MEM_FILTER_NODES;
 
@@ -2691,7 +2691,7 @@ void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...)
 		va_end(args);
 	}
 
-	pr_warn("%s: page allocation failure: order:%d, mode:0x%x\n",
+	pr_warn("%s: page allocation failure: order:%u, mode:0x%x\n",
 		current->comm, order, gfp_mask);
 
 	dump_stack();
@@ -3450,7 +3450,8 @@ void free_kmem_pages(unsigned long addr, unsigned int order)
 	}
 }
 
-static void *make_alloc_exact(unsigned long addr, unsigned order, size_t size)
+static void *make_alloc_exact(unsigned long addr, unsigned int order,
+		size_t size)
 {
 	if (addr) {
 		unsigned long alloc_end = addr + (PAGE_SIZE << order);
@@ -3502,7 +3503,7 @@ EXPORT_SYMBOL(alloc_pages_exact);
  */
 void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask)
 {
-	unsigned order = get_order(size);
+	unsigned int order = get_order(size);
 	struct page *p = alloc_pages_node(nid, gfp_mask, order);
 	if (!p)
 		return NULL;
@@ -3804,7 +3805,8 @@ void show_free_areas(unsigned int filter)
 	}
 
 	for_each_populated_zone(zone) {
-		unsigned long nr[MAX_ORDER], flags, order, total = 0;
+		unsigned int order;
+		unsigned long nr[MAX_ORDER], flags, total = 0;
 		unsigned char types[MAX_ORDER];
 
 		if (skip_free_areas_node(filter, zone_to_nid(zone)))
@@ -4153,7 +4155,7 @@ static void build_zonelists(pg_data_t *pgdat)
 	nodemask_t used_mask;
 	int local_node, prev_node;
 	struct zonelist *zonelist;
-	int order = current_zonelist_order;
+	unsigned int order = current_zonelist_order;
 
 	/* initialize zonelists */
 	for (i = 0; i < MAX_ZONELISTS; i++) {
@@ -6818,7 +6820,8 @@ int alloc_contig_range(unsigned long start, unsigned long end,
 		       unsigned migratetype)
 {
 	unsigned long outer_start, outer_end;
-	int ret = 0, order;
+	unsigned int order;
+	int ret = 0;
 
 	struct compact_control cc = {
 		.nr_migratepages = 0,
-- 
2.5.0

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1210286 — Re: [PATCHv3 5/5] mm: use 'unsigned int' for page order

On Wed 19-08-15 12:21:46, Kirill A. Shutemov wrote:
> Let's try to be consistent about data type of page order.

Looks good to me.

We still have *_control::order but that is not directly related to this
patch series.

> Signed-off-by: Kirill A. Shutemov <kirill.shutemov@linux.intel.com>

Acked-by: Michal Hocko <mhocko@suse.com>

Thanks!

> ---
>  include/linux/mm.h |  5 +++--
>  mm/hugetlb.c       | 19 ++++++++++---------
>  mm/internal.h      |  4 ++--
>  mm/page_alloc.c    | 27 +++++++++++++++------------
>  4 files changed, 30 insertions(+), 25 deletions(-)
> 
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index a4c4b7d07473..a75bbb3f7142 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -557,7 +557,7 @@ static inline compound_page_dtor *get_compound_page_dtor(struct page *page)
>  	return compound_page_dtors[page[1].compound_dtor];
>  }
>  
> -static inline int compound_order(struct page *page)
> +static inline unsigned int compound_order(struct page *page)
>  {
>  	if (!PageHead(page))
>  		return 0;
> @@ -1718,7 +1718,8 @@ extern void si_meminfo(struct sysinfo * val);
>  extern void si_meminfo_node(struct sysinfo *val, int nid);
>  
>  extern __printf(3, 4)
> -void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...);
> +void warn_alloc_failed(gfp_t gfp_mask, unsigned int order,
> +		const char *fmt, ...);
>  
>  extern void setup_per_cpu_pageset(void);
>  
> diff --git a/mm/hugetlb.c b/mm/hugetlb.c
> index 53c0709fd87b..bf64bfebc473 100644
> --- a/mm/hugetlb.c
> +++ b/mm/hugetlb.c
> @@ -817,7 +817,7 @@ static int hstate_next_node_to_free(struct hstate *h, nodemask_t *nodes_allowed)
>  
>  #if defined(CONFIG_CMA) && defined(CONFIG_X86_64)
>  static void destroy_compound_gigantic_page(struct page *page,
> -					unsigned long order)
> +					unsigned int order)
>  {
>  	int i;
>  	int nr_pages = 1 << order;
> @@ -832,7 +832,7 @@ static void destroy_compound_gigantic_page(struct page *page,
>  	__ClearPageHead(page);
>  }
>  
> -static void free_gigantic_page(struct page *page, unsigned order)
> +static void free_gigantic_page(struct page *page, unsigned int order)
>  {
>  	free_contig_range(page_to_pfn(page), 1 << order);
>  }
> @@ -876,7 +876,7 @@ static bool zone_spans_last_pfn(const struct zone *zone,
>  	return zone_spans_pfn(zone, last_pfn);
>  }
>  
> -static struct page *alloc_gigantic_page(int nid, unsigned order)
> +static struct page *alloc_gigantic_page(int nid, unsigned int order)
>  {
>  	unsigned long nr_pages = 1 << order;
>  	unsigned long ret, pfn, flags;
> @@ -912,7 +912,7 @@ static struct page *alloc_gigantic_page(int nid, unsigned order)
>  }
>  
>  static void prep_new_huge_page(struct hstate *h, struct page *page, int nid);
> -static void prep_compound_gigantic_page(struct page *page, unsigned long order);
> +static void prep_compound_gigantic_page(struct page *page, unsigned int order);
>  
>  static struct page *alloc_fresh_gigantic_page_node(struct hstate *h, int nid)
>  {
> @@ -945,9 +945,9 @@ static int alloc_fresh_gigantic_page(struct hstate *h,
>  static inline bool gigantic_page_supported(void) { return true; }
>  #else
>  static inline bool gigantic_page_supported(void) { return false; }
> -static inline void free_gigantic_page(struct page *page, unsigned order) { }
> +static inline void free_gigantic_page(struct page *page, unsigned int order) { }
>  static inline void destroy_compound_gigantic_page(struct page *page,
> -						unsigned long order) { }
> +						unsigned int order) { }
>  static inline int alloc_fresh_gigantic_page(struct hstate *h,
>  					nodemask_t *nodes_allowed) { return 0; }
>  #endif
> @@ -1073,7 +1073,7 @@ static void prep_new_huge_page(struct hstate *h, struct page *page, int nid)
>  	put_page(page); /* free it into the hugepage allocator */
>  }
>  
> -static void prep_compound_gigantic_page(struct page *page, unsigned long order)
> +static void prep_compound_gigantic_page(struct page *page, unsigned int order)
>  {
>  	int i;
>  	int nr_pages = 1 << order;
> @@ -1640,7 +1640,8 @@ found:
>  	return 1;
>  }
>  
> -static void __init prep_compound_huge_page(struct page *page, int order)
> +static void __init prep_compound_huge_page(struct page *page,
> +		unsigned int order)
>  {
>  	if (unlikely(order > (MAX_ORDER - 1)))
>  		prep_compound_gigantic_page(page, order);
> @@ -2351,7 +2352,7 @@ static int __init hugetlb_init(void)
>  module_init(hugetlb_init);
>  
>  /* Should be called on processing a hugepagesz=... option */
> -void __init hugetlb_add_hstate(unsigned order)
> +void __init hugetlb_add_hstate(unsigned int order)
>  {
>  	struct hstate *h;
>  	unsigned long i;
> diff --git a/mm/internal.h b/mm/internal.h
> index 89e21a07080a..9a9fc497593f 100644
> --- a/mm/internal.h
> +++ b/mm/internal.h
> @@ -157,7 +157,7 @@ __find_buddy_index(unsigned long page_idx, unsigned int order)
>  extern int __isolate_free_page(struct page *page, unsigned int order);
>  extern void __free_pages_bootmem(struct page *page, unsigned long pfn,
>  					unsigned int order);
> -extern void prep_compound_page(struct page *page, unsigned long order);
> +extern void prep_compound_page(struct page *page, unsigned int order);
>  #ifdef CONFIG_MEMORY_FAILURE
>  extern bool is_free_buddy_page(struct page *page);
>  #endif
> @@ -214,7 +214,7 @@ int find_suitable_fallback(struct free_area *area, unsigned int order,
>   * page cannot be allocated or merged in parallel. Alternatively, it must
>   * handle invalid values gracefully, and use page_order_unsafe() below.
>   */
> -static inline unsigned long page_order(struct page *page)
> +static inline unsigned int page_order(struct page *page)
>  {
>  	/* PageBuddy() must be checked by the caller */
>  	return page_private(page);
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 78859d47aaf4..347724850665 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -163,7 +163,7 @@ bool pm_suspended_storage(void)
>  #endif /* CONFIG_PM_SLEEP */
>  
>  #ifdef CONFIG_HUGETLB_PAGE_SIZE_VARIABLE
> -int pageblock_order __read_mostly;
> +unsigned int pageblock_order __read_mostly;
>  #endif
>  
>  static void __free_pages_ok(struct page *page, unsigned int order);
> @@ -441,7 +441,7 @@ static void free_compound_page(struct page *page)
>  	__free_pages_ok(page, compound_order(page));
>  }
>  
> -void prep_compound_page(struct page *page, unsigned long order)
> +void prep_compound_page(struct page *page, unsigned int order)
>  {
>  	int i;
>  	int nr_pages = 1 << order;
> @@ -641,7 +641,7 @@ static inline void __free_one_page(struct page *page,
>  	unsigned long combined_idx;
>  	unsigned long uninitialized_var(buddy_idx);
>  	struct page *buddy;
> -	int max_order = MAX_ORDER;
> +	unsigned int max_order = MAX_ORDER;
>  
>  	VM_BUG_ON(!zone_is_initialized(zone));
>  	VM_BUG_ON_PAGE(page->flags & PAGE_FLAGS_CHECK_AT_PREP, page);
> @@ -1436,7 +1436,7 @@ int move_freepages(struct zone *zone,
>  			  int migratetype)
>  {
>  	struct page *page;
> -	unsigned long order;
> +	unsigned int order;
>  	int pages_moved = 0;
>  
>  #ifndef CONFIG_HOLES_IN_ZONE
> @@ -1550,7 +1550,7 @@ static bool can_steal_fallback(unsigned int order, int start_mt)
>  static void steal_suitable_fallback(struct zone *zone, struct page *page,
>  							  int start_type)
>  {
> -	int current_order = page_order(page);
> +	unsigned int current_order = page_order(page);
>  	int pages;
>  
>  	/* Take ownership for orders >= pageblock_order */
> @@ -2657,7 +2657,7 @@ static DEFINE_RATELIMIT_STATE(nopage_rs,
>  		DEFAULT_RATELIMIT_INTERVAL,
>  		DEFAULT_RATELIMIT_BURST);
>  
> -void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...)
> +void warn_alloc_failed(gfp_t gfp_mask, unsigned int order, const char *fmt, ...)
>  {
>  	unsigned int filter = SHOW_MEM_FILTER_NODES;
>  
> @@ -2691,7 +2691,7 @@ void warn_alloc_failed(gfp_t gfp_mask, int order, const char *fmt, ...)
>  		va_end(args);
>  	}
>  
> -	pr_warn("%s: page allocation failure: order:%d, mode:0x%x\n",
> +	pr_warn("%s: page allocation failure: order:%u, mode:0x%x\n",
>  		current->comm, order, gfp_mask);
>  
>  	dump_stack();
> @@ -3450,7 +3450,8 @@ void free_kmem_pages(unsigned long addr, unsigned int order)
>  	}
>  }
>  
> -static void *make_alloc_exact(unsigned long addr, unsigned order, size_t size)
> +static void *make_alloc_exact(unsigned long addr, unsigned int order,
> +		size_t size)
>  {
>  	if (addr) {
>  		unsigned long alloc_end = addr + (PAGE_SIZE << order);
> @@ -3502,7 +3503,7 @@ EXPORT_SYMBOL(alloc_pages_exact);
>   */
>  void * __meminit alloc_pages_exact_nid(int nid, size_t size, gfp_t gfp_mask)
>  {
> -	unsigned order = get_order(size);
> +	unsigned int order = get_order(size);
>  	struct page *p = alloc_pages_node(nid, gfp_mask, order);
>  	if (!p)
>  		return NULL;
> @@ -3804,7 +3805,8 @@ void show_free_areas(unsigned int filter)
>  	}
>  
>  	for_each_populated_zone(zone) {
> -		unsigned long nr[MAX_ORDER], flags, order, total = 0;
> +		unsigned int order;
> +		unsigned long nr[MAX_ORDER], flags, total = 0;
>  		unsigned char types[MAX_ORDER];
>  
>  		if (skip_free_areas_node(filter, zone_to_nid(zone)))
> @@ -4153,7 +4155,7 @@ static void build_zonelists(pg_data_t *pgdat)
>  	nodemask_t used_mask;
>  	int local_node, prev_node;
>  	struct zonelist *zonelist;
> -	int order = current_zonelist_order;
> +	unsigned int order = current_zonelist_order;
>  
>  	/* initialize zonelists */
>  	for (i = 0; i < MAX_ZONELISTS; i++) {
> @@ -6818,7 +6820,8 @@ int alloc_contig_range(unsigned long start, unsigned long end,
>  		       unsigned migratetype)
>  {
>  	unsigned long outer_start, outer_end;
> -	int ret = 0, order;
> +	unsigned int order;
> +	int ret = 0;
>  
>  	struct compact_control cc = {
>  		.nr_migratepages = 0,
> -- 
> 2.5.0

-- 
Michal Hocko
SUSE Labs
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1210473

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

On Wed, Aug 19, 2015 at 12:21:41PM +0300, Kirill A. Shutemov wrote:
> Here's my attempt on fixing recently discovered race in compound_head().
> It should make compound_head() reliable in all contexts.
> 
> The patchset is against Linus' tree. Let me know if it need to be rebased
> onto different baseline.
> 
> It's expected to have conflicts with my page-flags patchset and probably
> should be applied before it.
> 
> v3:
>    - Fix build without hugetlb;
>    - Drop page->first_page;
>    - Update comment for free_compound_page();
>    - Use 'unsigned int' for page order;
> 
> v2: Per Hugh's suggestion page->compound_head is moved into third double
>     word. This way we can avoid memory overhead which v1 had in some
>     cases.
> 
>     This place in struct page is rather overloaded. More testing is
>     required to make sure we don't collide with anyone.

Andrew, can we have the patchset applied, if nobody has objections?

It applies cleanly into your patchstack just before my page-flags
patchset.

As expected, it causes few conflicts with patches:

 page-flags-introduce-page-flags-policies-wrt-compound-pages.patch
 mm-sanitize-page-mapping-for-tail-pages.patch
 include-linux-page-flagsh-rename-macros-to-avoid-collisions.patch

Updated patches with solved conflicts are attached.

Let me know if I need to do anything else about this.

Hugh, does it address your worry wrt page-flags?

Before you've mentioned races of whether the head page still agrees with
the tail. I don't think it's an issue: you can get this kind of race only
in very special environments like pfn scanner where you anyway need to
re-validate the page after stabilizing it.

Bloat from my page-flags is also reduced substantially. Size of your
page_is_locked() example in allnoconfig case reduced from 32 to 17 bytes.
With the patchset it look this way:

00003070 <page_is_locked>:
    3070:	8b 50 14             	mov    0x14(%eax),%edx
    3073:	f6 c2 01             	test   $0x1,%dl
    3076:	8d 4a ff             	lea    -0x1(%edx),%ecx
    3079:	0f 45 c1             	cmovne %ecx,%eax
    307c:	8b 00                	mov    (%eax),%eax
    307e:	24 01                	and    $0x1,%al
    3080:	c3                   	ret    

-- 
 Kirill A. Shutemov

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

#1210790

On Thu, 20 Aug 2015 15:31:07 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:

> On Wed, Aug 19, 2015 at 12:21:41PM +0300, Kirill A. Shutemov wrote:
> > Here's my attempt on fixing recently discovered race in compound_head().
> > It should make compound_head() reliable in all contexts.
> > 
> > The patchset is against Linus' tree. Let me know if it need to be rebased
> > onto different baseline.
> > 
> > It's expected to have conflicts with my page-flags patchset and probably
> > should be applied before it.
> > 
> > v3:
> >    - Fix build without hugetlb;
> >    - Drop page->first_page;
> >    - Update comment for free_compound_page();
> >    - Use 'unsigned int' for page order;
> > 
> > v2: Per Hugh's suggestion page->compound_head is moved into third double
> >     word. This way we can avoid memory overhead which v1 had in some
> >     cases.
> > 
> >     This place in struct page is rather overloaded. More testing is
> >     required to make sure we don't collide with anyone.
> 
> Andrew, can we have the patchset applied, if nobody has objections?

I've been hoping to hear from Hugh and I wasn't planning on processing
these before the 4.2 release.


--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1211501

On Thu, 20 Aug 2015, Andrew Morton wrote:
> On Thu, 20 Aug 2015 15:31:07 +0300 "Kirill A. Shutemov" <kirill@shutemov.name> wrote:
> 
> > On Wed, Aug 19, 2015 at 12:21:41PM +0300, Kirill A. Shutemov wrote:
> > > Here's my attempt on fixing recently discovered race in compound_head().
> > > It should make compound_head() reliable in all contexts.
> > > 
> > > The patchset is against Linus' tree. Let me know if it need to be rebased
> > > onto different baseline.
> > > 
> > > It's expected to have conflicts with my page-flags patchset and probably
> > > should be applied before it.
> > > 
> > > v3:
> > >    - Fix build without hugetlb;
> > >    - Drop page->first_page;
> > >    - Update comment for free_compound_page();
> > >    - Use 'unsigned int' for page order;
> > > 
> > > v2: Per Hugh's suggestion page->compound_head is moved into third double
> > >     word. This way we can avoid memory overhead which v1 had in some
> > >     cases.
> > > 
> > >     This place in struct page is rather overloaded. More testing is
> > >     required to make sure we don't collide with anyone.
> > 
> > Andrew, can we have the patchset applied, if nobody has objections?
> 
> I've been hoping to hear from Hugh and I wasn't planning on processing
> these before the 4.2 release.

I think this patchset is very good, in a variety of different ways.

Fixes a tricky race, deletes more code than it adds, shrinks kernel text,
deletes tricky functions relying on barriers, frees up a page flag bit,
removes a discrepancy between configs, is really neat in how PageTail
is necessarily false on all lru and lru-candidate pages, probably more.
Good job.

Yes, I did think the compound destructor enum stuff over-engineered,
and would have preferred just direct calls to free_compound_page()
or free_huge_page() myself.  But when I tried to make a patch on
top to do that, even when I left PageHuge out-of-line (which had
certainly not been my intention), it still generated more kernel
text than Kirill's enum version (maybe his "- 1" in compound_head
works better in some places than masking out 3, I didn't study);
so let's forget about that.

I've not actually run and tested with it, but I shall be pleased
when it gets in to mmotm, and will do so then.

As to whether it answers my doubts about his patch-flags patchset
already in mmotm (not your question here, Andrew, but Kirill's in
another of these mails): I'd say that it greatly reduces my doubts,
but does not entirely set me at ease with the bloat.

This set here gives us a compound_head() that is safe to tuck
inside PageFlags ops in that set there: that doesn't worry me
any more.  And the bloat is reduced enough that I don't think
it should be allowed to block Kirill's progress.

But I can't shake off the idea that someone somewhere (0day perf
results? Mel on an __spree?) is going to need to shave away some
of these hidden and rarely needed compound_head() calls one day.

Take __activate_page() in mm/swap.c as an example, something that
begins with a bold PageLRU && !PageActive && !PageUnevictable.
That function contains six sequences of the form
mov 0x20(%rdi),%rax; test $0x1,%al; je over_next; sub $0x1,%rax.

Five of which I expect could be avoided if we just did a
compound_head() conversion on entry.  I suppose any branch
predictor will do a fine job with the last five: am I just too
old-fashioned to be thinking we should (have the ability to)
eliminate them completely?

I'm not saying that we need to convert __activate_page, or anything
else, at this time; but I do think we shall want diet versions of
at least the simple PageFlags tests themselves (we should already
be sparing with the atomic ones), and need to establish convention
now for what the diet versions of PageFlags will be called.

Would __PageFlag be good enough?  Could we say that
__SetPageFlag and __ClearPageFlag omit the compound_head() -
we already have to think carefully when applying those?

Hugh
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

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

#1212004

On Sat, Aug 22, 2015 at 01:13:19PM -0700, Hugh Dickins wrote:
> Yes, I did think the compound destructor enum stuff over-engineered,
> and would have preferred just direct calls to free_compound_page()
> or free_huge_page() myself.  But when I tried to make a patch on
> top to do that, even when I left PageHuge out-of-line (which had
> certainly not been my intention), it still generated more kernel
> text than Kirill's enum version (maybe his "- 1" in compound_head
> works better in some places than masking out 3, I didn't study);
> so let's forget about that.

I had my agenda on ->compound_dtor: my refcounting patchset introduces
one more compound destructor. I wanted to avoid hardcoding them here.

-- 
 Kirill A. Shutemov
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

Back to top | Article view | linux.kernel

csiph-web