All of lore.kernel.org
 help / color / mirror / Atom feed
From: Tejun Heo <tj@kernel.org>
To: Dennis Zhou <dennisz@fb.com>
Cc: Christoph Lameter <cl@linux.com>,
	kernel-team@fb.com, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, Dennis Zhou <dennisszhou@gmail.com>
Subject: Re: [PATCH 08/10] percpu: change the number of pages marked in the first_chunk bitmaps
Date: Mon, 17 Jul 2017 15:26:02 -0400	[thread overview]
Message-ID: <20170717192602.GB585283@devbig577.frc2.facebook.com> (raw)
In-Reply-To: <20170716022315.19892-9-dennisz@fb.com>

Hello,

On Sat, Jul 15, 2017 at 10:23:13PM -0400, Dennis Zhou wrote:
> From: "Dennis Zhou (Facebook)" <dennisszhou@gmail.com>
> 
> This patch changes the allocator to only mark allocated pages for the
> region the population bitmap is used for. Prior, the bitmap was marked
> completely used as the first chunk was allocated and immutable. This is
> misleading because the first chunk may not be completely filled.
> Additionally, with moving the base_addr up in the previous patch, the
> population map no longer corresponds to what is being checked.

This in isolation makes sense although the rationale isn't clear from
the description.  Is it a mere cleanup or is this needed to enable
further changes?

> pcpu_nr_empty_pop_pages is used to ensure there are a handful of free
> pages around to serve atomic allocations. A new field, nr_empty_pop_pages,
> is added to the pcpu_chunk struct to keep track of the number of empty
> pages. This field is needed as the number of empty populated pages is
> globally kept track of and deltas are used to update it. This new field
> is exposed in percpu_stats.

But I can't see why this is being added or why this is in the same
patch with the previous change.

> Now that chunk->nr_pages is the number of pages the chunk is serving, it
> is nice to use this in the work function for population and freeing of
> chunks rather than use the global variable pcpu_unit_pages.

The same goes for the above part.  It's fine to collect misc changes
into a patch when they're trivial and related in some ways but the
content of this patch seems a bit random.

Thanks.

-- 
tejun

--
To unsubscribe, send a message with 'unsubscribe linux-mm' in
the body to majordomo@kvack.org.  For more info on Linux MM,
see: http://www.linux-mm.org/ .
Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>

WARNING: multiple messages have this Message-ID (diff)
From: Tejun Heo <tj@kernel.org>
To: Dennis Zhou <dennisz@fb.com>
Cc: Christoph Lameter <cl@linux.com>,
	kernel-team@fb.com, linux-kernel@vger.kernel.org,
	linux-mm@kvack.org, Dennis Zhou <dennisszhou@gmail.com>
Subject: Re: [PATCH 08/10] percpu: change the number of pages marked in the first_chunk bitmaps
Date: Mon, 17 Jul 2017 15:26:02 -0400	[thread overview]
Message-ID: <20170717192602.GB585283@devbig577.frc2.facebook.com> (raw)
In-Reply-To: <20170716022315.19892-9-dennisz@fb.com>

Hello,

On Sat, Jul 15, 2017 at 10:23:13PM -0400, Dennis Zhou wrote:
> From: "Dennis Zhou (Facebook)" <dennisszhou@gmail.com>
> 
> This patch changes the allocator to only mark allocated pages for the
> region the population bitmap is used for. Prior, the bitmap was marked
> completely used as the first chunk was allocated and immutable. This is
> misleading because the first chunk may not be completely filled.
> Additionally, with moving the base_addr up in the previous patch, the
> population map no longer corresponds to what is being checked.

This in isolation makes sense although the rationale isn't clear from
the description.  Is it a mere cleanup or is this needed to enable
further changes?

> pcpu_nr_empty_pop_pages is used to ensure there are a handful of free
> pages around to serve atomic allocations. A new field, nr_empty_pop_pages,
> is added to the pcpu_chunk struct to keep track of the number of empty
> pages. This field is needed as the number of empty populated pages is
> globally kept track of and deltas are used to update it. This new field
> is exposed in percpu_stats.

But I can't see why this is being added or why this is in the same
patch with the previous change.

> Now that chunk->nr_pages is the number of pages the chunk is serving, it
> is nice to use this in the work function for population and freeing of
> chunks rather than use the global variable pcpu_unit_pages.

The same goes for the above part.  It's fine to collect misc changes
into a patch when they're trivial and related in some ways but the
content of this patch seems a bit random.

