Messages in this thread Patch in this message | | | Date | Sun, 3 Oct 2004 22:08:10 +0200 | From | Ingo Molnar <> | Subject | Re: [patch] voluntary-preempt-2.6.9-rc2-mm4-S7 |
| |
* Lee Revell <rlrevell@joe-job.com> wrote:
> I believe we have found a bug. A user reported massive xruns with S7, > they turned out to be printk() overhead from tons of "using > smp_processor_id() in preemptible code" errors. The trace below > repeats over and over. Looks like raid0 is the problem.
the patch below should fix this issue - it's in rc3-mm1 already.
Ingo
* Reuben Farrelly <reuben-lkml@reub.net> wrote:
> Since upgrading from -mm3 to -mm4, I'm now getting messages like this > logged every second or so: > > Sep 27 18:28:06 tornado kernel: using smp_processor_id() in preemptible code: swapper/1 > Sep 27 18:28:06 tornado kernel: [<c0104dce>] dump_stack+0x17/0x19 > Sep 27 18:28:06 tornado kernel: [<c0117fc6>] smp_processor_id+0x80/0x86 > Sep 27 18:28:06 tornado kernel: [<c0282bf3>] make_request+0x174/0x2e7 > Sep 27 18:28:06 tornado kernel: [<c02073dd>] generic_make_request+0xda/0x190
this is the remove-bkl patch's debugging feature showing that there's more preempt-unsafe disk statistics code in the RAID code.
i've attached a patch that introduces preempt and non-preempt versions of the statistics code and updates the block code to use the appropriate ones - does this fix all the smp_processor_id() warnings you get?
Ingo
--- linux/drivers/block/ll_rw_blk.c.orig +++ linux/drivers/block/ll_rw_blk.c @@ -2099,13 +2099,13 @@ void drive_stat_acct(struct request *rq, return; if (rw == READ) { - disk_stat_add(rq->rq_disk, read_sectors, nr_sectors); + __disk_stat_add(rq->rq_disk, read_sectors, nr_sectors); if (!new_io) - disk_stat_inc(rq->rq_disk, read_merges); + __disk_stat_inc(rq->rq_disk, read_merges); } else if (rw == WRITE) { - disk_stat_add(rq->rq_disk, write_sectors, nr_sectors); + __disk_stat_add(rq->rq_disk, write_sectors, nr_sectors); if (!new_io) - disk_stat_inc(rq->rq_disk, write_merges); + __disk_stat_inc(rq->rq_disk, write_merges); } if (new_io) { disk_round_stats(rq->rq_disk); @@ -2151,12 +2151,12 @@ void disk_round_stats(struct gendisk *di { unsigned long now = jiffies; - disk_stat_add(disk, time_in_queue, + __disk_stat_add(disk, time_in_queue, disk->in_flight * (now - disk->stamp)); disk->stamp = now; if (disk->in_flight) - disk_stat_add(disk, io_ticks, (now - disk->stamp_idle)); + __disk_stat_add(disk, io_ticks, (now - disk->stamp_idle)); disk->stamp_idle = now; } @@ -2957,12 +2957,12 @@ void end_that_request_last(struct reques unsigned long duration = jiffies - req->start_time; switch (rq_data_dir(req)) { case WRITE: - disk_stat_inc(disk, writes); - disk_stat_add(disk, write_ticks, duration); + __disk_stat_inc(disk, writes); + __disk_stat_add(disk, write_ticks, duration); break; case READ: - disk_stat_inc(disk, reads); - disk_stat_add(disk, read_ticks, duration); + __disk_stat_inc(disk, reads); + __disk_stat_add(disk, read_ticks, duration); break; } disk_round_stats(disk); --- linux/include/linux/genhd.h.orig +++ linux/include/linux/genhd.h @@ -112,13 +112,14 @@ struct gendisk { /* * Macros to operate on percpu disk statistics: - * Since writes to disk_stats are serialised through the queue_lock, - * smp_processor_id() should be enough to get to the per_cpu versions - * of statistics counters + * + * The __ variants should only be called in critical sections. The full + * variants disable/enable preemption. */ #ifdef CONFIG_SMP -#define disk_stat_add(gendiskp, field, addnd) \ +#define __disk_stat_add(gendiskp, field, addnd) \ (per_cpu_ptr(gendiskp->dkstats, smp_processor_id())->field += addnd) + #define disk_stat_read(gendiskp, field) \ ({ \ typeof(gendiskp->dkstats->field) res = 0; \ @@ -142,7 +143,8 @@ static inline void disk_stat_set_all(str } #else -#define disk_stat_add(gendiskp, field, addnd) (gendiskp->dkstats.field += addnd) +#define __disk_stat_add(gendiskp, field, addnd) \ + (gendiskp->dkstats.field += addnd) #define disk_stat_read(gendiskp, field) (gendiskp->dkstats.field) static inline void disk_stat_set_all(struct gendisk *gendiskp, int value) { @@ -150,8 +152,21 @@ static inline void disk_stat_set_all(str } #endif -#define disk_stat_inc(gendiskp, field) disk_stat_add(gendiskp, field, 1) +#define disk_stat_add(gendiskp, field, addnd) \ + do { \ + preempt_disable(); \ + __disk_stat_add(gendiskp, field, addnd); \ + preempt_enable(); \ + } while (0) + +#define __disk_stat_dec(gendiskp, field) __disk_stat_add(gendiskp, field, -1) #define disk_stat_dec(gendiskp, field) disk_stat_add(gendiskp, field, -1) + +#define __disk_stat_inc(gendiskp, field) __disk_stat_add(gendiskp, field, 1) +#define disk_stat_inc(gendiskp, field) disk_stat_add(gendiskp, field, 1) + +#define __disk_stat_sub(gendiskp, field, subnd) \ + __disk_stat_add(gendiskp, field, -subnd) #define disk_stat_sub(gendiskp, field, subnd) \ disk_stat_add(gendiskp, field, -subnd) - To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
| |