RFC: ALTQ caller refactor

[Taylor R Campbell]

Date: Fri, 4 Mar 2016 12:01:52 +0900

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

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de

[Joerg Sonnenberger]

Hi,
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.
http://netbsd.org/~knakahara/altq-caller-refactor/altq-caller-refactor.patch
In above patch, I fix performance issue by the following two ways.
- make ALTQ's m_tag pool cache
- inline altq_*_pattr functions
Without these two fixes, the enabled ALTQ throughput degraded about 10%.
However, with these two fixes, the throughput is almost the same
performance.

Is there a good reason to keep the complexity of the pattr attribute
around? From working on the ALTQ code a few years ago I remember that it
is only used by some of the more experimental mechanisms and not used by
the few queueing disciplines people normally care about. It might be
easier to just get rid of it.

Joerg

--
Posted automagically by a mail2news gateway at muc.de e.V.
Please direct questions, flames, donations, etc. to news-***@muc.de