On Mon, Sep 19, 2016 at 06:49:25AM -0500, Muhammad Shahbaz wrote:
> This adds necessary changes in the OVS codebase to consume the C (macro) code
> generated by the OVS plugin in p4c-behavioral. It also updates the OVS build
> system to take a P4 program as input at 'configure' time.
Hi Shahbaz. Thanks for working through this. I'm enthusiastic about
it, but I have a ton of comments. That is natural, I hope, for such a
big change.
I'm only following up to one of the 4 patches across 2 repos, but
really it has to all be reviewed together, so I guess that you
understand that this applies to the first patch for p4c-behavioral and
ovs. The second patch in each case I'm not reviewing yet; I think
that they can wait.
"git am" reports a few whitespace errors:
/home/blp/nicira/p4c-behavioral/.git/rebase-apply/patch:409: space before
tab in indent.
return b->_${header_name}_ofs != UINT16_MAX \
/home/blp/nicira/p4c-behavioral/.git/rebase-apply/patch:410: space before
tab in indent.
? (char *) dp_packet_data(b) + b->_${header_name}_ofs \
/home/blp/nicira/p4c-behavioral/.git/rebase-apply/patch:411: space before
tab in indent.
: NULL; \
/home/blp/nicira/p4c-behavioral/.git/rebase-apply/patch:1014: space before
tab in indent.
miniflow_push_bytes__word_aligned_64(mf, _${header_name},
&_${header_name}, sizeof(struct _${header_name}_header), \
warning: 4 lines add whitespace errors.
Most of the changes here are new files, for the new "ovs" plugin. I
think that these could actually be in the OVS repo instead, and I
think that's a better location for them because OVS developers don't
expect to look somewhere else for code that is part of OVS. Can you
work on moving the files into the OVS repo?
I see that there's still one change that would need to be in
p4c-behavioral. I spent a few minutes trying to think of a better
way, but I didn't come up with one that made me happy. I think that
ultimately the P4-2016 implementation will offer a better approach but
this is OK for now.
We probably should not be generating anything for openvswitch.h,
because that is a fixed interface that we cannot change. We'll need
to come up with a way to be compatible with the kernel module, of
course.
I see a lot of use of identifiers whose names begin with _. That is a
little risky, because the C standards reserve names that begin with _,
so usually in OVS we use a trailing _ instead if we need a name that
is probably not in use. (Some of the _s don't seem needed at all.
For example, I don't know why the members of struct valid_header start
with _, since the members of a struct have a scope of their own, so
that they can't easily conflict with other identifiers.)
These generated files have a common structure: they declare macros
that similarly named "parent" headers invoke. Sometimes this is fine,
but in other cases I think that an alternate structure would be easier
to understand and easier to write. For example, take a look at
packets.h.h. This header declares a single macro, OVS_HDR_STRUCTS,
that expands to a lot of struct definitions.
include/openvswitch/packets.h then expands OVS_HDR_STRUCTS in one
place. This works fine, but it means that the definition of
OVS_HDR_STRUCTS is a long series of lines that end in \ to continue
the macro definition. That's hard to read and kind of painful. It
would be easier to read if the header dropped the macro definition and
instead just defined the structs itself. Then packets.h could just
include it in the right place to define the structs. I see potential
for similar readability improvements in other headers, such as
types.h.h.
Does this really support only fields that are a multiple of 8 bits in
length? If so, that is too bad.
Some of the names in the p4c code are very long and bizarre, such as
ordered_header_instances_non_virtual_field__name_width.
I see a lot of repetitive code in the .h.h headers to handle
8/16/32/64 bit cases, which makes the code harder to read. I think
that in many cases this could be unified by adding a few helpers, for
example perhaps helpers along the lines of:
def ntoh(arg, width):
if width == 8:
return "(%s)" % arg
elif width == 16:
return "ntohs(%s)" % arg
elif width == 32:
return "ntohl(%s)" % arg
elif width == 64:
return "ntohll(%s)" % arg
else:
assert False
The generated code appears to use a lot of casts, e.g. one example
from flow.c.h below. In most cases I don't understand the need for
the casts.
+//:: if aligned_metadata_bit_width == 8:
+ _${metadata_name}.hdr.${aligned_metadata_name} = (uint8_t)
${source_value}; \
+//:: elif aligned_metadata_bit_width == 16:
+ _${metadata_name}.hdr.${aligned_metadata_name} = htons((uint16_t)
${source_value}); \
+//:: elif aligned_metadata_bit_width == 32:
+ _${metadata_name}.hdr.${aligned_metadata_name} = htonl((uint32_t)
${source_value}); \
+//:: elif aligned_metadata_bit_width == 64:
+ _${metadata_name}.hdr.${aligned_metadata_name} = htonll((uint64_t)
${source_value}); \
+//:: else:
Here are a few comments specific to particular files.
meta-flow.h.h
-------------
Eventually it may not make sense to generate meta-flow.h, or at least
not the comments, since those comments just get parsed by another
program. I think that is it an OK approach at this stage.
This currently generates OXMs drawn from OXM_OF_*. This is a bad idea
because this OXM class is owned by ONF and so a P4 program could
conflict with an assignment that ONF makes later. I suggest using
NXOXM_ET_ instead.
I guess that only 1 bit of the "valid" fields are in use, so probably
instead of
* Type: be${8}.
this should be
* Type; u8 (low 1 bits).
flow.c.h
--------
I think that OVS_HDR_RESET_ATTRS can be integrated into
dp_packet_reset_offsets(). Then the existing call to
dp_packet_reset_offsets() from miniflow_extract() will do the right
thing.
The extraction code is pretty hard to follow. I hope that there is a
way to make it clearer. It would help a little to avoid so many cases
for 8/16/32/64. It might help to organize the generated code so that
it produces functions, instead of macros, that parse each header, but
I don't know whether that's practical.
It looks like the extraction code uses a couple of phases: first it
parses the fields and marks headers as valid and as "touched", and
then it appends the headers that were found to be valid to the
miniflow. I have a few related questions. First, when is there a
difference between valid and "touched", that is, why are both
maintained separately during parsing? Second, I would think that
headers are parsed in the order of their appearance in the packet and
that this would match the order of their appearance in the miniflow
struct. If so, is it necessary to use two phases; that is, can we
append and copy the headers into the miniflow as we encounter them?
If not: if I understand correctly the format for headers in struct
(mini)flow is now the same as the format in the packet, so can we just
save a pointer to the struct and then memcpy it in later in the second
phase? (It's not that 2 phases here are bad conceptually, but this
code is a fast path in some workloads.)
I think that OVS_FLOW_WC_MASK is incomplete. I think that it should
mask fields whose headers are valid, but not others. Similarly for
OVS_FLOW_WC_MAP.
packets.h.h
-----------
The packet_set__*() and packet_set_valid_*() functions are so trivial
that I recommend marking them "inline" and defining them in
packets.h.h to avoid overhead from doing an out-of-line call, although
come to think of it I'm not sure why they're better than just writing
the equivalent assignment statement inline. Do you think that these
functions introduce a valuable abstraction?
The "valid" bit for each header is held in a separate byte. This
seems likely to be excessive to me, since each one could use a single
bit. Did you consider simply using a single uint32_t or uint64_t and
assigning a bit to each header? It would be smaller and simpler in at
least some ways.
The definition of the "pad" array is risky because it could in some
cases have length 0, which MSVC rejects as an error.
meta-flow.c.h
-------------
s/VLAUE/VALUE/ in OVS_SET_VLAUE_CASES.
odp-util.c.h
------------
Why is OVS_FORMAT_ODP_KEY_ATTR_CASES defined? It is empty. Oh, I
see, it printing these cases was disabled, probably to avoid spurious
test failures. Please add a comment, if so.
Thanks a lot,
Ben.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
