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

Groups > linux.kernel > #1600706 > unrolled thread

Re: [PATCH v2] xfs: remove kmem_zalloc_greedy

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: [PATCH v2] xfs: remove kmem_zalloc_greedy "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-03-14 18:00 +0100
    Re: [PATCH v2] xfs: remove kmem_zalloc_greedy "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-03-14 19:10 +0100
      Re: [PATCH v2] xfs: remove kmem_zalloc_greedy "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-03-15 01:20 +0100
        Re: [PATCH v2] xfs: remove kmem_zalloc_greedy Michal Hocko <mhocko@kernel.org> - 2017-03-15 09:40 +0100
          Re: [PATCH v2] xfs: remove kmem_zalloc_greedy "Luis R. Rodriguez" <mcgrof@kernel.org> - 2017-03-15 16:50 +0100
            Re: [PATCH v2] xfs: remove kmem_zalloc_greedy "Darrick J. Wong" <darrick.wong@oracle.com> - 2017-03-15 17:50 +0100

#1600706 — Re: [PATCH v2] xfs: remove kmem_zalloc_greedy

On Tue, Mar 07, 2017 at 04:35:28PM -0800, Darrick J. Wong wrote:
> The sole remaining caller of kmem_zalloc_greedy is bulkstat, which uses
> it to grab 1-4 pages for staging of inobt records.  The infinite loop in
> the greedy allocation function is causing hangs[1] in generic/269, so
> just get rid of the greedy allocator in favor of kmem_zalloc_large.
> This makes bulkstat somewhat more likely to ENOMEM if there's really no
> pages to spare, but eliminates a source of hangs.
> 
> [1] http://lkml.kernel.org/r/20170301044634.rgidgdqqiiwsmfpj%40XZHOUW.usersys.redhat.com
> 
> Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> ---
> v2: remove single-page fallback
> ---

Since this fixes a hang how about *at the very least* a respective Fixes tag ?
This fixes an existing hang so what are the stable considerations here ? I
realize the answer is not easy but figured its worth asking.

