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-nakah...@iij.ad.jp> > > 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-nakah...@iij.ad.jp>
