Re: RFC: ALTQ caller refactor

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

   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.

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;

>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.)