[ovs-dev] [flow_metadata 0/5] improve and use flow_metadata more widely

[Rajahalme, Jarno (NSN - FI/Espoo)]

Ben,

On my reading the first three patches seem fine. I thought that the fact that the semantics of the struct flow_metadata has changed from "exportable metadata" to "all metadata, some of which is exportable to controllers" deserves a note somewhere, but then the original comment on flow_metadata is even more valid after these changes ("Represents the metadata fields of struct flow.").

I don't think there has been a decision to not make the hole in flow_tnl explicit, even though existing code relies on them being cleared (as in commit_odp_tunnel_action()).

This might not be as clean as you'd like, but would it be portable to repeat the definition of flow_metadata as the first fields of the struct flow (maybe via a macro) and then cast a struct flow * to a struct flow_metadata * when needed?

  Jarno

On May 16, 2013, at 23:44 , ext Ben Pfaff wrote:

> I agree about the "md.", but I don't know a better way in portable C.
> There are some ways that I don't think are better (e.g. macros that
> expand to md.in_port etc.) and ways that are not portable (e.g. the
> extension described in "Unnamed struct/union fields within
> structs/unions") in the GCC manual.  I doubt that either choice is
> acceptable, so I think that our options are:
> 
>        1. Don't use flow_metadata in struct flow.
> 
>        2. Accept the "md." option.
> 
> I lean toward #2, but the world won't end if we don't do it.
> 
> How do you feel about the first three patches?  They are independent
> of this bigger decision.
> 
> I need to look at the zeros issue too, I can't quite remember why we
> decided we didn't need them in flow_tnl.
> 
> On Mon, May 13, 2013 at 05:32:15AM +0000, Rajahalme, Jarno (NSN - FI/Espoo) wrote:
>> Getting rid of mostly dummy function parameters and duplication is
>> always nice, but I'd like it better if there was a way not to have
>> to see the "md." everywhere (start to miss C++ here..). Also, since
>> we care about the zeros at the end of struct flow, maybe we should
>> have them in struct flow_tnl and struct flow_metadata as well?
>> 
>>  Jarno
>> 
>> On May 11, 2013, at 2:42 , ext Ben Pfaff wrote:
>> 
>>> The "struct flow_metadata" is only used in a few specific places
>>> currently, but it can be generalized and used more broadly.  This
>>> series does that.
>>> 
>>> The first three patches seem quite reasonable and low-impact to me:
>>> flow: Use struct flow_tnl in struct flow_metadata.
>>> flow: Extend flow_metadata member 'in_port' from 16 to 32 bits.
>>> flow: Add skb_priority and skb_mark to struct flow_metadata.
>>> 
>>> The remaining two patches are more likely to prompt a reaction.  I
>>> am curious to see what that reaction is:
>>> flow: Use struct flow_metadata inside struct flow.
>>> flow: Make flow_extract()'s caller responsible for metadata.
>>> 
>>> lib/cfm.c               |    4 +-
>>> lib/dpif-netdev.c       |   15 +++---
>>> lib/flow.c              |   34 ++-----------
>>> lib/flow.h              |   51 ++++++++-----------
>>> lib/learning-switch.c   |   14 +++---
>>> lib/match.c             |  122 ++++++++++++++++++++++----------------------
>>> lib/meta-flow.c         |   72 +++++++++++++-------------
>>> lib/nx-match.c          |   17 ++++---
>>> lib/odp-util.c          |   44 ++++++++--------
>>> lib/ofp-parse.c         |    6 +--
>>> lib/ofp-print.c         |   17 ++++---
>>> lib/ofp-util.c          |   64 +++++++++++++-----------
>>> ofproto/netflow.c       |    4 +-
>>> ofproto/ofproto-dpif.c  |  128 +++++++++++++++++++++++++----------------------
>>> ofproto/ofproto.c       |    6 ++-
>>> ofproto/tunnel.c        |   32 ++++++------
>>> ofproto/tunnel.h        |    2 +-
>>> tests/test-bundle.c     |    8 +--
>>> tests/test-classifier.c |   58 ++++++++++-----------
>>> tests/test-flows.c      |    5 +-
>>> tests/test-multipath.c  |    6 +--
>>> tests/test-odp.c        |    2 +-
>>> 22 files changed, 350 insertions(+), 361 deletions(-)
>>> 
>>> -- 
>>> 1.7.10.4
>>> 
>>> _______________________________________________
>>> dev mailing list
>>> dev at openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/dev
>>