Jens Axboe wrote:
> This is the old ata_sg_is_last:
>
> static inline int
> ata_sg_is_last(struct scatterlist *sg, struct ata_queued_cmd *qc)
> {
> if (sg == &qc->pad_sgent)
> return 1;
> if (qc->pad_len)
> return 0;
> if (((sg - qc->__sg) + 1) == qc->n_elem)
> return 1;
> return 0;
> }
>
> and the new one:
>
> static inline int
> ata_sg_is_last(struct scatterlist *sg, struct ata_queued_cmd *qc)
> {
> if (sg == &qc->pad_sgent)
> return 1;
> if (qc->pad_len)
> return 0;
> if (qc->n_iter == qc->n_elem)
> return 1;
> return 0;
> }
>
> ->n_iter is how ata_qc_next_sg() walks over the sglist, I don't
> understand your reference to why depending on that during iteration
> would be bad?
Because that makes ata_sg iterator macros have hidden side effects
(nothing in the interface suggests it can't be nested and when somebody
actually nests it, finding what went wrong can be pretty difficult). I
think it would be better to have explicit ata_sg_iter passed around if
sg itself isn't enough to walk the sg list.
> So we could add a test for sg_last() there, but that would turn sg table
> iteration into an O(N^2) operation for those drivers that use
> ata_sg_is_last() with chained sg tables. I'd much rather just get rid of
> ata_sg_is_last(), it's only used to mark end-of-table entries for
> hardware. That logic can be performed cheaper.
Yeap, it can be removed but having "is this the last one?" test is just
nicer to low level drivers. With ata_sg_iter, I think we can do it in
O(N) by looking up and caching the next entry.
Thanks.
--
tejun
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
