All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kent Overstreet <kmo@daterainc.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: Jens Axboe <axboe@kernel.dk>, Andi Kleen <andi@firstfloor.org>,
	kvm-devel <kvm@vger.kernel.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	lkml <linux-kernel@vger.kernel.org>,
	lf-virt <virtualization@lists.linux-foundation.org>,
	target-devel <target-devel@vger.kernel.org>,
	Christoph Lameter <cl@gentwo.org>,
	Oleg Nesterov <oleg@redhat.com>, Tejun Heo <tj@kernel.org>,
	Christoph Lameter <cl@linux-foundation.org>,
	Ingo Molnar <mingo@redhat.com>
Subject: Re: [PATCH] percpu ida: Switch to cpumask_t, add some comments
Date: Wed, 28 Aug 2013 14:00:10 -0700	[thread overview]
Message-ID: <20130828210010.GD1357@kmo-pixel> (raw)
In-Reply-To: <20130828132550.d0ce4d3a4d40ee07e7e8a1c1@linux-foundation.org>

On Wed, Aug 28, 2013 at 01:25:50PM -0700, Andrew Morton wrote:
> On Wed, 28 Aug 2013 12:55:17 -0700 Kent Overstreet <kmo@daterainc.com> wrote:
> 
> > Fixup patch, addressing Andrew's review feedback:
> 
> Looks reasonable.
> 
> >  lib/idr.c           | 38 +++++++++++++++++++++-----------------
> 
> I still don't think it should be in this file.
> 
> You say that some as-yet-unmerged patches will tie the new code into
> the old ida code.  But will it do it in a manner which requires that
> the two reside in the same file?

Not require, no - but it's just intimate enough with my ida rewrite that
I think it makes sense; it makes some use of stuff that should be
internal to the ida code.

Mostly just sharing the lock though, since I got rid of the ida
interfaces that don't do locking, but percpu ida needs a lock that also
covers what ida needs.

It also makes use of a ganged allocation interface, but there's no real
reason ida can't expose that, it's just unlikely to be useful to
anything but percpu ida.

The other reason I think it makes sense to live in idr.c is more for
users of the code; as you pointed out as far as the user's perspective
percpu ida isn't doing anything fundamentally different from ida, so I
think it makes sense for the code to live in the same place as a
kindness to future kernel developers who are trying to find their way
around the various library code.

WARNING: multiple messages have this Message-ID (diff)
From: Kent Overstreet <kmo@daterainc.com>
To: Andrew Morton <akpm@linux-foundation.org>
Cc: "Nicholas A. Bellinger" <nab@linux-iscsi.org>,
	target-devel <target-devel@vger.kernel.org>,
	lf-virt <virtualization@lists.linux-foundation.org>,
	lkml <linux-kernel@vger.kernel.org>,
	kvm-devel <kvm@vger.kernel.org>,
	"Michael S. Tsirkin" <mst@redhat.com>,
	Asias He <asias@redhat.com>, Jens Axboe <axboe@kernel.dk>,
	Tejun Heo <tj@kernel.org>, Ingo Molnar <mingo@redhat.com>,
	Andi Kleen <andi@firstfloor.org>,
	Christoph Lameter <cl@gentwo.org>,
	Oleg Nesterov <oleg@redhat.com>,
	Christoph Lameter <cl@linux-foundation.org>
Subject: Re: [PATCH] percpu ida: Switch to cpumask_t, add some comments
Date: Wed, 28 Aug 2013 14:00:10 -0700	[thread overview]
Message-ID: <20130828210010.GD1357@kmo-pixel> (raw)
In-Reply-To: <20130828132550.d0ce4d3a4d40ee07e7e8a1c1@linux-foundation.org>

On Wed, Aug 28, 2013 at 01:25:50PM -0700, Andrew Morton wrote:
> On Wed, 28 Aug 2013 12:55:17 -0700 Kent Overstreet <kmo@daterainc.com> wrote:
> 
> > Fixup patch, addressing Andrew's review feedback:
> 
> Looks reasonable.
> 
> >  lib/idr.c           | 38 +++++++++++++++++++++-----------------
> 
> I still don't think it should be in this file.
> 
> You say that some as-yet-unmerged patches will tie the new code into
> the old ida code.  But will it do it in a manner which requires that
> the two reside in the same file?

Not require, no - but it's just intimate enough with my ida rewrite that
I think it makes sense; it makes some use of stuff that should be
internal to the ida code.

