[ovs-dev,V14,1/4] Flow and action changes for 802.1AD

From: "Thomas F. Herbert" <thomasfherbert@gmail.com>

ToDo: The flow structure needs to be updated to hold both the inner and outer
tpid since these are encoded by the kernel module.

The flow structure is updated to hold the customer tci.
Flow key extraction is changed to add support for ctci and both TPIDs.
Parsing to support pushing and popping with both single and double
tagged vlans. In response to reviewers comments on V6, all changes
affected by the flow structure are gathered in this patch.

Signed-off-by: Thomas F Herbert <thomasfherbert@gmail.com>
---
 lib/flow.c                   |  2 +-
 lib/flow.h                   | 12 +++++++++---
 lib/ofp-actions.c            | 32 +++++++++++++++++++-------------
 lib/ofp-actions.h            | 14 +++++++++++---
 ofproto/ofproto-dpif-xlate.c | 11 +++++++++++
 5 files changed, 51 insertions(+), 20 deletions(-)

On Mon, Nov 09, 2015 at 08:30:19PM -0800, Thomas F Herbert wrote:
> 
> 
> On 11/9/15 12:58 PM, Ben Pfaff wrote:
> >On Fri, Oct 02, 2015 at 05:31:58PM -0400, Thomas F Herbert wrote:
> >>From: "Thomas F. Herbert" <thomasfherbert@gmail.com>
> >>
> >>ToDo: The flow structure needs to be updated to hold both the inner and outer
> >>tpid since these are encoded by the kernel module.
> >I think I'd like to see the version with both TPIDs encoded.  That's the
> >way I've always envisioned this working.
> From an architectural view, it sounds indeed quite sensible. Also, it would
> get rid of the vlan depth calculation in the above code which is working but
> ugly. The $10 question is: are you implying that we should change the ABI by
> adding new OVS_KEY_ATTRIBs for the two TPIDs? This would raise backward
> compatibility and ABI issues.

I think that we covered this in-person at ovscon last week, but just in
case...  I don't think that this requires a kernel ABI change, because
the kernel ABI has always included the TPID.  I'm only suggesting that
"struct flow" consistently include the TPID; struct flow always goes
through some kind of translation whenever it passes out of OVS
userspace, so that shouldn't in itself break anything.

> >Also, once we have the TPIDs in place in struct flow, I am not sure
> >whether it is worthwhile to keep adding the VLAN_CFI bit to the TCI
> >value to indicate that a header is present.
> I assume you are suggesting that we determining a VLAN is present by
> checking eth_type_vlan(). With this we could allow any value for the VLAN
> including a 1 in the DEI bit. Maybe this should be a separate patch to
> eliminate the redundancy in openvswitch once the above change was made.

Maybe an example would help.  Suppose we define:
        struct flow_vlan {
            ovs_be32 tpid;
            ovs_be32 tci;
        };

and then in struct flow replace vlan_tci by "struct flow_vlan
vlans[2];".  (I'm not saying this is the way to go but it's a
possibility.)

