lkml.org 
[lkml]   [2012]   [Jul]   [25]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH] scsi: virtio-scsi: Fix address translation failure of HighMem pages used by sg list
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 stable@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


\
 
 \ /
  Last update: 2012-07-25 12:21    [from the cache]
©2003-2011 Jasper Spaans