Mostly just sharing the lock though, since I got rid of the ida
interfaces that don't do locking, but percpu ida needs a lock that also
covers what ida needs.

It also makes use of a ganged allocation interface, but there's no real
reason ida can't expose that, it's just unlikely to be useful to
anything but percpu ida.

The other reason I think it makes sense to live in idr.c is more for
users of the code; as you pointed out as far as the user's perspective
percpu ida isn't doing anything fundamentally different from ida, so I
think it makes sense for the code to live in the same place as a
kindness to future kernel developers who are trying to find their way
around the various library code.

  reply	other threads:[~2013-08-28 21:00 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2013-08-16 23:09 [PATCH-v3 0/4] target/vhost-scsi: Add per-cpu ida tag pre-allocation for v3.12 Nicholas A. Bellinger
2013-08-16 23:09 ` Nicholas A. Bellinger
2013-08-16 23:09 ` [PATCH-v3 1/4] idr: Percpu ida Nicholas A. Bellinger
2013-08-16 23:09   ` Nicholas A. Bellinger
2013-08-20 21:31   ` Andrew Morton
2013-08-26 20:14     ` Kent Overstreet
2013-08-26 20:14     ` Kent Overstreet
2013-08-28 19:53     ` Kent Overstreet
2013-08-28 19:53       ` Kent Overstreet
2013-08-28 20:23       ` Andrew Morton
2013-08-28 20:23       ` Andrew Morton
2013-08-28 20:44         ` Kent Overstreet
2013-08-28 20:44         ` Kent Overstreet
2013-08-28 20:50           ` Andrew Morton
2013-08-28 20:50             ` Andrew Morton
2013-08-28 21:12             ` Kent Overstreet
2013-08-28 21:12               ` Kent Overstreet
2013-08-28 21:15               ` Andrew Morton
2013-08-28 21:15                 ` Andrew Morton
2013-08-28 19:55     ` [PATCH] percpu ida: Switch to cpumask_t, add some comments Kent Overstreet
2013-08-28 19:55       ` Kent Overstreet
2013-08-28 20:25       ` Andrew Morton
2013-08-28 20:25         ` Andrew Morton
2013-08-28 21:00         ` Kent Overstreet [this message]
2013-08-28 21:00           ` Kent Overstreet
2013-08-28 21:10           ` Andrew Morton
2013-08-28 21:10           ` Andrew Morton
2013-08-28 21:23             ` Kent Overstreet
2013-08-28 21:26             ` Kent Overstreet
2013-08-28 21:26             ` Kent Overstreet
2013-08-28 21:36               ` Andrew Morton
2013-08-28 21:36                 ` Andrew Morton
2013-08-31  3:10                 ` Nicholas A. Bellinger
2013-08-31  3:10                   ` Nicholas A. Bellinger
2013-08-20 21:31   ` [PATCH-v3 1/4] idr: Percpu ida Andrew Morton
2013-08-21 18:25   ` Christoph Lameter
2013-08-21 18:25     ` Christoph Lameter
2013-08-26 20:23     ` Kent Overstreet
2013-08-26 20:23     ` Kent Overstreet
2013-08-16 23:09 ` [PATCH-v3 2/4] target: Add transport_init_session_tags using per-cpu ida Nicholas A. Bellinger
2013-08-16 23:09   ` Nicholas A. Bellinger
2013-08-16 23:09 ` [PATCH-v3 3/4] vhost/scsi: Convert to per-cpu ida_alloc + ida_free command map Nicholas A. Bellinger
2013-08-16 23:09   ` Nicholas A. Bellinger
2013-08-16 23:09 ` [PATCH-v3 4/4] vhost/scsi: Add pre-allocation for tv_cmd SGL + upages memory Nicholas A. Bellinger
2013-08-16 23:09   ` Nicholas A. Bellinger

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=20130828210010.GD1357@kmo-pixel \
    --to=kmo@daterainc.com \
    --cc=akpm@linux-foundation.org \
    --cc=andi@firstfloor.org \
    --cc=axboe@kernel.dk \
    --cc=cl@gentwo.org \
    --cc=cl@linux-foundation.org \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=mingo@redhat.com \
    --cc=mst@redhat.com \
    --cc=oleg@redhat.com \
    --cc=target-devel@vger.kernel.org \
    --cc=tj@kernel.org \
    --cc=virtualization@lists.linux-foundation.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.