From: Andrew Morton <akpm@linux-foundation.org> To: ngupta@vflare.org Cc: Pekka Enberg <penberg@cs.helsinki.fi>, Minchan Kim <minchan.kim@gmail.com>, Greg KH <greg@kroah.com>, Linux Driver Project <devel@driverdev.osuosl.org>, linux-mm <linux-mm@kvack.org>, linux-kernel <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 03/10] Use percpu stats Date: Wed, 11 Aug 2010 10:18:54 -0700 [thread overview] Message-ID: <20100811101854.1dc3a510.akpm@linux-foundation.org> (raw) In-Reply-To: <4C62D241.2040208@vflare.org> On Wed, 11 Aug 2010 22:09:29 +0530 Nitin Gupta <ngupta@vflare.org> wrote: > On 08/10/2010 10:04 AM, Andrew Morton wrote: > > On Mon, 9 Aug 2010 22:56:49 +0530 Nitin Gupta <ngupta@vflare.org> wrote: > > > >> +/* > >> + * Individual percpu values can go negative but the sum across all CPUs > >> + * must always be positive (we store various counts). So, return sum as > >> + * unsigned value. > >> + */ > >> +static u64 zram_get_stat(struct zram *zram, enum zram_stats_index idx) > >> { > >> - u64 val; > >> - > >> - spin_lock(&zram->stat64_lock); > >> - val = *v; > >> - spin_unlock(&zram->stat64_lock); > >> + int cpu; > >> + s64 val = 0; > >> + > >> + for_each_possible_cpu(cpu) { > >> + s64 temp; > >> + unsigned int start; > >> + struct zram_stats_cpu *stats; > >> + > >> + stats = per_cpu_ptr(zram->stats, cpu); > >> + do { > >> + start = u64_stats_fetch_begin(&stats->syncp); > >> + temp = stats->count[idx]; > >> + } while (u64_stats_fetch_retry(&stats->syncp, start)); > >> + val += temp; > >> + } > >> > >> + WARN_ON(val < 0); > >> return val; > >> } > > > > That reimplements include/linux/percpu_counter.h, poorly. > > > > Please see the June discussion "[PATCH v2 1/2] tmpfs: Quick token > > library to allow scalable retrieval of tokens from token jar" for some > > discussion. > > > > > > I read the discussion you pointed out but still fail to see how percpu_counters, > with all their overhead, What overhead? Send numbers. Then extrapolate those numbers to a machine which has 128 possible CPUs and 4 present CPUs. > are better than simple pcpu variable used in current > version. What is the advantage? Firstly, they'd have saved all the time you spent duplicating them. Secondly, getting additional users of the standard facility results in more testing and perhaps enhancement of that facility, thus benefiting other users too. Thirdly, using the standard facility permits your code to leverage enhancements which others add. Fourthly, they would result in a smaller kernel. You didn't really need me to teach you the benefits of code reuse did you? Please do not merge this code unless there is a good reason to do so and it has been shown that the standard facility cannot be suitably fixed or enhanced to address the deficiency.
WARNING: multiple messages have this Message-ID (diff)
From: Andrew Morton <akpm@linux-foundation.org> To: ngupta@vflare.org Cc: Pekka Enberg <penberg@cs.helsinki.fi>, Minchan Kim <minchan.kim@gmail.com>, Greg KH <greg@kroah.com>, Linux Driver Project <devel@driverdev.osuosl.org>, linux-mm <linux-mm@kvack.org>, linux-kernel <linux-kernel@vger.kernel.org> Subject: Re: [PATCH 03/10] Use percpu stats Date: Wed, 11 Aug 2010 10:18:54 -0700 [thread overview] Message-ID: <20100811101854.1dc3a510.akpm@linux-foundation.org> (raw) In-Reply-To: <4C62D241.2040208@vflare.org> On Wed, 11 Aug 2010 22:09:29 +0530 Nitin Gupta <ngupta@vflare.org> wrote: > On 08/10/2010 10:04 AM, Andrew Morton wrote: > > On Mon, 9 Aug 2010 22:56:49 +0530 Nitin Gupta <ngupta@vflare.org> wrote: > > > >> +/* > >> + * Individual percpu values can go negative but the sum across all CPUs > >> + * must always be positive (we store various counts). So, return sum as > >> + * unsigned value. > >> + */ > >> +static u64 zram_get_stat(struct zram *zram, enum zram_stats_index idx) > >> { > >> - u64 val; > >> - > >> - spin_lock(&zram->stat64_lock); > >> - val = *v; > >> - spin_unlock(&zram->stat64_lock); > >> + int cpu; > >> + s64 val = 0; > >> + > >> + for_each_possible_cpu(cpu) { > >> + s64 temp; > >> + unsigned int start; > >> + struct zram_stats_cpu *stats; > >> + > >> + stats = per_cpu_ptr(zram->stats, cpu); > >> + do { > >> + start = u64_stats_fetch_begin(&stats->syncp); > >> + temp = stats->count[idx]; > >> + } while (u64_stats_fetch_retry(&stats->syncp, start)); > >> + val += temp; > >> + } > >> > >> + WARN_ON(val < 0); > >> return val; > >> } > > > > That reimplements include/linux/percpu_counter.h, poorly. > > > > Please see the June discussion "[PATCH v2 1/2] tmpfs: Quick token > > library to allow scalable retrieval of tokens from token jar" for some > > discussion. > > > > > > I read the discussion you pointed out but still fail to see how percpu_counters, > with all their overhead, What overhead? Send numbers. Then extrapolate those numbers to a machine which has 128 possible CPUs and 4 present CPUs. > are better than simple pcpu variable used in current > version. What is the advantage? Firstly, they'd have saved all the time you spent duplicating them. Secondly, getting additional users of the standard facility results in more testing and perhaps enhancement of that facility, thus benefiting other users too. Thirdly, using the standard facility permits your code to leverage enhancements which others add. Fourthly, they would result in a smaller kernel. You didn't really need me to teach you the benefits of code reuse did you? Please do not merge this code unless there is a good reason to do so and it has been shown that the standard facility cannot be suitably fixed or enhanced to address the deficiency. -- 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>
next prev parent reply other threads:[~2010-08-11 17:17 UTC|newest] Thread overview: 137+ messages / expand[flat|nested] mbox.gz Atom feed top 2010-08-09 17:26 [PATCH 00/10] zram: various improvements and cleanups Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 17:26 ` [PATCH 01/10] Replace ioctls with sysfs interface Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 18:34 ` Pekka Enberg 2010-08-09 18:34 ` Pekka Enberg 2010-08-10 3:06 ` Nitin Gupta 2010-08-10 3:06 ` Nitin Gupta 2010-08-31 23:06 ` Dave Hansen 2010-08-31 23:06 ` Dave Hansen 2010-08-09 17:26 ` [PATCH 02/10] Remove need for explicit device initialization Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 18:36 ` Pekka Enberg 2010-08-09 18:36 ` Pekka Enberg 2010-08-10 3:38 ` Nitin Gupta 2010-08-10 3:38 ` Nitin Gupta 2010-08-09 17:26 ` [PATCH 03/10] Use percpu stats Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 18:44 ` Pekka Enberg 2010-08-09 18:44 ` Pekka Enberg 2010-08-10 4:34 ` Andrew Morton 2010-08-10 4:34 ` Andrew Morton 2010-08-11 16:39 ` Nitin Gupta 2010-08-11 16:39 ` Nitin Gupta 2010-08-11 17:18 ` Andrew Morton [this message] 2010-08-11 17:18 ` Andrew Morton 2010-08-30 16:20 ` Christoph Lameter 2010-08-30 16:20 ` Christoph Lameter 2010-08-31 20:31 ` Nitin Gupta 2010-08-31 20:31 ` Nitin Gupta 2010-08-31 21:28 ` Eric Dumazet 2010-08-31 21:28 ` Eric Dumazet 2010-08-31 21:35 ` Christoph Lameter 2010-08-31 21:35 ` Christoph Lameter 2010-08-31 21:41 ` Eric Dumazet 2010-08-31 21:41 ` Eric Dumazet 2010-09-01 20:05 ` Christoph Lameter 2010-09-01 20:05 ` Christoph Lameter 2010-09-01 20:38 ` Eric Dumazet 2010-09-01 20:38 ` Eric Dumazet 2010-09-02 0:34 ` Christoph Lameter 2010-09-02 0:34 ` Christoph Lameter 2010-08-31 5:36 ` Anton Blanchard 2010-08-31 5:36 ` Anton Blanchard 2010-09-01 3:41 ` Anton Blanchard 2010-09-01 3:41 ` Anton Blanchard 2010-09-01 3:51 ` Anton Blanchard 2010-09-01 3:51 ` Anton Blanchard 2010-09-17 20:59 ` Andrew Morton 2010-09-17 20:59 ` Andrew Morton 2010-08-09 17:26 ` [PATCH 04/10] Use percpu buffers Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 18:57 ` Pekka Enberg 2010-08-09 18:57 ` Pekka Enberg 2010-08-10 4:47 ` Nitin Gupta 2010-08-10 4:47 ` Nitin Gupta 2010-08-10 5:05 ` Pekka Enberg 2010-08-10 5:05 ` Pekka Enberg 2010-08-10 5:32 ` Nitin Gupta 2010-08-10 5:32 ` Nitin Gupta 2010-08-10 7:36 ` Pekka Enberg 2010-08-10 7:36 ` Pekka Enberg 2010-08-09 17:26 ` [PATCH 05/10] Reduce per table entry overhead by 4 bytes Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 18:59 ` Pekka Enberg 2010-08-09 18:59 ` Pekka Enberg 2010-08-10 4:55 ` Nitin Gupta 2010-08-10 4:55 ` Nitin Gupta 2010-08-09 17:26 ` [PATCH 06/10] Block discard support Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 19:03 ` Pekka Enberg 2010-08-09 19:03 ` Pekka Enberg 2010-08-10 2:23 ` Jens Axboe 2010-08-10 2:23 ` Jens Axboe 2010-08-10 2:23 ` Jens Axboe 2010-08-10 4:54 ` Nitin Gupta 2010-08-10 4:54 ` Nitin Gupta 2010-08-10 4:54 ` Nitin Gupta 2010-08-10 15:54 ` Jens Axboe 2010-08-10 15:54 ` Jens Axboe 2010-08-10 15:54 ` Jens Axboe 2010-08-09 17:26 ` [PATCH 07/10] Increase compressed page size threshold Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 18:32 ` Pekka Enberg 2010-08-09 18:32 ` Pekka Enberg 2010-08-09 17:26 ` [PATCH 08/10] Some cleanups Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 19:02 ` Pekka Enberg 2010-08-09 19:02 ` Pekka Enberg 2010-08-09 17:26 ` [PATCH 09/10] Update zram documentation Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 17:26 ` [PATCH 10/10] Document sysfs entries Nitin Gupta 2010-08-09 17:26 ` Nitin Gupta 2010-08-09 19:02 ` Pekka Enberg 2010-08-09 19:02 ` Pekka Enberg 2010-08-31 22:37 ` [PATCH 00/10] zram: various improvements and cleanups Greg KH 2010-08-31 22:37 ` Greg KH 2010-09-01 3:32 ` Anton Blanchard 2010-09-01 3:32 ` Anton Blanchard 2010-09-09 17:24 ` OOM panics with zram Dave Hansen 2010-09-09 17:24 ` Dave Hansen 2010-09-09 19:07 ` [patch -rc] oom: always return a badness score of non-zero for eligible tasks David Rientjes 2010-09-09 19:07 ` David Rientjes 2010-09-09 19:48 ` Dave Hansen 2010-09-09 19:48 ` Dave Hansen 2010-09-09 21:00 ` David Rientjes 2010-09-09 21:00 ` David Rientjes 2010-09-09 21:10 ` Dave Hansen 2010-09-09 21:10 ` Dave Hansen 2010-09-09 21:40 ` David Rientjes 2010-09-09 21:40 ` David Rientjes 2010-10-03 18:41 ` OOM panics with zram Nitin Gupta 2010-10-03 18:41 ` Nitin Gupta 2010-10-03 19:27 ` Dave Hansen 2010-10-03 19:27 ` Dave Hansen 2010-10-03 19:40 ` Nitin Gupta 2010-10-03 19:40 ` Nitin Gupta 2010-10-04 11:08 ` Ed Tomlinson 2010-10-04 11:08 ` Ed Tomlinson 2010-10-05 23:43 ` Greg KH 2010-10-05 23:43 ` Greg KH 2010-10-06 2:29 ` Nitin Gupta 2010-10-06 2:29 ` Nitin Gupta 2010-10-06 2:36 ` Greg KH 2010-10-06 2:36 ` Greg KH 2010-10-06 4:30 ` Nitin Gupta 2010-10-06 4:30 ` Nitin Gupta 2010-10-06 7:38 ` Pekka Enberg 2010-10-06 7:38 ` Pekka Enberg 2010-10-06 14:03 ` Greg KH 2010-10-06 14:03 ` Greg KH 2010-10-06 14:16 ` Pekka Enberg 2010-10-06 14:16 ` Pekka Enberg 2010-10-06 14:53 ` Nitin Gupta 2010-10-06 14:53 ` Nitin Gupta 2010-10-06 14:02 ` Greg KH 2010-10-06 14:02 ` Greg KH
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=20100811101854.1dc3a510.akpm@linux-foundation.org \ --to=akpm@linux-foundation.org \ --cc=devel@driverdev.osuosl.org \ --cc=greg@kroah.com \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=minchan.kim@gmail.com \ --cc=ngupta@vflare.org \ --cc=penberg@cs.helsinki.fi \ /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: linkBe 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.