[ovs-dev] [PATCH] lib: simplify flow_extract() API

Andy Zhou azhou at nicira.com
Sat Mar 1 00:43:16 UTC 2014


Pushed to the master with the changes suggested.


On Thu, Feb 27, 2014 at 3:21 PM, Jarno Rajahalme <jrajahalme at nicira.com>wrote:

> I like this, some comments below, otherwise:
>
> Acked-by: Jarno Rajahalme <jrajahalme at nicira.com>
>
> On Feb 26, 2014, at 6:22 PM, Andy Zhou <azhou at nicira.com> wrote:
>
> > Change the flow_extract() API to accept struct pkt_metadta,
> > instead of individual metadata fields. It will make the API more
> > logical and easier to maintain when we need to expand metadata
> > down the road.
> >
> > Signed-off-by: Andy Zhou <azhou at nicira.com>
> > ---
> > lib/dpif-netdev.c             |    6 ++----
> > lib/flow.c                    |   20 ++++++++------------
> > lib/flow.h                    |    4 ++--
> > lib/learning-switch.c         |    4 +++-
> > lib/ofp-print.c               |    4 +++-
> > lib/packets.c                 |   28 ++++++++++++++++++++++++++++
> > lib/packets.h                 |    7 +++++++
> > ofproto/ofproto-dpif-upcall.c |    5 +++--
> > ofproto/ofproto-dpif-xlate.c  |    5 ++++-
> > ofproto/ofproto-dpif.c        |    7 +++++--
> > ofproto/ofproto.c             |    8 ++++++--
> > tests/test-flows.c            |    4 +++-
> > utilities/ovs-ofctl.c         |    6 ++++--
> > 13 files changed, 78 insertions(+), 30 deletions(-)
> >
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index c4ba646..6b07817 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -1419,8 +1419,7 @@ dpif_netdev_execute(struct dpif *dpif, struct
> dpif_execute *execute)
> >     }
> >
> >     /* Extract flow key. */
> > -    flow_extract(execute->packet, md->skb_priority, md->pkt_mark,
> &md->tunnel,
> > -                 (union flow_in_port *)&md->in_port, &key);
> > +    flow_extract(execute->packet, md, &key);
> >
> >     ovs_rwlock_rdlock(&dp->port_rwlock);
> >     dp_netdev_execute_actions(dp, &key, execute->packet, md,
> execute->actions,
> > @@ -1693,8 +1692,7 @@ dp_netdev_port_input(struct dp_netdev *dp, struct
> ofpbuf *packet,
> >     if (packet->size < ETH_HEADER_LEN) {
> >         return;
> >     }
> > -    flow_extract(packet, md->skb_priority, md->pkt_mark, &md->tunnel,
> > -                 (union flow_in_port *)&md->in_port, &key);
> > +    flow_extract(packet, md, &key);
> >     netdev_flow = dp_netdev_lookup_flow(dp, &key);
> >     if (netdev_flow) {
> >         struct dp_netdev_actions *actions;
> > diff --git a/lib/flow.c b/lib/flow.c
> > index e7fe4d3..3fa61a8 100644
> > --- a/lib/flow.c
> > +++ b/lib/flow.c
> > @@ -35,6 +35,7 @@
> > #include "ofpbuf.h"
> > #include "openflow/openflow.h"
> > #include "packets.h"
> > +#include "odp-util.h"
> > #include "random.h"
> > #include "unaligned.h"
> >
> > @@ -361,8 +362,7 @@ invalid:
> >
> > }
> >
> > -/* Initializes 'flow' members from 'packet', 'skb_priority', 'tnl', and
> > - * 'in_port'.
> > +/* Initializes 'flow' members from 'packet' and 'md'
> >  *
> >  * Initializes 'packet' header pointers as follows:
> >  *
> > @@ -381,8 +381,7 @@ invalid:
> >  *      present and has a correct length, and otherwise NULL.
> >  */
> > void
> > -flow_extract(struct ofpbuf *packet, uint32_t skb_priority, uint32_t
> pkt_mark,
> > -             const struct flow_tnl *tnl, const union flow_in_port
> *in_port,
> > +flow_extract(struct ofpbuf *packet, const struct pkt_metadata *md,
> >              struct flow *flow)
> > {
> >     struct ofpbuf b = *packet;
> > @@ -392,15 +391,12 @@ flow_extract(struct ofpbuf *packet, uint32_t
> skb_priority, uint32_t pkt_mark,
> >
> >     memset(flow, 0, sizeof *flow);
> >
> > -    if (tnl) {
> > -        ovs_assert(tnl != &flow->tunnel);
> > -        flow->tunnel = *tnl;
> > +    flow->tunnel = md->tunnel;
> > +    if (md->in_port != ODPP_NONE) {
> > +        flow->in_port.odp_port = md->in_port;
> >     }
> > -    if (in_port) {
> > -        flow->in_port = *in_port;
> > -    }
> > -    flow->skb_priority = skb_priority;
> > -    flow->pkt_mark = pkt_mark;
> > +    flow->skb_priority = md->skb_priority;
> > +    flow->pkt_mark = md->pkt_mark;
> >
>
> It seems it could be beneficial to accept the 'md' argument as NULL and
> leave the metadata zeroed in that case. There are a number of callers that
> would benefit from this (see below).
>
> >     packet->l2   = b.data;
> >     packet->l2_5 = NULL;
> > diff --git a/lib/flow.h b/lib/flow.h
> > index 3109a84..1e8d137 100644
> > --- a/lib/flow.h
> > +++ b/lib/flow.h
> > @@ -32,6 +32,7 @@ struct ds;
> > struct flow_wildcards;
> > struct minimask;
> > struct ofpbuf;
> > +struct pkt_metadata;
> >
> > /* This sequence number should be incremented whenever anything
> involving flows
> >  * or the wildcarding of flows changes.  This will cause build assertion
> > @@ -173,8 +174,7 @@ struct flow_metadata {
> >     ofp_port_t in_port;              /* OpenFlow port or zero. */
> > };
> >
> > -void flow_extract(struct ofpbuf *, uint32_t priority, uint32_t mark,
> > -                  const struct flow_tnl *, const union flow_in_port
> *in_port,
> > +void flow_extract(struct ofpbuf *, const struct pkt_metadata *md,
> >                   struct flow *);
> >
> > void flow_zero_wildcards(struct flow *, const struct flow_wildcards *);
> > diff --git a/lib/learning-switch.c b/lib/learning-switch.c
> > index 8efbce1..ef2976f 100644
> > --- a/lib/learning-switch.c
> > +++ b/lib/learning-switch.c
> > @@ -547,6 +547,7 @@ process_packet_in(struct lswitch *sw, const struct
> ofp_header *oh)
> >     struct ofputil_packet_in pi;
> >     uint32_t queue_id;
> >     ofp_port_t out_port;
> > +    struct pkt_metadata md;
>
> >
> >     uint64_t ofpacts_stub[64 / 8];
> >     struct ofpbuf ofpacts;
> > @@ -575,7 +576,8 @@ process_packet_in(struct lswitch *sw, const struct
> ofp_header *oh)
> >     /* Extract flow data from 'opi' into 'flow'. */
> >     ofpbuf_use_const(&pkt, pi.packet, pi.packet_len);
> >     in_port_.ofp_port = pi.fmd.in_port;
> > -    flow_extract(&pkt, 0, 0, NULL, &in_port_, &flow);
> > +    pkt_metadata_init(&md, &in_port_);
> > +    flow_extract(&pkt, &md,  &flow);
>
> Use NULL 'md' here, and set the port after extract:
>
>     flow.in_port_.ofp_port = pi.fmd.in_port;
>
> A rationale for this would be that pkt_metadata does not really exist in
> this case, as even the port number is an OpenFlow port number, rather than
> a datapath port number.
>
> >     flow.tunnel.tun_id = pi.fmd.tun_id;
> >
> >     /* Choose output port. */
> > diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> > index 4c89b36..06e64f6 100644
> > --- a/lib/ofp-print.c
> > +++ b/lib/ofp-print.c
> > @@ -46,6 +46,7 @@
> > #include "packets.h"
> > #include "type-props.h"
> > #include "unaligned.h"
> > +#include "odp-util.h"
> > #include "util.h"
> >
> > static void ofp_print_queue_name(struct ds *string, uint32_t port);
> > @@ -58,11 +59,12 @@ char *
> > ofp_packet_to_string(const void *data, size_t len)
> > {
> >     struct ds ds = DS_EMPTY_INITIALIZER;
> > +    const struct pkt_metadata md = PKT_METADATA_INITIALIZER(ODPP_NONE);
> >     struct ofpbuf buf;
> >     struct flow flow;
> >
> >     ofpbuf_use_const(&buf, data, len);
> > -    flow_extract(&buf, 0, 0, NULL, NULL, &flow);
> > +    flow_extract(&buf, &md, &flow);
>
> Use NULL 'md' argument instead.
>
> >     flow_format(&ds, &flow);
> >
> >     if (buf.l7) {
> > diff --git a/lib/packets.c b/lib/packets.c
> > index 7238f42..0286954 100644
> > --- a/lib/packets.c
> > +++ b/lib/packets.c
> > @@ -29,6 +29,7 @@
> > #include "dynamic-string.h"
> > #include "ofpbuf.h"
> > #include "ovs-thread.h"
> > +#include "odp-util.h"
> > #include "unaligned.h"
> >
> > const struct in6_addr in6addr_exact = IN6ADDR_EXACT_INIT;
> > @@ -991,3 +992,30 @@ packet_format_tcp_flags(struct ds *s, uint16_t
> tcp_flags)
> >         ds_put_cstr(s, "[800]");
> >     }
> > }
> > +
> > +void pkt_metadata_init(struct pkt_metadata *md,
> > +                       const union flow_in_port* in_port)
> > +{
> > +    pkt_metadata_init_full(md, NULL, 0, 0, in_port);
> > +}
>
> This might not be needed.
>
> > +
> > +void pkt_metadata_init_full(struct pkt_metadata *md, const struct
> flow_tnl *tnl,
> > +                            const uint32_t skb_priority,
> > +                            const uint32_t pkt_mark,
> > +                            const union flow_in_port *in_port)
> > +{
> > +
>
> Extra blank line here.
>
> > +    tnl ? memcpy(&md->tunnel, tnl, sizeof(md->tunnel))
> > +        : memset(&md->tunnel, 0, sizeof(md->tunnel));
> > +
> > +    md->skb_priority = skb_priority;
> > +    md->pkt_mark = pkt_mark;
> > +    md->in_port = in_port ? in_port->odp_port : ODPP_NONE;
> > +}
>
> This would be better named "pot_metadata_init()", once the previous
> function is removed.
>
> > +
> > +void pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow
> *flow)
> > +{
> > +
>
> Extra blanck line here.
>
> > +    pkt_metadata_init_full(md, &flow->tunnel, flow->skb_priority,
> > +                           flow->pkt_mark, &flow->in_port);
> > +}
> > diff --git a/lib/packets.h b/lib/packets.h
> > index 1855a1c..5e1d52d 100644
> > --- a/lib/packets.h
> > +++ b/lib/packets.h
> > @@ -42,6 +42,13 @@ struct pkt_metadata {
> > #define PKT_METADATA_INITIALIZER(PORT) \
> >     (struct pkt_metadata){ { 0, 0, 0, 0, 0, 0}, 0, 0, (PORT) }
> >
> > +void pkt_metadata_init(struct pkt_metadata *md, const union
> flow_in_port *in_port);
> > +void pkt_metadata_init_full(struct pkt_metadata *md, const struct
> flow_tnl *tnl,
> > +                            const uint32_t skb_priority,
> > +                            const uint32_t pkt_mark,
> > +                            const union flow_in_port *in_port);
> > +void pkt_metadata_from_flow(struct pkt_metadata *md, const struct flow
> *flow);
> > +
>
> > bool dpid_from_string(const char *s, uint64_t *dpidp);
> >
> > #define ETH_ADDR_LEN           6
> > diff --git a/ofproto/ofproto-dpif-upcall.c
> b/ofproto/ofproto-dpif-upcall.c
> > index e4f81a1..6cc0b67 100644
> > --- a/ofproto/ofproto-dpif-upcall.c
> > +++ b/ofproto/ofproto-dpif-upcall.c
> > @@ -983,9 +983,10 @@ handle_upcalls(struct handler *handler, struct list
> *upcalls)
> >         type = classify_upcall(upcall);
> >         if (type == MISS_UPCALL) {
> >             uint32_t hash;
> > +            struct pkt_metadata md;
> >
> > -            flow_extract(packet, flow.skb_priority, flow.pkt_mark,
> > -                         &flow.tunnel, &flow.in_port, &miss->flow);
> > +            pkt_metadata_from_flow(&md, &flow);
> > +            flow_extract(packet, &md, &miss->flow);
> >
> >             hash = flow_hash(&miss->flow, 0);
> >             existing_miss = flow_miss_find(&misses, ofproto, &miss->flow,
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 89d92af..0955d64 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -3184,12 +3184,15 @@ xlate_send_packet(const struct ofport_dpif
> *ofport, struct ofpbuf *packet)
> >     struct xport *xport;
> >     struct ofpact_output output;
> >     struct flow flow;
> > +    struct pkt_metadata md;
>
> >     union flow_in_port in_port_;
> >
> >     ofpact_init(&output.ofpact, OFPACT_OUTPUT, sizeof output);
> >     /* Use OFPP_NONE as the in_port to avoid special packet processing.
> */
> >     in_port_.ofp_port = OFPP_NONE;
> > -    flow_extract(packet, 0, 0, NULL, &in_port_, &flow);
> > +
> > +    pkt_metadata_init(&md, &in_port_);
> > +    flow_extract(packet, &md, &flow);
> >
>
> Use NULL 'md' and set the port to OFPP_NONE after extract.
>
> >     ovs_rwlock_rdlock(&xlate_rwlock);
> >     xport = xport_lookup(ofport);
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index c597114..6c48c18 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -3784,11 +3784,14 @@ parse_flow_and_packet(int argc, const char
> *argv[],
> >             flow_compose(packet, flow);
> >         } else {
> >             union flow_in_port in_port = flow->in_port;
> > +            struct pkt_metadata md;
> >
> >             /* Use the metadata from the flow and the packet argument
> >              * to reconstruct the flow. */
> > -            flow_extract(packet, flow->skb_priority, flow->pkt_mark,
> NULL,
> > -                         &in_port, flow);
> > +            pkt_metadata_init_full(&md, NULL, flow->skb_priority,
> > +                                   flow->pkt_mark, &in_port);
> > +
>
> Seems you could use the pkt_metadata_from_flow() here? I guess even the
> flow->tunnel is zeroes here?
>
> > +            flow_extract(packet, &md, flow);
> >         }
> >     }
> >
> > diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
> > index 7117e1f..e400b32 100644
> > --- a/ofproto/ofproto.c
> > +++ b/ofproto/ofproto.c
> > @@ -2715,9 +2715,11 @@ run_rule_executes(struct ofproto *ofproto)
> >     LIST_FOR_EACH_SAFE (e, next, list_node, &executes) {
> >         union flow_in_port in_port_;
> >         struct flow flow;
> > +        struct pkt_metadata md;
> >
> >         in_port_.ofp_port = e->in_port;
> > -        flow_extract(e->packet, 0, 0, NULL, &in_port_, &flow);
> > +        pkt_metadata_init(&md, &in_port_);
> > +        flow_extract(e->packet, &md, &flow);
>
> Use NULL 'md' and set the port number here:
>
>      flow.in_port_.ofp_port = e->in_port;
>
> >         ofproto->ofproto_class->rule_execute(e->rule, &flow, e->packet);
> >
> >         rule_execute_destroy(e);
> > @@ -2928,6 +2930,7 @@ handle_packet_out(struct ofconn *ofconn, const
> struct ofp_header *oh)
> >     struct flow flow;
> >     union flow_in_port in_port_;
> >     enum ofperr error;
> > +    struct pkt_metadata md;
> >
> >     COVERAGE_INC(ofproto_packet_out);
> >
> > @@ -2961,7 +2964,8 @@ handle_packet_out(struct ofconn *ofconn, const
> struct ofp_header *oh)
> >
> >     /* Verify actions against packet, then send packet if successful. */
> >     in_port_.ofp_port = po.in_port;
> > -    flow_extract(payload, 0, 0, NULL, &in_port_, &flow);
> > +    pkt_metadata_init(&md, &in_port_);
> > +    flow_extract(payload, &md, &flow);
>
> Use NULL 'md' and set the port after.
>
> >     error = ofproto_check_ofpacts(p, po.ofpacts, po.ofpacts_len);
> >     if (!error) {
> >         error = p->ofproto_class->packet_out(p, payload, &flow,
> > diff --git a/tests/test-flows.c b/tests/test-flows.c
> > index 2910035..7907523 100644
> > --- a/tests/test-flows.c
> > +++ b/tests/test-flows.c
> > @@ -58,6 +58,7 @@ main(int argc OVS_UNUSED, char *argv[])
> >         struct ofp10_match extracted_match;
> >         struct match match;
> >         struct flow flow;
> > +        struct pkt_metadata md;
> >         union flow_in_port in_port_;
> >         n++;
> >
> > @@ -69,7 +70,8 @@ main(int argc OVS_UNUSED, char *argv[])
> >         }
> >
> >         in_port_.ofp_port = u16_to_ofp(1);
> > -        flow_extract(packet, 0, 0, NULL, &in_port_, &flow);
> > +        pkt_metadata_init(&md, &in_port_);
> > +        flow_extract(packet, &md, &flow);
>
> Use NULL 'md' and set the port after.
>
> >         match_wc_init(&match, &flow);
> >         ofputil_match_to_ofp10_match(&match, &extracted_match);
> >
> > diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> > index 4ab9ca4..e62e646 100644
> > --- a/utilities/ovs-ofctl.c
> > +++ b/utilities/ovs-ofctl.c
> > @@ -1864,12 +1864,13 @@ ofctl_ofp_parse_pcap(int argc OVS_UNUSED, char
> *argv[])
> >         struct ofpbuf *packet;
> >         long long int when;
> >         struct flow flow;
> > +        const struct pkt_metadata md =
> PKT_METADATA_INITIALIZER(ODPP_NONE);
> >
> >         error = ovs_pcap_read(file, &packet, &when);
> >         if (error) {
> >             break;
> >         }
> > -        flow_extract(packet, 0, 0, NULL, NULL, &flow);
> > +        flow_extract(packet, &md, &flow);
>
> Use NULL 'md' instead.
>
> >         if (flow.dl_type == htons(ETH_TYPE_IP)
> >             && flow.nw_proto == IPPROTO_TCP
> >             && (is_openflow_port(flow.tp_src, argv + 2) ||
> > @@ -3208,6 +3209,7 @@ ofctl_parse_pcap(int argc OVS_UNUSED, char *argv[])
> >     for (;;) {
> >         struct ofpbuf *packet;
> >         struct flow flow;
> > +        const struct pkt_metadata md =
> PKT_METADATA_INITIALIZER(ODPP_NONE);
> >         int error;
> >
> >         error = ovs_pcap_read(pcap, &packet, NULL);
> > @@ -3217,7 +3219,7 @@ ofctl_parse_pcap(int argc OVS_UNUSED, char *argv[])
> >             ovs_fatal(error, "%s: read failed", argv[1]);
> >         }
> >
> > -        flow_extract(packet, 0, 0, NULL, NULL, &flow);
> > +        flow_extract(packet, &md, &flow);
>
> Use NULL 'md' instead.
>
> >         flow_print(stdout, &flow);
> >         putchar('\n');
> >         ofpbuf_delete(packet);
> > --
> > 1.7.9.5
> >
> > _______________________________________________
> > dev mailing list
> > dev at openvswitch.org
> > http://openvswitch.org/mailman/listinfo/dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://openvswitch.org/pipermail/dev/attachments/20140228/1065110b/attachment-0001.html>


More information about the dev mailing list