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

[Stefan Richter <stefanr@s5r6.in-berlin.de>]

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.)


>>>> 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.


>>> 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.


> 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.
-- 
Stefan Richter
-=====-==--- =--- --=-=
http://arcgraph.de/sr/