Re: A new network interface send queue API

[Greg Troxel <gdt%lexort.com@localhost>]

Jason Thorpe <thorpej%me.com@localhost> writes:

I have only quibbles and the odds are good it's more an explication than
real, or that the fine points I bring up are not worth it.

> ==> void ifq_fini(struct ifqueue *);
> Finalize and tear down an interface queue.  For ifp->if_snd, this is done
> for you in if_detach().

Presumably this can sleep and involves deleting all packets and waiting
for locks.

> ==> int ifq_put(struct ifqueue *, struct mbuf *);
> Put a packet into the interface queue.  This is the equivalent of the
> old IFQ_ENQUEUE().  Returns 0 on success or an error code indicating
> the mode of failure (ENOBUFS is the queue is full).  This function always
> consumes the packet (either places it in the queue or frees it if an
> error occurs).

I do not see a 'is queue full', and that feels like a top-level
architectural decision that we don't want queueing or flow control ahead
fo the queue.  But we could add a function later (put that didn't free
on error?) later if necessary and probably it isn't.

> ==> struct mbuf *ifq_stage(struct ifqueue *);
> Stage a packet for output.  This is the rough equivalent of IFQ_POLL(),
> but it makes some additional guarantees.  Namely, ifq_stage() is guaranteed
> to return the same mbuf each time it is called, no matter what queueing
> discipline the ifqueue uses, until one of 3 things happens: the packet
> is committed, aborted, or re-staged (see below).  ifq_get() (see below)
> will also return the currently-staged packet before dipping into the
> queue, if one exists.  As noted above, if the ifqueue state is
> IFQ_STATE_BUSY and there are no more packets (staged or otherwise)
> in the queue, ifq_stage() will atomically set the state to IFQ_STATE_READY
> and return NULL.

I don't like 'stage' as a word, but I realize that's a messy
discussion.  The point here is two-fold

  The queue must select the next packet, if it hasn't.  For more than
  FIFO this could be complicated (priority, rate limits by class), but
  it's work that needs to be done.  Once selected that choice is frozen.

  Return a pointer to the packet which can be read, but consider that
  the packet is still in the queue, both in terms of the queue owning
  the storage, and in terms of the packet counting towards logical queue
  occupancy.

To me the point is "ifq_peek" with a rule that once a packet has been
passed to peek, it's fixed.

I would then want to see

  ifq_peek_reset (bad name)

to tell the queue: you are relieved of the requirement to return the
same packet.   But maybe there is no need.

> ==> void ifq_restage(struct ifqueue *, struct mbuf *);
> "Re-stage" the currently-staged packet with a new one.  This is the
> replacement for prior uses of IF_PREPEND().  A packet must already
> be staged in the ifqueue.  Once the new packet has taken the place
> of the old one, the old packet will be freed.  It is OK to call
> ifq_restage() with the pointer to the currently-staged packet; this
> case is detected and treated as a no-op.

This allows the driver to change the contents.   I don't understand why
you'd want to do this, vs just say "sorry, I peeked and I can't cope
now".

> ==> struct mbuf *ifq_get(struct ifqueue *);
> Get a packet from the queue.  This is the equivalent of the old IFQ_DEQUEUE(),
> with the caveat that if a packet has been staged, it will be returned before
> ifq_get() dips into the queue.  ifq_get(), like ifq_stage(), will perform the
> state transition from IFQ_STATE_BUSY to IFQ_STATE_READY, under the same
> conditions.

I don't like this language about "into the queue".  I guess there is a
fundamental issue not clear about whether the staged packet is in the
queue or it isn't.  Presumably ifq_fini would free it, so I say it's in.

This staging concept is a requirement on queueing disciplines to have
the ability to pick a next packet within the queue and maintain that
choice.  Clealry one can do this with a queue and an on-deck slot, but
if you're tols the queue is 10 packets, that's a 9 packet queue and the
on-deck slot.

> ==> void ifq_commit(struct ifqueue *);
> Commits the currently-staged packet, freeing up the staging area for another
> packet.  The ifq_stage() / ifq_commit() combination is the rough equivalent
> of the existing IFQ_POLL() / IFQ_DEQUEUE() pattern.  The caller is responsible
> for freeing the packet once the transmission has completed or the mbuf
> is otherwise no longer needed.  Typically, for a network interface that is
> doing DMA directly from the mbuf, the packet will be freed in the interrupt
> handler.

How is this different from just calling ifq_get?  Is it an error to
  ifq_stage
  ifq_get
or is that ok?  It could be ifq_peek_consume()  to be clearly the 2nd
half of peek where you take it.

Presumably ifq_commit time is the timestamp that should be used for
updating rate-based queueing data, as the sent time.  Even though which
packets were eligible was assessed at _stage time.

This says "frees up the staging area for another packet".  Clearly a
queueing discipline can make decisions when it wants.  Perhaps instead

  Removes the currently-staged packet from the queue.  An
  ifq_stage/ifq_commit pair is equivalent to ifq_get.  After ifq_commit
  the staging area is empty.  (A queue must choose a packet for the
  staging area when ifq_stage is called again, and is of course free to
  make decisions at other times.)
 

> ==> void ifq_abort(struct ifqueue *);
> This function is intended to be used when transmission of the packet has
> encountered a fatal error.  The packet is removed from the staging area
> and freed.

Do we expect this?  Is this "something is wrong with this packet?" vs
resources?   How is this different from ifq_commit?