Hello, Jeff. On 06/26/2010 05:45 AM, Jeff Garzik wrote: > No, it's not. ata_qc_complete() is an indicator that _one_ completion > event occurred, not _all_ completion events for that port. Well, it can indicte the start of cluster of completions, which is the necessary information anyway. From the second call on, it's a simple flag test and return. I doubt it will affect anything even w/ high performance SSDs but please read on. > Converting drivers to use ata_qc_complete_multiple() completely misses > the point: ata_qc_complete_multiple() is doing exactly the same thing > as those drivers: calling ata_qc_complete() in a loop. The point of ata_qc_complete_multiple() is giving libata core layer better visibility into how commands are being executed, which in turn allows (continued below) > ata_qc_complete() is -- as its name implies -- completing a single qc. > Your patch introduces an unconditional controller-wide unexpect_irq() > call. It's a layering violation. > > You can trivially trace through ata_qc_complete_multiple() after patch > #11 is applied, and see the result... for $N completion bits passed to > ata_qc_complete_multiple(), you call > unexpect_irq() > expect_irq() > in rapid succession $N times, once for each ata_qc_complete() call in > the loop of ata_qc_complete_multiple(). For something that is not > needed for modern SATA controllers. ata_qc_complete_multiple() call [un]expect_irq() only once by introducing an internal completion function w/o irq expect handling, say ata_qc_complete_raw() and making both ata_qc_complete() and ata_qc_complete_multiple() simple wrapper around it w/ irq expect handling. > The preferred solution would be something that only touches legacy > controllers, namely: > > *) create ata_port_complete(), with the implementation > > ata_qc_complete() > unexpect_irq() > > *) then call ata_port_complete() in the legacy code that needs > unexpect_irq() > > We don't want to burden modern SATA drivers with the overhead of > dealing with silly PATA/SATA1 legacy irq nastiness, particularly the > ugliness of calling I think we're much better off applying it to all the drivers. IRQ expecting is very cheap and scalable and there definitely are plenty of IRQ delivery problems with modern controllers although their patterns tend to be different from legacy ones. Plus, it will also be useful for power state predictions. > unexpect_irq() + expect_irq() > > for a number of NCQ commands, in a tight loop! As I wrote above, I don't think N*unexpect_irq() really matters but with ata_qc_complete_multiple() conversion, there will only be single pair of expect/unexpect for each cluster of completions anyway. Thanks. -- tejun -- 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
