Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
> > The reverse mapping
> > code hast to be less than 0.1KB.
>
> If reverse mapping means bus_to_virt(), then I would suggest not to
> provide it since it is a confusing interface. OTOH, only a few drivers
> need or want to retrieve the virtual address that lead to some bus dma
Your SCSI code went the other way; the logic is about the same.
That's easy enough ... I'm not going to argue that point any longer.
The driver might even have Real Intelligence to apply. But I wonder
how many assumptions drivers will end up making about those dma
mappings. It may be important to expose the "logical" page size to
the driver ("don't cross 4k boundaries"); currently it's hidden.
Other than that L1_CACHE goof, it seems this was the main thing
needing to change in the I API sent by. Sound right? Implementation
would be a different question. I'm not in the least attached to what
I sent by, but some implementation is needed. Slab-like, or buddy? :)
> Does 'usable' apply to Java applications ? :-)
Servers and other non-gui tools? I don't see why not. You can make
good systems software in many languages. There are advantages to
not having those classes of memory-related bugs. I'm looking forward
to GCC 3.0 with GCJ, compiling Java just like C. But that's OT.
- Dave
-
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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
On Fri, 9 Mar 2001, David Brownell wrote: > Gérard -- > > > Just for information to people that want to complexify the > > pci_alloc_consistent() interface thats looks simple and elegant to me: > > I certainly didn't propose that! Just a layer on top of the > pci_alloc_consistent code -- used as a page allocator, just > like you used it. > > > > The object file of the allocator as seen in sym2 is as tiny as 3.4K > > unstripped and 2.5K stripped. > > What I sent along just compiled to 2.3 KB ... stripped, and "-O". > Maybe smaller with normal kernel flags. The reverse mapping > code hast to be less than 0.1KB. If reverse mapping means bus_to_virt(), then I would suggest not to provide it since it is a confusing interface. OTOH, only a few drivers need or want to retrieve the virtual address that lead to some bus dma address and they should check that this virtual address is still valid prior to using it. As I wrote, some trivial hashed list can be used by such drivers (as sym* do). > I looked at your code, but it didn't seem straightforward to reuse. > I think the allocation and deallocation costs can be pretty comparable > in the two implementations. Your implementation might even fit behind > the API I sent. They're both layers over pci_*_consistent (and both > have address-to-address mappings, implemented much the same). I wanted the code as short as possible since the driver code is already very large. On the other hand there are bunches of #ifdef to deal with all still alive kernel versions. As a result, the code may well not be general nor clean enough to be moved to the kernel. Just what it actually does is fairly simple. > > Now, if modern programmers are expecting Java-like interfaces for writing > > kernel software, it is indeed another story. :-) > > Only if when you wrote "Java-like" you really meant "reusable"! :) Hmmm... 'reusable' implies 'usable'... Does 'usable' apply to Java applications ? :-) Gérard. - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
Gérard -- > Just for information to people that want to complexify the > pci_alloc_consistent() interface thats looks simple and elegant to me: I certainly didn't propose that! Just a layer on top of the pci_alloc_consistent code -- used as a page allocator, just like you used it. > The object file of the allocator as seen in sym2 is as tiny as 3.4K > unstripped and 2.5K stripped. What I sent along just compiled to 2.3 KB ... stripped, and "-O". Maybe smaller with normal kernel flags. The reverse mapping code hast to be less than 0.1KB. I looked at your code, but it didn't seem straightforward to reuse. I think the allocation and deallocation costs can be pretty comparable in the two implementations. Your implementation might even fit behind the API I sent. They're both layers over pci_*_consistent (and both have address-to-address mappings, implemented much the same). > Now, if modern programmers are expecting Java-like interfaces for writing > kernel software, it is indeed another story. :-) Only if when you wrote "Java-like" you really meant "reusable"! :) - Dave - 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/
Re: [linux-usb-devel] Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
> Date: Fri, 09 Mar 2001 13:14:03 -0800 > From: David Brownell <[EMAIL PROTECTED]> >[...] > It feels to me like you're being inconsistent here, objecting > to a library API for some functionality (mapping) yet not for > any of the other functionality (alignment, small size, poisoning > and so on). And yet when Pete Zaitcev described what that > mapping code actually involved, you didn't object. So you've > succeeded in confusing me. Care to unconfuse? I did not propose an API or library which would be equal amond equals with first rate citizens of pci_alloc_xxx and friends. I pointed out that driver can do tracking of reverse mappings at very little cost by using offset [Alan remarked to that how hash can use page number]; so, one may say that I supported DaveM's viewpoint. No wonder he did not object. -- Pete - 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/
Re: [linux-usb-devel] Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
> I wonder if it may be feasible to allocate a bunch of contiguous
> pages. Then, whenever the hardware returns a bus address, subtract
> the remembered bus address of the zone start, add the offset to
> the virtual and voila.
Even if not you can hash by page number not low bits so the hash is way
smaller. You (in most cases) can also write the entry number in the relevant
tables onto the end of the object in spare space (or in front of it)
Something as trivial as
struct usb_thingy
{
u32 thing_id;
u32 flags;
struct usb_thingy *next;
#ifndef __LP64__
u32 pad
#endif
struct usb_controller_goo goo;
}
Alan
-
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/
Re: [linux-usb-devel] Re: SLAB vs. pci_alloc_xxx in usb-uhci patch[RFC: API]
> > Given that some hardware must return the dma addresses, why > > should it be a good thing to have an API that doesn't expose > > the notion of a reverse mapping? At this level -- not the lower > > level code touching hardware PTEs. > > Because its' _very_ expensive on certain machines. You have to do > 1 or more I/O accesses to get at the PTEs. Except, I said this was NOT at that level. Those costs don't need to be incurred, but you are reasoning as if they did. > If you add this reverse notion to just one API (the dma pool one) then > people will complain (rightly) that there is not orthogonality in the > API since the other mapping functions do not provide it. "Orthogonality" is the wrong word there. In fact, this is a highly orthogonal approach: each layer deals with distinct problems. (Which is why I'd ignore that complaint.) There's a bunch of functionality drivers need to have, and which the pci_*_consistent() layer APIs (rightly) don't provide. Just like a kmem_cache provides functionality that's not visible through the generic page allocator code; except that this needs to work with the pci-specific page allocator. It feels to me like you're being inconsistent here, objecting to a library API for some functionality (mapping) yet not for any of the other functionality (alignment, small size, poisoning and so on). And yet when Pete Zaitcev described what that mapping code actually involved, you didn't object. So you've succeeded in confusing me. Care to unconfuse? - Dave - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
On Fri, 9 Mar 2001, David Brownell wrote: > > > > > extern void * > > > > > pci_pool_dma_to_cpu (struct pci_pool *pool, dma_addr_t handle); > > > > > > > > Do lots of drivers need the reverse mapping? It wasn't on my todo list > > > > yet. > > > > > > Some hardware (like OHCI) talks to drivers using those dma handles. > > > > I wonder if it may be feasible to allocate a bunch of contiguous > > pages. Then, whenever the hardware returns a bus address, subtract > > the remembered bus address of the zone start, add the offset to > > the virtual and voila. > > That's effectively what the implementation I posted is doing. > > Simple math ... as soon as you get the right "logical page", > and that page size could become a per-pool tunable. Currently > one logical page is PAGE_SIZE; there are some issues to > deal with in terms of not crossing page boundaries. > > There can be multiple such pages, known to the pool allocator > and hidden from the device drivers. I'd expect most USB host > controllers wouldn't allocate more than one or two pages, so > the cost of this function would typically be small. Just for information to people that want to complexify the pci_alloc_consistent() interface thats looks simple and elegant to me: (Hopefully, I am not off topic here) 1) The sym53c8xx driver and friends expect this simple interface to return naturally aligned memory chunks. It mostly allocates 1 page at a time. 2) The sym* drivers use a very simple allocator that keeps track of bus addresses for each chunk (page sized). The object file of the allocator as seen in sym2 is as tiny as 3.4K unstripped and 2.5K stripped. 3) The drivers need reverse virtual addresses for the DSA bus addresses and implements a simplistic hashed list that costs no more than 10 lines of trivial C code. Btw, as a result, if pci_alloc_consistent() will ever return not naturally aligned addresses the sym* drivers will be broken for Linux. This leaves me very surprised by all this noise given the _no_ issue I have seen using the pci_alloc_consistent() interface. It looked to me a _lot_ more simple to use than the equivalent interfaces I have had to suffer with other O/Ses. Now, if modern programmers are expecting Java-like interfaces for writing kernel software, it is indeed another story. :-) Gérard. - 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/
Re: [linux-usb-devel] Re: SLAB vs. pci_alloc_xxx in usb-uhci patch[RFC: API]
David Brownell writes: > Given that some hardware must return the dma addresses, why > should it be a good thing to have an API that doesn't expose > the notion of a reverse mapping? At this level -- not the lower > level code touching hardware PTEs. Because its' _very_ expensive on certain machines. You have to do 1 or more I/O accesses to get at the PTEs. If you add this reverse notion to just one API (the dma pool one) then people will complain (rightly) that there is not orthogonality in the API since the other mapping functions do not provide it. No, it is unacceptable. Later, David S. Miller [EMAIL PROTECTED] - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
David Brownell writes: > > Do lots of drivers need the reverse mapping? It wasn't on my todo list > > yet. > > Some hardware (like OHCI) talks to drivers using those dma handles. Drivers for such hardware will this keep track of the information necessary to make this reverse mapping. Later, David S. Miller [EMAIL PROTECTED] - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
Pete Zaitcev writes: > > Some hardware (like OHCI) talks to drivers using those dma handles. > > I wonder if it may be feasible to allocate a bunch of contiguous > pages. Then, whenever the hardware returns a bus address, subtract > the remembered bus address of the zone start, add the offset to > the virtual and voila. Thankfully, some people are not lazy :-) Later, David S. Miller [EMAIL PROTECTED] - 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/
Re: [linux-usb-devel] Re: SLAB vs. pci_alloc_xxx in usb-uhci patch[RFC: API]
> > > Do lots of drivers need the reverse mapping? It wasn't on my todo list > > > yet. > > > > I am against any API which provides this. It can be extremely > > expensive to do this on some architectures, The implementation I posted needed no architecture-specific knowledge. If cost is the issue, fine -- this makes it finite, (not infinite), and some drivers can eliminate that cost. > >and since the rest > > of the PCI dma API does not provide such an interface neither > > should the pool routines. > > The API I hacked together for uhci.c didn't have this. But it didn't handle the OHCI done-list processing, and we've heard a lot more about pci_*_consistent being needed with OHCI than with UHCI; it's more common on non-Intel architectures. Given that some hardware must return the dma addresses, why should it be a good thing to have an API that doesn't expose the notion of a reverse mapping? At this level -- not the lower level code touching hardware PTEs. - Dave - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
> > > > extern void * > > > > pci_pool_dma_to_cpu (struct pci_pool *pool, dma_addr_t handle); > > > > > > Do lots of drivers need the reverse mapping? It wasn't on my todo list > > > yet. > > > > Some hardware (like OHCI) talks to drivers using those dma handles. > > I wonder if it may be feasible to allocate a bunch of contiguous > pages. Then, whenever the hardware returns a bus address, subtract > the remembered bus address of the zone start, add the offset to > the virtual and voila. That's effectively what the implementation I posted is doing. Simple math ... as soon as you get the right "logical page", and that page size could become a per-pool tunable. Currently one logical page is PAGE_SIZE; there are some issues to deal with in terms of not crossing page boundaries. There can be multiple such pages, known to the pool allocator and hidden from the device drivers. I'd expect most USB host controllers wouldn't allocate more than one or two pages, so the cost of this function would typically be small. - Dave - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
> Date: Fri, 09 Mar 2001 10:29:22 -0800 > From: David Brownell <[EMAIL PROTECTED]> > > > extern void * > > > pci_pool_dma_to_cpu (struct pci_pool *pool, dma_addr_t handle); > > > > Do lots of drivers need the reverse mapping? It wasn't on my todo list > > yet. > > Some hardware (like OHCI) talks to drivers using those dma handles. I wonder if it may be feasible to allocate a bunch of contiguous pages. Then, whenever the hardware returns a bus address, subtract the remembered bus address of the zone start, add the offset to the virtual and voila. -- Pete - 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/
Re: [linux-usb-devel] Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
> Drivers can keep track of this kind of information themselves, > and that is what I tell every driver author to do who complains > of a lack of a "bus_to_virt()" type thing, it's just lazy > programming. I'd agree. There are _good_ reasons for having reverse mappings especially on certain architectures, but thats for stuff like cache management. Its stuff the drivers have no business poking their noses directly into - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
> > unlike the slab allocator bug(s) I pointed out. (And which > > Manfred seems to have gone silent on.) > > which bugs? See my previous email ... its behavior contradicts its spec, and I'd sent a patch. You said you wanted kmalloc to have an "automagic redzoning" feature, which would involve one more change (to the flags used in kmalloc init when magic redzoning is in effect). I'd expected a response. > > * this can easily be optimized, but the best fix would be to > > * make this just a bus-specific front end to mm/slab.c logic. > > > Adding that new frond end was already on my todo list for 2.5, but it > means modifying half of mm/slab.c. Exactly why I think we need a usable solution changing that half! And why I asked for feedback about the API, not a focus on this particular implementation. > > if (align < L1_CACHE_BYTES) > > align = L1_CACHE_BYTES; > > Why? To see who was awake, of course! That shouldn't be there. > > /* Convert a DMA mapping to its cpu address (as returned by pci_pool_alloc). > > * Don't assume this is cheap, although on some platforms it may be simple > > * macros adding a constant to the DMA handle. > > */ > > extern void * > > pci_pool_dma_to_cpu (struct pci_pool *pool, dma_addr_t handle); > > Do lots of drivers need the reverse mapping? It wasn't on my todo list > yet. Some hardware (like OHCI) talks to drivers using those dma handles. - Dave - 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/
Re: [linux-usb-devel] Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
On Fri, Mar 09, 2001, David S. Miller <[EMAIL PROTECTED]> wrote: > Manfred Spraul writes: > > Do lots of drivers need the reverse mapping? It wasn't on my todo list > > yet. > > I am against any API which provides this. It can be extremely > expensive to do this on some architectures, and since the rest > of the PCI dma API does not provide such an interface neither > should the pool routines. The API I hacked together for uhci.c didn't have this. > Drivers can keep track of this kind of information themselves, > and that is what I tell every driver author to do who complains > of a lack of a "bus_to_virt()" type thing, it's just lazy > programming. Once I worked around not having a "bus_to_virt()" type thing I was happier with the resulting code. I completely agree. JE - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
Manfred Spraul writes: > Do lots of drivers need the reverse mapping? It wasn't on my todo list > yet. I am against any API which provides this. It can be extremely expensive to do this on some architectures, and since the rest of the PCI dma API does not provide such an interface neither should the pool routines. Drivers can keep track of this kind of information themselves, and that is what I tell every driver author to do who complains of a lack of a "bus_to_virt()" type thing, it's just lazy programming. Later, David S. Miller [EMAIL PROTECTED] - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch [RFC: API]
David S. Miller writes:
> Russell King writes:
> > A while ago, I looked at what was required to convert the OHCI driver
> > to pci_alloc_consistent, and it turns out that the current interface is
> > highly sub-optimal. It looks good on the face of it, but it _really_
> > does need sub-page allocations to make sense for USB.
> >
> > At the time, I didn't feel like creating a custom sub-allocator just
> > for USB, ...
>
> Gerard Roudier wrote for the sym53c8xx driver the exact thing
> UHCI/OHCI need for this.
I looked at that, and it didn't seem it'd drop in very easily.
I'd be interested in feedback on the API below. It can
handle the USB host controllers (EHCI/OHCI/UHCI) on
currently unsupported hardware (the second bug I noted).
Questions: is it general enough for other drivers to use?
Should I package it as a patch for 2.4?
There's also a simple implementation, which works (to limited
testing) but would IMO better be replaced by something using
the innards of the slab allocator (mm/slab.c). That'd need new
APIs inside that allocator ... those changes seem like 2.5 fixes,
unlike the slab allocator bug(s) I pointed out. (And which
Manfred seems to have gone silent on.)
- Dave
/* LOGICALLY: */
/*
* This works like a pci-dma-consistent kmem_cache_t would, and handles
* alignment guarantees for its (small) memory blocks. Many PCI device
* drivers need such functionality.
*/
struct pci_pool;
/* Create a pool - flags are SLAB_* */
extern struct pci_pool *
pci_pool_create (const char *name, struct pci_dev *pdev,
int size, int align, int flags);
/* Get memory from a pool - flags are GFP_KERNEL or GFP_ATOMIC */
extern void *
pci_pool_alloc (struct pci_pool *pool, int flags, dma_addr_t *handle);
/* Return memory too a pool */
extern void
pci_pool_free (struct pci_pool *pool, void *vaddr, dma_addr_t handle);
/* Destroy a pool */
extern void
pci_pool_destroy (struct pci_pool *pool);
/* Convert a DMA mapping to its cpu address (as returned by pci_pool_alloc).
* Don't assume this is cheap, although on some platforms it may be simple
* macros adding a constant to the DMA handle.
*/
extern void *
pci_pool_dma_to_cpu (struct pci_pool *pool, dma_addr_t handle);
/* LOGICALLY: drivers/linux/pci.c */
/*
There should be some memory pool allocator that this code
can use ... it'd be a layer (just like kmem_cache_alloc and
kmalloc) with an API that exposes PCI devices, dma address
mappings, and hardware alignment requirements.
Until that happens, this quick'n'dirty implementation of the
API should work, and should be fast enough for small pools
that use at most a few pages (and small allocation units).
*/
struct pci_pool {
char name [32];
struct pci_dev *dev;
struct list_head pages;
int size;
int align;
int flags;
int blocks_offset;
int blocks_per_page;
};
/* pci consistent pages allocated in units of LOGICAL_PAGE_SIZE, layout:
* - pci_page (always in the 'slab')
* - bitmap (with blocks_per_page bits)
* - blocks (starting at blocks_offset)
*
* this can easily be optimized, but the best fix would be to
* make this just a bus-specific front end to mm/slab.c logic.
*/
struct pci_page {
struct list_head pages;
dma_addr_t dma;
unsigned long bitmap [0];
};
#define LOGICAL_PAGE_SIZE PAGE_SIZE
/* irq-safe; protects any pool's pages and bitmaps */
static spinlock_t pci_page_lock = SPIN_LOCK_UNLOCKED;
#define POISON_BYTE 0xa5
/**
* pci_pool_create - Creates a pool of pci consistent memory blocks, for dma.
* @name: name of pool, for diagnostics
* @pdev: pci device that will be doing the DMA
* @size: size of the blocks in this pool.
* @align: alignment requirement for blocks; must be a power of two
* @flags: SLAB_* (or GFP_*) flags (not all are supported).
*
* Returns a pci allocation pool with the requested characteristics, or
* null if one can't be created.
*
* Memory from the pool will always be aligned to at least L1_CACHE_BYTES,
* or more strictly if requested. The actual size be larger than requested.
* Such memory will all have "consistent" DMA mappings, accessible by both
* devices and their drivers without using cache flushing primitives.
*
* FIXME: probably need a way to specify "objects must not cross boundaries
* of N Bytes", required by some device interfaces.
*/
extern struct pci_pool *
pci_pool_create (const char *name, struct pci_dev *pdev,
int size, int align, int flags)
{
struct pci_pool *retval;
size_t temp;
if ((LOGICAL_PAGE_SIZE - sizeof (struct pci_page)) < size)
return 0;
if (!(retval = kmalloc (sizeof *retval, flags)))
return retval;
if (align < L1_CACHE_BYTES)
align = L1_CACHE_BYTES;
if (size < align)
size = align;
else if (size % align)
size = (size + align - 1) & ~(align - 1);
strncpy (retval->name, name, sizeof retval->name);
retval->name [sizeof retval->name - 1] = 0;
retval->dev = pdev;
INIT_LIST_HEAD (&retval->pages);
retval->size = size;
ret
RE: [linux-usb-devel] Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
Are there any large-memory machines that need pci_alloc_consistent() in the USB controller driver? If not, then let's just set up an uncached mapping of all of DRAM and use a modified version of virt_to_bus and bus_to_virt. It gets around all the issues of having a better allocator of uncached memory. Even with a slab allocator for uncached memory, modified versions of virt_to_bus and bus_to_virt would have to be used. The other choice is cache invalidation and flushing around all the code that touches ED's and TD's. This might not be too bad if it is encapsulated in _read and _write macros. How performance critical is the ED and TD code? The latest ARM -NP patches work on SA1110 with the SA controller (which is OHCI-like). With minimal tweaks they work on SA110/Footbridge with an OHCI controller. We will be submitting these patches as tweaks. It's not beautiful, but it doesn't change the mainstream (x86) code significantly. Jamey - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
> > (1) CONFIG_SLAB_DEBUG breaks the documented > > requirement that the slab cache return adequately aligned > > data ... > > adequately aligned for the _cpu_, not for some controllers. It's neither > documented that HW_CACHEALIGN aligns to 16 byte boundaries It's documented in mm/slab.c (line 612 in my ac7+tweaks). "Aligns to hardware cache line." The only cacheline define I've found is in for L1_CACHE_BYTES, which is often more than 16 (not on x86). If you don't like the patch I forwarded, then please submit one that changes the kmem_cache_create() API spec ... one or the other is needed. (I'd not change that API!!) > nor that kmalloc uses HW_CACHEALIGN. ... > > > + /* redzoning would break cache alignment requirements */ > > + if (flags & SLAB_HWCACHE_ALIGN) > > + flags &= ~SLAB_RED_ZONE; > > The problem is that you've just disabled red zoning for kmalloc. And > kmalloc is the only case where redzoning is important: ... If kmalloc wants to get auto redzoning, then I think it shouldn't be using SLAB_HWCACHE_ALIGN when CONFIG_SLAB_DEBUG. That'd be another simple fix. (Or, make it use some new flag that's just a performance hint that can be ignored for debugging.) > I think everyone agrees that (2) correct fix. I was saying that there were two bugs (two fixes needed), and you're saying that there's only one ... despite the evidence of the API spec. But you could persuade me there's a third bug: kmalloc misuse of that kmem_cache API. > I see 2 temporary workarounds: either your patch or > > + #ifdef CONFIG_SLAB_DEBUG > + #error > + #endif > > in uhci.c. Better in linux/drivers/usb/Config.in instead. All the host controller drivers rely on the kmem_cache_create() API spec to be followed. (Even the OHCI driver, when using kmalloc not kmem_cache.) - Dave - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
David Brownell wrote: > > There are two problems I see. > > (1) CONFIG_SLAB_DEBUG breaks the documented > requirement that the slab cache return adequately aligned > data ... adequately aligned for the _cpu_, not for some controllers. It's neither documented that HW_CACHEALIGN aligns to 16 byte boundaries nor that kmalloc uses HW_CACHEALIGN. > which the appended patch should probably handle > nicely (something like it sure did :-) and with less danger > than the large patch you posted. > > (2) The USB host controller drivers all need something > like a pci_consistent slab cache, which doesn't currently > exist. I have something like that in the works, and David > Miller noted one driver that I may steal from. > > - Dave > > --- slab.c-orig Tue Mar 6 15:01:26 2001 > +++ slab.c Tue Mar 6 15:05:58 2001 > @@ -676,12 +676,10 @@ > } > > #if DEBUG > + /* redzoning would break cache alignment requirements */ > + if (flags & SLAB_HWCACHE_ALIGN) > + flags &= ~SLAB_RED_ZONE; The problem is that you've just disabled red zoning for kmalloc. And kmalloc is the only case where redzoning is important: If a caller uses kmem_cache_alloc() for a structure then he won't write beyond the end of the structure. I think everyone agrees that (2) correct fix. I see 2 temporary workarounds: either your patch or + #ifdef CONFIG_SLAB_DEBUG + #error + #endif in uhci.c. -- Manfred - 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/
Re: [linux-usb-devel] Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
> > At the time, I didn't feel like creating a custom sub-allocator just > > for USB, ... > > > > I'd be good to get it done "properly" at some point though. > > Something like > > struct pci_pool *pci_alloc_consistent_pool(int objectsize, int align) struct pci_pool * pci_create_consistent_pool (struct pci_dev *dev, int size, int align) and similar for freeing the pool ... pci_alloc_consistent() needs the device, presumably since some devices may need to dma into specific memory. I'd probably want at least "flags" from kmem_cache_create(). > pci_alloc_pool_consistent(pool,.. > pci_free_pool_consistent(pool,.. These should have signatures just like pci_alloc_consistent() and pci_free_consistent() except they take the pci_pool, not a pci_dev. Oh, and likely GFP_ flags to control blocking. > Where the pool allocator does page grabbing and chaining Given an agreement on API, I suspect Johannes' patch could get quickly generalized. Then debugging support (like in slab.c) could be added later. - Dave - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
> Anyways, is this the end of the discussion regarding my patch?
I think one of the maintainers for usb-uhci (Georg) said he'd
want the general fix ...
> Manfred said plainly "usb-uhci is broken", Alan kinda
> manuevered around my small problem, Dave Brownell looks
> unconvinced. So?
There are two problems I see.
(1) CONFIG_SLAB_DEBUG breaks the documented
requirement that the slab cache return adequately aligned
data ... which the appended patch should probably handle
nicely (something like it sure did :-) and with less danger
than the large patch you posted.
(2) The USB host controller drivers all need something
like a pci_consistent slab cache, which doesn't currently
exist. I have something like that in the works, and David
Miller noted one driver that I may steal from.
- Dave
--- slab.c-orig Tue Mar 6 15:01:26 2001
+++ slab.c Tue Mar 6 15:05:58 2001
@@ -676,12 +676,10 @@
}
#if DEBUG
+ /* redzoning would break cache alignment requirements */
+ if (flags & SLAB_HWCACHE_ALIGN)
+ flags &= ~SLAB_RED_ZONE;
if (flags & SLAB_RED_ZONE) {
- /*
- * There is no point trying to honour cache alignment
- * when redzoning.
- */
- flags &= ~SLAB_HWCACHE_ALIGN;
size += 2*BYTES_PER_WORD; /* words for redzone */
}
#endif
-
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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
> From: "David S. Miller" <[EMAIL PROTECTED]> > Date: Mon, 5 Mar 2001 20:53:21 -0800 (PST) >[...] > Gerard Roudier wrote for the sym53c8xx driver the exact thing > UHCI/OHCI need for this. Thanks for the hint. *** Anyways, is this the end of the discussion regarding my patch? If it goes in, then fine. If not, fine too... Just let me know so that I can close the bug properly. Manfred said plainly "usb-uhci is broken", Alan kinda manuevered around my small problem, Dave Brownell looks unconvinced. So? -- Pete - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
Russell King writes: > A while ago, I looked at what was required to convert the OHCI driver > to pci_alloc_consistent, and it turns out that the current interface is > highly sub-optimal. It looks good on the face of it, but it _really_ > does need sub-page allocations to make sense for USB. > > At the time, I didn't feel like creating a custom sub-allocator just > for USB, and since then I haven't had the inclination nor motivation > to go back to trying to get my USB mouse or iPAQ communicating via USB. > (I've not used this USB port for 3 years anyway). Gerard Roudier wrote for the sym53c8xx driver the exact thing UHCI/OHCI need for this. I think people are pissing their pants over the pci_alloc_consistent interface for no reason. It gives PAGE
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
> At the time, I didn't feel like creating a custom sub-allocator just > for USB, and since then I haven't had the inclination nor motivation > to go back to trying to get my USB mouse or iPAQ communicating via USB. > (I've not used this USB port for 3 years anyway). > > I'd be good to get it done "properly" at some point though. Something like struct pci_pool *pci_alloc_consistent_pool(int objectsize, int align) pci_alloc_pool_consistent(pool,.. pci_free_pool_consistent(pool,.. ?? Where the pool allocator does page grabbing and chaining > - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
On Mon, Mar 05, 2001 at 02:52:24PM -0800, David Brownell wrote: > I didn't see that thread. I agree, pci_alloc_consistent() already has > a signature that's up to the job. The change you suggest would need > to affect every architecture's implementation of that code ... which > somehow seems not the best solution at this time. Needless to say that USB is currently broken for the architectures that need pci_alloc_consistent. A while ago, I looked at what was required to convert the OHCI driver to pci_alloc_consistent, and it turns out that the current interface is highly sub-optimal. It looks good on the face of it, but it _really_ does need sub-page allocations to make sense for USB. At the time, I didn't feel like creating a custom sub-allocator just for USB, and since then I haven't had the inclination nor motivation to go back to trying to get my USB mouse or iPAQ communicating via USB. (I've not used this USB port for 3 years anyway). I'd be good to get it done "properly" at some point though. -- Russell King ([EMAIL PROTECTED])The developer of ARM Linux http://www.arm.linux.org.uk/personal/aboutme.html - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
> > And mm/slab.c changes semantics when CONFIG_SLAB_DEBUG > > is set: it ignores SLAB_HWCACHE_ALIGN. That seems more like > > the root cause of the problem to me! > > HWCACHE_ALIGN does not guarantee a certain byte alignment. And > additionally it's not even guaranteed that kmalloc() uses that > HWCACHE_ALIGN. > Uhci is broken, not my slab code ;-) If so, the slab code docs/specs are broken too ... I just checked in my development tree (2.4.2-ac7 plus goodies) and what's written is that HWCACHE_ALIGN is to "Align the objects in this cache to a hardware cacheline." Which contradicts what you just wrote; it's supposed to always align, not do so only when convenient. Clearly, something in mm/slab.c needs to change even if it's just changing the spec for kmem_cache_create() behavior. > I'd switch to pci_alloc_consistent with some optimizations to avoid > wasting a complete page for each DMA header. (I haven't seen Johannes > patch, but we discussed the problem 6 weeks ago and that proposal was > the end of the thread) I didn't see that thread. I agree, pci_alloc_consistent() already has a signature that's up to the job. The change you suggest would need to affect every architecture's implementation of that code ... which somehow seems not the best solution at this time. Perhaps we should have different functions for pci consistent alloc at the "hardware" level (what we have now) and at the "slab" level. That's sort of what Johannes' patch did, except it was specific to that one driver rather than being a generic utility. Of course, I'd rather that such a generic package have all the debug support (except broken redzoning :-) that the current slab code does. - Dave - 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/
Re: SLAB vs. pci_alloc_xxx in usb-uhci patch
> And mm/slab.c changes semantics when CONFIG_SLAB_DEBUG > is set: it ignores SLAB_HWCACHE_ALIGN. That seems more like > the root cause of the problem to me! > HWCACHE_ALIGN does not guarantee a certain byte alignment. And additionally it's not even guaranteed that kmalloc() uses that HWCACHE_ALIGN. Uhci is broken, not my slab code ;-) > I think that the pci_alloc_consistent patch that Johannes sent >by for "uhci.c" would be a better approach. Though I'd like >to see that be more general ... say, making mm/slab.c know >about such things. Add a simple abstraction, and that should >be it -- right? :-) I looked at it, and there are 2 problems that make it virtually impossible to integrate kmem_cache_alloc() with pci memory alloc without a major redesign: * pci_alloc_consistent returns 2 values, kmem_cache_alloc() only one. This one would be possible to work around. * the slab allocator heavily relies on the 'struct page' structure, but it's not guaranteed that it exists for pci_alloced memory. I'd switch to pci_alloc_consistent with some optimizations to avoid wasting a complete page for each DMA header. (I haven't seen Johannes patch, but we discussed the problem 6 weeks ago and that proposal was the end of the thread) -- Manfred - 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/
SLAB vs. pci_alloc_xxx in usb-uhci patch
Hi, everyone:
When I turn FORCE_DEBUG on in mm/slab.c, usb-uhci driver stops
working. It turned out that DMA headers must be aligned on 16.
Slab poisoning violates that assumption.
I have come up with a fix which USB folks did not like, but
they did not object to discussion on linux-kernel. Here is it.
The current code does something like this:
struct dmahdr {
__u32 head;
};
struct desc {
struct dmahdr h;
struct desc *next;
int last_used;
};
struct desc *p;
unsigned long busaddr;
p = kmalloc_z(sizeof(struct desc), GFP_SOMETHING);
busaddr = virt_to_bus(p);
writel(busaddr, ioremap_cookie + UHCI_SOME_REGISTER);
I changed it to this:
struct dmahdr {
__u32 head;
};
struct desc {
struct dmahdr *hp;
struct desc *next;
int last_used;
};
struct desc *p;
void *dp;
unsigned long busaddr;
p = kmalloc_z(sizeof(struct desc), GFP_SOMETHING);
dp = kmalloc_z(sizeof(struct dmahdr) + 15, GFP_SOMETHING);
dp = (dp + 15) & ~15;
p->hp = dp;
busaddr = virt_to_bus(p->hp);
writel(busaddr, ioremap_cookie + UHCI_SOME_REGISTER);
This way, after testing, we may replace second kmalloc and virt_to_bus
with pci_alloc_consistent.
Actual patch is more complicated due to checks, zeroing, and so on.
It is attached below.
Here is a mail that sums up the disagreement:
Date: Sun, 04 Mar 2001 13:10:26 -0800
From: David Brownell <[EMAIL PROTECTED]>
Subject: Re: [linux-usb-devel] Patch for usb-uhci and poisoned slab (2.4.2-ac7)
To: Peter Zaitcev <[EMAIL PROTECTED]>
Cc: [EMAIL PROTECTED]
> I found that usb-uhci fails when FORCE_DEBUG is set in mm/slab.c
> because it expects 16 bytes alignment for structures it allocates.
And mm/slab.c changes semantics when CONFIG_SLAB_DEBUG
is set: it ignores SLAB_HWCACHE_ALIGN. That seems more like
the root cause of the problem to me!
It's a lot simpler to patch mm/slab.c so its semantics don't change.
That is, don't resolve clashes between HWCACHE_ALIGN and
automagic redzoning in favor of redzoning any more.
> I did not go all the way to using pci_alloc_single,
> pci_alloc_consistent and friends, because I am not too sure
> in my hand, and also because I believe in gradual change
> (in this case).
That big a patch is rather non-gradual ... :-)
I think that the pci_alloc_consistent patch that Johannes sent
by for "uhci.c" would be a better approach. Though I'd like
to see that be more general ... say, making mm/slab.c know
about such things. Add a simple abstraction, and that should
be it -- right? :-)
- Dave
I see the Dave's argument as 1. Slab must honor HWCACHE_ALIGN
when debugged; 2. My patch is too big for too little gain.
It is understandable, but I find it hard to agree. First,
mixing the controller imposed alignment with cache alignment
is quite misleading. Second, is more "phylosophical" - yes
I can work without global poisoning. I just do want to use it.
I fancy forced slab poisoning, is it so wrong?
I need our benevolent dictatiorship to judge. Or else ... umm...
well, I guess, nothing will happen, we would just have broken
code as we always did :0
-- Pete
diff -ur -X ../dontdiff linux-2.4.2-ac7/drivers/usb/usb-uhci-debug.h
linux-2.4.2-ac7-p3/drivers/usb/usb-uhci-debug.h
--- linux-2.4.2-ac7/drivers/usb/usb-uhci-debug.hSat Jul 8 19:38:16 2000
+++ linux-2.4.2-ac7-p3/drivers/usb/usb-uhci-debug.h Sat Mar 3 11:27:45 2001
@@ -1,25 +1,25 @@
-#ifdef DEBUG
+#ifdef DEBUG_DUMP
static void __attribute__((__unused__)) uhci_show_qh (puhci_desc_t qh)
{
if (qh->type != QH_TYPE) {
dbg("qh has not QH_TYPE");
return;
}
- dbg("QH @ %p/%08lX:", qh, virt_to_bus (qh));
+ dbg("QH @ %p->%p/%08lX:", qh, qh->hwp, virt_to_bus (qh->hwp));
- if (qh->hw.qh.head & UHCI_PTR_TERM)
+ if (qh->hwp->qh.head & UHCI_PTR_TERM)
dbg("Head Terminate");
else
dbg("Head: %s @ %08X",
- (qh->hw.qh.head & UHCI_PTR_QH?"QH":"TD"),
- qh->hw.qh.head & ~UHCI_PTR_BITS);
+ (qh->hwp->qh.head & UHCI_PTR_QH?"QH":"TD"),
+ qh->hwp->qh.head & ~UHCI_PTR_BITS);
- if (qh->hw.qh.element & UHCI_PTR_TERM)
+ if (qh->hwp->qh.element & UHCI_PTR_TERM)
dbg("Element Terminate");
else
dbg("Element: %s @ %08X",
- (qh->hw.qh.element & UHCI_PTR_QH?"QH":"TD"),
- qh->hw.qh.element & ~UHCI_PTR_BITS);
+ (qh->hwp->qh.element & UHCI_PTR_QH?"QH":"TD"),
+ qh->hwp->qh.element & ~UHCI_PTR_BITS);
}
#endif
@@ -27,7 +27,7 @@
{
char *spid;
- switch (td->hw.td.info & 0xff) {
+ switch (td->hwp->td.info & 0xff) {
case USB_PID_SETUP:
spid = "SETUP";
break;
@@ -42,50 +42,52 @@