FWIW I trace kmem_zalloc_greedy()'s introduction back to 2006 77e4635ae1917
("[XFS] Add a greedy allocation interface, allocating within a min/max size
range.") through v2.6.19 days...

  Luis

[toc] | [next] | [standalone]

#1600784

On Tue, Mar 14, 2017 at 05:57:45PM +0100, Luis R. Rodriguez wrote:
> On Tue, Mar 07, 2017 at 04:35:28PM -0800, Darrick J. Wong wrote:
> > The sole remaining caller of kmem_zalloc_greedy is bulkstat, which uses
> > it to grab 1-4 pages for staging of inobt records.  The infinite loop in
> > the greedy allocation function is causing hangs[1] in generic/269, so
> > just get rid of the greedy allocator in favor of kmem_zalloc_large.
> > This makes bulkstat somewhat more likely to ENOMEM if there's really no
> > pages to spare, but eliminates a source of hangs.
> > 
> > [1] http://lkml.kernel.org/r/20170301044634.rgidgdqqiiwsmfpj%40XZHOUW.usersys.redhat.com
> > 
> > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > ---
> > v2: remove single-page fallback
> > ---
> 
> Since this fixes a hang how about *at the very least* a respective Fixes tag ?
> This fixes an existing hang so what are the stable considerations here ? I
> realize the answer is not easy but figured its worth asking.

I didn't think it was appropriate to "Fixes: 77e4635ae1917" since we're
not fixing _greedy so much as we are killing it.  The patch fixes an
infinite retry hang when bulkstat tries a memory allocation that cannot
be satisfied; and having done that, realizes there are no remaining
callers of _greedy and garbage collects it.  The code that was there
before also seems capable of sleeping forever, I think.

So the minimally invasive fix is to apply the allocation conversion in
bulkstat, and if there aren't any other callers of _greedy then you can
get rid of it too.

> FWIW I trace kmem_zalloc_greedy()'s introduction back to 2006 77e4635ae1917
> ("[XFS] Add a greedy allocation interface, allocating within a min/max size
> range.") through v2.6.19 days...

--D

> 
>   Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

#1600958

On Tue, Mar 14, 2017 at 11:07:38AM -0700, Darrick J. Wong wrote:
> On Tue, Mar 14, 2017 at 05:57:45PM +0100, Luis R. Rodriguez wrote:
> > On Tue, Mar 07, 2017 at 04:35:28PM -0800, Darrick J. Wong wrote:
> > > The sole remaining caller of kmem_zalloc_greedy is bulkstat, which uses
> > > it to grab 1-4 pages for staging of inobt records.  The infinite loop in
> > > the greedy allocation function is causing hangs[1] in generic/269, so
> > > just get rid of the greedy allocator in favor of kmem_zalloc_large.
> > > This makes bulkstat somewhat more likely to ENOMEM if there's really no
> > > pages to spare, but eliminates a source of hangs.
> > > 
> > > [1] http://lkml.kernel.org/r/20170301044634.rgidgdqqiiwsmfpj%40XZHOUW.usersys.redhat.com
> > > 
> > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > ---
> > > v2: remove single-page fallback
> > > ---
> > 
> > Since this fixes a hang how about *at the very least* a respective Fixes tag ?
> > This fixes an existing hang so what are the stable considerations here ? I
> > realize the answer is not easy but figured its worth asking.
> 
> I didn't think it was appropriate to "Fixes: 77e4635ae1917" since we're
> not fixing _greedy so much as we are killing it.  The patch fixes an
> infinite retry hang when bulkstat tries a memory allocation that cannot
> be satisfied; and having done that, realizes there are no remaining
> callers of _greedy and garbage collects it.  The code that was there
> before also seems capable of sleeping forever, I think.
> 
> So the minimally invasive fix is to apply the allocation conversion in
> bulkstat, and if there aren't any other callers of _greedy then you can
> get rid of it too.

For the stake of stable XFS users then why not do the less invasive change
first, Cc stable, and then move on to the less backward portable solution ?

  Luis

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

#1601137

On Wed 15-03-17 01:14:27, Luis R. Rodriguez wrote:
> On Tue, Mar 14, 2017 at 11:07:38AM -0700, Darrick J. Wong wrote:
> > On Tue, Mar 14, 2017 at 05:57:45PM +0100, Luis R. Rodriguez wrote:
> > > On Tue, Mar 07, 2017 at 04:35:28PM -0800, Darrick J. Wong wrote:
> > > > The sole remaining caller of kmem_zalloc_greedy is bulkstat, which uses
> > > > it to grab 1-4 pages for staging of inobt records.  The infinite loop in
> > > > the greedy allocation function is causing hangs[1] in generic/269, so
> > > > just get rid of the greedy allocator in favor of kmem_zalloc_large.
> > > > This makes bulkstat somewhat more likely to ENOMEM if there's really no
> > > > pages to spare, but eliminates a source of hangs.
> > > > 
> > > > [1] http://lkml.kernel.org/r/20170301044634.rgidgdqqiiwsmfpj%40XZHOUW.usersys.redhat.com
> > > > 
> > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > ---
> > > > v2: remove single-page fallback
> > > > ---
> > > 
> > > Since this fixes a hang how about *at the very least* a respective Fixes tag ?
> > > This fixes an existing hang so what are the stable considerations here ? I
> > > realize the answer is not easy but figured its worth asking.
> > 
> > I didn't think it was appropriate to "Fixes: 77e4635ae1917" since we're
> > not fixing _greedy so much as we are killing it.  The patch fixes an
> > infinite retry hang when bulkstat tries a memory allocation that cannot
> > be satisfied; and having done that, realizes there are no remaining
> > callers of _greedy and garbage collects it.  The code that was there
> > before also seems capable of sleeping forever, I think.
> > 
> > So the minimally invasive fix is to apply the allocation conversion in
> > bulkstat, and if there aren't any other callers of _greedy then you can
> > get rid of it too.
> 
> For the stake of stable XFS users then why not do the less invasive change
> first, Cc stable, and then move on to the less backward portable solution ?

The thing is that the permanent failures for vmalloc were so unlikely
prior to 5d17a73a2ebe ("vmalloc: back off when the current task is
killed") that this was basically a non-issue before this (4.11) merge
window.
-- 
Michal Hocko
SUSE Labs

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

#1601499

On Wed, Mar 15, 2017 at 09:35:29AM +0100, Michal Hocko wrote:
> On Wed 15-03-17 01:14:27, Luis R. Rodriguez wrote:
> > On Tue, Mar 14, 2017 at 11:07:38AM -0700, Darrick J. Wong wrote:
> > > On Tue, Mar 14, 2017 at 05:57:45PM +0100, Luis R. Rodriguez wrote:
> > > > On Tue, Mar 07, 2017 at 04:35:28PM -0800, Darrick J. Wong wrote:
> > > > > The sole remaining caller of kmem_zalloc_greedy is bulkstat, which uses
> > > > > it to grab 1-4 pages for staging of inobt records.  The infinite loop in
> > > > > the greedy allocation function is causing hangs[1] in generic/269, so
> > > > > just get rid of the greedy allocator in favor of kmem_zalloc_large.
> > > > > This makes bulkstat somewhat more likely to ENOMEM if there's really no
> > > > > pages to spare, but eliminates a source of hangs.
> > > > > 
> > > > > [1] http://lkml.kernel.org/r/20170301044634.rgidgdqqiiwsmfpj%40XZHOUW.usersys.redhat.com
> > > > > 
> > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > ---
> > > > > v2: remove single-page fallback
> > > > > ---
> > > > 
> > > > Since this fixes a hang how about *at the very least* a respective Fixes tag ?
> > > > This fixes an existing hang so what are the stable considerations here ? I
> > > > realize the answer is not easy but figured its worth asking.
> > > 
> > > I didn't think it was appropriate to "Fixes: 77e4635ae1917" since we're
> > > not fixing _greedy so much as we are killing it.  The patch fixes an
> > > infinite retry hang when bulkstat tries a memory allocation that cannot
> > > be satisfied; and having done that, realizes there are no remaining
> > > callers of _greedy and garbage collects it.  The code that was there
> > > before also seems capable of sleeping forever, I think.
> > > 
> > > So the minimally invasive fix is to apply the allocation conversion in
> > > bulkstat, and if there aren't any other callers of _greedy then you can
> > > get rid of it too.
> > 
> > For the stake of stable XFS users then why not do the less invasive change
> > first, Cc stable, and then move on to the less backward portable solution ?
> 
> The thing is that the permanent failures for vmalloc were so unlikely
> prior to 5d17a73a2ebe ("vmalloc: back off when the current task is
> killed") that this was basically a non-issue before this (4.11) merge
> window.

I see, this seems like critical information to add to the commit log.
Also, will this be at least pushed to v4.11 ?

  Luis

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

#1601547

On Wed, Mar 15, 2017 at 04:43:27PM +0100, Luis R. Rodriguez wrote:
> On Wed, Mar 15, 2017 at 09:35:29AM +0100, Michal Hocko wrote:
> > On Wed 15-03-17 01:14:27, Luis R. Rodriguez wrote:
> > > On Tue, Mar 14, 2017 at 11:07:38AM -0700, Darrick J. Wong wrote:
> > > > On Tue, Mar 14, 2017 at 05:57:45PM +0100, Luis R. Rodriguez wrote:
> > > > > On Tue, Mar 07, 2017 at 04:35:28PM -0800, Darrick J. Wong wrote:
> > > > > > The sole remaining caller of kmem_zalloc_greedy is bulkstat, which uses
> > > > > > it to grab 1-4 pages for staging of inobt records.  The infinite loop in
> > > > > > the greedy allocation function is causing hangs[1] in generic/269, so
> > > > > > just get rid of the greedy allocator in favor of kmem_zalloc_large.
> > > > > > This makes bulkstat somewhat more likely to ENOMEM if there's really no
> > > > > > pages to spare, but eliminates a source of hangs.
> > > > > > 
> > > > > > [1] http://lkml.kernel.org/r/20170301044634.rgidgdqqiiwsmfpj%40XZHOUW.usersys.redhat.com
> > > > > > 
> > > > > > Signed-off-by: Darrick J. Wong <darrick.wong@oracle.com>
> > > > > > ---
> > > > > > v2: remove single-page fallback
> > > > > > ---
> > > > > 
> > > > > Since this fixes a hang how about *at the very least* a respective Fixes tag ?
> > > > > This fixes an existing hang so what are the stable considerations here ? I
> > > > > realize the answer is not easy but figured its worth asking.
> > > > 
> > > > I didn't think it was appropriate to "Fixes: 77e4635ae1917" since we're
> > > > not fixing _greedy so much as we are killing it.  The patch fixes an
> > > > infinite retry hang when bulkstat tries a memory allocation that cannot
> > > > be satisfied; and having done that, realizes there are no remaining
> > > > callers of _greedy and garbage collects it.  The code that was there
> > > > before also seems capable of sleeping forever, I think.
> > > > 
> > > > So the minimally invasive fix is to apply the allocation conversion in
> > > > bulkstat, and if there aren't any other callers of _greedy then you can
> > > > get rid of it too.
> > > 
> > > For the stake of stable XFS users then why not do the less invasive change
> > > first, Cc stable, and then move on to the less backward portable solution ?
> > 
> > The thing is that the permanent failures for vmalloc were so unlikely
> > prior to 5d17a73a2ebe ("vmalloc: back off when the current task is
> > killed") that this was basically a non-issue before this (4.11) merge
> > window.
> 
> I see, this seems like critical information to add to the commit log.
> Also, will this be at least pushed to v4.11 ?

It's already in rc2.

--D

> 
>   Luis
> --
> To unsubscribe from this list: send the line "unsubscribe linux-xfs" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web