Add 802.1p priority code point to vlan(4).

[araujo]

Summary

Add support to priority code point (PCP) that is an 3-bit field which refers to the
IEEE 802.1p class of service and maps to the frame priority
level.

Values in order of priority are: 1 (Background (lowest)), 0 (Best
effort (default)), 2 (Excellent effort), 3 (Critical
applications), 4 (Video, < 100ms latency), 5 (Video, < 10ms
latency), 6 (Internetwork control), 7 (Network control
(highest)).

Note: This patch was obtained from pfsense project, according with the license, Robert Watson made big part of this code.

Test Plan

• Two machines with the same VLAN id(10) and set the PCP first to 5. After a while change the PCP to 1.
• Capture the traffic with tcpdump(1).

tcpdump.pcap24 KBDownload

[glebius]

Can we pass an mbuf with M_VLANTAG and valid ether_vtag up the stack? At quick glance I see no problems with that. If yes, then we don't need to allocate any mbuf_tags(9) and pass the value in ether_vtag of mbuf.

[araujo]

In D801#5, @glebius wrote:

Can we pass an mbuf with M_VLANTAG and valid ether_vtag up the stack? At quick glance I see no problems with that. If yes, then we don't need to allocate any mbuf_tags(9) and pass the value in ether_vtag of mbuf.

I think it is possible; my only concern is with the vlan_input(), to be more exactly at line 1270. But if there is no objection for now, I would like to check it in a third stage. As my second step would be make 802.1q workable on pf(4) and ipfw(8) as well.

[glebius]

What is the concern with line 1270? If the tag is set in the mbuf header unconditionally, the whole clause isn't needed.

May be it is worth to post the entire patch, including pf/ipfw? Then it'll be more clear.

[araujo]

In D801#7, @glebius wrote:

What is the concern with line 1270? If the tag is set in the mbuf header unconditionally, the whole clause isn't needed.

May be it is worth to post the entire patch, including pf/ipfw? Then it'll be more clear.

Actually on pf(4) it is using the mbuf_tags(9) to check the PCP. The ipfw(8) is not ready yet and will cost a bit of time to be finished, but also on ipfw(8) the plan is to use the mbuf_tags(9) too, to check the PCP.

Soon as I finish the ipfw(8), I will post all entire patch using mbuf_tags(9), and we can discuss again how we can improve it.

Thanks by all inputs.

[melifaro]

It is very good to have an ability to set static PCP's via firewall or inside ifconfig. I wonder if we can support (probably) much more native / used way: e.g. inherit PCP from DSCP/TC of IPv4 / IPv6 header.

