Il 25/07/2012 11:22, Boaz Harrosh ha scritto: >>> >> for_each_sg(table->sgl, sg_elem, table->nents, i) >>> >> - sg_set_buf(&sg[idx++], sg_virt(sg_elem), >>> >> sg_elem->length); >>> >> + sg_set_page(&sg[idx++], sg_page(sg_elem), >>> >> sg_elem->length, >>> >> + sg_elem->offset); >> > >> > This can simply be >> > >> > sg[idx++] = *sg_elem; >> > >> > Can you repost it with this change, and also add sta...@vger.kernel.org >> > to the Cc? Thanks very much! >> > > > No! Please use sg_set_page()! Look at sg_set_page(), which calls > sg_assign_page(). > It has all these jump over chained arrays. When you'll start using long > sg_lists (which you should) then jumping from chain to chain must go through > sg_page(sg_elem) && sg_assign_page(), As in the original patch.
Hi Boaz, actually it seems to me that using sg_set_page is wrong, because it will not copy the end marker from table->sgl to sg[]. If something chained the sg[] scatterlist onto something else, sg_next's test for sg_is_last would go beyond the table->nents-th item and access invalid memory. Using chained sglists is on my to-do list, I expect that it would make a nice performance improvement. However, I was a bit confused as to what's the plan there; there is hardly any user, and many arches still do not define ARCH_HAS_SG_CHAIN. Do you have any pointer to discussions or LWN articles? I would need to add support for the long sglists to virtio; this is not a problem, but in the past Rusty complained that long sg-lists are not well suited to virtio (which would like to add elements not just at the beginning of a given sglist, but also at the end). It seems to me that virtio would prefer to work with a struct scatterlist ** rather than a long sglist. Paolo -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
