Re: RFC: ALTQ caller refactor

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

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 <k-nakahara%iij.ad.jp@localhost>
> 
>    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 <k-nakahara%iij.ad.jp@localhost>