[ovs-dev] ofproto: Add appctl command to show Datapath features

Submitted by andy zhou on March 13, 2017, 9:21 p.m.

Details

Message ID 1489440092-13945-1-git-send-email-azhou@ovn.org
State Accepted
Headers show

Commit Message

andy zhou March 13, 2017, 9:21 p.m.
Exporting Datapath runtime detected features can be useful for
both debugging and for writing system unit testing easier.

Signed-off-by: Andy Zhou <azhou@ovn.org>
---
 ofproto/ofproto-dpif.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

Comments

Joe Stringer March 15, 2017, 1:19 a.m.
On 13 March 2017 at 14:21, Andy Zhou <azhou@ovn.org> wrote:
> Exporting Datapath runtime detected features can be useful for
> both debugging and for writing system unit testing easier.
>
> Signed-off-by: Andy Zhou <azhou@ovn.org>

Can we perform some kind of build-time check how many fields are in
'support' and ensure that this function is extended when people add
new probes?

Looks otherwise trivial and straightforward (and useful!), thanks.
Couple of minor comments below.

Acked-by: Joe Stringer <joe@ovn.org>

> ---
>  ofproto/ofproto-dpif.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 52 insertions(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index df779c2..af70ab3 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -4891,6 +4891,38 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED,
>  }
>
>  static void
> +show_dp_feature_b(struct ds *ds, const char *feature, bool b)
> +{
> +    ds_put_format(ds, "%s: %s\n", feature, b ? "Yes" : "No");
> +}
> +
> +static void
> +show_dp_feature_s(struct ds *ds, const char *feature, size_t s)
> +{
> +    ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s);
> +}
> +
> +static void
> +dpif_show_support(const struct dpif_backer_support *support, struct ds *ds)
> +{
> +    show_dp_feature_b(ds, "Variable length userdata",
> +                      support->variable_length_userdata);
> +    show_dp_feature_b(ds, "Masked set action", support->masked_set_action);
> +    show_dp_feature_b(ds, "Tunnel push pop",   support->tnl_push_pop);
> +    show_dp_feature_b(ds, "Ufid",              support->ufid);
> +    show_dp_feature_b(ds, "Trunc action",      support->trunc);
> +    show_dp_feature_b(ds, "Clone action",      support->clone);
> +    show_dp_feature_s(ds, "Max MPLS depth",    support->odp.max_mpls_depth);
> +    show_dp_feature_b(ds, "Recirc",            support->odp.recirc);
> +    show_dp_feature_b(ds, "CT state",          support->odp.ct_state);
> +    show_dp_feature_b(ds, "CT zone",           support->odp.ct_zone);
> +    show_dp_feature_b(ds, "CT mark",           support->odp.ct_mark);
> +    show_dp_feature_b(ds, "CT label",          support->odp.ct_label);
> +    show_dp_feature_b(ds, "CT State NAT",      support->odp.ct_state_nat);
> +    show_dp_feature_s(ds, "Max sample nesting",support->sample_nesting);

Needs an extra whitespace on this last line, before support->sample_nesting

> +}
> +
> +static void
>  dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
>  {
>      const struct shash_node **ofprotos;
> @@ -4899,7 +4931,6 @@ dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
>      size_t i;
>
>      dpif_get_dp_stats(backer->dpif, &dp_stats);
> -
>      ds_put_format(ds, "%s: hit:%"PRIu64" missed:%"PRIu64"\n",
>                    dpif_name(backer->dpif), dp_stats.n_hit, dp_stats.n_missed);
>
> @@ -5119,6 +5150,24 @@ disable_datapath_clone(struct unixctl_conn *conn OVS_UNUSED,
>  }
>
>  static void
> +ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
> +                                      int argc, const char *argv[],
> +                                      void *aux OVS_UNUSED)
> +{
> +    struct ds ds = DS_EMPTY_INITIALIZER;
> +    const char *br = argv[argc -1];
> +    struct ofproto_dpif *ofproto = ofproto_dpif_lookup(br);
> +
> +    if (!ofproto) {
> +        unixctl_command_reply_error(conn, "no such bridge");
> +        return;
> +    }
> +
> +    dpif_show_support(&ofproto->backer->support, &ds);
> +    unixctl_command_reply(conn, ds_cstr(&ds));
> +}
> +
> +static void
>  ofproto_unixctl_init(void)
>  {
>      static bool registered;
> @@ -5139,6 +5188,8 @@ ofproto_unixctl_init(void)
>                               ofproto_unixctl_dpif_dump_dps, NULL);
>      unixctl_command_register("dpif/show", "", 0, 0, ofproto_unixctl_dpif_show,
>                               NULL);
> +    unixctl_command_register("dpif/show-dp-features", "bridge", 1, 1,
> +                             ofproto_unixctl_dpif_show_dp_features, NULL);
>      unixctl_command_register("dpif/dump-flows", "[-m] bridge", 1, 2,
>                               ofproto_unixctl_dpif_dump_flows, NULL);
>
> --
> 1.8.3.1
>
> _______________________________________________
> dev mailing list
> dev@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
andy zhou March 15, 2017, 8:36 a.m.
On Tue, Mar 14, 2017 at 6:19 PM, Joe Stringer <joe@ovn.org> wrote:
> On 13 March 2017 at 14:21, Andy Zhou <azhou@ovn.org> wrote:
>> Exporting Datapath runtime detected features can be useful for
>> both debugging and for writing system unit testing easier.
>>
>> Signed-off-by: Andy Zhou <azhou@ovn.org>
>
> Can we perform some kind of build-time check how many fields are in
> 'support' and ensure that this function is extended when people add
> new probes?
>
That would be nice.  I don't have any concrete at the moment.

