| Message ID | 1523242444-76467-10-git-send-email-dlu998@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Justin Pettit |
| Headers | show |
| Series | Userspace datapath: Add fragmentation support. | expand |
> On Apr 8, 2018, at 7:54 PM, Darrell Ball <dlu998@gmail.com> wrote: > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > index 60c8986..adcf42b 100644 > --- a/lib/ct-dpif.c > +++ b/lib/ct-dpif.c > @@ -209,6 +209,30 @@ int ct_dpif_ipf_get_status(struct dpif *dpif, bool *ipf_v4_enabled, > : EOPNOTSUPP); > } > > +int > +ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **dump_ctx) > +{ > + return (dpif->dpif_class->ipf_dump_start > + ? dpif->dpif_class->ipf_dump_start(dpif, dump_ctx) > + : EOPNOTSUPP); > +} > + > +int > +ct_dpif_ipf_dump_next(struct dpif *dpif, void *dump_ctx, char **dump) > +{ > + return (dpif->dpif_class->ipf_dump_next > + ? dpif->dpif_class->ipf_dump_next(dpif, dump_ctx, dump) > + : EOPNOTSUPP); > +} > + > +int > +ct_dpif_ipf_dump_done(struct dpif *dpif, void *dump_ctx) > +{ > + return (dpif->dpif_class->ipf_dump_done > + ? dpif->dpif_class->ipf_dump_done(dpif, dump_ctx) > + : EOPNOTSUPP); > +} It would be helpful to have descriptions for these functions, including mentioning that '*dump' must be freed and that ct_dpif_ipf_dump_done() must be called after a call to ct_dpif_ipf_dump_start() > diff --git a/lib/dpctl.c b/lib/dpctl.c > index 84064cd..7c1aa65 100644 > --- a/lib/dpctl.c > +++ b/lib/dpctl.c > > @@ -1852,12 +1853,37 @@ dpctl_ct_ipf_set_nfrag_max(int argc, const char *argv[], > return error; > } > > +static void > +dpctl_dump_ipf(struct dpif *dpif, struct dpctl_params *dpctl_p) > +{ > + struct ipf_dump_ctx *dump_ctx; > + char *dump; > + > + int error = ct_dpif_ipf_dump_start(dpif, &dump_ctx); > + if (error) { > + dpctl_error(dpctl_p, error, "starting ipf dump"); > + return; > + } > + > + dpctl_print(dpctl_p, "\n\tFragment Lists:\n\n"); I think this is a single list. > diff --git a/lib/dpctl.man b/lib/dpctl.man > index 2e8c287..afb270e 100644 > --- a/lib/dpctl.man > +++ b/lib/dpctl.man > @@ -296,6 +296,7 @@ module while fragments are incomplete, but will timeout after 15 seconds. > Memory pool sizing should be set accordingly when fragmentation is enabled. > . > .TP > -\*(DX\fBipf\-get\-status\fR [\fIdp\fR] > +\*(DX\fBipf\-get\-status\fR [\fIdp\fR] [\fIverbose\fR] > Gets the configuration settings and fragment counters associated with the > -fragmentation handling of the userspace datapath connection tracker. > +fragmentation handling of the userspace datapath connection tracker. If > +verbose is specified, also dumps the ipf list entries. We usually use "-m" and "--more" as the flags to indicate verbose. The description of "dump-conntrack" provides one example. Also, like "dump-conntrack", it might be nice to provide a "zone" argument. > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > index 82fbbfc..385394f 100644 > --- a/lib/dpif-provider.h > +++ b/lib/dpif-provider.h > @@ -24,6 +24,7 @@ > > #include "openflow/openflow.h" > #include "dpif.h" > +#include "ipf.h" > #include "util.h" > > #ifdef __cplusplus > @@ -457,6 +458,10 @@ struct dpif_class { > unsigned int *, bool *, unsigned int *, unsigned int *, > unsigned int *, unsigned int *, unsigned int *, > unsigned int *); > + int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx **); > + /* Gets an ipf list entry to display. */ > + int (*ipf_dump_next)(struct dpif *, void *, char **); In reference to this comment, it doesn't really get an ipf entry as much as write it to a buffer. > +static void > +ipf_dump_create(const struct ipf_list *ipf_list, struct ds *ds) > +{ > + > + ds_put_cstr(ds, "frag list elem=("); Since the header will state that this is a frag list, I don't think it's necessary to prepend this to each element. Ideally this would look and act similar to other dump commands such as "dump-conntrack". This appears to be just adding a verbose option to the previous patch, so I think these patches can just be merged into a single one. Thanks, --Justin
On Thu, Jun 7, 2018 at 12:11 AM, Justin Pettit <jpettit@ovn.org> wrote: > > > On Apr 8, 2018, at 7:54 PM, Darrell Ball <dlu998@gmail.com> wrote: > > > > diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c > > index 60c8986..adcf42b 100644 > > --- a/lib/ct-dpif.c > > +++ b/lib/ct-dpif.c > > @@ -209,6 +209,30 @@ int ct_dpif_ipf_get_status(struct dpif *dpif, bool > *ipf_v4_enabled, > > : EOPNOTSUPP); > > } > > > > +int > > +ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx > **dump_ctx) > > +{ > > + return (dpif->dpif_class->ipf_dump_start > > + ? dpif->dpif_class->ipf_dump_start(dpif, dump_ctx) > > + : EOPNOTSUPP); > > +} > > + > > +int > > +ct_dpif_ipf_dump_next(struct dpif *dpif, void *dump_ctx, char **dump) > > +{ > > + return (dpif->dpif_class->ipf_dump_next > > + ? dpif->dpif_class->ipf_dump_next(dpif, dump_ctx, dump) > > + : EOPNOTSUPP); > > +} > > + > > +int > > +ct_dpif_ipf_dump_done(struct dpif *dpif, void *dump_ctx) > > +{ > > + return (dpif->dpif_class->ipf_dump_done > > + ? dpif->dpif_class->ipf_dump_done(dpif, dump_ctx) > > + : EOPNOTSUPP); > > +} > > It would be helpful to have descriptions for these functions, including > mentioning that '*dump' must be freed and that ct_dpif_ipf_dump_done() must > be called after a call to ct_dpif_ipf_dump_start() > sure; I added descriptions > > > diff --git a/lib/dpctl.c b/lib/dpctl.c > > index 84064cd..7c1aa65 100644 > > --- a/lib/dpctl.c > > +++ b/lib/dpctl.c > > > > @@ -1852,12 +1853,37 @@ dpctl_ct_ipf_set_nfrag_max(int argc, const char > *argv[], > > return error; > > } > > > > +static void > > +dpctl_dump_ipf(struct dpif *dpif, struct dpctl_params *dpctl_p) > > +{ > > + struct ipf_dump_ctx *dump_ctx; > > + char *dump; > > + > > + int error = ct_dpif_ipf_dump_start(dpif, &dump_ctx); > > + if (error) { > > + dpctl_error(dpctl_p, error, "starting ipf dump"); > > + return; > > + } > > + > > + dpctl_print(dpctl_p, "\n\tFragment Lists:\n\n"); > > I think this is a single list. > It is a list of fragment lists, where one fragment list represents a list of fragments, so there are multiple "fragment lists". > > > diff --git a/lib/dpctl.man b/lib/dpctl.man > > index 2e8c287..afb270e 100644 > > --- a/lib/dpctl.man > > +++ b/lib/dpctl.man > > @@ -296,6 +296,7 @@ module while fragments are incomplete, but will > timeout after 15 seconds. > > Memory pool sizing should be set accordingly when fragmentation is > enabled. > > . > > .TP > > -\*(DX\fBipf\-get\-status\fR [\fIdp\fR] > > +\*(DX\fBipf\-get\-status\fR [\fIdp\fR] [\fIverbose\fR] > > Gets the configuration settings and fragment counters associated with the > > -fragmentation handling of the userspace datapath connection tracker. > > +fragmentation handling of the userspace datapath connection tracker. If > > +verbose is specified, also dumps the ipf list entries. > > We usually use "-m" and "--more" as the flags to indicate verbose. The > description of "dump-conntrack" provides one example. Also, like > "dump-conntrack", it might be nice to provide a "zone" argument. > Since I saw both "verbose" and "more" used, I was not sure which was preferred I switched over to "more"; it seems to cover a wider range of meanings. I left out the "zone" option for now; these lists are expected to be very transient under normal conditions unlike conntrack and the if there is an error condition with lots of fragment lists, the user can always just grep the zone value. > > > diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h > > index 82fbbfc..385394f 100644 > > --- a/lib/dpif-provider.h > > +++ b/lib/dpif-provider.h > > @@ -24,6 +24,7 @@ > > > > #include "openflow/openflow.h" > > #include "dpif.h" > > +#include "ipf.h" > > #include "util.h" > > > > #ifdef __cplusplus > > @@ -457,6 +458,10 @@ struct dpif_class { > > unsigned int *, bool *, unsigned int *, unsigned int *, > > unsigned int *, unsigned int *, unsigned int *, > > unsigned int *); > > + int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx **); > > + /* Gets an ipf list entry to display. */ > > + int (*ipf_dump_next)(struct dpif *, void *, char **); > > In reference to this comment, it doesn't really get an ipf entry as much > as write it to a buffer. > > > +static void > > +ipf_dump_create(const struct ipf_list *ipf_list, struct ds *ds) > > +{ > > + > > + ds_put_cstr(ds, "frag list elem=("); > > Since the header will state that this is a frag list, I don't think it's > necessary to prepend this to each element. Ideally this would look and act > similar to other dump commands such as "dump-conntrack". > it is not needed; I dropped the prepending. > > This appears to be just adding a verbose option to the previous patch, so > I think these patches can just be merged into a single one. > I originally spliced out this patch since it covers internal state, whereas the first patch is externally observable information and thinking it was easier to review being split up. Merging together is also fine and easier for me to administer, so sure, I merged them now. > > Thanks, > > --Justin > > >
diff --git a/lib/ct-dpif.c b/lib/ct-dpif.c index 60c8986..adcf42b 100644 --- a/lib/ct-dpif.c +++ b/lib/ct-dpif.c @@ -209,6 +209,30 @@ int ct_dpif_ipf_get_status(struct dpif *dpif, bool *ipf_v4_enabled, : EOPNOTSUPP); } +int +ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **dump_ctx) +{ + return (dpif->dpif_class->ipf_dump_start + ? dpif->dpif_class->ipf_dump_start(dpif, dump_ctx) + : EOPNOTSUPP); +} + +int +ct_dpif_ipf_dump_next(struct dpif *dpif, void *dump_ctx, char **dump) +{ + return (dpif->dpif_class->ipf_dump_next + ? dpif->dpif_class->ipf_dump_next(dpif, dump_ctx, dump) + : EOPNOTSUPP); +} + +int +ct_dpif_ipf_dump_done(struct dpif *dpif, void *dump_ctx) +{ + return (dpif->dpif_class->ipf_dump_done + ? dpif->dpif_class->ipf_dump_done(dpif, dump_ctx) + : EOPNOTSUPP); +} + void ct_dpif_entry_uninit(struct ct_dpif_entry *entry) { diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h index 8a24128..0db17d9 100644 --- a/lib/ct-dpif.h +++ b/lib/ct-dpif.h @@ -17,6 +17,7 @@ #ifndef CT_DPIF_H #define CT_DPIF_H +#include "ipf.h" #include "openvswitch/types.h" #include "packets.h" @@ -209,6 +210,9 @@ int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *, unsigned int *, bool *, unsigned int *, unsigned int *, unsigned int *, unsigned int *, unsigned int *, unsigned int *); +int ct_dpif_ipf_dump_start(struct dpif *dpif, struct ipf_dump_ctx **); +int ct_dpif_ipf_dump_next(struct dpif *dpif, void *, char **); +int ct_dpif_ipf_dump_done(struct dpif *dpif, void *); void ct_dpif_entry_uninit(struct ct_dpif_entry *); void ct_dpif_format_entry(const struct ct_dpif_entry *, struct ds *, bool verbose, bool print_stats); diff --git a/lib/dpctl.c b/lib/dpctl.c index 84064cd..7c1aa65 100644 --- a/lib/dpctl.c +++ b/lib/dpctl.c @@ -35,6 +35,7 @@ #include "dpif.h" #include "openvswitch/dynamic-string.h" #include "flow.h" +#include "ipf.h" #include "openvswitch/match.h" #include "netdev.h" #include "netdev-dpdk.h" @@ -1852,12 +1853,37 @@ dpctl_ct_ipf_set_nfrag_max(int argc, const char *argv[], return error; } +static void +dpctl_dump_ipf(struct dpif *dpif, struct dpctl_params *dpctl_p) +{ + struct ipf_dump_ctx *dump_ctx; + char *dump; + + int error = ct_dpif_ipf_dump_start(dpif, &dump_ctx); + if (error) { + dpctl_error(dpctl_p, error, "starting ipf dump"); + return; + } + + dpctl_print(dpctl_p, "\n\tFragment Lists:\n\n"); + while (!(error = ct_dpif_ipf_dump_next(dpif, dump_ctx, &dump))) { + dpctl_print(dpctl_p, "%s\n", dump); + free(dump); + } + + if (error && error != EOF) { + dpctl_error(dpctl_p, error, "dumping ipf entry"); + } + + ct_dpif_ipf_dump_done(dpif, dump_ctx); +} + static int dpctl_ct_ipf_get_status(int argc, const char *argv[], struct dpctl_params *dpctl_p) { struct dpif *dpif; - int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif, 2); + int error = dpctl_ct_open_dp(argc, argv, dpctl_p, &dpif, 3); if (!error) { bool ipf_v4_enabled; unsigned int min_v4_frag_size; @@ -1916,6 +1942,11 @@ dpctl_ct_ipf_get_status(int argc, const char *argv[], } else { dpctl_error(dpctl_p, error, "ipf status could not be retrieved"); + return error; + } + + if (argc > 1 && !strncmp(argv[argc - 1], "verbose", 7)) { + dpctl_dump_ipf(dpif, dpctl_p); } dpif_close(dpif); @@ -2229,7 +2260,7 @@ static const struct dpctl_command all_commands[] = { dpctl_ct_ipf_set_min_frag, DP_RW }, { "ipf-set-maxfrags", "[dp] maxfrags", 1, 2, dpctl_ct_ipf_set_nfrag_max, DP_RW }, - { "ipf-get-status", "[dp]", 0, 1, dpctl_ct_ipf_get_status, + { "ipf-get-status", "[dp] [verbose]", 0, 2, dpctl_ct_ipf_get_status, DP_RO }, { "help", "", 0, INT_MAX, dpctl_help, DP_RO }, { "list-commands", "", 0, INT_MAX, dpctl_list_commands, DP_RO }, diff --git a/lib/dpctl.man b/lib/dpctl.man index 2e8c287..afb270e 100644 --- a/lib/dpctl.man +++ b/lib/dpctl.man @@ -296,6 +296,7 @@ module while fragments are incomplete, but will timeout after 15 seconds. Memory pool sizing should be set accordingly when fragmentation is enabled. . .TP -\*(DX\fBipf\-get\-status\fR [\fIdp\fR] +\*(DX\fBipf\-get\-status\fR [\fIdp\fR] [\fIverbose\fR] Gets the configuration settings and fragment counters associated with the -fragmentation handling of the userspace datapath connection tracker. +fragmentation handling of the userspace datapath connection tracker. If +verbose is specified, also dumps the ipf list entries. diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index 35094f0..37f786b 100644 --- a/lib/dpif-netdev.c +++ b/lib/dpif-netdev.c @@ -5924,6 +5924,28 @@ dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED, return 0; } +static int +dpif_netdev_ipf_dump_start(struct dpif *dpif OVS_UNUSED, + struct ipf_dump_ctx **ipf_dump_ctx) +{ + return ipf_dump_start(ipf_dump_ctx); +} + +static int +dpif_netdev_ipf_dump_next(struct dpif *dpif OVS_UNUSED, + void *ipf_dump_ctx, char **dump) +{ + return ipf_dump_next(ipf_dump_ctx, dump); +} + +static int +dpif_netdev_ipf_dump_done(struct dpif *dpif OVS_UNUSED, + void *ipf_dump_ctx) +{ + return ipf_dump_done(ipf_dump_ctx); + +} + const struct dpif_class dpif_netdev_class = { "netdev", dpif_netdev_init, @@ -5976,6 +5998,9 @@ const struct dpif_class dpif_netdev_class = { dpif_netdev_ipf_set_min_frag, dpif_netdev_ipf_set_nfrag_max, dpif_netdev_ipf_get_status, + dpif_netdev_ipf_dump_start, + dpif_netdev_ipf_dump_next, + dpif_netdev_ipf_dump_done, dpif_netdev_meter_get_features, dpif_netdev_meter_set, dpif_netdev_meter_get, diff --git a/lib/dpif-netlink.c b/lib/dpif-netlink.c index 73bf31a..e0833fb 100644 --- a/lib/dpif-netlink.c +++ b/lib/dpif-netlink.c @@ -2998,6 +2998,9 @@ const struct dpif_class dpif_netlink_class = { NULL, /* ipf_set_min_frag */ NULL, /* ipf_set_nfrag_max */ NULL, /* ipf_get_status */ + NULL, /* ipf_dump_start */ + NULL, /* ipf_dump_next */ + NULL, /* ipf_dump_done */ dpif_netlink_meter_get_features, dpif_netlink_meter_set, dpif_netlink_meter_get, diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h index 82fbbfc..385394f 100644 --- a/lib/dpif-provider.h +++ b/lib/dpif-provider.h @@ -24,6 +24,7 @@ #include "openflow/openflow.h" #include "dpif.h" +#include "ipf.h" #include "util.h" #ifdef __cplusplus @@ -457,6 +458,10 @@ struct dpif_class { unsigned int *, bool *, unsigned int *, unsigned int *, unsigned int *, unsigned int *, unsigned int *, unsigned int *); + int (*ipf_dump_start)(struct dpif *, struct ipf_dump_ctx **); + /* Gets an ipf list entry to display. */ + int (*ipf_dump_next)(struct dpif *, void *, char **); + int (*ipf_dump_done)(struct dpif *, void *); /* Meters */ /* Queries 'dpif' for supported meter features. diff --git a/lib/ipf.c b/lib/ipf.c index a62f4ea..2963dd5 100644 --- a/lib/ipf.c +++ b/lib/ipf.c @@ -52,6 +52,10 @@ enum ipf_list_state { IPF_LIST_STATE_NUM, }; +static char *ipf_state_name[IPF_LIST_STATE_NUM] = + {"unused", "reassemble fail", "other frag", "first frag", "last frag", + "first/last frag", "complete"}; + enum ipf_list_type { IPF_FRAG_COMPLETED_LIST, IPF_FRAG_EXPIRY_LIST, @@ -1310,3 +1314,73 @@ ipf_get_status(struct ipf_status *ipf_status) ipf_status->n6frag_overlap = atomic_count_get(&n6frag_overlap); return 0; } + +struct ipf_dump_ctx { + struct hmap_position bucket_pos; +}; + +int +ipf_dump_start(struct ipf_dump_ctx **ipf_dump_ctx) +{ + *ipf_dump_ctx = xzalloc(sizeof **ipf_dump_ctx); + return 0; +} + +static void +ipf_dump_create(const struct ipf_list *ipf_list, struct ds *ds) +{ + + ds_put_cstr(ds, "frag list elem=("); + if (ipf_list->key.dl_type == htons(ETH_TYPE_IP)) { + ds_put_format(ds, "src="IP_FMT",dst="IP_FMT",", + IP_ARGS(ipf_list->key.src_addr.ipv4_aligned), + IP_ARGS(ipf_list->key.dst_addr.ipv4_aligned)); + } else { + ds_put_cstr(ds, "src="); + ipv6_format_addr(&ipf_list->key.src_addr.ipv6_aligned, ds); + ds_put_cstr(ds, ",dst="); + ipv6_format_addr(&ipf_list->key.dst_addr.ipv6_aligned, ds); + ds_put_cstr(ds, ","); + } + + ds_put_format(ds, "recirc_id=%u,ip_id=%u,dl_type=0x%x,zone=%u,nw_proto=%u", + ipf_list->key.recirc_id, ntohl(ipf_list->key.ip_id), + ntohs(ipf_list->key.dl_type), ipf_list->key.zone, + ipf_list->key.nw_proto); + + ds_put_format(ds, ",num_fragments=%u,state=%s", + ipf_list->last_inuse_idx + 1, + ipf_state_name[ipf_list->state]); + + ds_put_cstr(ds, ")"); +} + +int +ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx, char **dump) +{ + ipf_lock_lock(&ipf_lock); + + struct hmap_node *node = hmap_at_position(&frag_lists, + &ipf_dump_ctx->bucket_pos); + if (!node) { + ipf_lock_unlock(&ipf_lock); + return EOF; + } else { + struct ipf_list *ipf_list_; + INIT_CONTAINER(ipf_list_, node, node); + struct ipf_list ipf_list = *ipf_list_; + ipf_lock_unlock(&ipf_lock); + struct ds ds = DS_EMPTY_INITIALIZER; + ipf_dump_create(&ipf_list, &ds); + *dump = xstrdup(ds.string); + ds_destroy(&ds); + return 0; + } +} + +int +ipf_dump_done(struct ipf_dump_ctx *ipf_dump_ctx) +{ + free(ipf_dump_ctx); + return 0; +} diff --git a/lib/ipf.h b/lib/ipf.h index a9fee06..866c711 100644 --- a/lib/ipf.h +++ b/lib/ipf.h @@ -72,4 +72,15 @@ ipf_set_nfrag_max(uint32_t value); int ipf_get_status(struct ipf_status *ipf_status); +struct ipf_dump_ctx; + +int +ipf_dump_start(struct ipf_dump_ctx **ipf_dump_ctx); + +int +ipf_dump_next(struct ipf_dump_ctx *ipf_dump_ctx, char **dump); + +int +ipf_dump_done(struct ipf_dump_ctx *ipf_dump_ctx); + #endif /* ipf.h */
A verbose option is added to dump the frag lists. Signed-off-by: Darrell Ball <dlu998@gmail.com> --- lib/ct-dpif.c | 24 +++++++++++++++++ lib/ct-dpif.h | 4 +++ lib/dpctl.c | 35 +++++++++++++++++++++++-- lib/dpctl.man | 5 ++-- lib/dpif-netdev.c | 25 ++++++++++++++++++ lib/dpif-netlink.c | 3 +++ lib/dpif-provider.h | 5 ++++ lib/ipf.c | 74 +++++++++++++++++++++++++++++++++++++++++++++++++++++ lib/ipf.h | 11 ++++++++ 9 files changed, 182 insertions(+), 4 deletions(-)