| Message ID | 1552380103-32599-1-git-send-email-ophirmu@mellanox.com |
|---|---|
| State | Deferred |
| Delegated to: | Ian Stokes |
| Headers | show |
| Series | [ovs-dev,v1] dpif: Add marked packets stats | expand |
On 12.03.2019 11:41, Ophir Munk wrote: > This commit adds marked packets statistics. Following commit [1], > received packets can be marked with a mark id which is associated > with the flow on which the packets were recieved. This commits counts > the marked packets per flow and displays the result when executing > datapath dump-flow command with a "-m" (more verbosity) option, as > shown in [2]. > Packets are marked only when hw-offload option is enabled and only on > some netdev classes such as netdev-dpdk. In all other cases marked > packets are not counted and are not displayed when executing the command > in [2]. > > [1] > Commit 241bad15d9 ("dpif-netdev: associate flow with a mark id") > > [2] > ovs-appctl dpif/dump-flows -m <bridge-name> > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > --- Hi Ophir. Thanks for working on this, but I don't think that flow stats is the right place to expose information like that. We already have pmd-stats/perf-show appctl calls that prints various hit stats for PMD threads. And, I think, 'flow mark hits' or something similar should be one of these stats. Right now flow mark hits are not accounted there and that is the issue. Regarding the flow stats, we probably need to expose status of the flow, i.e. if it was offloaded or not, like it done for tc offloading. Right now all the flows reported by dpif-netdev as not offloaded. See 'flow->attrs.offloaded = false;' in dp_netdev_flow_to_dpif_flow(). And we probably need to change this from boolean to some 'yes/no/partial'. BTW, your current implementation assumes that packets had flow mark if 'flow->mark' is set, but this is could be not true, because packets could arrive while offloading is in progress or offloading could be started between packets arrival and finishing of their processing. Best regards, Ilya Maximets. > lib/dpif-netdev.c | 16 ++++++++++++++++ > lib/dpif.c | 9 +++++++++ > lib/dpif.h | 3 +++ > ofproto/ofproto-dpif.c | 4 ++++ > 4 files changed, 32 insertions(+) > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > index 4d6d0c3..ef2f8f1 100644 > --- a/lib/dpif-netdev.c > +++ b/lib/dpif-netdev.c > @@ -486,6 +486,8 @@ struct dp_netdev_flow_stats { > atomic_ullong packet_count; /* Number of packets matched. */ > atomic_ullong byte_count; /* Number of bytes matched. */ > atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. */ > + /* Offload stats. */ > + atomic_ullong mark_count; /* Number of marked packets matched. */ > }; > > /* A flow in 'dp_netdev_pmd_thread's 'flow_table'. > @@ -3008,6 +3010,9 @@ get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow_, > stats->used = used; > atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags); > stats->tcp_flags = flags; > + /* Offload stats. */ > + atomic_read_relaxed(&netdev_flow->stats.mark_count, &n); > + stats->n_marked = n; > } > > /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for > @@ -6124,6 +6129,15 @@ dp_netdev_flow_used(struct dp_netdev_flow *netdev_flow, int cnt, int size, > atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags); > } > > +static void > +dp_netdev_flow_marked(struct dp_netdev_flow *netdev_flow, uint32_t mark, > + int count) > +{ > + if (mark != INVALID_FLOW_MARK) { > + non_atomic_ullong_add(&netdev_flow->stats.mark_count, count); > + } > +} > + > static int > dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet *packet_, > struct flow *flow, struct flow_wildcards *wc, ovs_u128 *ufid, > @@ -6244,6 +6258,8 @@ packet_batch_per_flow_execute(struct packet_batch_per_flow *batch, > dp_netdev_flow_used(flow, batch->array.count, batch->byte_count, > batch->tcp_flags, pmd->ctx.now / 1000); > > + dp_netdev_flow_marked(flow, batch->flow->mark, batch->array.count); > + > actions = dp_netdev_flow_get_actions(flow); > > dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow, > diff --git a/lib/dpif.c b/lib/dpif.c > index 457c9bf..bb077a5 100644 > --- a/lib/dpif.c > +++ b/lib/dpif.c > @@ -880,9 +880,18 @@ dpif_flow_stats_extract(const struct flow *flow, const struct dp_packet *packet, > stats->tcp_flags = ntohs(flow->tcp_flags); > stats->n_bytes = dp_packet_size(packet); > stats->n_packets = 1; > + stats->n_marked = 0; > stats->used = used; > } > > +void > +dpif_offload_stats_format(const struct dpif_flow_stats *stats, struct ds *s) > +{ > + if (stats->n_marked) { > + ds_put_format(s, "marked:%"PRIu64", ", stats->n_marked); > + } > +} > + > /* Appends a human-readable representation of 'stats' to 's'. */ > void > dpif_flow_stats_format(const struct dpif_flow_stats *stats, struct ds *s) > diff --git a/lib/dpif.h b/lib/dpif.h > index 475d5a6..7c22afd 100644 > --- a/lib/dpif.h > +++ b/lib/dpif.h > @@ -492,6 +492,8 @@ struct dpif_flow_stats { > uint64_t n_bytes; > long long int used; > uint16_t tcp_flags; > + /* Offload stats. */ > + uint64_t n_marked; > }; > > struct dpif_flow_attrs { > @@ -507,6 +509,7 @@ struct dpif_flow_dump_types { > void dpif_flow_stats_extract(const struct flow *, const struct dp_packet *packet, > long long int used, struct dpif_flow_stats *); > void dpif_flow_stats_format(const struct dpif_flow_stats *, struct ds *); > +void dpif_offload_stats_format(const struct dpif_flow_stats *, struct ds *); > > enum dpif_flow_put_flags { > DPIF_FP_CREATE = 1 << 0, /* Allow creating a new flow. */ > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > index 21dd54b..af37ecb 100644 > --- a/ofproto/ofproto-dpif.c > +++ b/ofproto/ofproto-dpif.c > @@ -4416,6 +4416,7 @@ rule_construct(struct rule *rule_) > rule->stats.n_packets = 0; > rule->stats.n_bytes = 0; > rule->stats.used = rule->up.modified; > + rule->stats.n_marked = 0; > rule->recirc_id = 0; > rule->new_rule = NULL; > rule->forward_counts = false; > @@ -5709,6 +5710,9 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn, > odp_flow_format(f.key, f.key_len, f.mask, f.mask_len, > portno_names, &ds, verbosity); > ds_put_cstr(&ds, ", "); > + if (verbosity) { > + dpif_offload_stats_format(&f.stats, &ds); > + } > dpif_flow_stats_format(&f.stats, &ds); > ds_put_cstr(&ds, ", actions:"); > format_odp_actions(&ds, f.actions, f.actions_len, portno_names); >
> -----Original Message----- > From: Ilya Maximets <i.maximets@samsung.com> > Sent: Tuesday, March 12, 2019 11:11 AM > To: Ophir Munk <ophirmu@mellanox.com>; ovs-dev@openvswitch.org > Cc: Asaf Penso <asafp@mellanox.com>; Ian Stokes <ian.stokes@intel.com>; > Shahaf Shuler <shahafs@mellanox.com>; Olga Shern > <olgas@mellanox.com>; Kevin Traynor <ktraynor@redhat.com>; Roni Bar > Yanai <roniba@mellanox.com> > Subject: Re: [PATCH v1] dpif: Add marked packets stats > > On 12.03.2019 11:41, Ophir Munk wrote: > > This commit adds marked packets statistics. Following commit [1], > > received packets can be marked with a mark id which is associated > > with the flow on which the packets were recieved. This commits counts > > the marked packets per flow and displays the result when executing > > datapath dump-flow command with a "-m" (more verbosity) option, as > > shown in [2]. > > Packets are marked only when hw-offload option is enabled and only on > > some netdev classes such as netdev-dpdk. In all other cases marked > > packets are not counted and are not displayed when executing the > command > > in [2]. > > > > [1] > > Commit 241bad15d9 ("dpif-netdev: associate flow with a mark id") > > > > [2] > > ovs-appctl dpif/dump-flows -m <bridge-name> > > > > Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > > --- > > Hi Ophir. > > Thanks for working on this, but I don't think that flow stats is the right > place to expose information like that. We already have pmd-stats/perf-show > appctl calls that prints various hit stats for PMD threads. And, I think, > 'flow mark hits' or something similar should be one of these stats. > Right now flow mark hits are not accounted there and that is the issue. > > Regarding the flow stats, we probably need to expose status of the flow, > i.e. if it was offloaded or not, like it done for tc offloading. > Right now all the flows reported by dpif-netdev as not offloaded. > See 'flow->attrs.offloaded = false;' in dp_netdev_flow_to_dpif_flow(). > And we probably need to change this from boolean to some 'yes/no/partial'. > > BTW, your current implementation assumes that packets had flow mark if > 'flow->mark' is set, but this is could be not true, because packets could > arrive while offloading is in progress or offloading could be started > between packets arrival and finishing of their processing. > > Best regards, Ilya Maximets. Hi Ilya, We agree that flow should be marked as offloaded yes/no/partial, but seems this is not enough. Like you said, since hardware offload is executed in a different thread, it might be that the number of packets that were marked/handled by hardware is different than the total packets of the flow. If there is a load on the offload or just high latency, it might be that in some cases the difference will be significant. Although I agree that you will probably be able to see the problem In the pmd-stats (assuming mark will be added there), ,it will be less clear. For example, there are probably flows that cannot be offloaded at all so you won't expect mark count for them. Another example is a bug, we say that the flow was offloaded but in fact it is not. Of course you should expect the code to be tested, but the field is always different. Maybe we can add it we special flag for hardware offload specific information. Thanks > > > lib/dpif-netdev.c | 16 ++++++++++++++++ > > lib/dpif.c | 9 +++++++++ > > lib/dpif.h | 3 +++ > > ofproto/ofproto-dpif.c | 4 ++++ > > 4 files changed, 32 insertions(+) > > > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > > index 4d6d0c3..ef2f8f1 100644 > > --- a/lib/dpif-netdev.c > > +++ b/lib/dpif-netdev.c > > @@ -486,6 +486,8 @@ struct dp_netdev_flow_stats { > > atomic_ullong packet_count; /* Number of packets matched. */ > > atomic_ullong byte_count; /* Number of bytes matched. */ > > atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. */ > > + /* Offload stats. */ > > + atomic_ullong mark_count; /* Number of marked packets matched. > */ > > }; > > > > /* A flow in 'dp_netdev_pmd_thread's 'flow_table'. > > @@ -3008,6 +3010,9 @@ get_dpif_flow_stats(const struct > dp_netdev_flow *netdev_flow_, > > stats->used = used; > > atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags); > > stats->tcp_flags = flags; > > + /* Offload stats. */ > > + atomic_read_relaxed(&netdev_flow->stats.mark_count, &n); > > + stats->n_marked = n; > > } > > > > /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for > > @@ -6124,6 +6129,15 @@ dp_netdev_flow_used(struct dp_netdev_flow > *netdev_flow, int cnt, int size, > > atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags); > > } > > > > +static void > > +dp_netdev_flow_marked(struct dp_netdev_flow *netdev_flow, > uint32_t mark, > > + int count) > > +{ > > + if (mark != INVALID_FLOW_MARK) { > > + non_atomic_ullong_add(&netdev_flow->stats.mark_count, count); > > + } > > +} > > + > > static int > > dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet > *packet_, > > struct flow *flow, struct flow_wildcards *wc, ovs_u128 *ufid, > > @@ -6244,6 +6258,8 @@ packet_batch_per_flow_execute(struct > packet_batch_per_flow *batch, > > dp_netdev_flow_used(flow, batch->array.count, batch->byte_count, > > batch->tcp_flags, pmd->ctx.now / 1000); > > > > + dp_netdev_flow_marked(flow, batch->flow->mark, batch- > >array.count); > > + > > actions = dp_netdev_flow_get_actions(flow); > > > > dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow, > > diff --git a/lib/dpif.c b/lib/dpif.c > > index 457c9bf..bb077a5 100644 > > --- a/lib/dpif.c > > +++ b/lib/dpif.c > > @@ -880,9 +880,18 @@ dpif_flow_stats_extract(const struct flow *flow, > const struct dp_packet *packet, > > stats->tcp_flags = ntohs(flow->tcp_flags); > > stats->n_bytes = dp_packet_size(packet); > > stats->n_packets = 1; > > + stats->n_marked = 0; > > stats->used = used; > > } > > > > +void > > +dpif_offload_stats_format(const struct dpif_flow_stats *stats, struct ds > *s) > > +{ > > + if (stats->n_marked) { > > + ds_put_format(s, "marked:%"PRIu64", ", stats->n_marked); > > + } > > +} > > + > > /* Appends a human-readable representation of 'stats' to 's'. */ > > void > > dpif_flow_stats_format(const struct dpif_flow_stats *stats, struct ds *s) > > diff --git a/lib/dpif.h b/lib/dpif.h > > index 475d5a6..7c22afd 100644 > > --- a/lib/dpif.h > > +++ b/lib/dpif.h > > @@ -492,6 +492,8 @@ struct dpif_flow_stats { > > uint64_t n_bytes; > > long long int used; > > uint16_t tcp_flags; > > + /* Offload stats. */ > > + uint64_t n_marked; > > }; > > > > struct dpif_flow_attrs { > > @@ -507,6 +509,7 @@ struct dpif_flow_dump_types { > > void dpif_flow_stats_extract(const struct flow *, const struct dp_packet > *packet, > > long long int used, struct dpif_flow_stats *); > > void dpif_flow_stats_format(const struct dpif_flow_stats *, struct ds *); > > +void dpif_offload_stats_format(const struct dpif_flow_stats *, struct ds > *); > > > > enum dpif_flow_put_flags { > > DPIF_FP_CREATE = 1 << 0, /* Allow creating a new flow. */ > > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > > index 21dd54b..af37ecb 100644 > > --- a/ofproto/ofproto-dpif.c > > +++ b/ofproto/ofproto-dpif.c > > @@ -4416,6 +4416,7 @@ rule_construct(struct rule *rule_) > > rule->stats.n_packets = 0; > > rule->stats.n_bytes = 0; > > rule->stats.used = rule->up.modified; > > + rule->stats.n_marked = 0; > > rule->recirc_id = 0; > > rule->new_rule = NULL; > > rule->forward_counts = false; > > @@ -5709,6 +5710,9 @@ ofproto_unixctl_dpif_dump_flows(struct > unixctl_conn *conn, > > odp_flow_format(f.key, f.key_len, f.mask, f.mask_len, > > portno_names, &ds, verbosity); > > ds_put_cstr(&ds, ", "); > > + if (verbosity) { > > + dpif_offload_stats_format(&f.stats, &ds); > > + } > > dpif_flow_stats_format(&f.stats, &ds); > > ds_put_cstr(&ds, ", actions:"); > > format_odp_actions(&ds, f.actions, f.actions_len, portno_names); > >
On 12.03.2019 17:53, Roni Bar Yanai wrote: > > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: Tuesday, March 12, 2019 11:11 AM >> To: Ophir Munk <ophirmu@mellanox.com>; ovs-dev@openvswitch.org >> Cc: Asaf Penso <asafp@mellanox.com>; Ian Stokes <ian.stokes@intel.com>; >> Shahaf Shuler <shahafs@mellanox.com>; Olga Shern >> <olgas@mellanox.com>; Kevin Traynor <ktraynor@redhat.com>; Roni Bar >> Yanai <roniba@mellanox.com> >> Subject: Re: [PATCH v1] dpif: Add marked packets stats >> >> On 12.03.2019 11:41, Ophir Munk wrote: >>> This commit adds marked packets statistics. Following commit [1], >>> received packets can be marked with a mark id which is associated >>> with the flow on which the packets were recieved. This commits counts >>> the marked packets per flow and displays the result when executing >>> datapath dump-flow command with a "-m" (more verbosity) option, as >>> shown in [2]. >>> Packets are marked only when hw-offload option is enabled and only on >>> some netdev classes such as netdev-dpdk. In all other cases marked >>> packets are not counted and are not displayed when executing the >> command >>> in [2]. >>> >>> [1] >>> Commit 241bad15d9 ("dpif-netdev: associate flow with a mark id") >>> >>> [2] >>> ovs-appctl dpif/dump-flows -m <bridge-name> >>> >>> Signed-off-by: Ophir Munk <ophirmu@mellanox.com> >>> --- >> >> Hi Ophir. >> >> Thanks for working on this, but I don't think that flow stats is the right >> place to expose information like that. We already have pmd-stats/perf-show >> appctl calls that prints various hit stats for PMD threads. And, I think, >> 'flow mark hits' or something similar should be one of these stats. >> Right now flow mark hits are not accounted there and that is the issue. >> >> Regarding the flow stats, we probably need to expose status of the flow, >> i.e. if it was offloaded or not, like it done for tc offloading. >> Right now all the flows reported by dpif-netdev as not offloaded. >> See 'flow->attrs.offloaded = false;' in dp_netdev_flow_to_dpif_flow(). >> And we probably need to change this from boolean to some 'yes/no/partial'. >> >> BTW, your current implementation assumes that packets had flow mark if >> 'flow->mark' is set, but this is could be not true, because packets could >> arrive while offloading is in progress or offloading could be started >> between packets arrival and finishing of their processing. >> >> Best regards, Ilya Maximets. > > Hi Ilya, > > We agree that flow should be marked as offloaded yes/no/partial, but seems this is not enough. > Like you said, since hardware offload is executed in a different thread, it might be that the number > of packets that were marked/handled by hardware is different than the total packets of the flow. > If there is a load on the offload or just high latency, it might be that in some cases the difference will be significant. > Although I agree that you will probably be able to see the problem In the pmd-stats (assuming mark will be added there), > ,it will be less clear. For example, there are probably flows that cannot be offloaded at all so you won't expect > mark count for them. Another example is a bug, we say that the flow was offloaded but in fact it is not. > Of course you should expect the code to be tested, but the field is always different. > Maybe we can add it we special flag for hardware offload specific information. Not sure if I understand your point correctly, but isn't it expected that if flow is offloaded than all the packets through the flow had flow mark? In case of a bug it's most likely that it happened after the flow mark association and your flow stats will be wrong anyway. The only trusted source of the number of "offloaded" packets could be the number of real packets that had flow mark set on receive. And these stats should be displayed by PMD stats. If everything works as expected, all the packets for offloaded flow will be marked (except first few of them). If something will go wrong, we can't trust these flow stats. We can rely only on PMD stats. So, I don't see the profit from having these special flow stats. > > Thanks >> >>> lib/dpif-netdev.c | 16 ++++++++++++++++ >>> lib/dpif.c | 9 +++++++++ >>> lib/dpif.h | 3 +++ >>> ofproto/ofproto-dpif.c | 4 ++++ >>> 4 files changed, 32 insertions(+) >>> >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>> index 4d6d0c3..ef2f8f1 100644 >>> --- a/lib/dpif-netdev.c >>> +++ b/lib/dpif-netdev.c >>> @@ -486,6 +486,8 @@ struct dp_netdev_flow_stats { >>> atomic_ullong packet_count; /* Number of packets matched. */ >>> atomic_ullong byte_count; /* Number of bytes matched. */ >>> atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. */ >>> + /* Offload stats. */ >>> + atomic_ullong mark_count; /* Number of marked packets matched. >> */ >>> }; >>> >>> /* A flow in 'dp_netdev_pmd_thread's 'flow_table'. >>> @@ -3008,6 +3010,9 @@ get_dpif_flow_stats(const struct >> dp_netdev_flow *netdev_flow_, >>> stats->used = used; >>> atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags); >>> stats->tcp_flags = flags; >>> + /* Offload stats. */ >>> + atomic_read_relaxed(&netdev_flow->stats.mark_count, &n); >>> + stats->n_marked = n; >>> } >>> >>> /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for >>> @@ -6124,6 +6129,15 @@ dp_netdev_flow_used(struct dp_netdev_flow >> *netdev_flow, int cnt, int size, >>> atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags); >>> } >>> >>> +static void >>> +dp_netdev_flow_marked(struct dp_netdev_flow *netdev_flow, >> uint32_t mark, >>> + int count) >>> +{ >>> + if (mark != INVALID_FLOW_MARK) { >>> + non_atomic_ullong_add(&netdev_flow->stats.mark_count, count); >>> + } >>> +} >>> + >>> static int >>> dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet >> *packet_, >>> struct flow *flow, struct flow_wildcards *wc, ovs_u128 *ufid, >>> @@ -6244,6 +6258,8 @@ packet_batch_per_flow_execute(struct >> packet_batch_per_flow *batch, >>> dp_netdev_flow_used(flow, batch->array.count, batch->byte_count, >>> batch->tcp_flags, pmd->ctx.now / 1000); >>> >>> + dp_netdev_flow_marked(flow, batch->flow->mark, batch- >>> array.count); >>> + >>> actions = dp_netdev_flow_get_actions(flow); >>> >>> dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow, >>> diff --git a/lib/dpif.c b/lib/dpif.c >>> index 457c9bf..bb077a5 100644 >>> --- a/lib/dpif.c >>> +++ b/lib/dpif.c >>> @@ -880,9 +880,18 @@ dpif_flow_stats_extract(const struct flow *flow, >> const struct dp_packet *packet, >>> stats->tcp_flags = ntohs(flow->tcp_flags); >>> stats->n_bytes = dp_packet_size(packet); >>> stats->n_packets = 1; >>> + stats->n_marked = 0; >>> stats->used = used; >>> } >>> >>> +void >>> +dpif_offload_stats_format(const struct dpif_flow_stats *stats, struct ds >> *s) >>> +{ >>> + if (stats->n_marked) { >>> + ds_put_format(s, "marked:%"PRIu64", ", stats->n_marked); >>> + } >>> +} >>> + >>> /* Appends a human-readable representation of 'stats' to 's'. */ >>> void >>> dpif_flow_stats_format(const struct dpif_flow_stats *stats, struct ds *s) >>> diff --git a/lib/dpif.h b/lib/dpif.h >>> index 475d5a6..7c22afd 100644 >>> --- a/lib/dpif.h >>> +++ b/lib/dpif.h >>> @@ -492,6 +492,8 @@ struct dpif_flow_stats { >>> uint64_t n_bytes; >>> long long int used; >>> uint16_t tcp_flags; >>> + /* Offload stats. */ >>> + uint64_t n_marked; >>> }; >>> >>> struct dpif_flow_attrs { >>> @@ -507,6 +509,7 @@ struct dpif_flow_dump_types { >>> void dpif_flow_stats_extract(const struct flow *, const struct dp_packet >> *packet, >>> long long int used, struct dpif_flow_stats *); >>> void dpif_flow_stats_format(const struct dpif_flow_stats *, struct ds *); >>> +void dpif_offload_stats_format(const struct dpif_flow_stats *, struct ds >> *); >>> >>> enum dpif_flow_put_flags { >>> DPIF_FP_CREATE = 1 << 0, /* Allow creating a new flow. */ >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>> index 21dd54b..af37ecb 100644 >>> --- a/ofproto/ofproto-dpif.c >>> +++ b/ofproto/ofproto-dpif.c >>> @@ -4416,6 +4416,7 @@ rule_construct(struct rule *rule_) >>> rule->stats.n_packets = 0; >>> rule->stats.n_bytes = 0; >>> rule->stats.used = rule->up.modified; >>> + rule->stats.n_marked = 0; >>> rule->recirc_id = 0; >>> rule->new_rule = NULL; >>> rule->forward_counts = false; >>> @@ -5709,6 +5710,9 @@ ofproto_unixctl_dpif_dump_flows(struct >> unixctl_conn *conn, >>> odp_flow_format(f.key, f.key_len, f.mask, f.mask_len, >>> portno_names, &ds, verbosity); >>> ds_put_cstr(&ds, ", "); >>> + if (verbosity) { >>> + dpif_offload_stats_format(&f.stats, &ds); >>> + } >>> dpif_flow_stats_format(&f.stats, &ds); >>> ds_put_cstr(&ds, ", actions:"); >>> format_odp_actions(&ds, f.actions, f.actions_len, portno_names); >>>
> -----Original Message----- > From: Ilya Maximets <i.maximets@samsung.com> > Sent: Tuesday, March 12, 2019 5:25 PM > To: Roni Bar Yanai <roniba@mellanox.com>; Ophir Munk > <ophirmu@mellanox.com>; ovs-dev@openvswitch.org > Cc: Asaf Penso <asafp@mellanox.com>; Ian Stokes <ian.stokes@intel.com>; > Shahaf Shuler <shahafs@mellanox.com>; Olga Shern > <olgas@mellanox.com>; Kevin Traynor <ktraynor@redhat.com> > Subject: Re: [PATCH v1] dpif: Add marked packets stats > > On 12.03.2019 17:53, Roni Bar Yanai wrote: > > > > > >> -----Original Message----- > >> From: Ilya Maximets <i.maximets@samsung.com> > >> Sent: Tuesday, March 12, 2019 11:11 AM > >> To: Ophir Munk <ophirmu@mellanox.com>; ovs-dev@openvswitch.org > >> Cc: Asaf Penso <asafp@mellanox.com>; Ian Stokes > <ian.stokes@intel.com>; > >> Shahaf Shuler <shahafs@mellanox.com>; Olga Shern > >> <olgas@mellanox.com>; Kevin Traynor <ktraynor@redhat.com>; Roni Bar > >> Yanai <roniba@mellanox.com> > >> Subject: Re: [PATCH v1] dpif: Add marked packets stats > >> > >> On 12.03.2019 11:41, Ophir Munk wrote: > >>> This commit adds marked packets statistics. Following commit [1], > >>> received packets can be marked with a mark id which is associated > >>> with the flow on which the packets were recieved. This commits counts > >>> the marked packets per flow and displays the result when executing > >>> datapath dump-flow command with a "-m" (more verbosity) option, as > >>> shown in [2]. > >>> Packets are marked only when hw-offload option is enabled and only on > >>> some netdev classes such as netdev-dpdk. In all other cases marked > >>> packets are not counted and are not displayed when executing the > >> command > >>> in [2]. > >>> > >>> [1] > >>> Commit 241bad15d9 ("dpif-netdev: associate flow with a mark id") > >>> > >>> [2] > >>> ovs-appctl dpif/dump-flows -m <bridge-name> > >>> > >>> Signed-off-by: Ophir Munk <ophirmu@mellanox.com> > >>> --- > >> > >> Hi Ophir. > >> > >> Thanks for working on this, but I don't think that flow stats is the right > >> place to expose information like that. We already have pmd-stats/perf- > show > >> appctl calls that prints various hit stats for PMD threads. And, I think, > >> 'flow mark hits' or something similar should be one of these stats. > >> Right now flow mark hits are not accounted there and that is the issue. > >> > >> Regarding the flow stats, we probably need to expose status of the flow, > >> i.e. if it was offloaded or not, like it done for tc offloading. > >> Right now all the flows reported by dpif-netdev as not offloaded. > >> See 'flow->attrs.offloaded = false;' in dp_netdev_flow_to_dpif_flow(). > >> And we probably need to change this from boolean to some > 'yes/no/partial'. > >> > >> BTW, your current implementation assumes that packets had flow mark if > >> 'flow->mark' is set, but this is could be not true, because packets could > >> arrive while offloading is in progress or offloading could be started > >> between packets arrival and finishing of their processing. > >> > >> Best regards, Ilya Maximets. > > > > Hi Ilya, > > > > We agree that flow should be marked as offloaded yes/no/partial, but > seems this is not enough. > > Like you said, since hardware offload is executed in a different thread, it > might be that the number > > of packets that were marked/handled by hardware is different than the > total packets of the flow. > > If there is a load on the offload or just high latency, it might be that in some > cases the difference will be significant. > > Although I agree that you will probably be able to see the problem In the > pmd-stats (assuming mark will be added there), > > ,it will be less clear. For example, there are probably flows that cannot be > offloaded at all so you won't expect > > mark count for them. Another example is a bug, we say that the flow was > offloaded but in fact it is not. > > Of course you should expect the code to be tested, but the field is always > different. > > Maybe we can add it we special flag for hardware offload specific > information. > > Not sure if I understand your point correctly, but isn't it expected that > if flow is offloaded than all the packets through the flow had flow mark? > In case of a bug it's most likely that it happened after the flow mark > association and your flow stats will be wrong anyway. > The only trusted source of the number of "offloaded" packets could be the > number of real packets that had flow mark set on receive. And these stats > should be displayed by PMD stats. > > If everything works as expected, all the packets for offloaded flow will be > marked (except first few of them). > If something will go wrong, we can't trust these flow stats. We can rely only Why you can't trust the flow stats? This is the only point in the code where you know that the flow was really handled by hardware. > on PMD stats. > So, I don't see the profit from having these special flow stats. > I'm looking at it from the OVS user point of view. I have thousands of flows running. If the statistics is only in the PMD I can see how many packets were marked and how many we're not. Let's say the ratio is not that good, how do I look deeper? Is it because for some flows there is bug on matching? Is it because of the latency in adding flows? Is it because some flow pattern is not supported at all? Agree this is only needed for debugging, but I don't see other way user can get this information. > > > > Thanks > >> > >>> lib/dpif-netdev.c | 16 ++++++++++++++++ > >>> lib/dpif.c | 9 +++++++++ > >>> lib/dpif.h | 3 +++ > >>> ofproto/ofproto-dpif.c | 4 ++++ > >>> 4 files changed, 32 insertions(+) > >>> > >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c > >>> index 4d6d0c3..ef2f8f1 100644 > >>> --- a/lib/dpif-netdev.c > >>> +++ b/lib/dpif-netdev.c > >>> @@ -486,6 +486,8 @@ struct dp_netdev_flow_stats { > >>> atomic_ullong packet_count; /* Number of packets matched. */ > >>> atomic_ullong byte_count; /* Number of bytes matched. */ > >>> atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. > */ > >>> + /* Offload stats. */ > >>> + atomic_ullong mark_count; /* Number of marked packets > matched. > >> */ > >>> }; > >>> > >>> /* A flow in 'dp_netdev_pmd_thread's 'flow_table'. > >>> @@ -3008,6 +3010,9 @@ get_dpif_flow_stats(const struct > >> dp_netdev_flow *netdev_flow_, > >>> stats->used = used; > >>> atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags); > >>> stats->tcp_flags = flags; > >>> + /* Offload stats. */ > >>> + atomic_read_relaxed(&netdev_flow->stats.mark_count, &n); > >>> + stats->n_marked = n; > >>> } > >>> > >>> /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for > >>> @@ -6124,6 +6129,15 @@ dp_netdev_flow_used(struct > dp_netdev_flow > >> *netdev_flow, int cnt, int size, > >>> atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags); > >>> } > >>> > >>> +static void > >>> +dp_netdev_flow_marked(struct dp_netdev_flow *netdev_flow, > >> uint32_t mark, > >>> + int count) > >>> +{ > >>> + if (mark != INVALID_FLOW_MARK) { > >>> + non_atomic_ullong_add(&netdev_flow->stats.mark_count, > count); > >>> + } > >>> +} > >>> + > >>> static int > >>> dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct > dp_packet > >> *packet_, > >>> struct flow *flow, struct flow_wildcards *wc, ovs_u128 *ufid, > >>> @@ -6244,6 +6258,8 @@ packet_batch_per_flow_execute(struct > >> packet_batch_per_flow *batch, > >>> dp_netdev_flow_used(flow, batch->array.count, batch->byte_count, > >>> batch->tcp_flags, pmd->ctx.now / 1000); > >>> > >>> + dp_netdev_flow_marked(flow, batch->flow->mark, batch- > >>> array.count); > >>> + > >>> actions = dp_netdev_flow_get_actions(flow); > >>> > >>> dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow, > >>> diff --git a/lib/dpif.c b/lib/dpif.c > >>> index 457c9bf..bb077a5 100644 > >>> --- a/lib/dpif.c > >>> +++ b/lib/dpif.c > >>> @@ -880,9 +880,18 @@ dpif_flow_stats_extract(const struct flow *flow, > >> const struct dp_packet *packet, > >>> stats->tcp_flags = ntohs(flow->tcp_flags); > >>> stats->n_bytes = dp_packet_size(packet); > >>> stats->n_packets = 1; > >>> + stats->n_marked = 0; > >>> stats->used = used; > >>> } > >>> > >>> +void > >>> +dpif_offload_stats_format(const struct dpif_flow_stats *stats, struct > ds > >> *s) > >>> +{ > >>> + if (stats->n_marked) { > >>> + ds_put_format(s, "marked:%"PRIu64", ", stats->n_marked); > >>> + } > >>> +} > >>> + > >>> /* Appends a human-readable representation of 'stats' to 's'. */ > >>> void > >>> dpif_flow_stats_format(const struct dpif_flow_stats *stats, struct ds > *s) > >>> diff --git a/lib/dpif.h b/lib/dpif.h > >>> index 475d5a6..7c22afd 100644 > >>> --- a/lib/dpif.h > >>> +++ b/lib/dpif.h > >>> @@ -492,6 +492,8 @@ struct dpif_flow_stats { > >>> uint64_t n_bytes; > >>> long long int used; > >>> uint16_t tcp_flags; > >>> + /* Offload stats. */ > >>> + uint64_t n_marked; > >>> }; > >>> > >>> struct dpif_flow_attrs { > >>> @@ -507,6 +509,7 @@ struct dpif_flow_dump_types { > >>> void dpif_flow_stats_extract(const struct flow *, const struct dp_packet > >> *packet, > >>> long long int used, struct dpif_flow_stats *); > >>> void dpif_flow_stats_format(const struct dpif_flow_stats *, struct ds *); > >>> +void dpif_offload_stats_format(const struct dpif_flow_stats *, struct > ds > >> *); > >>> > >>> enum dpif_flow_put_flags { > >>> DPIF_FP_CREATE = 1 << 0, /* Allow creating a new flow. */ > >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c > >>> index 21dd54b..af37ecb 100644 > >>> --- a/ofproto/ofproto-dpif.c > >>> +++ b/ofproto/ofproto-dpif.c > >>> @@ -4416,6 +4416,7 @@ rule_construct(struct rule *rule_) > >>> rule->stats.n_packets = 0; > >>> rule->stats.n_bytes = 0; > >>> rule->stats.used = rule->up.modified; > >>> + rule->stats.n_marked = 0; > >>> rule->recirc_id = 0; > >>> rule->new_rule = NULL; > >>> rule->forward_counts = false; > >>> @@ -5709,6 +5710,9 @@ ofproto_unixctl_dpif_dump_flows(struct > >> unixctl_conn *conn, > >>> odp_flow_format(f.key, f.key_len, f.mask, f.mask_len, > >>> portno_names, &ds, verbosity); > >>> ds_put_cstr(&ds, ", "); > >>> + if (verbosity) { > >>> + dpif_offload_stats_format(&f.stats, &ds); > >>> + } > >>> dpif_flow_stats_format(&f.stats, &ds); > >>> ds_put_cstr(&ds, ", actions:"); > >>> format_odp_actions(&ds, f.actions, f.actions_len, portno_names); > >>>
On 12.03.2019 18:51, Roni Bar Yanai wrote: > > >> -----Original Message----- >> From: Ilya Maximets <i.maximets@samsung.com> >> Sent: Tuesday, March 12, 2019 5:25 PM >> To: Roni Bar Yanai <roniba@mellanox.com>; Ophir Munk >> <ophirmu@mellanox.com>; ovs-dev@openvswitch.org >> Cc: Asaf Penso <asafp@mellanox.com>; Ian Stokes <ian.stokes@intel.com>; >> Shahaf Shuler <shahafs@mellanox.com>; Olga Shern >> <olgas@mellanox.com>; Kevin Traynor <ktraynor@redhat.com> >> Subject: Re: [PATCH v1] dpif: Add marked packets stats >> >> On 12.03.2019 17:53, Roni Bar Yanai wrote: >>> >>> >>>> -----Original Message----- >>>> From: Ilya Maximets <i.maximets@samsung.com> >>>> Sent: Tuesday, March 12, 2019 11:11 AM >>>> To: Ophir Munk <ophirmu@mellanox.com>; ovs-dev@openvswitch.org >>>> Cc: Asaf Penso <asafp@mellanox.com>; Ian Stokes >> <ian.stokes@intel.com>; >>>> Shahaf Shuler <shahafs@mellanox.com>; Olga Shern >>>> <olgas@mellanox.com>; Kevin Traynor <ktraynor@redhat.com>; Roni Bar >>>> Yanai <roniba@mellanox.com> >>>> Subject: Re: [PATCH v1] dpif: Add marked packets stats >>>> >>>> On 12.03.2019 11:41, Ophir Munk wrote: >>>>> This commit adds marked packets statistics. Following commit [1], >>>>> received packets can be marked with a mark id which is associated >>>>> with the flow on which the packets were recieved. This commits counts >>>>> the marked packets per flow and displays the result when executing >>>>> datapath dump-flow command with a "-m" (more verbosity) option, as >>>>> shown in [2]. >>>>> Packets are marked only when hw-offload option is enabled and only on >>>>> some netdev classes such as netdev-dpdk. In all other cases marked >>>>> packets are not counted and are not displayed when executing the >>>> command >>>>> in [2]. >>>>> >>>>> [1] >>>>> Commit 241bad15d9 ("dpif-netdev: associate flow with a mark id") >>>>> >>>>> [2] >>>>> ovs-appctl dpif/dump-flows -m <bridge-name> >>>>> >>>>> Signed-off-by: Ophir Munk <ophirmu@mellanox.com> >>>>> --- >>>> >>>> Hi Ophir. >>>> >>>> Thanks for working on this, but I don't think that flow stats is the right >>>> place to expose information like that. We already have pmd-stats/perf- >> show >>>> appctl calls that prints various hit stats for PMD threads. And, I think, >>>> 'flow mark hits' or something similar should be one of these stats. >>>> Right now flow mark hits are not accounted there and that is the issue. >>>> >>>> Regarding the flow stats, we probably need to expose status of the flow, >>>> i.e. if it was offloaded or not, like it done for tc offloading. >>>> Right now all the flows reported by dpif-netdev as not offloaded. >>>> See 'flow->attrs.offloaded = false;' in dp_netdev_flow_to_dpif_flow(). >>>> And we probably need to change this from boolean to some >> 'yes/no/partial'. >>>> >>>> BTW, your current implementation assumes that packets had flow mark if >>>> 'flow->mark' is set, but this is could be not true, because packets could >>>> arrive while offloading is in progress or offloading could be started >>>> between packets arrival and finishing of their processing. >>>> >>>> Best regards, Ilya Maximets. >>> >>> Hi Ilya, >>> >>> We agree that flow should be marked as offloaded yes/no/partial, but >> seems this is not enough. >>> Like you said, since hardware offload is executed in a different thread, it >> might be that the number >>> of packets that were marked/handled by hardware is different than the >> total packets of the flow. >>> If there is a load on the offload or just high latency, it might be that in some >> cases the difference will be significant. >>> Although I agree that you will probably be able to see the problem In the >> pmd-stats (assuming mark will be added there), >>> ,it will be less clear. For example, there are probably flows that cannot be >> offloaded at all so you won't expect >>> mark count for them. Another example is a bug, we say that the flow was >> offloaded but in fact it is not. >>> Of course you should expect the code to be tested, but the field is always >> different. >>> Maybe we can add it we special flag for hardware offload specific >> information. >> >> Not sure if I understand your point correctly, but isn't it expected that >> if flow is offloaded than all the packets through the flow had flow mark? >> In case of a bug it's most likely that it happened after the flow mark >> association and your flow stats will be wrong anyway. >> The only trusted source of the number of "offloaded" packets could be the >> number of real packets that had flow mark set on receive. And these stats >> should be displayed by PMD stats. >> >> If everything works as expected, all the packets for offloaded flow will be >> marked (except first few of them). >> If something will go wrong, we can't trust these flow stats. We can rely only > Why you can't trust the flow stats? This is the only point in the code where > you know that the flow was really handled by hardware. We can't trust them because flow stats in this patch calculated not taking into account if packets really had flow marks. You're assuming that if flow associated with some mark than all the packets from this flow had marks. But this is equal to just assuming that all the packets from stats.n_packets had flow marks. > >> on PMD stats. >> So, I don't see the profit from having these special flow stats. >> > I'm looking at it from the OVS user point of view. I have thousands of flows running. > If the statistics is only in the PMD I can see how many packets were marked and how many > we're not. Let's say the ratio is not that good, how do I look deeper? > Is it because for some flows there is bug on matching? Is it because of the latency in adding flows? Flow stats will be useless in case of any of these issues, because we can't trust them. See above. > Is it because some flow pattern is not supported at all? If pattern is not supported, flow should not have associated mark. And the flow offloading status should be "no". If it has associated mark, than it's a bug that makes flow stats useless. > Agree this is only needed for debugging, but I don't see other way user can get this information. > >>> >>> Thanks >>>> >>>>> lib/dpif-netdev.c | 16 ++++++++++++++++ >>>>> lib/dpif.c | 9 +++++++++ >>>>> lib/dpif.h | 3 +++ >>>>> ofproto/ofproto-dpif.c | 4 ++++ >>>>> 4 files changed, 32 insertions(+) >>>>> >>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c >>>>> index 4d6d0c3..ef2f8f1 100644 >>>>> --- a/lib/dpif-netdev.c >>>>> +++ b/lib/dpif-netdev.c >>>>> @@ -486,6 +486,8 @@ struct dp_netdev_flow_stats { >>>>> atomic_ullong packet_count; /* Number of packets matched. */ >>>>> atomic_ullong byte_count; /* Number of bytes matched. */ >>>>> atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. >> */ >>>>> + /* Offload stats. */ >>>>> + atomic_ullong mark_count; /* Number of marked packets >> matched. >>>> */ >>>>> }; >>>>> >>>>> /* A flow in 'dp_netdev_pmd_thread's 'flow_table'. >>>>> @@ -3008,6 +3010,9 @@ get_dpif_flow_stats(const struct >>>> dp_netdev_flow *netdev_flow_, >>>>> stats->used = used; >>>>> atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags); >>>>> stats->tcp_flags = flags; >>>>> + /* Offload stats. */ >>>>> + atomic_read_relaxed(&netdev_flow->stats.mark_count, &n); >>>>> + stats->n_marked = n; >>>>> } >>>>> >>>>> /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for >>>>> @@ -6124,6 +6129,15 @@ dp_netdev_flow_used(struct >> dp_netdev_flow >>>> *netdev_flow, int cnt, int size, >>>>> atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags); >>>>> } >>>>> >>>>> +static void >>>>> +dp_netdev_flow_marked(struct dp_netdev_flow *netdev_flow, >>>> uint32_t mark, >>>>> + int count) >>>>> +{ >>>>> + if (mark != INVALID_FLOW_MARK) { >>>>> + non_atomic_ullong_add(&netdev_flow->stats.mark_count, >> count); >>>>> + } >>>>> +} >>>>> + >>>>> static int >>>>> dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct >> dp_packet >>>> *packet_, >>>>> struct flow *flow, struct flow_wildcards *wc, ovs_u128 *ufid, >>>>> @@ -6244,6 +6258,8 @@ packet_batch_per_flow_execute(struct >>>> packet_batch_per_flow *batch, >>>>> dp_netdev_flow_used(flow, batch->array.count, batch->byte_count, >>>>> batch->tcp_flags, pmd->ctx.now / 1000); >>>>> >>>>> + dp_netdev_flow_marked(flow, batch->flow->mark, batch- >>>>> array.count); >>>>> + >>>>> actions = dp_netdev_flow_get_actions(flow); >>>>> >>>>> dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow, >>>>> diff --git a/lib/dpif.c b/lib/dpif.c >>>>> index 457c9bf..bb077a5 100644 >>>>> --- a/lib/dpif.c >>>>> +++ b/lib/dpif.c >>>>> @@ -880,9 +880,18 @@ dpif_flow_stats_extract(const struct flow *flow, >>>> const struct dp_packet *packet, >>>>> stats->tcp_flags = ntohs(flow->tcp_flags); >>>>> stats->n_bytes = dp_packet_size(packet); >>>>> stats->n_packets = 1; >>>>> + stats->n_marked = 0; >>>>> stats->used = used; >>>>> } >>>>> >>>>> +void >>>>> +dpif_offload_stats_format(const struct dpif_flow_stats *stats, struct >> ds >>>> *s) >>>>> +{ >>>>> + if (stats->n_marked) { >>>>> + ds_put_format(s, "marked:%"PRIu64", ", stats->n_marked); >>>>> + } >>>>> +} >>>>> + >>>>> /* Appends a human-readable representation of 'stats' to 's'. */ >>>>> void >>>>> dpif_flow_stats_format(const struct dpif_flow_stats *stats, struct ds >> *s) >>>>> diff --git a/lib/dpif.h b/lib/dpif.h >>>>> index 475d5a6..7c22afd 100644 >>>>> --- a/lib/dpif.h >>>>> +++ b/lib/dpif.h >>>>> @@ -492,6 +492,8 @@ struct dpif_flow_stats { >>>>> uint64_t n_bytes; >>>>> long long int used; >>>>> uint16_t tcp_flags; >>>>> + /* Offload stats. */ >>>>> + uint64_t n_marked; >>>>> }; >>>>> >>>>> struct dpif_flow_attrs { >>>>> @@ -507,6 +509,7 @@ struct dpif_flow_dump_types { >>>>> void dpif_flow_stats_extract(const struct flow *, const struct dp_packet >>>> *packet, >>>>> long long int used, struct dpif_flow_stats *); >>>>> void dpif_flow_stats_format(const struct dpif_flow_stats *, struct ds *); >>>>> +void dpif_offload_stats_format(const struct dpif_flow_stats *, struct >> ds >>>> *); >>>>> >>>>> enum dpif_flow_put_flags { >>>>> DPIF_FP_CREATE = 1 << 0, /* Allow creating a new flow. */ >>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c >>>>> index 21dd54b..af37ecb 100644 >>>>> --- a/ofproto/ofproto-dpif.c >>>>> +++ b/ofproto/ofproto-dpif.c >>>>> @@ -4416,6 +4416,7 @@ rule_construct(struct rule *rule_) >>>>> rule->stats.n_packets = 0; >>>>> rule->stats.n_bytes = 0; >>>>> rule->stats.used = rule->up.modified; >>>>> + rule->stats.n_marked = 0; >>>>> rule->recirc_id = 0; >>>>> rule->new_rule = NULL; >>>>> rule->forward_counts = false; >>>>> @@ -5709,6 +5710,9 @@ ofproto_unixctl_dpif_dump_flows(struct >>>> unixctl_conn *conn, >>>>> odp_flow_format(f.key, f.key_len, f.mask, f.mask_len, >>>>> portno_names, &ds, verbosity); >>>>> ds_put_cstr(&ds, ", "); >>>>> + if (verbosity) { >>>>> + dpif_offload_stats_format(&f.stats, &ds); >>>>> + } >>>>> dpif_flow_stats_format(&f.stats, &ds); >>>>> ds_put_cstr(&ds, ", actions:"); >>>>> format_odp_actions(&ds, f.actions, f.actions_len, portno_names); >>>>>
> > > > Not sure if I understand your point correctly, but isn't it expected > > that if flow is offloaded than all the packets through the flow had flow > mark? Not always. For fully HW offloaded traffic - packets will not be marked. It is planned to have another HW offload counter (which is different from the mark counter). > > In case of a bug it's most likely that it happened after the flow mark > > association and your flow stats will be wrong anyway. > > The only trusted source of the number of "offloaded" packets could be > > the number of real packets that had flow mark set on receive. And > > these stats should be displayed by PMD stats. > > Not sure about that. It seems there is no one to one relationship between a PMD and a flow. In order to correctly debug (how many offloaded packets actually got a mark) I think you need a flow resolution statistics. > > If everything works as expected, all the packets for offloaded flow > > will be marked (except first few of them). > > If something will go wrong, we can't trust these flow stats. We can > > rely only > Why you can't trust the flow stats? This is the only point in the code where > you know that the flow was really handled by hardware. > > > on PMD stats. > > So, I don't see the profit from having these special flow stats. > > > I'm looking at it from the OVS user point of view. I have thousands of flows > running. > If the statistics is only in the PMD I can see how many packets were marked > and how many we're not. Let's say the ratio is not that good, how do I look > deeper? > Is it because for some flows there is bug on matching? Is it because of the > latency in adding flows? > Is it because some flow pattern is not supported at all? > Agree this is only needed for debugging, but I don't see other way user can > get this information. > Do we agree that knowing how many marked packets per flow were received is something valuable? It should be used to confirm correct functionality and improve performance for offloaded traffic. If we agree then we should discuss how to implement it. I don't think that PMD level statistics can be used. Any other ideas? Adding a debug flag "-d" to the dpif dump-flow? Creating a new command for dpif-netdev? Please advise. > > > > > > Thanks > > >> > > >>> lib/dpif-netdev.c | 16 ++++++++++++++++ > > >>> lib/dpif.c | 9 +++++++++ > > >>> lib/dpif.h | 3 +++ > > >>> ofproto/ofproto-dpif.c | 4 ++++ > > >>> 4 files changed, 32 insertions(+) > > >>> > > >>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index > > >>> 4d6d0c3..ef2f8f1 100644 > > >>> --- a/lib/dpif-netdev.c > > >>> +++ b/lib/dpif-netdev.c > > >>> @@ -486,6 +486,8 @@ struct dp_netdev_flow_stats { > > >>> atomic_ullong packet_count; /* Number of packets matched. */ > > >>> atomic_ullong byte_count; /* Number of bytes matched. */ > > >>> atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. > > */ > > >>> + /* Offload stats. */ > > >>> + atomic_ullong mark_count; /* Number of marked packets > > matched. > > >> */ > > >>> }; > > >>> > > >>> /* A flow in 'dp_netdev_pmd_thread's 'flow_table'. > > >>> @@ -3008,6 +3010,9 @@ get_dpif_flow_stats(const struct > > >> dp_netdev_flow *netdev_flow_, > > >>> stats->used = used; > > >>> atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags); > > >>> stats->tcp_flags = flags; > > >>> + /* Offload stats. */ > > >>> + atomic_read_relaxed(&netdev_flow->stats.mark_count, &n); > > >>> + stats->n_marked = n; > > >>> } > > >>> > > >>> /* Converts to the dpif_flow format, using 'key_buf' and > > >>> 'mask_buf' for @@ -6124,6 +6129,15 @@ > dp_netdev_flow_used(struct > > dp_netdev_flow > > >> *netdev_flow, int cnt, int size, > > >>> atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags); > > >>> } > > >>> > > >>> +static void > > >>> +dp_netdev_flow_marked(struct dp_netdev_flow *netdev_flow, > > >> uint32_t mark, > > >>> + int count) > > >>> +{ > > >>> + if (mark != INVALID_FLOW_MARK) { > > >>> + non_atomic_ullong_add(&netdev_flow->stats.mark_count, > > count); > > >>> + } > > >>> +} > > >>> + > > >>> static int > > >>> dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct > > dp_packet > > >> *packet_, > > >>> struct flow *flow, struct flow_wildcards *wc, > > >>> ovs_u128 *ufid, @@ -6244,6 +6258,8 @@ > > >>> packet_batch_per_flow_execute(struct > > >> packet_batch_per_flow *batch, > > >>> dp_netdev_flow_used(flow, batch->array.count, batch- > >byte_count, > > >>> batch->tcp_flags, pmd->ctx.now / 1000); > > >>> > > >>> + dp_netdev_flow_marked(flow, batch->flow->mark, batch- > > >>> array.count); > > >>> + > > >>> actions = dp_netdev_flow_get_actions(flow); > > >>> > > >>> dp_netdev_execute_actions(pmd, &batch->array, true, > > >>> &flow->flow, diff --git a/lib/dpif.c b/lib/dpif.c index > > >>> 457c9bf..bb077a5 100644 > > >>> --- a/lib/dpif.c > > >>> +++ b/lib/dpif.c > > >>> @@ -880,9 +880,18 @@ dpif_flow_stats_extract(const struct flow > > >>> *flow, > > >> const struct dp_packet *packet, > > >>> stats->tcp_flags = ntohs(flow->tcp_flags); > > >>> stats->n_bytes = dp_packet_size(packet); > > >>> stats->n_packets = 1; > > >>> + stats->n_marked = 0; > > >>> stats->used = used; > > >>> } > > >>> > > >>> +void > > >>> +dpif_offload_stats_format(const struct dpif_flow_stats *stats, > > >>> +struct > > ds > > >> *s) > > >>> +{ > > >>> + if (stats->n_marked) { > > >>> + ds_put_format(s, "marked:%"PRIu64", ", stats->n_marked); > > >>> + } > > >>> +} > > >>> + > > >>> /* Appends a human-readable representation of 'stats' to 's'. */ > > >>> void dpif_flow_stats_format(const struct dpif_flow_stats *stats, > > >>> struct ds > > *s) > > >>> diff --git a/lib/dpif.h b/lib/dpif.h index 475d5a6..7c22afd 100644 > > >>> --- a/lib/dpif.h > > >>> +++ b/lib/dpif.h > > >>> @@ -492,6 +492,8 @@ struct dpif_flow_stats { > > >>> uint64_t n_bytes; > > >>> long long int used; > > >>> uint16_t tcp_flags; > > >>> + /* Offload stats. */ > > >>> + uint64_t n_marked; > > >>> }; > > >>> > > >>> struct dpif_flow_attrs { > > >>> @@ -507,6 +509,7 @@ struct dpif_flow_dump_types { void > > >>> dpif_flow_stats_extract(const struct flow *, const struct > > >>> dp_packet > > >> *packet, > > >>> long long int used, struct > > >>> dpif_flow_stats *); void dpif_flow_stats_format(const struct > > >>> dpif_flow_stats *, struct ds *); > > >>> +void dpif_offload_stats_format(const struct dpif_flow_stats *, > > >>> +struct > > ds > > >> *); > > >>> > > >>> enum dpif_flow_put_flags { > > >>> DPIF_FP_CREATE = 1 << 0, /* Allow creating a new flow. */ > > >>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index > > >>> 21dd54b..af37ecb 100644 > > >>> --- a/ofproto/ofproto-dpif.c > > >>> +++ b/ofproto/ofproto-dpif.c > > >>> @@ -4416,6 +4416,7 @@ rule_construct(struct rule *rule_) > > >>> rule->stats.n_packets = 0; > > >>> rule->stats.n_bytes = 0; > > >>> rule->stats.used = rule->up.modified; > > >>> + rule->stats.n_marked = 0; > > >>> rule->recirc_id = 0; > > >>> rule->new_rule = NULL; > > >>> rule->forward_counts = false; @@ -5709,6 +5710,9 @@ > > >>> ofproto_unixctl_dpif_dump_flows(struct > > >> unixctl_conn *conn, > > >>> odp_flow_format(f.key, f.key_len, f.mask, f.mask_len, > > >>> portno_names, &ds, verbosity); > > >>> ds_put_cstr(&ds, ", "); > > >>> + if (verbosity) { > > >>> + dpif_offload_stats_format(&f.stats, &ds); > > >>> + } > > >>> dpif_flow_stats_format(&f.stats, &ds); > > >>> ds_put_cstr(&ds, ", actions:"); > > >>> format_odp_actions(&ds, f.actions, f.actions_len, > > >>> portno_names); > > >>>
On 12.03.2019 19:18, Ophir Munk wrote: > >>> >>> Not sure if I understand your point correctly, but isn't it expected >>> that if flow is offloaded than all the packets through the flow had flow >> mark? > > Not always. For fully HW offloaded traffic - packets will not be marked. > It is planned to have another HW offload counter (which is different from the mark counter). Is it possible for one flow to be fully and partially offloaded at the same time? Seems strange. In case of full offloading you don't need a separate counter, you just need to retrieve stats.n_packets from the HW. > >>> In case of a bug it's most likely that it happened after the flow mark >>> association and your flow stats will be wrong anyway. >>> The only trusted source of the number of "offloaded" packets could be >>> the number of real packets that had flow mark set on receive. And >>> these stats should be displayed by PMD stats. >>> > > Not sure about that. It seems there is no one to one relationship between a PMD and a flow. There is. You may get per-PMD flow stats by 'ovs-appctl dpctl/dump-flows'. And you may compare them with PMD hit stats. > In order to correctly debug (how many offloaded packets actually got a mark) Current patch will not help with issues like this where some packets of one flow has marks and other packets from the same flow has not. > I think you need a flow resolution statistics. > >>> If everything works as expected, all the packets for offloaded flow >>> will be marked (except first few of them). >>> If something will go wrong, we can't trust these flow stats. We can >>> rely only >> Why you can't trust the flow stats? This is the only point in the code where >> you know that the flow was really handled by hardware. >> >>> on PMD stats. >>> So, I don't see the profit from having these special flow stats. >>> >> I'm looking at it from the OVS user point of view. I have thousands of flows >> running. >> If the statistics is only in the PMD I can see how many packets were marked >> and how many we're not. Let's say the ratio is not that good, how do I look >> deeper? >> Is it because for some flows there is bug on matching? Is it because of the >> latency in adding flows? >> Is it because some flow pattern is not supported at all? >> Agree this is only needed for debugging, but I don't see other way user can >> get this information. >> > > Do we agree that knowing how many marked packets per flow were received is something valuable? Not sure. At least not in current implementation. > It should be used to confirm correct functionality and improve performance for offloaded traffic. > If we agree then we should discuss how to implement it. > I don't think that PMD level statistics can be used. See above. > Any other ideas? > Adding a debug flag "-d" to the dpif dump-flow? > Creating a new command for dpif-netdev? > Please advise. > >>>> >>>> Thanks >>>>> >>>>>> lib/dpif-netdev.c | 16 ++++++++++++++++ >>>>>> lib/dpif.c | 9 +++++++++ >>>>>> lib/dpif.h | 3 +++ >>>>>> ofproto/ofproto-dpif.c | 4 ++++ >>>>>> 4 files changed, 32 insertions(+) >>>>>> >>>>>> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index >>>>>> 4d6d0c3..ef2f8f1 100644 >>>>>> --- a/lib/dpif-netdev.c >>>>>> +++ b/lib/dpif-netdev.c >>>>>> @@ -486,6 +486,8 @@ struct dp_netdev_flow_stats { >>>>>> atomic_ullong packet_count; /* Number of packets matched. */ >>>>>> atomic_ullong byte_count; /* Number of bytes matched. */ >>>>>> atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. >>> */ >>>>>> + /* Offload stats. */ >>>>>> + atomic_ullong mark_count; /* Number of marked packets >>> matched. >>>>> */ >>>>>> }; >>>>>> >>>>>> /* A flow in 'dp_netdev_pmd_thread's 'flow_table'. >>>>>> @@ -3008,6 +3010,9 @@ get_dpif_flow_stats(const struct >>>>> dp_netdev_flow *netdev_flow_, >>>>>> stats->used = used; >>>>>> atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags); >>>>>> stats->tcp_flags = flags; >>>>>> + /* Offload stats. */ >>>>>> + atomic_read_relaxed(&netdev_flow->stats.mark_count, &n); >>>>>> + stats->n_marked = n; >>>>>> } >>>>>> >>>>>> /* Converts to the dpif_flow format, using 'key_buf' and >>>>>> 'mask_buf' for @@ -6124,6 +6129,15 @@ >> dp_netdev_flow_used(struct >>> dp_netdev_flow >>>>> *netdev_flow, int cnt, int size, >>>>>> atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags); >>>>>> } >>>>>> >>>>>> +static void >>>>>> +dp_netdev_flow_marked(struct dp_netdev_flow *netdev_flow, >>>>> uint32_t mark, >>>>>> + int count) >>>>>> +{ >>>>>> + if (mark != INVALID_FLOW_MARK) { >>>>>> + non_atomic_ullong_add(&netdev_flow->stats.mark_count, >>> count); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> static int >>>>>> dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct >>> dp_packet >>>>> *packet_, >>>>>> struct flow *flow, struct flow_wildcards *wc, >>>>>> ovs_u128 *ufid, @@ -6244,6 +6258,8 @@ >>>>>> packet_batch_per_flow_execute(struct >>>>> packet_batch_per_flow *batch, >>>>>> dp_netdev_flow_used(flow, batch->array.count, batch- >>> byte_count, >>>>>> batch->tcp_flags, pmd->ctx.now / 1000); >>>>>> >>>>>> + dp_netdev_flow_marked(flow, batch->flow->mark, batch- >>>>>> array.count); >>>>>> + >>>>>> actions = dp_netdev_flow_get_actions(flow); >>>>>> >>>>>> dp_netdev_execute_actions(pmd, &batch->array, true, >>>>>> &flow->flow, diff --git a/lib/dpif.c b/lib/dpif.c index >>>>>> 457c9bf..bb077a5 100644 >>>>>> --- a/lib/dpif.c >>>>>> +++ b/lib/dpif.c >>>>>> @@ -880,9 +880,18 @@ dpif_flow_stats_extract(const struct flow >>>>>> *flow, >>>>> const struct dp_packet *packet, >>>>>> stats->tcp_flags = ntohs(flow->tcp_flags); >>>>>> stats->n_bytes = dp_packet_size(packet); >>>>>> stats->n_packets = 1; >>>>>> + stats->n_marked = 0; >>>>>> stats->used = used; >>>>>> } >>>>>> >>>>>> +void >>>>>> +dpif_offload_stats_format(const struct dpif_flow_stats *stats, >>>>>> +struct >>> ds >>>>> *s) >>>>>> +{ >>>>>> + if (stats->n_marked) { >>>>>> + ds_put_format(s, "marked:%"PRIu64", ", stats->n_marked); >>>>>> + } >>>>>> +} >>>>>> + >>>>>> /* Appends a human-readable representation of 'stats' to 's'. */ >>>>>> void dpif_flow_stats_format(const struct dpif_flow_stats *stats, >>>>>> struct ds >>> *s) >>>>>> diff --git a/lib/dpif.h b/lib/dpif.h index 475d5a6..7c22afd 100644 >>>>>> --- a/lib/dpif.h >>>>>> +++ b/lib/dpif.h >>>>>> @@ -492,6 +492,8 @@ struct dpif_flow_stats { >>>>>> uint64_t n_bytes; >>>>>> long long int used; >>>>>> uint16_t tcp_flags; >>>>>> + /* Offload stats. */ >>>>>> + uint64_t n_marked; >>>>>> }; >>>>>> >>>>>> struct dpif_flow_attrs { >>>>>> @@ -507,6 +509,7 @@ struct dpif_flow_dump_types { void >>>>>> dpif_flow_stats_extract(const struct flow *, const struct >>>>>> dp_packet >>>>> *packet, >>>>>> long long int used, struct >>>>>> dpif_flow_stats *); void dpif_flow_stats_format(const struct >>>>>> dpif_flow_stats *, struct ds *); >>>>>> +void dpif_offload_stats_format(const struct dpif_flow_stats *, >>>>>> +struct >>> ds >>>>> *); >>>>>> >>>>>> enum dpif_flow_put_flags { >>>>>> DPIF_FP_CREATE = 1 << 0, /* Allow creating a new flow. */ >>>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index >>>>>> 21dd54b..af37ecb 100644 >>>>>> --- a/ofproto/ofproto-dpif.c >>>>>> +++ b/ofproto/ofproto-dpif.c >>>>>> @@ -4416,6 +4416,7 @@ rule_construct(struct rule *rule_) >>>>>> rule->stats.n_packets = 0; >>>>>> rule->stats.n_bytes = 0; >>>>>> rule->stats.used = rule->up.modified; >>>>>> + rule->stats.n_marked = 0; >>>>>> rule->recirc_id = 0; >>>>>> rule->new_rule = NULL; >>>>>> rule->forward_counts = false; @@ -5709,6 +5710,9 @@ >>>>>> ofproto_unixctl_dpif_dump_flows(struct >>>>> unixctl_conn *conn, >>>>>> odp_flow_format(f.key, f.key_len, f.mask, f.mask_len, >>>>>> portno_names, &ds, verbosity); >>>>>> ds_put_cstr(&ds, ", "); >>>>>> + if (verbosity) { >>>>>> + dpif_offload_stats_format(&f.stats, &ds); >>>>>> + } >>>>>> dpif_flow_stats_format(&f.stats, &ds); >>>>>> ds_put_cstr(&ds, ", actions:"); >>>>>> format_odp_actions(&ds, f.actions, f.actions_len, >>>>>> portno_names); >>>>>>
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 4d6d0c3..ef2f8f1 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -486,6 +486,8 @@ struct dp_netdev_flow_stats { atomic_ullong packet_count; /* Number of packets matched. */ atomic_ullong byte_count; /* Number of bytes matched. */ atomic_uint16_t tcp_flags; /* Bitwise-OR of seen tcp_flags values. */ + /* Offload stats. */ + atomic_ullong mark_count; /* Number of marked packets matched. */ }; /* A flow in 'dp_netdev_pmd_thread's 'flow_table'. @@ -3008,6 +3010,9 @@ get_dpif_flow_stats(const struct dp_netdev_flow *netdev_flow_, stats->used = used; atomic_read_relaxed(&netdev_flow->stats.tcp_flags, &flags); stats->tcp_flags = flags; + /* Offload stats. */ + atomic_read_relaxed(&netdev_flow->stats.mark_count, &n); + stats->n_marked = n; } /* Converts to the dpif_flow format, using 'key_buf' and 'mask_buf' for @@ -6124,6 +6129,15 @@ dp_netdev_flow_used(struct dp_netdev_flow *netdev_flow, int cnt, int size, atomic_store_relaxed(&netdev_flow->stats.tcp_flags, flags); } +static void +dp_netdev_flow_marked(struct dp_netdev_flow *netdev_flow, uint32_t mark, + int count) +{ + if (mark != INVALID_FLOW_MARK) { + non_atomic_ullong_add(&netdev_flow->stats.mark_count, count); + } +} + static int dp_netdev_upcall(struct dp_netdev_pmd_thread *pmd, struct dp_packet *packet_, struct flow *flow, struct flow_wildcards *wc, ovs_u128 *ufid, @@ -6244,6 +6258,8 @@ packet_batch_per_flow_execute(struct packet_batch_per_flow *batch, dp_netdev_flow_used(flow, batch->array.count, batch->byte_count, batch->tcp_flags, pmd->ctx.now / 1000); + dp_netdev_flow_marked(flow, batch->flow->mark, batch->array.count); + actions = dp_netdev_flow_get_actions(flow); dp_netdev_execute_actions(pmd, &batch->array, true, &flow->flow, diff --git a/lib/dpif.c b/lib/dpif.c index 457c9bf..bb077a5 100644 --- a/lib/dpif.c +++ b/lib/dpif.c @@ -880,9 +880,18 @@ dpif_flow_stats_extract(const struct flow *flow, const struct dp_packet *packet, stats->tcp_flags = ntohs(flow->tcp_flags); stats->n_bytes = dp_packet_size(packet); stats->n_packets = 1; + stats->n_marked = 0; stats->used = used; } +void +dpif_offload_stats_format(const struct dpif_flow_stats *stats, struct ds *s) +{ + if (stats->n_marked) { + ds_put_format(s, "marked:%"PRIu64", ", stats->n_marked); + } +} + /* Appends a human-readable representation of 'stats' to 's'. */ void dpif_flow_stats_format(const struct dpif_flow_stats *stats, struct ds *s) diff --git a/lib/dpif.h b/lib/dpif.h index 475d5a6..7c22afd 100644 --- a/lib/dpif.h +++ b/lib/dpif.h @@ -492,6 +492,8 @@ struct dpif_flow_stats { uint64_t n_bytes; long long int used; uint16_t tcp_flags; + /* Offload stats. */ + uint64_t n_marked; }; struct dpif_flow_attrs { @@ -507,6 +509,7 @@ struct dpif_flow_dump_types { void dpif_flow_stats_extract(const struct flow *, const struct dp_packet *packet, long long int used, struct dpif_flow_stats *); void dpif_flow_stats_format(const struct dpif_flow_stats *, struct ds *); +void dpif_offload_stats_format(const struct dpif_flow_stats *, struct ds *); enum dpif_flow_put_flags { DPIF_FP_CREATE = 1 << 0, /* Allow creating a new flow. */ diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index 21dd54b..af37ecb 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -4416,6 +4416,7 @@ rule_construct(struct rule *rule_) rule->stats.n_packets = 0; rule->stats.n_bytes = 0; rule->stats.used = rule->up.modified; + rule->stats.n_marked = 0; rule->recirc_id = 0; rule->new_rule = NULL; rule->forward_counts = false; @@ -5709,6 +5710,9 @@ ofproto_unixctl_dpif_dump_flows(struct unixctl_conn *conn, odp_flow_format(f.key, f.key_len, f.mask, f.mask_len, portno_names, &ds, verbosity); ds_put_cstr(&ds, ", "); + if (verbosity) { + dpif_offload_stats_format(&f.stats, &ds); + } dpif_flow_stats_format(&f.stats, &ds); ds_put_cstr(&ds, ", actions:"); format_odp_actions(&ds, f.actions, f.actions_len, portno_names);
This commit adds marked packets statistics. Following commit [1], received packets can be marked with a mark id which is associated with the flow on which the packets were recieved. This commits counts the marked packets per flow and displays the result when executing datapath dump-flow command with a "-m" (more verbosity) option, as shown in [2]. Packets are marked only when hw-offload option is enabled and only on some netdev classes such as netdev-dpdk. In all other cases marked packets are not counted and are not displayed when executing the command in [2]. [1] Commit 241bad15d9 ("dpif-netdev: associate flow with a mark id") [2] ovs-appctl dpif/dump-flows -m <bridge-name> Signed-off-by: Ophir Munk <ophirmu@mellanox.com> --- lib/dpif-netdev.c | 16 ++++++++++++++++ lib/dpif.c | 9 +++++++++ lib/dpif.h | 3 +++ ofproto/ofproto-dpif.c | 4 ++++ 4 files changed, 32 insertions(+)