Re: [ovs-dev] [patch v8 9/9] ipf: Add fragmentation status reporting.
> On Aug 13, 2018, at 2:19 PM, Darrell Ball wrote:
>
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 5ec36bd..b3e7ce7 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > ..
> > +static int
> > +dpctl_ct_ipf_get_status(int argc, const char *argv[],
> > +struct dpctl_params *dpctl_p)
> > +{
> > +struct dpif *dpif;
> > +int error = opt_dpif_open(argc, argv, dpctl_p, 3, );
> > +if (!error) {
> > +bool ipf_v4_enabled;
> > +unsigned int min_v4_frag_size;
> > +unsigned int nfrag_max;
> > +unsigned int nfrag;
> > +unsigned int n4frag_accepted;
> > +unsigned int n4frag_completed_sent;
> > +unsigned int n4frag_expired_sent;
> > +unsigned int n4frag_too_small;
> > +unsigned int n4frag_overlap;
> > +unsigned int min_v6_frag_size;
> > +bool ipf_v6_enabled;
> > +unsigned int n6frag_accepted;
> > +unsigned int n6frag_completed_sent;
> > +unsigned int n6frag_expired_sent;
> > +unsigned int n6frag_too_small;
> > +unsigned int n6frag_overlap;
> > +error = ct_dpif_ipf_get_status(dpif, _v4_enabled,
> > +_v4_frag_size, _max, , _accepted,
> > +_completed_sent, _expired_sent,
> > _too_small,
> > +_overlap, _v6_enabled, _v6_frag_size,
> > +_accepted, _completed_sent, _expired_sent,
> > +_too_small, _overlap);
> > +
>
> If we just use 'ipf_status', we can delete a lot of these variable
> declarations.
>
>
> I understand; I commented earlier about this. The reason for not using
> ipf_status is not because I like typing lots.
>
> The reasoning is:
>
> 1/ ipf_status is specific to the userspace datapath. The kernel datapath and
> various hardware offload
> datapaths may not use or even be able to use the same datatype.
> Note that dpctl and dpif code is intended to be generic across datapaths.
>
> 2/ Using ipf_status exposes what is intended to be private internal
> datastructures to external modules
> I was trying to avoid that, if possible, and use base types in common code.
>
> Perhaps, the easiest way to address this to simply have a dpif_ipf_status,
> which is defined similarly to ipf_status, at
> least initially ? These can diverge later if needed,
Yes, I had those same thoughts, so I think just creating a separate
"dpif_ipf_status" would be a good solutions.
--Justin
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [patch v8 9/9] ipf: Add fragmentation status reporting.
Thanks Justin
By the way, I just noticed that you reviewed most patches on Aug 1.
Since the company e-mail is filtering some OVS list e-mails and I have
relied on
it just for notifications and was otherwise busy, I missed those. I am going
through those carefully now.
One outstanding comment inline that I wanted to double check.
On Fri, Aug 10, 2018 at 8:00 PM, Justin Pettit wrote:
>
> > On Jul 16, 2018, at 4:39 PM, Darrell Ball wrote:
> >
> > void
> > ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
> > {
> > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> > index f886ab9..2ff7e26 100644
> > --- a/lib/ct-dpif.h
> > +++ b/lib/ct-dpif.h
> > @@ -204,6 +204,15 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t
> *nconns);
> > int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
> > int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
> > int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
> > +int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
> > + unsigned int *, unsigned int *, unsigned int
> *,
> > + unsigned int *, unsigned int *, 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 **);
>
> On my system, I needed to declare "ipf_dump_ctx" for the build to finish.
>
I re-ran Travis
Bssically the same code as V8
Here is the link:
Travis:
https://travis-ci.org/darball/ovs_master
The corresponding public git repo branch:
https://github.com/darball/ovs_master/commits/conntrack
Possibility, the build issue you saw is related to your proposed change in
removing #include "ipf.h"
from patch 7.
diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
index 6eb55b4b0197..f8a31921e16a 100644
--- a/lib/ct-dpif.h
+++ b/lib/ct-dpif.h
@@ -17,7 +17,6 @@
#ifndef CT_DPIF_H
#define CT_DPIF_H
-#include "ipf.h"
#include "openvswitch/types.h"
#include "packets.h"
ipf.h has the declaration for ipf_dump_ctx.
>
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 5ec36bd..b3e7ce7 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > ..
> > +static int
> > +dpctl_ct_ipf_get_status(int argc, const char *argv[],
> > +struct dpctl_params *dpctl_p)
> > +{
> > +struct dpif *dpif;
> > +int error = opt_dpif_open(argc, argv, dpctl_p, 3, );
> > +if (!error) {
> > +bool ipf_v4_enabled;
> > +unsigned int min_v4_frag_size;
> > +unsigned int nfrag_max;
> > +unsigned int nfrag;
> > +unsigned int n4frag_accepted;
> > +unsigned int n4frag_completed_sent;
> > +unsigned int n4frag_expired_sent;
> > +unsigned int n4frag_too_small;
> > +unsigned int n4frag_overlap;
> > +unsigned int min_v6_frag_size;
> > +bool ipf_v6_enabled;
> > +unsigned int n6frag_accepted;
> > +unsigned int n6frag_completed_sent;
> > +unsigned int n6frag_expired_sent;
> > +unsigned int n6frag_too_small;
> > +unsigned int n6frag_overlap;
> > +error = ct_dpif_ipf_get_status(dpif, _v4_enabled,
> > +_v4_frag_size, _max, , _accepted,
> > +_completed_sent, _expired_sent,
> _too_small,
> > +_overlap, _v6_enabled, _v6_frag_size,
> > +_accepted, _completed_sent,
> _expired_sent,
> > +_too_small, _overlap);
> > +
>
> If we just use 'ipf_status', we can delete a lot of these variable
> declarations.
>
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 76bc1d9..db551ea 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -6553,6 +6553,60 @@ dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif
> OVS_UNUSED,
> > return ipf_set_max_nfrags(max_frags);
> > }
> >
> > +static int
> > +dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> > +bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> > +unsigned int *nfrag_max, unsigned int *nfrag,
> > +unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> > +unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> > +unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> > +unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> > +unsigned int *n6frag_completed_sent, unsigned int
> *n6frag_expired_sent,
> > +unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> > +{
> > +struct ipf_status ipf_status;
> > +ipf_get_status(_status);
> > +*ipf_v4_enabled = ipf_status.ifp_v4_enabled;
> > +*min_v4_frag_size = ipf_status.min_v4_frag_size;
> > +*nfrag_max = ipf_status.nfrag_max;
> > +*nfrag = ipf_status.nfrag;
> > +*n4frag_accepted = ipf_status.n4frag_accepted;
> > +*n4frag_completed_sent = ipf_status.n4frag_completed_sent;
> >
Re: [ovs-dev] [patch v8 9/9] ipf: Add fragmentation status reporting.
Thanks.
Darrell
On Fri, Aug 10, 2018 at 8:00 PM, Justin Pettit wrote:
>
> > On Jul 16, 2018, at 4:39 PM, Darrell Ball wrote:
> >
> > void
> > ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
> > {
> > diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> > index f886ab9..2ff7e26 100644
> > --- a/lib/ct-dpif.h
> > +++ b/lib/ct-dpif.h
> > @@ -204,6 +204,15 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t
> *nconns);
> > int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
> > int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
> > int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
> > +int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
> > + unsigned int *, unsigned int *, unsigned int
> *,
> > + unsigned int *, unsigned int *, 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 **);
>
> On my system, I needed to declare "ipf_dump_ctx" for the build to finish.
>
> > diff --git a/lib/dpctl.c b/lib/dpctl.c
> > index 5ec36bd..b3e7ce7 100644
> > --- a/lib/dpctl.c
> > +++ b/lib/dpctl.c
> > ..
> > +static int
> > +dpctl_ct_ipf_get_status(int argc, const char *argv[],
> > +struct dpctl_params *dpctl_p)
> > +{
> > +struct dpif *dpif;
> > +int error = opt_dpif_open(argc, argv, dpctl_p, 3, );
> > +if (!error) {
> > +bool ipf_v4_enabled;
> > +unsigned int min_v4_frag_size;
> > +unsigned int nfrag_max;
> > +unsigned int nfrag;
> > +unsigned int n4frag_accepted;
> > +unsigned int n4frag_completed_sent;
> > +unsigned int n4frag_expired_sent;
> > +unsigned int n4frag_too_small;
> > +unsigned int n4frag_overlap;
> > +unsigned int min_v6_frag_size;
> > +bool ipf_v6_enabled;
> > +unsigned int n6frag_accepted;
> > +unsigned int n6frag_completed_sent;
> > +unsigned int n6frag_expired_sent;
> > +unsigned int n6frag_too_small;
> > +unsigned int n6frag_overlap;
> > +error = ct_dpif_ipf_get_status(dpif, _v4_enabled,
> > +_v4_frag_size, _max, , _accepted,
> > +_completed_sent, _expired_sent,
> _too_small,
> > +_overlap, _v6_enabled, _v6_frag_size,
> > +_accepted, _completed_sent,
> _expired_sent,
> > +_too_small, _overlap);
> > +
>
> If we just use 'ipf_status', we can delete a lot of these variable
> declarations.
>
I understand; I commented earlier about this. The reason for not using
ipf_status is not because I like typing lots.
The reasoning is:
1/ ipf_status is specific to the userspace datapath. The kernel datapath
and various hardware offload
datapaths may not use or even be able to use the same datatype.
Note that dpctl and dpif code is intended to be generic across datapaths.
2/ Using ipf_status exposes what is intended to be private internal
datastructures to external modules
I was trying to avoid that, if possible, and use base types in common code.
Perhaps, the easiest way to address this to simply have a dpif_ipf_status,
which is defined similarly to ipf_status, at
least initially ? These can diverge later if needed,
>
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 76bc1d9..db551ea 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -6553,6 +6553,60 @@ dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif
> OVS_UNUSED,
> > return ipf_set_max_nfrags(max_frags);
> > }
> >
> > +static int
> > +dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> > +bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> > +unsigned int *nfrag_max, unsigned int *nfrag,
> > +unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> > +unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> > +unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> > +unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> > +unsigned int *n6frag_completed_sent, unsigned int
> *n6frag_expired_sent,
> > +unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> > +{
> > +struct ipf_status ipf_status;
> > +ipf_get_status(_status);
> > +*ipf_v4_enabled = ipf_status.ifp_v4_enabled;
> > +*min_v4_frag_size = ipf_status.min_v4_frag_size;
> > +*nfrag_max = ipf_status.nfrag_max;
> > +*nfrag = ipf_status.nfrag;
> > +*n4frag_accepted = ipf_status.n4frag_accepted;
> > +*n4frag_completed_sent = ipf_status.n4frag_completed_sent;
> > +*n4frag_expired_sent = ipf_status.n4frag_expired_sent;
> > +*n4frag_too_small = ipf_status.n4frag_too_small;
> > +*n4frag_overlap = ipf_status.n4frag_overlap;
> > +*ipf_v6_enabled =
Re: [ovs-dev] [patch v8 9/9] ipf: Add fragmentation status reporting.
> On Jul 16, 2018, at 4:39 PM, Darrell Ball wrote:
>
> void
> ct_dpif_entry_uninit(struct ct_dpif_entry *entry)
> {
> diff --git a/lib/ct-dpif.h b/lib/ct-dpif.h
> index f886ab9..2ff7e26 100644
> --- a/lib/ct-dpif.h
> +++ b/lib/ct-dpif.h
> @@ -204,6 +204,15 @@ int ct_dpif_get_nconns(struct dpif *dpif, uint32_t
> *nconns);
> int ct_dpif_ipf_set_enabled(struct dpif *, bool v6, bool enable);
> int ct_dpif_ipf_set_min_frag(struct dpif *, bool, uint32_t);
> int ct_dpif_ipf_set_max_nfrags(struct dpif *, uint32_t);
> +int ct_dpif_ipf_get_status(struct dpif *dpif, bool *, unsigned int *,
> + unsigned int *, unsigned int *, unsigned int *,
> + unsigned int *, unsigned int *, 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 **);
On my system, I needed to declare "ipf_dump_ctx" for the build to finish.
> diff --git a/lib/dpctl.c b/lib/dpctl.c
> index 5ec36bd..b3e7ce7 100644
> --- a/lib/dpctl.c
> +++ b/lib/dpctl.c
> ..
> +static int
> +dpctl_ct_ipf_get_status(int argc, const char *argv[],
> +struct dpctl_params *dpctl_p)
> +{
> +struct dpif *dpif;
> +int error = opt_dpif_open(argc, argv, dpctl_p, 3, );
> +if (!error) {
> +bool ipf_v4_enabled;
> +unsigned int min_v4_frag_size;
> +unsigned int nfrag_max;
> +unsigned int nfrag;
> +unsigned int n4frag_accepted;
> +unsigned int n4frag_completed_sent;
> +unsigned int n4frag_expired_sent;
> +unsigned int n4frag_too_small;
> +unsigned int n4frag_overlap;
> +unsigned int min_v6_frag_size;
> +bool ipf_v6_enabled;
> +unsigned int n6frag_accepted;
> +unsigned int n6frag_completed_sent;
> +unsigned int n6frag_expired_sent;
> +unsigned int n6frag_too_small;
> +unsigned int n6frag_overlap;
> +error = ct_dpif_ipf_get_status(dpif, _v4_enabled,
> +_v4_frag_size, _max, , _accepted,
> +_completed_sent, _expired_sent, _too_small,
> +_overlap, _v6_enabled, _v6_frag_size,
> +_accepted, _completed_sent, _expired_sent,
> +_too_small, _overlap);
> +
If we just use 'ipf_status', we can delete a lot of these variable declarations.
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index 76bc1d9..db551ea 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -6553,6 +6553,60 @@ dpif_netdev_ipf_set_max_nfrags(struct dpif *dpif
> OVS_UNUSED,
> return ipf_set_max_nfrags(max_frags);
> }
>
> +static int
> +dpif_netdev_ipf_get_status(struct dpif *dpif OVS_UNUSED,
> +bool *ipf_v4_enabled, unsigned int *min_v4_frag_size,
> +unsigned int *nfrag_max, unsigned int *nfrag,
> +unsigned int *n4frag_accepted, unsigned int *n4frag_completed_sent,
> +unsigned int *n4frag_expired_sent, unsigned int *n4frag_too_small,
> +unsigned int *n4frag_overlap, bool *ipf_v6_enabled,
> +unsigned int *min_v6_frag_size, unsigned int *n6frag_accepted,
> +unsigned int *n6frag_completed_sent, unsigned int *n6frag_expired_sent,
> +unsigned int *n6frag_too_small, unsigned int *n6frag_overlap)
> +{
> +struct ipf_status ipf_status;
> +ipf_get_status(_status);
> +*ipf_v4_enabled = ipf_status.ifp_v4_enabled;
> +*min_v4_frag_size = ipf_status.min_v4_frag_size;
> +*nfrag_max = ipf_status.nfrag_max;
> +*nfrag = ipf_status.nfrag;
> +*n4frag_accepted = ipf_status.n4frag_accepted;
> +*n4frag_completed_sent = ipf_status.n4frag_completed_sent;
> +*n4frag_expired_sent = ipf_status.n4frag_expired_sent;
> +*n4frag_too_small = ipf_status.n4frag_too_small;
> +*n4frag_overlap = ipf_status.n4frag_overlap;
> +*ipf_v6_enabled = ipf_status.ifp_v6_enabled;
> +*min_v6_frag_size = ipf_status.min_v6_frag_size;
> +*n6frag_accepted = ipf_status.n6frag_accepted;
> +*n6frag_completed_sent = ipf_status.n6frag_completed_sent;
> +*n6frag_expired_sent = ipf_status.n6frag_expired_sent;
> +*n6frag_too_small = ipf_status.n6frag_too_small;
> +*n6frag_overlap = ipf_status.n6frag_overlap;
> +return 0;
> +}
As, I'll suggest later, this can be reduced to just a couple of lines if
'ipf_status' is passed into this function.
> diff --git a/lib/dpif-provider.h b/lib/dpif-provider.h
> index 214f570..2e0d596 100644
> --- a/lib/dpif-provider.h
> +++ b/lib/dpif-provider.h
> @@ -451,6 +451,22 @@ struct dpif_class {
> int (*ipf_set_min_frag)(struct dpif *, bool v6, uint32_t min_frag);
> /* Set maximum number of fragments tracked. */
> int (*ipf_set_max_nfrags)(struct dpif *, uint32_t max_nfrags);
> +/* Get fragmentation configuration status and counters. */
> +int (*ipf_get_status)(struct dpif *, bool
