On Thu, Oct 23 2008, Elias Oltmanns wrote: > Jens Axboe <jens.axboe@xxxxxxxxxx> wrote: > > On Thu, Oct 23 2008, Jens Axboe wrote: > >> On Thu, Oct 23 2008, Tejun Heo wrote: > > > >> > while (done_mask) { > >> > struct ata_queued_cmd *qc; > >> > unsigned int next = __ffs(done_mask); > >> > > >> > tag += next; > >> > if ((qc = ata_qc_from_tag(ap, tag))) { > >> > ata_qc_complete(qc); > >> > nr_done++; > >> > } > >> > next++; > >> > tag += next; > >> > done_mask >>= next; > >> > } > >> > >> That doesn't work (you're adding next to tag twice), it needs a little > >> tweak: > >> > >> while (done_mask) { > >> struct ata_queued_cmd *qc; > >> unsigned int next = __ffs(done_mask); > >> > >> if ((qc = ata_qc_from_tag(ap, tag + next))) { > >> ata_qc_complete(qc); > >> nr_done++; > >> } > >> next++; > >> tag += next; > >> done_mask >>= next; > >> } > >> > >> and I think it should work. Not tested yet :-) > > > > Pondered some more, and it can't work. The problem is that if we > > complete tag 31, we attempt to shift done_mask down by 32 bits. On a > > 32-bit arch, that's not defined. So we DO need a check like the existing > > one, or something similar. > > > > So I don't think we need to make changes to this patch either, at least > > unless one of you can come up with a better check that avoids a branch. > > What about a switch outside the while loop: > > if (done_mask == ATA_MAX_QUEUE >> 1) { > if ((qc = ata_qc_from_tag(ap, ATA_MAX_QUEUE >> 1))) { > ata_qc_complete(qc); > nr_done = 1; > } > } else > while (done_mask) > ... > > Alternatively, you could just alter tag and done_mask (tag = > ATA_MAX_QUEUE >> 2, done_mask = 2) and enter the while loop > unconditionally. But then, you claimed that there will hardly ever be > more than one command to complete, so my suggestions will probably not > improve anything in real life. Honestly, I think the current check is a lot cleaner then. -- Jens Axboe -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html