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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

[Index of Archives]     [Kernel Newbies]     [Netfilter]     [Bugtraq]     [Photo]     [Stuff]     [Gimp]     [Yosemite News]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Video 4 Linux]     [Linux for the blind]     [Linux Resources]
  Powered by Linux