Thanks.

-- 
tejun

  reply	other threads:[~2017-07-17 19:26 UTC|newest]

Thread overview: 72+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-07-16  2:23 [PATCH 00/10] percpu: replace percpu area map allocator with bitmap allocator Dennis Zhou
2017-07-16  2:23 ` Dennis Zhou
2017-07-16  2:23 ` [PATCH 01/10] percpu: pcpu-stats change void buffer to int buffer Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-17 14:44   ` Tejun Heo
2017-07-17 14:44     ` Tejun Heo
2017-07-16  2:23 ` [PATCH 02/10] percpu: change the format for percpu_stats output Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-17 14:46   ` Tejun Heo
2017-07-17 14:46     ` Tejun Heo
2017-07-16  2:23 ` [PATCH 03/10] percpu: expose pcpu_nr_empty_pop_pages in pcpu_stats Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-17 14:47   ` Tejun Heo
2017-07-17 14:47     ` Tejun Heo
2017-07-16  2:23 ` [PATCH 04/10] percpu: update the header comment and pcpu_build_alloc_info comments Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-17 14:53   ` Tejun Heo
2017-07-17 14:53     ` Tejun Heo
2017-07-16  2:23 ` [PATCH 05/10] percpu: change reserved_size to end page aligned Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-16  4:01   ` kbuild test robot
2017-07-16  5:11   ` kbuild test robot
2017-07-17 16:46   ` Tejun Heo
2017-07-17 16:46     ` Tejun Heo
2017-07-17 19:10     ` Dennis Zhou
2017-07-17 19:10       ` Dennis Zhou
2017-07-24 20:04     ` Dennis Zhou
2017-07-24 20:04       ` Dennis Zhou
2017-07-16  2:23 ` [PATCH 06/10] percpu: modify base_addr to be region specific Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-17 18:57   ` Tejun Heo
2017-07-17 18:57     ` Tejun Heo
2017-07-18 19:26   ` Josef Bacik
2017-07-18 19:26     ` Josef Bacik
2017-07-18 19:36     ` Matthew Wilcox
2017-07-18 19:36       ` Matthew Wilcox
2017-07-19 14:20       ` Josef Bacik
2017-07-19 14:20         ` Josef Bacik
2017-07-16  2:23 ` [PATCH 07/10] percpu: fix misnomer in schunk/dchunk variable names Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-17 19:10   ` Tejun Heo
2017-07-17 19:10     ` Tejun Heo
2017-07-24 20:07     ` Dennis Zhou
2017-07-24 20:07       ` Dennis Zhou
2017-07-16  2:23 ` [PATCH 08/10] percpu: change the number of pages marked in the first_chunk bitmaps Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-17 19:26   ` Tejun Heo [this message]
2017-07-17 19:26     ` Tejun Heo
2017-07-24 20:13     ` Dennis Zhou
2017-07-24 20:13       ` Dennis Zhou
2017-07-16  2:23 ` [PATCH 09/10] percpu: replace area map allocator with bitmap allocator Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-17 23:27   ` Tejun Heo
2017-07-17 23:27     ` Tejun Heo
2017-07-24 21:37     ` Dennis Zhou
2017-07-24 21:37       ` Dennis Zhou
2017-07-19 19:11   ` Josef Bacik
2017-07-19 19:11     ` Josef Bacik
2017-07-19 22:19     ` Dennis Zhou
2017-07-19 22:19       ` Dennis Zhou
2017-07-19 19:16   ` Josef Bacik
2017-07-19 19:16     ` Josef Bacik
2017-07-19 22:13     ` Dennis Zhou
2017-07-19 22:13       ` Dennis Zhou
2017-07-16  2:23 ` [PATCH 10/10] percpu: add optimizations on allocation path for the " Dennis Zhou
2017-07-16  2:23   ` Dennis Zhou
2017-07-17 23:32   ` Tejun Heo
2017-07-17 23:32     ` Tejun Heo
2017-07-18 19:15 ` [PATCH 00/10] percpu: replace percpu area map allocator with " Josef Bacik
2017-07-18 19:15   ` Josef Bacik
2017-07-24 21:14   ` Dennis Zhou
2017-07-24 21:14     ` Dennis Zhou

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20170717192602.GB585283@devbig577.frc2.facebook.com \
    --to=tj@kernel.org \
    --cc=cl@linux.com \
    --cc=dennisszhou@gmail.com \
    --cc=dennisz@fb.com \
    --cc=kernel-team@fb.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-mm@kvack.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.