Hi, Thank you for comments.
On 2016/03/04 12:23, Taylor R Campbell wrote: > Date: Fri, 4 Mar 2016 12:01:52 +0900 > From: Kengo NAKAHARA <[email protected]> > > On 2016/02/23 13:52, Kengo NAKAHARA wrote: > > I want to uniform IFQ_ENQUEUE arguments whether ALTQ is defined or not. > > # And I also want to eliminate ALTQ_DECL and ALTQ_COMMA macros... > > So, I make struct altq_pktattr from IFQ_CLASSIFY and IFQ_ENQUEUE > argument > > to m_tag. Here is the patch. > > > > [...] > > > > Could you comment this patch? Any comments are welcome. In particular, > > comments compared with the following thread. > > http://mail-index.netbsd.org/tech-kern/2016/02/19/msg020238.html > > There is no objection for about two weeks. Can I commit above patch? > > If there is no objection, I will commit it after a few days or a week. > > One of the questions from the thread you cited is whether you can put > it in the mbuf packet header instead of using an m_tag. I have no > opinion on that, but it would be nice to address that question. Hmm, I feel it is excessive to put mbuf packet. My main intention is not increasing ALTQ performance but just removing pattr argument from IFQ_ENQUEUE. Hmm..., maybe I don't have a sense... > One comment on the patch after a quick glance: > > + altq_pattr_cache = pool_cache_init(sizeof(struct altq_pktattr) > + + sizeof(struct m_tag), > + 0, 0, 0, "altqmtag", NULL, IPL_NET, NULL, NULL, NULL); > > The addition of sizes here may not necessarily work. In particular, > the pointer returned from pool_cache_get will be aligned, but adding > sizeof(struct altq_pktattr) to that pointer does not necessarily give > an aligned result: > > p = pool_cache_get(altq_pattr_cache, ...); > pktattr = p; > tag = (char *)p + sizeof(struct altq_pktattr); > > E.g., if struct altq_pktattr contains a single char member, then > sizeof(struct altq_pktattr) will be 1, but if struct m_tag starts with > a pointer, then you need padding between the char and the pointer. > > If you want to do this, you should create a structure containing both > objects: > > struct altq_pktattr_tag { > struct altq_pktattr apt_pktattr; > struct m_tag apt_tag; > }; > > Then use the members of that structure: > > p = pool_cache_get(altq_pattr_cache, ...); > pktattr = &p->apt_pktattr; > tag = &p->apt_tag; Hmm..., sorry, I am confused. Just to confirm, m_tag needs metadata (struct m_tag) in the front of m_tag data(e.g. in this case struct altq_pktqttr), so both struct m_tag and sturct altq_pktattr are necessary, I think. It is similar to malloc(len + sizeof(struct m_tag)) in m_tag_get(). Could you tell me about the reason which the addition of sizes may not necessarily work in detail? >>From the pktattr or the tag, you can recover p, to pass to > pool_cache_put, if you need: > > p = container_of(pktattr, struct altq_pktattr_tag, apt_pktattr); > p = container_of(tag, struct altq_pktattr_tag, apt_tag); > > This is a safer idiom no matter how struct altq_pktattr and struct > m_tag are defined. (I don't know what's in their actual definitions, > and it may turn out to be that no compiler will ever add padding and > that the arithmetic will happen to work out -- but using an auxiliary > structure makes the intent clearer anyway.) I see. I modify to such way. Thanks, -- ////////////////////////////////////////////////////////////////////// Internet Initiative Japan Inc. Device Engineering Section, Core Product Development Department, Product Division, Technology Unit Kengo NAKAHARA <[email protected]>
