RFC: vlan(4) use pkthdr instead of mtag

[Shoichi Yamaguchi]

Hi,

This is a proposal of changes to improve VLAN packet handling.

Currently, FreeBSD, DragonFly BSD, and OpenBSD use pkthdr structure
to store VLAN id in each packet. On the other hand, NetBSD uses mtag.

To check the impact for the performance of the both implementations,
I ported pkthdr VLAN id implementation to NetBSD and measure
its performance.

Here is the patch.
https://gist.githubusercontent.com/s-ymgch228/6597cfc4b6f79c6c62fcdf25003acb55/raw/adaf793dfc56f71438bc68e67530f9e68a884f4d/vlan_mtag.patch

Here is the result of performance evaluation.
https://gist.github.com/s-ymgch228/6597cfc4b6f79c6c62fcdf25003acb55/raw/adaf793dfc56f71438bc68e67530f9e68a884f4d/performance_vlan-pkthdr.pdf
- ipgen (https://github.com/iij/ipgen) was used for the measurement.
- The kernel configuration was amd64/GENERIC enabled NET_MPSAFE for
both mtag and pkthdr

From the above result, my implementation improves the performance by
about 10 %. And all 0% rate items are saturated in the both
measurements.

Could you comment about this patch and result? I hope this patch will be
merged.

Thanks,
--
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Shoichi YAMAGUCHI <s-***@iij.ad.jp>

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

[Christos Zoulas]

Post by Shoichi Yamaguchi
Could you comment about this patch and result? I hope this patch will be
merged.

Please commit it. Looks great; it simplifies the code too.

christos


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

[Shoichi YAMAGUCHI]

Thank you for the comment.

Post by Christos Zoulas
Please commit it. Looks great; it simplifies the code too.

If no more comments will be posted for a few days, I'll ask
***@n.o. or ozaki-***@n.o. who are my co-worker to
commit the patch. Because I'm not a NetBSD developer.

Regards,
s-***@IIJ

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

[Shoichi YAMAGUCHI]

Thank you for the comments.

I updated the patch:
https://gist.githubusercontent.com/s-ymgch228/6597cfc4b6f79c6c62fcdf25003acb55/raw/8237c14badb390355613794f5e3dee92431a89d2/vlan_mtag.patch
https://gist.githubusercontent.com/s-ymgch228/6597cfc4b6f79c6c62fcdf25003acb55/raw/7169afbe597f3fb562cdc6afc4ef9caaa04d2542/vlan_mtag.patch.diff

Changes:
- sys/mbuf.h
- Add a comment like /* 1: Used to be VLAN ID */
- Add line breaks to M_COPYFLAGS
- if_vlan.c
- Fix branches
- if_ether.h
- Introduce vlan_has_tag and use it in drivers

the comments for the size of pkthdr on LP64 was not updated because
the result that I checked the size using the test code like
"printf("%lu\n", sizeof(struct pkthdr))" was 56 on amd64 after
applying the patch

Regards,
s-***@IIJ
--
Internet Initiative Japan Inc.

Device Engineering Section,
IoT Platform Development Department,
Network Division,
Technology Unit

Shoichi YAMAGUCHI <s-***@iij.ad.jp>

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

[Shoichi YAMAGUCHI]

Hi,

I updated the patch again.
https://gist.githubusercontent.com/s-ymgch228/6597cfc4b6f79c6c62fcdf25003acb55/raw/afeb931b5efa25295b5b93bcf804ba8dc5b47e86/vlan_mtag.patch
https://gist.githubusercontent.com/s-ymgch228/6597cfc4b6f79c6c62fcdf25003acb55/raw/afeb931b5efa25295b5b93bcf804ba8dc5b47e86/vlan_mtag.patch.diff

Changes:
- Rename VLAN_TAG_VALUE to vlan_get_tag
- Remove a empty branch in pq3etsec_tx_offload
- Fix the wrong mask (0xFFFF => 0x0FFF)
- Remove the modification of cxgb_sge.c

cxgb_sge.c's t3_encap should not predicate the VLAN support.

Sorry, I don't understand you. Does this remark means the functions
related to VLAN like vlan_has_tag should use with "#if NVLAN > 0" ?

Currently, cxgb_sge.c's VLAN_SUPPORTED doesn't work on NetBSD,
because it uses pkthdr to store a VLAN tag before my patch. And I don't
mean to fix it now. By the reason, I changed not to modify the code
in the updated patch.

if_ti.c's ti_rxeof is using the wrong mask now, if I interprete the
comment correctly?

Thank you the comment. I took a same mistake in VLAN_TAG_VALUE
and fixed them.

Thanks,
s-***@IIJ

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

[Shoichi YAMAGUCHI]

No, I mean it just shouldn't use __predict_false for the branch. That's
a stupid assumption to make. Does that make it clearer?

I understand, thanks.
Should I modify not to use __predict_false in my patch at once?
I think it isn't related to my patch directly and better to do with
the fix of the other issue that VLAN_SUPPORTED does not work.

In VLAN_TAG_VALUE it doesn't matter as it stores the entry in the field
reusing the remaining bits. In this case, the other bits of the vlan
field where used for other purpses.

I think it doesn't matter, too. But, I want to change it to 0x0FFF instead of
0xFFFF, because 4095 is used for the mask in VLAN_TAG_VALUE
before my patch.

Regards,
s-***@IIJ

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

[Shoichi YAMAGUCHI]

Hi,

If there is no comment or question, I'll ask my co-workers to merge this patch
within a few days.
Are there any other comments or questions?

Thanks,

[Shoichi YAMAGUCHI]

Hi,

I updated the patch again.
https://gist.githubusercontent.com/s-ymgch228/6597cfc4b6f79c6c62fcdf25003acb55/raw/bf3b295aeeb6ae63965d533a828d8f5e360c884e/vlan_mtag.patch
- Introduce new functions related to vlan to cxgb_sge.c's t3_encap
and remove __predict_false
- add a KASSERT to vlan_set_tag

Thanks for your advices.

Thanks,
-- s-***@IIJ

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