> Looks otherwise trivial and straightforward (and useful!), thanks.
> Couple of minor comments below.
>
> Acked-by: Joe Stringer <joe@ovn.org>

Thanks for the review. Pushed.
>
>> ---
>>  ofproto/ofproto-dpif.c | 53 +++++++++++++++++++++++++++++++++++++++++++++++++-
>>  1 file changed, 52 insertions(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index df779c2..af70ab3 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -4891,6 +4891,38 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED,
>>  }
>>
>>  static void
>> +show_dp_feature_b(struct ds *ds, const char *feature, bool b)
>> +{
>> +    ds_put_format(ds, "%s: %s\n", feature, b ? "Yes" : "No");
>> +}
>> +
>> +static void
>> +show_dp_feature_s(struct ds *ds, const char *feature, size_t s)
>> +{
>> +    ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s);
>> +}
>> +
>> +static void
>> +dpif_show_support(const struct dpif_backer_support *support, struct ds *ds)
>> +{
>> +    show_dp_feature_b(ds, "Variable length userdata",
>> +                      support->variable_length_userdata);
>> +    show_dp_feature_b(ds, "Masked set action", support->masked_set_action);
>> +    show_dp_feature_b(ds, "Tunnel push pop",   support->tnl_push_pop);
>> +    show_dp_feature_b(ds, "Ufid",              support->ufid);
>> +    show_dp_feature_b(ds, "Trunc action",      support->trunc);
>> +    show_dp_feature_b(ds, "Clone action",      support->clone);
>> +    show_dp_feature_s(ds, "Max MPLS depth",    support->odp.max_mpls_depth);
>> +    show_dp_feature_b(ds, "Recirc",            support->odp.recirc);
>> +    show_dp_feature_b(ds, "CT state",          support->odp.ct_state);
>> +    show_dp_feature_b(ds, "CT zone",           support->odp.ct_zone);
>> +    show_dp_feature_b(ds, "CT mark",           support->odp.ct_mark);
>> +    show_dp_feature_b(ds, "CT label",          support->odp.ct_label);
>> +    show_dp_feature_b(ds, "CT State NAT",      support->odp.ct_state_nat);
>> +    show_dp_feature_s(ds, "Max sample nesting",support->sample_nesting);
>
> Needs an extra whitespace on this last line, before support->sample_nesting
>
O.K. Fixed.

>> +}
>> +
>> +static void
>>  dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
>>  {
>>      const struct shash_node **ofprotos;
>> @@ -4899,7 +4931,6 @@ dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
>>      size_t i;
>>
>>      dpif_get_dp_stats(backer->dpif, &dp_stats);
>> -
>>      ds_put_format(ds, "%s: hit:%"PRIu64" missed:%"PRIu64"\n",
>>                    dpif_name(backer->dpif), dp_stats.n_hit, dp_stats.n_missed);
>>
>> @@ -5119,6 +5150,24 @@ disable_datapath_clone(struct unixctl_conn *conn OVS_UNUSED,
>>  }
>>
>>  static void
>> +ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
>> +                                      int argc, const char *argv[],
>> +                                      void *aux OVS_UNUSED)
>> +{
>> +    struct ds ds = DS_EMPTY_INITIALIZER;
>> +    const char *br = argv[argc -1];
>> +    struct ofproto_dpif *ofproto = ofproto_dpif_lookup(br);
>> +
>> +    if (!ofproto) {
>> +        unixctl_command_reply_error(conn, "no such bridge");
>> +        return;
>> +    }
>> +
>> +    dpif_show_support(&ofproto->backer->support, &ds);
>> +    unixctl_command_reply(conn, ds_cstr(&ds));
>> +}
>> +
>> +static void
>>  ofproto_unixctl_init(void)
>>  {
>>      static bool registered;
>> @@ -5139,6 +5188,8 @@ ofproto_unixctl_init(void)
>>                               ofproto_unixctl_dpif_dump_dps, NULL);
>>      unixctl_command_register("dpif/show", "", 0, 0, ofproto_unixctl_dpif_show,
>>                               NULL);
>> +    unixctl_command_register("dpif/show-dp-features", "bridge", 1, 1,
>> +                             ofproto_unixctl_dpif_show_dp_features, NULL);
>>      unixctl_command_register("dpif/dump-flows", "[-m] bridge", 1, 2,
>>                               ofproto_unixctl_dpif_dump_flows, NULL);
>>
>> --
>> 1.8.3.1
>>
>> _______________________________________________
>> dev mailing list
>> dev@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Patch hide | download patch | download mbox

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index df779c2..af70ab3 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -4891,6 +4891,38 @@  ofproto_unixctl_dpif_dump_dps(struct unixctl_conn *conn, int argc OVS_UNUSED,
 }
 
 static void
