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@openvswitch.org
>>> http://openvswitch.org/mailman/listinfo/dev
>>
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
