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 -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