+show_dp_feature_b(struct ds *ds, const char *feature, bool b)
+{
+    ds_put_format(ds, "%s: %s\n", feature, b ? "Yes" : "No");
+}
+
+static void
+show_dp_feature_s(struct ds *ds, const char *feature, size_t s)
+{
+    ds_put_format(ds, "%s: %"PRIuSIZE"\n", feature, s);
+}
+
+static void
+dpif_show_support(const struct dpif_backer_support *support, struct ds *ds)
+{
+    show_dp_feature_b(ds, "Variable length userdata",
+                      support->variable_length_userdata);
+    show_dp_feature_b(ds, "Masked set action", support->masked_set_action);
+    show_dp_feature_b(ds, "Tunnel push pop",   support->tnl_push_pop);
+    show_dp_feature_b(ds, "Ufid",              support->ufid);
+    show_dp_feature_b(ds, "Trunc action",      support->trunc);
+    show_dp_feature_b(ds, "Clone action",      support->clone);
+    show_dp_feature_s(ds, "Max MPLS depth",    support->odp.max_mpls_depth);
+    show_dp_feature_b(ds, "Recirc",            support->odp.recirc);
+    show_dp_feature_b(ds, "CT state",          support->odp.ct_state);
+    show_dp_feature_b(ds, "CT zone",           support->odp.ct_zone);
+    show_dp_feature_b(ds, "CT mark",           support->odp.ct_mark);
+    show_dp_feature_b(ds, "CT label",          support->odp.ct_label);
+    show_dp_feature_b(ds, "CT State NAT",      support->odp.ct_state_nat);
+    show_dp_feature_s(ds, "Max sample nesting",support->sample_nesting);
+}
+
+static void
 dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
 {
     const struct shash_node **ofprotos;
@@ -4899,7 +4931,6 @@  dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
     size_t i;
 
     dpif_get_dp_stats(backer->dpif, &dp_stats);
-
     ds_put_format(ds, "%s: hit:%"PRIu64" missed:%"PRIu64"\n",
                   dpif_name(backer->dpif), dp_stats.n_hit, dp_stats.n_missed);
 
@@ -5119,6 +5150,24 @@  disable_datapath_clone(struct unixctl_conn *conn OVS_UNUSED,
 }
 
 static void
+ofproto_unixctl_dpif_show_dp_features(struct unixctl_conn *conn,
+                                      int argc, const char *argv[],
+                                      void *aux OVS_UNUSED)
+{
+    struct ds ds = DS_EMPTY_INITIALIZER;
+    const char *br = argv[argc -1];
+    struct ofproto_dpif *ofproto = ofproto_dpif_lookup(br);
+
+    if (!ofproto) {
+        unixctl_command_reply_error(conn, "no such bridge");
+        return;
+    }
+
+    dpif_show_support(&ofproto->backer->support, &ds);
+    unixctl_command_reply(conn, ds_cstr(&ds));
+}
+
+static void
 ofproto_unixctl_init(void)
 {
     static bool registered;
@@ -5139,6 +5188,8 @@  ofproto_unixctl_init(void)
                              ofproto_unixctl_dpif_dump_dps, NULL);
     unixctl_command_register("dpif/show", "", 0, 0, ofproto_unixctl_dpif_show,
                              NULL);
+    unixctl_command_register("dpif/show-dp-features", "bridge", 1, 1,
+                             ofproto_unixctl_dpif_show_dp_features, NULL);
     unixctl_command_register("dpif/dump-flows", "[-m] bridge", 1, 2,
                              ofproto_unixctl_dpif_dump_flows, NULL);