While there are of course some cases when you just need to set PCP statically (and you can't get it from L3 header and/or update it) there are a lot of other cases when it is easier to set proper DSCP (on application level, or using FW) and then get it automatically propagated to L2 frame.
It will be great to support something like "vlanpcp inherit" ifconfig option to enforce default DSCP propagation (and ability to configure non-default DSCP-to-PCP mutation map would be even more great)

[araujo]

In D801#12, @melifaro wrote:

It is very good to have an ability to set static PCP's via firewall or inside ifconfig. I wonder if we can support (probably) much more native / used way: e.g. inherit PCP from DSCP/TC of IPv4 / IPv6 header.

Maybe it is possible, but right now, I have no clue how to do it.

While there are of course some cases when you just need to set PCP statically (and you can't get it from L3 header and/or update it) there are a lot of other cases when it is easier to set proper DSCP (on application level, or using FW) and then get it automatically propagated to L2 frame.
It will be great to support something like "vlanpcp inherit" ifconfig option to enforce default DSCP propagation (and ability to configure non-default DSCP-to-PCP mutation map would be even more great)

The idea is really nice, but right now I have no plan to do any of it. The only thing that I'm focused now is make it works with ipfw(8). However I'm gonna add it in my TODO list for future work.

[rwatson]

FWIW, the main reason I didn't follow up on committing this was time, but a secondary reason was the general belief that m_tags were not the right place to store 802.1q-related data as we already had an mbuf-header field that should have been suitable for it.

My memory is, unfortunately, pretty hazy about how we found ourselves in that straight: I think, in particular, there was a desire to be able to add PCP state to data that would later be encapsulated in 802.1q even if the originating packet didn't have VLAN state -- e.g., if you are bridging a packet from a regular ethernet to a VLAN trunk, and pf(4) was used to add priority state, but M_VLANTAG wasn't appropriate for the mbuf as it hadn't been decoded in the input path as Ethernet. This might best be resolved by a hybrid scheme, in which if M_VLANTAG is set, then we use the in-mbuf-header field, and otherwise use an m_tag. Whether to reconcile immediately (write in the mbuf header if present; otherwise an m_tag) or to do it in output processing (if there is an m_tag, override the mbuf-header version) is something I'm unclear on.

Regardless, I'm really pleased to see that this work might get picked up: handling 802.1q priorities and allowing firewall use is quite useful. I did this work for Adara because they had some quite specific needs, and generalising for IPFW, etc, is very useful to do.

[araujo]

Thank everybody that made review, I'm gonna back to work on it soon after my wedding.

[gnn]

Was there an update to this based on new work? It's generally useful and ought to go in, with stated modifications of course.

[araujo]

In D801#91095, @gnn wrote:

Was there an update to this based on new work? It's generally useful and ought to go in, with stated modifications of course.

@gnn unfortunately I didn't touch on this patch for a long time, and I did lost part of ipfw implementation too. I will take it again.

Thanks for the request :)

Best,

[araujo]

Update this patch with the latest CURRENT code.
I'm checking the comments and will try to address them.

[mgrooms_shrew.net]

Just wanted to offer some feedback: I've applied this patch to a 10.2-RELEASE host and have been running it for a week now with zero issues. I also have a patch extracted from pfSense that allows pf to match on or set the vlan pcp values. Both were required to build a FreeBSD pf firewall that works well with Google Fiber internet service. With these applied, I'm able to push/pull the full gigabit up/down using a tiny single core ESXi VM. Without the patches I'm only able to get about 10 megabit up.

[adrian]

hiya,

this looks ready to commit. Any other feedback?

[araujo]

This part is ready to be committed!
There is the pf part that it is possible to use the pfsense patch that works great.

The ipfw patch, I lost my vm with it, and I have no time to redo it again.

But, I think this part we could push and other people probably will take the ipfw, the pf part I can check as there is a patch ready to be used.

If no objection, I will commit it. @gnn, @adrian ?

[adrian]

+1 for committing this part. Then yeah, the ipfw bits should be easy!

[mgrooms_shrew.net]

Anything I can do to help as far as testing goes?

[gnn]

Do it.

[mgrooms_shrew.net]

Could this please make it into the tree for the 11 release? Also, is there a review in progress for the pf related changes yet?

[araujo]

Hi,

Sorry my delay, I got busy with other things, the pf patch is on the way... I will probably commit it next week.

Sorry again.

[araujo]

• Update this patch including the pf changes.

I will include @kp and @loos to review it too.

[mgrooms_shrew.net]

The pf_ieee8021q_setpcp is being called twice in the ipv6 code path. I submitted a patch to fix this in pfSense so it should apply here as well ...

https://github.com/pfsense/FreeBSD-src/commit/f9c13be737f3ceaf1694a8af973373f7a4d0ce22

You probably just pulled an older version of the diff that still had the bug.

[araujo]

• Import patch from pfsense:
  
  Correct a mis-merge when imported VLAN PCP code:

There was a mistake when code was imported that lead us to end up with
duplicated block of code on IPv6 code path while one of these blocks
belong to IPv4.

Whithout this patch pf wouldn't apply vlan pcp value correctly to
outbound IPv4 traffic.

Patch: pf_802.1p.diff
Submitted by: Matthew Grooms <[e-mail]>

[araujo]

In D801#136279, @mgrooms_shrew.net wrote:

The pf_ieee8021q_setpcp is being called twice in the ipv6 code path. I submitted a patch to fix this in pfSense so it should apply here as well ...

https://github.com/pfsense/FreeBSD-src/commit/f9c13be737f3ceaf1694a8af973373f7a4d0ce22

You probably just pulled an older version of the diff that still had the bug.

Please, could you double check/test it again?
I just import your patch and adapt it with the current code.

There are too many changes on pf from pfSense, I guess now, we are covered with the changes needed for 802.1p.

[bapt]

sys/sys/priv.h

344

cosmetic but alignment is wrong here

[kp]

I think I see why the extension to PF was done, but I'm not convinced this is the best way.

PF doesn't deal with layer 2, and this is very much a layer 2 change. Also, the syntax is very different from what openbsd do, and while out pfs are already rather different I'd like to avoid introducing even more differences.

Wouldn't it be better to pass through ALTQ and use the packets priorities as a basis for the PCP (perhaps with a mapping of priority to PCP value)?

[mgrooms_shrew.net]

I was able to get some light testing done with this patch applied. Things appear to be working well. NOTE: You'r missing a closing bracket in sys/netpfil/pf/pf.c around line 6078.

sys/netpfil/pf/pf.c

6078 ↗

(On Diff #16498)

Your missing a closing bracket here.

[mgrooms_shrew.net]

I noticed that code slush begins May 27th, which is two days from now. Can I help test anything else?

[kp]

In D801#138630, @mgrooms_shrew.net wrote:

I noticed that code slush begins May 27th, which is two days from now. Can I help test anything else?

I think this patch isn't going to be ready before the code slush.
I'm honestly not happy with the approach taken here to set and use the PCP bits.

That said, it's clearly a valuable feature, so I'd like to see the patch evolve into something everyone is happy with.
Perhaps this is something we can discuss face-to-face at BSDCan.

[araujo]

In D801#139427, @kristof wrote:

In D801#138630, @mgrooms_shrew.net wrote:

I noticed that code slush begins May 27th, which is two days from now. Can I help test anything else?

I think this patch isn't going to be ready before the code slush.
I'm honestly not happy with the approach taken here to set and use the PCP bits.

That said, it's clearly a valuable feature, so I'd like to see the patch evolve into something everyone is happy with.
Perhaps this is something we can discuss face-to-face at BSDCan.

I won't be at BSDCan this year again!
But I would be very happy if someone could takeover the pf part of this patch.

Best,

[adrian]

I'm okay with this. It works, right? And it's working code, so I don't mind if there are some minor disagreements. I'd rather we have the feature than not.

[kp]

In D801#140977, @adrian wrote:

I'm okay with this. It works, right? And it's working code, so I don't mind if there are some minor disagreements. I'd rather we have the feature than not.

I'm concerned about being stuck with the new pf keywords. Once we add it we can't just drop it again without at least a generous transition period.

I'll see if I can work up an alternative around BSDCan. (I usually manage to find a bit of time for BSD things around conferences.)

[araujo]

In D801#141068, @kristof wrote:

In D801#140977, @adrian wrote:

I'm okay with this. It works, right? And it's working code, so I don't mind if there are some minor disagreements. I'd rather we have the feature than not.

I'm concerned about being stuck with the new pf keywords. Once we add it we can't just drop it again without at least a generous transition period.

Make sense, maybe I can commit only those parts that has nothing to do with pf.
At least the user can set it via ifconfig.

I'll see if I can work up an alternative around BSDCan. (I usually manage to find a bit of time for BSD things around conferences.)

That will be amazing from you!

[mgrooms_shrew.net]

In D801#139427, @kristof wrote:

In D801#138630, @mgrooms_shrew.net wrote:

I noticed that code slush begins May 27th, which is two days from now. Can I help test anything else?

I think this patch isn't going to be ready before the code slush.
I'm honestly not happy with the approach taken here to set and use the PCP bits.

Are you only objecting to the pf portions of the patch? If so, can the static vlan pcp support be committed without the pf changes? I've been running with it for a month+ without issue. The pf related changes could always be pushed to another review.

[kp]

In D801#141126, @mgrooms_shrew.net wrote:

Are you only objecting to the pf portions of the patch? If so, can the static vlan pcp support be committed without the pf changes? I've been running with it for a month+ without issue. The pf related changes could always be pushed to another review.

That's the most pressing concern, yes.

My plan is to use an abstract priority in pf (and the rest of the stack), translating that into PCP values only in the vlan code. There'd be a mapping of priorities to PCP values, which would be a different configuration from what's implemented here, but it should be relatively straightforward to support the current configuration and that mapping at the same time.

So, yes, let's include everything except for the pf bits for now. That'll likely make this less useful that it'd be with the pf bits, but still better than nothing at all. I'll still try to come up with a proposal for my view in the next week or two.

[loos]

I'm accepting this revision because it is well tested, but I'm not against the commit of ifconfig/vlan parts only (without the pf parts).

I'll be at BSDCan too and it is fine to discuss the pf changes there.

Can we ask araujo@ to go ahead and commit the patch without any of the pf changes ?

sys/net/if_vlan.c

130–133

Another cosmetic fix, alignment.

[araujo]

Thanks guys,

I will commit the code without pf changes tomorrow.

[araujo]

I will keep this review open because of pf(4) bits.

[kp]

I've posted my proposed patch in https://reviews.freebsd.org/D6786

[kp]

Also, what's the reasoning behind the default-disabled setting for the sysctl? It's a little surprising that setting things in pf won't work, unless the sysctl is set.

I understand that there are performance implications, but people who really care about performance will tweak things anyway (so it'd be better to enable by default).

[araujo]

In D801#142706, @kristof wrote:

Also, what's the reasoning behind the default-disabled setting for the sysctl? It's a little surprising that setting things in pf won't work, unless the sysctl is set.

I understand that there are performance implications, but people who really care about performance will tweak things anyway (so it'd be better to enable by default).

That sysctl exist just because not everybody would use PCP by default and as you mentioned, it has performance implications when enabled.

[araujo]

Close in favor of another pf patch: https://reviews.freebsd.org/D6786