Re: virtio(-scsi) vs. chained sg_lists (was Re: [PATCH] scsi: virtio-scsi: Fix address translation failure of HighMem pages used by sg list)

[Boaz Harrosh <bharrosh@panasas.com>]

On 07/25/2012 11:06 PM, Paolo Bonzini wrote:

> Il 25/07/2012 21:16, Boaz Harrosh ha scritto:
>> The picture confused me. It looked like the first element is the virtio_scsi_cmd_req
>> not an sgilist-element that points to the struct's buffer.
>>
>> In that case then yes your plan of making a two-elements fragment that points to the
>> original scsi-sglist is perfect. All you have to do is that, and all you have to do
>> at virtio is use the sg_for_each macro and you are done.
>>
>> You don't need any sglist allocation or reshaping. And you can easily support
>> chaining. Looks like order of magnitude more simple then what you do now
> 
> It is.
> 
>> So what is the problem?
> 
> That not all architectures have ARCH_HAS_SG_CHAIN (though all those I
> care about do).  So I need to go through all architectures and make sure
> they use for_each_sg, or at least to change ARCH_HAS_SG_CHAIN to a
> Kconfig define so that dependencies can be expressed properly.
> 


What is actually preventing ARCH_HAS_SG_CHAIN from all these ARCHES?
is that the DMA drivers not using for_each_sg(). Sounds like easy
to fix.

But yes a deep change would convert ARCH_HAS_SG_CHAIN to a Kconfig.

If you want to be lazy, like me, You might just put a BUILD_BUG_ON
in code, requesting the user to disable the driver for this ARCH.

I bet there is more things to do at ARCH to enable virtualization
then just support ARCH_HAS_SG_CHAIN. Be it just another requirement.

If you Document it and make sure current ARCHs are fine, it should
not ever trigger.

>> And BTW you won't need that new __sg_set_page API anymore.
> 
> Kind of.
> 
>    sg_init_table(sg, 2);
>    sg_set_buf(sg[0], req, sizeof(req));
>    sg_chain(sg[1], scsi_out(sc));
> 
> is still a little bit worse than
> 
>    __sg_set_buf(sg[0], req, sizeof(req));
>    __sg_chain(sg[1], scsi_out(sc));
> 


I believe they are the same, specially for the
on the stack 2 elements array. Actually I think
In both cases you need to at least call sg_init_table()
once after allocation, No?

Your old code with big array copy and re-shaping was
a better example of the need for your new API. Which I agree.

But please for my sake do not call it __sg_chain. Call it
something like sg_chain_not_end(). I hate those "__" which
for god sack means what? 
(A good name is when I don't have to read the code, an "__"
 means "fuck you go read the code")

> Paolo


Thanks
Boaz