Re: IFQ_ENQUEUE argument refactor (was Re: RFC: ALTQ caller refactor)

[Taylor R Campbell <campbell+netbsd-tech-kern%mumble.net@localhost>]

   Date: Tue, 29 Mar 2016 17:45:20 +0900
   From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>

   On 2016/03/25 23:05, Taylor R Campbell wrote:
   > Either or altq_get_pattr or IFQ_ENQUEUE should KASSERT(pattr != NULL)
   > or similar.

   Hmm, I think the functions which are set to ifq->altq_enqueue by
   altq_attach() can accept NULL and handle it. The functions are following.
   [...]

   So, I think KASSERT may be too strict. Could you comment it?
   BTW, it is hard to know whether altq_m_tag_get() succeed or not in
   above patch, so I add some logs when altq_m_tag_get() failed.

You're right -- I don't know what I was thinking when I said you
should kassert.  Instead of an aprint, how about a dtrace probe?

   Here is the updated patch.
       http://www.netbsd.org/~knakahara/ifq-enqueue-refactor/ifq-enqueue-refactor-2.patch

   Could you comment it?

altq_get_pattr still seems to use pointer arithmetic:

	return (struct altq_pktattr *)(mtag + 1);

Is there a reason it does this instead of using container_of like I
suggested in my last message?

Hmm...  It looks like there are some paths out of IFQ_CLASSIFY that do
not lead to IFQ_ENQUEUE, which would cause a memory leak.  For
example, in atm_output, IFQ_CLASSIFY is unconditional, but every
`senderr' branch ends in `goto bad', which skips ifq_enqueue.  When
the pktattrs were stack-allocated that was OK, but now we need to
explicitly release them with altq_delete_pattr.

Except maybe it won't be a memory leak -- maybe it will attempt to
free(9) a tag that was allocated with pool_cache(9), via m_freem ->
MFREE -> m_tag_delete_chain -> m_tag_delete -> m_tag_free -> free.
When all tags were created with m_tag_get that was correct, but now it
is necessary to clean them up explicitly.