Re: [PATCH/RFC] SPI core: turn transfers to be linked list

[Vitaly Wool <vwool@ru.mvista.com>]

David Brownell wrote:

>>>Hmm, color me confused.  Is there something preventing a driver from
>>>having its own freelist (or whatever), in cases where kmalloc doesn't
>>>suffice?  If not, why should the core change?  And what sort of driver
>>>measurements are you doing, to conclude that kmalloc doesn't suffice?
>>> 
>>>
>>>      
>>>
>>Can't get what you're talking about. A freelist of what?
>>    
>>
>
>Of whatever the driver wants.  You had pointed at some code that
>boiled down to keeping a freelist of some structures in the core.
>Such code doesn't _need_ to be in the core at all ...
>  
>
Correct.
But you're missing two things: a) it wasn't in the core, it was the 
library and b) it doesn't matter.
It's just a simple stack-based memory allocation model which is _very 
attractive_ to use and which *can't* be used with transfers as arrays.

>
>  
>
>>Basically the idea of the custom lightweight allocation is the 
>>following: allocate a page and divide into regions of the same size, 
>>initialize and use these regions as a stack.
>>    
>>
>
>There already a lot of allocators that support that, like kmem_cache_t
>or dma_pool or mempool_t ... no more, please!  They don't all have that
>"stack"/freelist notion though; easy for drivers to do that, IMO.
>
>Drivers that don't want to hit the heap will preallocate everything
>they can, and they'll probably have their own freelists.  That wins
>by eliminating the slab subroutine calls from what are often hot
>code paths, along with their fault handling logic ... and removing
>the need (and testing) for fault handling wins by improving robustness.
>  
>
See b) above. Doesn't matter where to put an implementation of this 
memory allocation model. Matters whether it's usable or not. Currently 
it's not, especially if a driver is complicated enuff to have transfers 
of arbitrary size.

>>>I'd have said that since this increases the per-transfer costs (to set
>>>up and manage the list memberships) you want to increase the weight of
>>>that part of the API, not decrease it.  ;)
>>> 
>>>
>>>      
>>>
>>Disagree. Let's look deeper. kmalloc itself is more heavyweght than 
>>setting up list memberships.
>>    
>>
>
>But kmalloc was previously optional, yes?  And should still be ...
>  
>
Whoops. How can it be optional if we want to have an async transfer?

>
>  
>
>>The list setting commands are pretty essential and will not add a lot to 
>>the assembly code.
>>    
>>
>
>I'm not totally averse to such changes, but you don't seem to be making
>the best arguments.  Example:  they're clearly not "essential" because
>transfer queues work today with the lists at the spi_message level.
>  
>
Not sure if I got you here, sorry.

>
>  
>
>>And your understanding is not quite correct. What if I'm going to send a 
>>chain of 5 messages? I'll allocate 5 spi_msg's in my case which all are 
>>gonna be of the same size -- thus the technique described above is 
>>applicable. In case of your core it's not.
>>    
>>
>
>I must have been deceived then by this little utility:
>
>static inline struct spi_message *spi_message_alloc(unsigned ntrans, gfp_t flags)
>{
>        struct spi_message *m;
>
>        m = kzalloc(sizeof(struct spi_message)
>                        + ntrans * sizeof(struct spi_transfer),
>                        flags);
>        if (m) {
>                m->transfers = (void *)(m + 1);
>                m->n_transfer = ntrans;
>        }
>        return m;
>}
>
>Sure looks to me like spi_message_alloc(5, SLAB_ATOMIC) should do the trick.
>One allocation, always the same size.  And kzalloc() could easily optimize
>that to use the "size-192" slab cache (or whatever), like kmalloc() does;
>that'd be a small speedup.
>  
>
'5' is only an example. What if there gonna be 7, 10, 12? What if the 
possible cases for a driver are 1, 2, 5, 7, 12 and 14 transfers per message?
Of course the allocation won't be of the same size always. Moreover, the 
size is gonna differ a lot.

>
>  
>
>>>Could you elaborate on this problem you perceive?  This isn't the only
>>>driver API in Linux to talk in terms of arrays describing transfers,
>>>even potentially large arrays.  
>>>      
>>>
>>The problem is: we're using real-time enhancements patch developed by 
>>Ingo/Sven/Daniel etc. You cannot call kmalloc from the interrupt 
>>context  if you're using this patch. Yeah, yeah -- the interrupt 
>>handlers are in threads by default, but we can't go for that since we 
>>want immediate acknowledgement from the interrupt context, and that 
>>implies spi_message/spi_transfer allocation.
>>    
>>
>
>Could you elaborate a bit here?  You seem to be implying that for some
>reason one of your SPI related drivers must use non-threaded hardirqs,
>AND (news to me, if true) that such hardirqs can't kmalloc(), AND that
>it can't use any of several widely used strategies to avoid hitting
>things like the slab allocator.  (That last seems hardest to believe...)
>
>I asked earlier what sort of performance measurements you're making
>that are leading you to these conclusions.  I'm still wondering.  :)
>  
>
This is mostly a stability issue, not a performance thing.

>>>Consider how "struct scatterlist" is used, and how USB manages the
>>>descriptors for isochronous transfers.  They don't use linked lists
>>>there, and haven't seemed to suffer from it.
>>> 
>>>
>>>      
>>>
>>Not sure if I understand why it's relevant to what we're discussing.
>>    
>>
>
>Examples of driver interfaces in Linux that work fine using arrays to couple
>a group of transfers.  If you see some problem that makes a compelling
>argument that the SPI must change, it would also affect drivers using
>those interfaces too, at least a little bit ... right?  Does it?
>  
>
I'm not concerned much with USB mow :), though I know guys having _a 
lot_ of trouble trying USB devices work within the real-time Linux 
enhancememts environment I was referring to earlier ;) Probably a lot of 
memory allocations within interrupt context... which is a always-avoid 
thing for any real-time system BTW...

Vitaly