| Message ID | 20130828195517.GF8032@kmo-pixel (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 28 Aug 2013 14:00:10 -0700 Kent Overstreet <kmo@daterainc.com> wrote: > 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. I found things to be quite the opposite - it took 5 minutes of staring, head-scratching, double-checking and penny-dropping before I was confident that the newly-added code actually has nothing at all to do with the current code. Putting it in the same file was misleading, and I got misled. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, Aug 28, 2013 at 02:10:19PM -0700, Andrew Morton wrote: > On Wed, 28 Aug 2013 14:00:10 -0700 Kent Overstreet <kmo@daterainc.com> wrote: > > > 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. > > I found things to be quite the opposite - it took 5 minutes of staring, > head-scratching, double-checking and penny-dropping before I was > confident that the newly-added code actually has nothing at all to do > with the current code. Putting it in the same file was misleading, and > I got misled. Ok... and I could see how the fact that it currently _doesn't_ have anything to do with the existing code would be confusing... Do you think that if/when it's making use of the ida rewrite it'll be ok? Or would you still prefer to have it in a new file (and if so, any preference on the naming?) -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 28 Aug 2013 14:23:58 -0700 Kent Overstreet <kmo@daterainc.com> wrote: > > I found things to be quite the opposite - it took 5 minutes of staring, > > head-scratching, double-checking and penny-dropping before I was > > confident that the newly-added code actually has nothing at all to do > > with the current code. Putting it in the same file was misleading, and > > I got misled. > > Ok... and I could see how the fact that it currently _doesn't_ have > anything to do with the existing code would be confusing... > > Do you think that if/when it's making use of the ida rewrite it'll be > ok? Or would you still prefer to have it in a new file I'm constitutionally reluctant to ever assume that any out-of-tree code will be merged. Maybe you'll get hit by a bus, and maybe the code sucks ;) Are you sure that the two things are so tangled together that they must live in the same file? If there's some nice layering between ida and percpu_ida then perhaps such a physical separation would remain appropriate? > (and if so, any preference on the naming?) percpu_ida.c? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 2013-08-28 at 14:36 -0700, Andrew Morton wrote: > On Wed, 28 Aug 2013 14:23:58 -0700 Kent Overstreet <kmo@daterainc.com> wrote: > > > > I found things to be quite the opposite - it took 5 minutes of staring, > > > head-scratching, double-checking and penny-dropping before I was > > > confident that the newly-added code actually has nothing at all to do > > > with the current code. Putting it in the same file was misleading, and > > > I got misled. > > > > Ok... and I could see how the fact that it currently _doesn't_ have > > anything to do with the existing code would be confusing... > > > > Do you think that if/when it's making use of the ida rewrite it'll be > > ok? Or would you still prefer to have it in a new file > > I'm constitutionally reluctant to ever assume that any out-of-tree code > will be merged. Maybe you'll get hit by a bus, and maybe the code > sucks ;) > > Are you sure that the two things are so tangled together that they must > live in the same file? If there's some nice layering between ida and > percpu_ida then perhaps such a physical separation would remain > appropriate? > > > (and if so, any preference on the naming?) > > percpu_ida.c? Hi Andrew, I've folded Kent's two patches from this thread into the -v4 commit, and moved the logic from idr.[c,h] to percpu_ida.[c,h] as per your above recommendation. The cpumask_t changes are working as expected thus far, and will be going out a -v5 series for you to review -> signoff shortly. Thank you, --nab -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/include/linux/idr.h b/include/linux/idr.h index f0db12b..cdf39be 100644 --- a/include/linux/idr.h +++ b/include/linux/idr.h @@ -267,7 +267,7 @@ struct percpu_ida { * will just keep looking - but the bitmap _must_ be set whenever a * percpu freelist does have tags. */ - unsigned long *cpus_have_tags; + cpumask_t cpus_have_tags; struct { spinlock_t lock; diff --git a/lib/idr.c b/lib/idr.c index 26495e1..15c021c 100644 --- a/lib/idr.c +++ b/lib/idr.c @@ -1178,7 +1178,13 @@ EXPORT_SYMBOL(ida_init); #define IDA_PCPU_SIZE ((IDA_PCPU_BATCH_MOVE * 3) / 2) struct percpu_ida_cpu { + /* + * Even though this is percpu, we need a lock for tag stealing by remote + * CPUs: + */ spinlock_t lock; + + /* nr_free/freelist form a stack of free IDs */ unsigned nr_free; unsigned freelist[]; }; @@ -1209,21 +1215,21 @@ static inline void steal_tags(struct percpu_ida *pool, unsigned cpus_have_tags, cpu = pool->cpu_last_stolen; struct percpu_ida_cpu *remote; - for (cpus_have_tags = bitmap_weight(pool->cpus_have_tags, nr_cpu_ids); + for (cpus_have_tags = cpumask_weight(&pool->cpus_have_tags); cpus_have_tags * IDA_PCPU_SIZE > pool->nr_tags / 2; cpus_have_tags--) { - cpu = find_next_bit(pool->cpus_have_tags, nr_cpu_ids, cpu); + cpu = cpumask_next(cpu, &pool->cpus_have_tags); - if (cpu == nr_cpu_ids) - cpu = find_first_bit(pool->cpus_have_tags, nr_cpu_ids); + if (cpu >= nr_cpu_ids) + cpu = cpumask_first(&pool->cpus_have_tags); - if (cpu == nr_cpu_ids) + if (cpu >= nr_cpu_ids) BUG(); pool->cpu_last_stolen = cpu; remote = per_cpu_ptr(pool->tag_cpu, cpu); - clear_bit(cpu, pool->cpus_have_tags); + cpumask_clear_cpu(cpu, &pool->cpus_have_tags); if (remote == tags) continue; @@ -1246,6 +1252,10 @@ static inline void steal_tags(struct percpu_ida *pool, } } +/* + * Pop up to IDA_PCPU_BATCH_MOVE IDs off the global freelist, and push them onto + * our percpu freelist: + */ static inline void alloc_global_tags(struct percpu_ida *pool, struct percpu_ida_cpu *tags) { @@ -1317,8 +1327,8 @@ int percpu_ida_alloc(struct percpu_ida *pool, gfp_t gfp) if (tags->nr_free) { tag = tags->freelist[--tags->nr_free]; if (tags->nr_free) - set_bit(smp_processor_id(), - pool->cpus_have_tags); + cpumask_set_cpu(smp_processor_id(), + &pool->cpus_have_tags); } spin_unlock(&pool->lock); @@ -1363,8 +1373,8 @@ void percpu_ida_free(struct percpu_ida *pool, unsigned tag) spin_unlock(&tags->lock); if (nr_free == 1) { - set_bit(smp_processor_id(), - pool->cpus_have_tags); + cpumask_set_cpu(smp_processor_id(), + &pool->cpus_have_tags); wake_up(&pool->wait); } @@ -1398,7 +1408,6 @@ EXPORT_SYMBOL_GPL(percpu_ida_free); void percpu_ida_destroy(struct percpu_ida *pool) { free_percpu(pool->tag_cpu); - kfree(pool->cpus_have_tags); free_pages((unsigned long) pool->freelist, get_order(pool->nr_tags * sizeof(unsigned))); } @@ -1428,7 +1437,7 @@ int percpu_ida_init(struct percpu_ida *pool, unsigned long nr_tags) /* Guard against overflow */ if (nr_tags > (unsigned) INT_MAX + 1) { - pr_err("tags.c: nr_tags too large\n"); + pr_err("percpu_ida_init(): nr_tags too large\n"); return -EINVAL; } @@ -1442,11 +1451,6 @@ int percpu_ida_init(struct percpu_ida *pool, unsigned long nr_tags) pool->nr_free = nr_tags; - pool->cpus_have_tags = kzalloc(BITS_TO_LONGS(nr_cpu_ids) * - sizeof(unsigned long), GFP_KERNEL); - if (!pool->cpus_have_tags) - goto err; - pool->tag_cpu = __alloc_percpu(sizeof(struct percpu_ida_cpu) + IDA_PCPU_SIZE * sizeof(unsigned), sizeof(unsigned));
Fixup patch, addressing Andrew's review feedback: Signed-off-by: Kent Overstreet <kmo@daterainc.com> --- include/linux/idr.h | 2 +- lib/idr.c | 38 +++++++++++++++++++++----------------- 2 files changed, 22 insertions(+), 18 deletions(-)