Then, we know that there's at least one VLAN in 'flow' if
'flow->vlans[0].tpid != 0' and that there are two VLANs in 'flow' if
'flow->vlans[1].tpid != 0' (presumably they'd be left-justified so you
wouldn't get just the latter).

On 11/25/15 12:54 AM, Ben Pfaff wrote:
> On Mon, Nov 09, 2015 at 08:30:19PM -0800, Thomas F Herbert wrote:
>>
>> On 11/9/15 12:58 PM, Ben Pfaff wrote:
>>> On Fri, Oct 02, 2015 at 05:31:58PM -0400, Thomas F Herbert wrote:
>>>> From: "Thomas F. Herbert" <thomasfherbert@gmail.com>
>>>>
>>>> ToDo: The flow structure needs to be updated to hold both the inner and outer
>>>> tpid since these are encoded by the kernel module.
>>> I think I'd like to see the version with both TPIDs encoded.  That's the
>>> way I've always envisioned this working.
>>  From an architectural view, it sounds indeed quite sensible. Also, it would
>> get rid of the vlan depth calculation in the above code which is working but
>> ugly. The $10 question is: are you implying that we should change the ABI by
>> adding new OVS_KEY_ATTRIBs for the two TPIDs? This would raise backward
>> compatibility and ABI issues.
> I think that we covered this in-person at ovscon last week, but just in
> case...  I don't think that this requires a kernel ABI change, because
> the kernel ABI has always included the TPID.  I'm only suggesting that
> "struct flow" consistently include the TPID; struct flow always goes
> through some kind of translation whenever it passes out of OVS
> userspace, so that shouldn't in itself break anything.
Yes, thanks for the comment. I will update patch to encode and decode 
both tpid's.
>
>>> Also, once we have the TPIDs in place in struct flow, I am not sure
>>> whether it is worthwhile to keep adding the VLAN_CFI bit to the TCI
>>> value to indicate that a header is present.
>> I assume you are suggesting that we determining a VLAN is present by
>> checking eth_type_vlan(). With this we could allow any value for the VLAN
>> including a 1 in the DEI bit. Maybe this should be a separate patch to
>> eliminate the redundancy in openvswitch once the above change was made.
There was a separate comment by Pravin discussing eliminating the DEI. 
Now that we are encoding tpid's explicitly we could check tpid ethertype 
to determine if a vlan is present but probably that should wait for 
another patch.
> Maybe an example would help.  Suppose we define:
>          struct flow_vlan {
>              ovs_be32 tpid;
>              ovs_be32 tci;
>          };
Yes, I did this in the kernel patch.
>
> and then in struct flow replace vlan_tci by "struct flow_vlan
> vlans[2];".  (I'm not saying this is the way to go but it's a
> possibility.)
>
> Then, we know that there's at least one VLAN in 'flow' if
> 'flow->vlans[0].tpid != 0' and that there are two VLANs in 'flow' if
> 'flow->vlans[1].tpid != 0' (presumably they'd be left-justified so you
> wouldn't get just the latter).
Yes, this will make for cleaner code to calculate depth of encapsulation 
vlan.   I think that since vlans are never nested more then 2 deep, I 
could encode it the same way I do in the kernel flow key with two 
structs instead of an array of structs.

On Wed, Dec 30, 2015 at 07:29:15PM -0500, Thomas F Herbert wrote:
> 
> 
> On 11/9/15 3:58 PM, Ben Pfaff wrote:
> >On Fri, Oct 02, 2015 at 05:31:58PM -0400, Thomas F Herbert wrote:
> >>From: "Thomas F. Herbert" <thomasfherbert@gmail.com>
> >>
> >>ToDo: The flow structure needs to be updated to hold both the inner and outer
> >>tpid since these are encoded by the kernel module.
> >I think I'd like to see the version with both TPIDs encoded.  That's the
> >way I've always envisioned this working.
> I have been working on the changes to add the inner and outer tpids to the
> flow struct this week. I changed flow struct to place two vlan structs right
> after the dl_type with the outer first followed by the inner. This is done
> so the miniflow would still work with few modifications because dl_type and
> outer tci are sequential. However, this raises the issue as to whether the
> outer tpid and the inner vlan, tci and tpid should also be added to the
> miniflow. What are your thoughts?

I don't understand the question.  A miniflow and a flow represent the
same data, only a miniflow has a compressed format.

On Mon, Jan 04, 2016 at 02:46:09PM -0500, Thomas F Herbert wrote:
> 
> 
> On 1/4/16 1:20 PM, Ben Pfaff wrote:
> >On Wed, Dec 30, 2015 at 07:29:15PM -0500, Thomas F Herbert wrote:
> >>
> >>On 11/9/15 3:58 PM, Ben Pfaff wrote:
> >>>On Fri, Oct 02, 2015 at 05:31:58PM -0400, Thomas F Herbert wrote:
> >>>>From: "Thomas F. Herbert" <thomasfherbert@gmail.com>
> >>>>
> >>>>ToDo: The flow structure needs to be updated to hold both the inner and outer
> >>>>tpid since these are encoded by the kernel module.
> >>>I think I'd like to see the version with both TPIDs encoded.  That's the
> >>>way I've always envisioned this working.
> >>I have been working on the changes to add the inner and outer tpids to the
> >>flow struct this week. I changed flow struct to place two vlan structs right
> >>after the dl_type with the outer first followed by the inner. This is done
> >>so the miniflow would still work with few modifications because dl_type and
> >>outer tci are sequential. However, this raises the issue as to whether the
> >>outer tpid and the inner vlan, tci and tpid should also be added to the
> >>miniflow. What are your thoughts?
> >I don't understand the question.  A miniflow and a flow represent the
> >same data, only a miniflow has a compressed format.
> Yes, they represent or should represent the same data but that wasn't my
> question. Maybe the question will be clearer when I submit the patch. The
> miniflow_extract function requires some changes that wasn't done for
> previous versions of this patch. The current miniflow extract compresses
> only the outer vlan_tci, dl_type. For this version, I am writing a
> miniflow_push_vlan() macro to compress both the tpid and tci into the
> miniflow and call the same function to compress the inner tpid and tci as
> well.

OK.  I think it will be clearer from the patch.