On Tue, Jul 16, 2002 at 07:22:53PM -0700, cgd@broadcom.com wrote:

 > So... I think that might be a mistake.  Specifically, now there's no
 > way to have a single 'copy' operation gather disjoint chunks into a
 > contiguous destination buffer, as one might want for, say, coalescing
 > packets for a network interface w/ "less-than-desirable" alignment
 > constraints.

Ok, I added it back.  (I also discovered I needed it for doing
userland access in a sane fashion, though not for the reasons you
might think.)

 > Comments on the new document:
 > 
 > * intro: you might want to enunciate the notion of 'zero or more input
 >   regions', 'one output region', and 'same lengths for each.' here.

Ok, added text there.

 > * DMOVER_REQ_WAIT: You probably want an additional flag which forces
 >   the processing code to spin, rather than sleeping.  Horrible, yes,
 >   but desirable if you're using it in a place where you can't sleep
 >   but do know that the data mover will be substantially faster or
 >   provide other benefits as compared to a CPU-based copy.  (an example
 >   of this might be a pmap zero page routine.)

The restriction, of course, is that that can never be called from an
interrupt handler.  Any thoughts on a name?  Where should the spinwait
be done?  In the back-end or the midlayer?

 > * so, all these immediate value things...  should probably be a union,
 >   or even just a single 64-bit type w/ e.g. fill8 specified to use
 >   only the low byte value.  maybe if you want to do something even
 >   better (i heard you talking about byte order problems), maybe an
 >   array of bytes, and specify how they're copied (and that fill8 uses
 >   the first one, fill16 uses the first 2, etc.)

In the code, they are all union'd together.  I've been considering making
it:

	uint8_t dreq_imm[8];

to eliminate any confusion about byte ordering.  Thoughts?

 > * There's still some things missing in session method specification
 >   interface, from where I sit.  e.g., you might want to specify the
 >   expected likelyhood of (lack of) reuse of the data, either via
 >   specifying a vague "locality" value (say, 0-3), or by providing
 >   explicit flags like, "try to avoid putting this data into l1 cache,
 >   l2 cache, etc."  The latter type of thing may be translated into,
 >   say, flags on HW requests by a hardware back-end, or specific
 >   prefetch ops or a specific pattern of accesses by a SW back-end.
 > 
 >   Maybe you could say "copy-<something>", but after you have a flag or
 >   two that really does get tedious.

Yah, this would be nice.  I'll see what I can come up with.

 > * copy should be defined to not operate on overlapping regions.  more
 >   generally, methods should be "restartable" in case of HW removal and
 >   the need to re-assign the request to a different back-end.

Yah, I've added a bunch of verbosity about this.

 > * can dmover_request_alloc fail?

Hm, yes.  Guess I should mention that in the manpage :-)

 > * Is it the only interface by which one can get request structures, or
 >   may they be allocated by other means?  if the latter is OK, specify
 >   that they must be zeroed so that we can add things in the future w/o
 >   too much pain.  (e.g., priority, but i don't know if that should go
 >   on session or request.)

Well, nothing frees the request structures implicitly, so certainly clients
could provide their own.  However, I'm trying to keep ABI considerations
in mind.  I suppose I could note the ABI consideration in the document, and
explicitly allow callers to provide their own request structures if they
want to do so.

 > * assymetry between dmover_request_alloc's allocation of input buffers
 >   and dmover_request_free's freeing of them is ... dangerous.  (i.e.,
 >   to get it to alloc you pass NULL, to get it to free you have to pass
 >   the allocated ptr!)  Perhaps: pass inbuf to dmover_request_alloc to
 >   use pre-existing buffer, else it will allocate.  if you provide
 >   buffer, you must dispose of it yourself.  if it allocates, it sets
 >   flag in request and deallocates on free.  don't take buffer ptr in
 >   free.  if free free's something passed in arbitrarily, you'll lose
 >   due to malloc types, too.

Yah, I'll clean this up.

 > * what happens if you destroy a session while requests are active?
 >   What happes if you free a request while it is active?

Right now, it panics.  I haven't addressed that yet.

 > * puzzled by description of use from interrupt context...  you can use
 >   it, at, say, IPL_BIO and IPL_SOFTNET, but not IPL_NET (between the
 >   two)?  does this mean from interrupt context caused by interrupts of
 >   those types, or called under splFOO()?  (IPL == priority level, not
 >   "context," but you knew that.  8-)

Ah, I need to document that interrupt handlers for hardware back-ends
should be registered at IPL_BIO.  The intent is to allow various routines
to be called from handlers at IPL_BIO, IPL_SOFTNET, or IPL_SOFTCLOCK.  I've
changed the wording here.

 > * should probably say that dmover_algdesc, dmover_backend should have
 >   all unspecified fields zero-initted (initted in data section, or
 >   bzero'd before filling in the known ones) so that things can be
 >   added later safely.

Okay.

 > * if you do that, not sure what point there is to have the dmb_speed
 >   there now, since it's not used for much...

Well, right now, it's used to cause my test system to prefer the
hardware mover over the software one.

-- 
        -- Jason R. Thorpe <thorpej@wasabisystems.com>