Re: [patch resend] firewire: ieee1394: Move away from SG_ALL

[Boaz Harrosh <bharrosh@panasas.com>]

Stefan Richter wrote:
> Boaz Harrosh wrote:
>> Stefan Richter wrote:
>>> James Bottomley wrote:
>>>> On Mon, 2008-08-04 at 20:08 +0200, Stefan Richter wrote:
>>>>> As a discussion reminded me today, I believe I should merge the
>>>>> following patch (could have done so much earlier in fact).  Or is there
>>>>> anything speaking against it?
>>>> The value 255 is chosen because it worked before.  What you need to do
>>>> is establish what the upper limit for firewire is (or indeed if it has
>>>> one).
>>> The limit is 65535, following from SBP-2 clause 5.1.2, definition of 
>>> data_size.
>>>
>>> [Side note:  The SBP-2 s/g list (a.k.a. page table) consists of 64bit 
>>> wide entries and needs to be contiguous in memory from the POV of the 
>>> FireWire PCI or PCIe controller, and the SBP-2 target reads the table 
>>> from the initiator's memory.  The (fw-)sbp2 driver builds this table as 
>>> a copy of the kernel's s/g list; but this was certainly already to the 
>>> reader clear from the context in the diff.]
>> Does the above mean you need contiguous physical memory to hold the   
>> sbp2's sg table. If so then it should be allocated with alloc_coherent and
>> maybe total size up to a PAGE_SIZE.
> 
> It doesn't have to be cache-coherent but it indeed needs to be memory
> which can be DMA-mapped in one piece.
> 
> (DMA direction is DMA_TO_DEVICE.  Lifetime of the mapping is from before
> the ORB is committed until after we received completion status, or the
> request timed out and its ORB was aborted.  fw-sbp2 implements exactly
> this lifetime, while sbp2 keeps the DMA mapping around from before login
> to the target until after logout.)
> 

OK you are right, DMA_TO_DEVICE is less problematic in regard to variables
that are changed by the CPU that share cacheline with DMAed memory. As
long as the code has a cache-sync before the start of transfer.
But I see you'll take care of that below.

> 
>>>>> Meanwhile, SG_ALL has been redefined from 255 to SCSI_MAX_SG_SEGMENTS =
>>>>> 128 without me noticing it.  But since several popular architectures
>>>>> define ARCH_HAS_SG_CHAIN, we may want to go back to 255 in (fw-)sbp2.
>>>>> (Besides, we should also do some tests to figure out an actual optimum.
>>>>> And fw-sbp2.c's struct sbp2_command_orb should become variably sized.)
>>>> Don't bother with optmium ... that's block's job based on what it sees
>>>> from the completions.  All we need to know is maximum.
>> From my tests, the block layer will, very fast, send you chained commands.
>> if you set it to 255 you'll see two chained fragments. In iscsi we have
>> 4096 and it is very good for performance. However since you are pre-allocating
>> the memory per can_queue commands, this can get big, and the memory overhead
>> can be an impact on performance.
> 
> OK.  This gives me a better picture.
> 
> 
>>> OK, with the small caveat that the current (fw-)sbp2 driver code is 
>>> somewhat simplistic WRT page table handling and geared towards rather 
>>> short page tables.  But this may be possible to enhance without too much 
>>> difficulty.
>>>
>>>>> Does the most relevant user of large transfers with SBP-2 devices,
>>>>> sd-mod, use chained s/g lists?
>>>> pass, but firewire is a reasonably slow bus by modern standards, and you
>>> A 3200 Mb/s spec exists :-) (though no silicon yet, to my knowledge).
>> As you said above and since bitrate is increasing, a 255 is a good 
>> preallocated value, but perhaps it could also be a module parameter so
>> fast cards can have a bigger pre-allocated sg table. (per canQ command).
>> Also for embedded systems, they might want to lower this value.
> 
> A preset according to connection speed could also be done by the driver
> without user intervention.
> 

Even better thanks, that would be ideal. 

(Except for the embedded systems that would want to keep it low, perhaps
at configuration time)

> 
>>>> have the protocol overhead for each ORB, so I'd guess there's a point at
>>>> which increasing the transaction size doesn't buy anything.
>>>>
>>>> James
>> From re-inspecting the code I can see that the struct sbp2_command_orb
>> might have problems with none DMA coherent ARCHs.
> 
> Hmm.  This would be a bug.  But from what I see while scrolling through
> fw-sbp2 and through sbp2, they both do it right.  Well, to be very
> strict, sbp2 should dma_sync_single_for_cpu() at a slightly different
> place than it does now.  I shall patch this.
> 

Yes I agree with you. Keep me posted I'll review the code

> 
>> I think for safety
>> and flexibility the sbp2_command_orb.page_table array should be
>> dynamically allocated at init time
> 
> The older driver sbp2 uses a fixed-size pool of ORBs and s/g tables for
> the whole time that it is logged in into a target.  The author of the
> newer fw-sbp2 driver chose to keep ORB and s/g table only around for the
> lifetime of a request, and I think it's a valid approach.
> 
>> with even maybe alloc_coherent.
> 
> alloc_coherent isn't necessary.  Even code simplicity wouldn't profit
> too much IMO; we already have to DMA-map the data buffer for each
> request anyway.

OK I'm convinced. I think the proposed patch, (funny I wrote it) is good
as a first measure to take matters into drivers hands and avoid any
unintentional side effects from scsi constants.

Thanks
Boaz