On Mar 1, 2005, at 7:12 PM, Bill Studenmund wrote:

> I think it'd be better to just return the chain. While you're right 
> that
> chances are we will just drop the packet, I think there are times we 
> may
> want to do something different with it. Yes, that would be using this
> routine for something other than fixing up DMA segments... If we always
> free the chain on error, it'd be very hard to support doing something 
> with
> the chain.

Actually, I've re-examined how driver *_start() routines typically work
and they do need to do 'something different' that free mbuf chain
right away.

Typical pattern is:

	for(;;) {
		IFQ_POLL(..., &m0);
		if (m0 == NULL)
			break;

		/* check if there is enough of free DMA descriptors */
		...

		error = bus_dmamap_load_mbuf();
		if (error) {
			if (error == EFBIG) {
				/* unable to load, too many DMA segments */
				/* DROP MBUF CHAIN */
				IFQ_DEQUEUE(..., m0);
				m_freem(m0);
				break;
			}

			/* Short on resources, just stop for now. */
			break;
		}

		IFQ_DEQUEUE(&ifp->if_snd, m0);

		/*
		 * WE ARE NOW COMMITTED TO TRANSMITTING THE PACKET.
		 */

		/* modify device structures to send the packet */
		...
	}

If m_defrag() were to be called at at the 'DROP MBUF CHAIN' place and free
the old chain, later IFQ_DEQUEUE() would use stale pointer.

I also misread the semantics of the current FreeBSD m_defrag(), thought
it frees the old mbuf chain :)

Given this, I propose following for NetBSD m_defrag():
* signature struct mbuf *m_defrag(struct mbuf *m0, int flags, int nseg_goal)
* function returns defragmented mbuf chain on success, or NULL on failure 
* function fails if there is not enough memory to allocate new mbuf chain
  memory (iff M_NOWAIT) or if it would not be able to meet the nseg_goal
  fragmentation goal
* success means the function has defragmented the mbuf chain
  into <= nseg_goal continuous virtual memory segments, and the defragmented
  virtual memory segments cross the PAGE_SIZE boundary maximum
  (nseg_goal - 1) times; this is to guarantee the result consists
  of nseg_goal physical pages maximum
* old mbuf chain kept intact in either success or error case

Usage pattern would be:

	error = bus_dmamap_load_mbuf(...);
	if (error) {
		if (error == EFBIG) {
			m_new = m_defrag(m0, M_NOWAIT, STGE_MAXSEGS);
			if (m_new == NULL
			    || (error = bus_dmamap_load_mbuf(...,m_new)) ) {
				/* too many DMA segments and cannot defrag */
				if (m_new)
					m_freem(m_new);
				IFQ_DEQUEUE(..., m0);
				m_freem(m0);
				break;
			}

			/* do something do use m_new instead m0 */
		}
	}

Does this sound right?

Jaromir
-- 
Jaromir Dolecek <jdolecek@NetBSD.org>            http://www.NetBSD.cz/
-=- We can walk our road together if our goals are all the same;     -=-
-=- We can run alone and free if we pursue a different aim.          -=-