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

[Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>]

Hi,

Thank you for comments.

On 2016/03/25 23:05, Taylor R Campbell wrote:
>    Date: Fri, 25 Mar 2016 17:26:14 +0900
>    From: Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>
> 
>    I rebase and modify a little(use container_of). Here is the patch.
>        http://www.netbsd.org/~knakahara/ifq-enqueue-refactor/ifq-enqueue-refactor.patch
> 
>    Does anyone have Any comments? Any comments are welcome.
> 
> Minor nit in altq_m_tag_get: KNF style is `return foo;', not `return
> (foo);'.

Thanks, I fix it. There are similar issues in altq_subr.c, I will fix
them where I modify around them.

> altq_get_pattr should use container_of too, not pointer arithmetic:
> 
> 	mtag = m_tag_find(m, PACKET_TAG_ALTQ_PATTR, NULL);
> 	if (mtag != NULL) {
> 		struct altq_pktattr_tag *apt =
> 		    container_of(mtag, struct altq_pktattr_tag, apt_tag);
> 		return &apt->apt_pktattr;
> 	}
> 
> 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.
    - blue_enqueue()@sys/altq/altq_blue.c
    - cbq_enqueue()@sys/altq/altq_cbq.c
    - fifoq_enqueue()@sys/altq/altq_fifoq.c
    - hfsc_enqueue()@sys/altq/altq_hfsc.c
    - jobs_enqueue()@sys/altq/altq_jobs.c
    - priq_enqueue()@sys/altq/altq_priq.c
    - red_enqueue()@sys/altq/altq_red.c
    - rio_enqueue()@sys/altq/altq_rio.c
    - wfq_ifenqueue()@sys/altq/altq_wfq.c

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.


> Why add struct ifnet::if_altq_classify?  Why not just teach
> altq_etherclassify to call altq_set_pattr?  From the comments above
> altq_etherclassify, it sounds like it's meant to be a temporary
> workaround for a deficiency in altq.  Especially the just-in-time
> assignment of ifp->if_altq_classify in ieee80211_deliver_data seems
> wrong to me.

You are quite right. I revert the modification about
ifnet::if_altq_classify and fix altq_etherclassify() to use
altq_m_tag_get().

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

Could you comment it?
And, does anyone have any comments? Any comments are welcome.


Thanks,

-- 
//////////////////////////////////////////////////////////////////////
Internet Initiative Japan Inc.

Device Engineering Section,
Core Product Development Department,
Product Division,
Technology Unit

Kengo NAKAHARA <k-nakahara%iij.ad.jp@localhost>