Re: [ovs-dev] [PATCH v3 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables
On Wed, Aug 14, 2019 at 1:28 PM Yi-Hung Wei wrote:
> On Tue, Aug 13, 2019 at 7:46 PM Darrell Ball wrote:
> >
> > Thanks for the patch
> >
> > Some high level comments:
> >
> > 1/ The ct_tp_kill_list code is still in common code
> > I think we discussed moving that to the dpif backer code
> > ct_timeout_policy_unref() is adding to this deferred kill list which
> is not needed for userspace
> > datapath.
> > 2/ clear_existing_ct_timeout_policies() is in common code, but only does
> something if
> > ct_dpif_timeout_policy_dump_start/next/done are realized in the datatype
> type specific code
> > (which is only for the kernel code, which is correct). I think it would
> be cleaner and less confusing
> > just to make the API clear_existing_ct_timeout_policies() kernel
> specific; i.e. in dpif-netlink.
>
>
> Thanks for review. I address most of the code changes as in the
> detailed inline code review.
>
> For the two high level concerns, it is mainly because currently
> ct_tp_kill_list is maintained in ofproto-dpif.c I thought about to
> move the ct_tp_kill_list implementation from dpif_backer (in
> ofproto-dpif.c) to dpif-netlink.c, and here is why I still keep it in
> ofproto-dpif.c in the dpif_backer layer in this version.
>
> AFAIK, currently, we do not have a proper place to store
> ct_tp_kill_list in dpif-netlink.c in the userspace. dpif-netlink is
> for the kernel datapath implementation, all the information that we
> configured to dpif-netlink are directly pass down into the kernel
> currently. In userspace datapath, we can store userspace specific
> information in "struct dp_netdev", but there is no such place in
> dpif-neltink for now. In this case, it is naturally to maintain the
> ct_tp_kill_list one level up in the dpif_backer layer.
>
> Anyhow, we can always make proper change on the way we maintain
> timeout policy in ofproto-dpif layer when the dpif-netdev
> implementation is introduced.
>
As discussed, lets defer.
>
>
> > On Mon, Aug 12, 2019 at 5:55 PM Yi-Hung Wei
> wrote:
> >> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> >> index 751535249e21..3013d83e96a0 100644
> >> --- a/ofproto/ofproto-dpif.c
> >> +++ b/ofproto/ofproto-dpif.c
> >> @@ -694,6 +718,8 @@ struct odp_garbage {
> >>
> >> static void check_support(struct dpif_backer *backer);
> >>
> >> +#define MAX_TIMEOUT_POLICY_ID UINT32_MAX
> >
> >
> > seems like random placement; could be moved where it is used.
> >
>
> ok, I will move it right before ct_zone_config_init().
>
>
> >> +static struct ct_timeout_policy *
> >> +ct_timeout_policy_alloc__(void)
> >> +{
> >> +struct ct_timeout_policy *ct_tp = xzalloc(sizeof *ct_tp);
> >> +simap_init(_tp->tp);
> >> +return ct_tp;
> >> +}
> >
> >
> > by using above API, you are not saving any code and maybe more error
> prone
> >
>
> This function is used in ct_timeout_policy_alloc() and
> clear_existing_ct_timeout_policies(). So are you sugguesting to expand
> it in these two functions?
>
ohh. I missed the other usage; I don't feel strongly either way.
>
> >>
> >> +
> >> +static struct ct_timeout_policy *
> >> +ct_timeout_policy_alloc(struct simap *tp, struct id_pool *tp_ids)
> >> +{
> >> +struct simap_node *node;
> >> +
> >> +struct ct_timeout_policy *ct_tp = ct_timeout_policy_alloc__();
> >> +SIMAP_FOR_EACH (node, tp) {
> >> +simap_put(_tp->tp, node->name, node->data);
> >> +}
> >> +
> >> +if (!id_pool_alloc_id(tp_ids, _tp->tp_id)) {
> >> +VLOG_ERR_RL(, "failed to allocate timeout policy id.");
> >> +simap_destroy(_tp->tp);
> >> +free(tp);
> >
> >
> > I think you rather need to free 'ct_tp'; i.e. free(ct_tp)
>
> Yes, thanks for spotting this bug.
>
>
> >> +static void
> >> +clear_existing_ct_timeout_policies(struct dpif_backer *backer)
> >> +{
> >> +/* In kernel datapath, when OVS starts, there may be some
> pre-existing
> >> + * timeout policies in the kernel. To avoid reassign the same
> timeout
> >> + * policy ids, we dump all the pre-existing timeout policies and
> keep
> >> + * the ids in the pool. Since OVS will not use those timeout
> policies
> >> + * for new datapath flow, we add them to the kill list and remove
> >> + * them later on. */
> >> +void *state;
> >> +
> >> +int err = ct_dpif_timeout_policy_dump_start(backer->dpif, );
> >> +if (err) {
> >> +return;
> >> +}
> >
> >
> > can be
> >
> > + if (ct_dpif_timeout_policy_dump_start(backer->dpif, )) {
> > +return;
> > +}
> >
> > similarly below
>
> Sure, I will get rid of 'int err' in v4.
>
> >> +
> >> +struct ct_dpif_timeout_policy cdtp;
> >> +while (!(err = ct_dpif_timeout_policy_dump_next(backer->dpif,
> state,
> >> +))) {
> >> +struct ct_timeout_policy *ct_tp = ct_timeout_policy_alloc__();
> >> +ct_tp->tp_id = cdtp.id;
> >> +id_pool_add(backer->tp_ids, cdtp.id);
> >> +
Re: [ovs-dev] [PATCH v3 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables
On Tue, Aug 13, 2019 at 7:46 PM Darrell Ball wrote:
>
> Thanks for the patch
>
> Some high level comments:
>
> 1/ The ct_tp_kill_list code is still in common code
> I think we discussed moving that to the dpif backer code
> ct_timeout_policy_unref() is adding to this deferred kill list which is
> not needed for userspace
> datapath.
> 2/ clear_existing_ct_timeout_policies() is in common code, but only does
> something if
> ct_dpif_timeout_policy_dump_start/next/done are realized in the datatype type
> specific code
> (which is only for the kernel code, which is correct). I think it would be
> cleaner and less confusing
> just to make the API clear_existing_ct_timeout_policies() kernel specific;
> i.e. in dpif-netlink.
Thanks for review. I address most of the code changes as in the
detailed inline code review.
For the two high level concerns, it is mainly because currently
ct_tp_kill_list is maintained in ofproto-dpif.c I thought about to
move the ct_tp_kill_list implementation from dpif_backer (in
ofproto-dpif.c) to dpif-netlink.c, and here is why I still keep it in
ofproto-dpif.c in the dpif_backer layer in this version.
AFAIK, currently, we do not have a proper place to store
ct_tp_kill_list in dpif-netlink.c in the userspace. dpif-netlink is
for the kernel datapath implementation, all the information that we
configured to dpif-netlink are directly pass down into the kernel
currently. In userspace datapath, we can store userspace specific
information in "struct dp_netdev", but there is no such place in
dpif-neltink for now. In this case, it is naturally to maintain the
ct_tp_kill_list one level up in the dpif_backer layer.
Anyhow, we can always make proper change on the way we maintain
timeout policy in ofproto-dpif layer when the dpif-netdev
implementation is introduced.
> On Mon, Aug 12, 2019 at 5:55 PM Yi-Hung Wei wrote:
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 751535249e21..3013d83e96a0 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -694,6 +718,8 @@ struct odp_garbage {
>>
>> static void check_support(struct dpif_backer *backer);
>>
>> +#define MAX_TIMEOUT_POLICY_ID UINT32_MAX
>
>
> seems like random placement; could be moved where it is used.
>
ok, I will move it right before ct_zone_config_init().
>> +static struct ct_timeout_policy *
>> +ct_timeout_policy_alloc__(void)
>> +{
>> +struct ct_timeout_policy *ct_tp = xzalloc(sizeof *ct_tp);
>> +simap_init(_tp->tp);
>> +return ct_tp;
>> +}
>
>
> by using above API, you are not saving any code and maybe more error prone
>
This function is used in ct_timeout_policy_alloc() and
clear_existing_ct_timeout_policies(). So are you sugguesting to expand
it in these two functions?
>>
>> +
>> +static struct ct_timeout_policy *
>> +ct_timeout_policy_alloc(struct simap *tp, struct id_pool *tp_ids)
>> +{
>> +struct simap_node *node;
>> +
>> +struct ct_timeout_policy *ct_tp = ct_timeout_policy_alloc__();
>> +SIMAP_FOR_EACH (node, tp) {
>> +simap_put(_tp->tp, node->name, node->data);
>> +}
>> +
>> +if (!id_pool_alloc_id(tp_ids, _tp->tp_id)) {
>> +VLOG_ERR_RL(, "failed to allocate timeout policy id.");
>> +simap_destroy(_tp->tp);
>> +free(tp);
>
>
> I think you rather need to free 'ct_tp'; i.e. free(ct_tp)
Yes, thanks for spotting this bug.
>> +static void
>> +clear_existing_ct_timeout_policies(struct dpif_backer *backer)
>> +{
>> +/* In kernel datapath, when OVS starts, there may be some pre-existing
>> + * timeout policies in the kernel. To avoid reassign the same timeout
>> + * policy ids, we dump all the pre-existing timeout policies and keep
>> + * the ids in the pool. Since OVS will not use those timeout policies
>> + * for new datapath flow, we add them to the kill list and remove
>> + * them later on. */
>> +void *state;
>> +
>> +int err = ct_dpif_timeout_policy_dump_start(backer->dpif, );
>> +if (err) {
>> +return;
>> +}
>
>
> can be
>
> + if (ct_dpif_timeout_policy_dump_start(backer->dpif, )) {
> +return;
> +}
>
> similarly below
Sure, I will get rid of 'int err' in v4.
>> +
>> +struct ct_dpif_timeout_policy cdtp;
>> +while (!(err = ct_dpif_timeout_policy_dump_next(backer->dpif, state,
>> +))) {
>> +struct ct_timeout_policy *ct_tp = ct_timeout_policy_alloc__();
>> +ct_tp->tp_id = cdtp.id;
>> +id_pool_add(backer->tp_ids, cdtp.id);
>> +ovs_list_insert(>ct_tp_kill_list, _tp->list_node);
>
>
> not sure why you add to beginning here rather than end; was there some deeper
> meaning at play ?
Not really, I am fine to add it on either side tho.
>> +static void
>> +ct_del_zone_timeout_policy(const char *datapath_type, uint16_t zone)
>> +{
>> +struct dpif_backer *backer;
>> +struct ct_zone *ct_zone;
>> +
>> +backer =
Re: [ovs-dev] [PATCH v3 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables
Thanks for the patch
Some high level comments:
1/ The ct_tp_kill_list code is still in common code
I think we discussed moving that to the dpif backer code
ct_timeout_policy_unref() is adding to this deferred kill list which is
not needed for userspace
datapath.
2/ clear_existing_ct_timeout_policies() is in common code, but only does
something if
ct_dpif_timeout_policy_dump_start/next/done are realized in the datatype
type specific code
(which is only for the kernel code, which is correct). I think it would be
cleaner and less confusing
just to make the API clear_existing_ct_timeout_policies() kernel specific;
i.e. in dpif-netlink.
Some other comments inline
On Mon, Aug 12, 2019 at 5:55 PM Yi-Hung Wei wrote:
> This patch consumes the CT_Zone and CT_Timeout_Policy tables, maintains
> the zone-based configuration in the vswitchd. Whenever there is a
> database change, vswitchd will read the datapath, CT_Zone, and
> CT_Timeout_Policy tables from ovsdb, builds an internal snapshot of the
> database configuration in bridge.c, and pushes down the change into
> ofproto and dpif layer.
>
> If a new zone-based timeout policy is added, it updates the zone to
> timeout policy mapping in the per datapath type datapath structure in
> dpif-backer, and pushes down the timeout policy into the datapath via
> dpif interface.
>
> If a timeout policy is no longer used, for kernel datapath, vswitchd
> may not be able to remove it from datapath immediately since
> datapath flows can still reference the to-be-deleted timeout policies.
> Thus, we keep an timeout policy kill list, that vswitchd will go
> back to the list periodically and try to kill the unused timeout policies.
>
> Signed-off-by: Yi-Hung Wei
> ---
> ofproto/ofproto-dpif.c | 293
> +
> ofproto/ofproto-dpif.h | 10 ++
> ofproto/ofproto-provider.h | 10 ++
> ofproto/ofproto.c | 30 +
> ofproto/ofproto.h | 5 +
> vswitchd/bridge.c | 202 +++
> 6 files changed, 550 insertions(+)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 751535249e21..3013d83e96a0 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -156,6 +156,25 @@ struct ofport_dpif {
> size_t n_qdscp;
> };
>
> +struct ct_timeout_policy {
> +int ref_count; /* The number of ct zones that use this
> + * timeout policy. */
> +uint32_t tp_id; /* Timeout policy id in the datapath. */
> +struct simap tp;/* A map from timeout policy attribute to
> + * timeout value. */
> +struct hmap_node node; /* Element in struct dpif_backer's
> "ct_tps"
> + * cmap. */
> +struct ovs_list list_node; /* Element in struct dpif_backer's
> + * "ct_tp_kill_list" list. */
> +};
> +
> +struct ct_zone {
> +uint16_t zone_id;
> +struct ct_timeout_policy *ct_tp;
> +struct cmap_node node; /* Element in struct dpif_backer's
> + * "ct_zones" cmap. */
> +};
> +
> static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
> ofp_port_t);
>
> @@ -196,6 +215,9 @@ static struct hmap all_ofproto_dpifs_by_uuid =
>
> static bool ofproto_use_tnl_push_pop = true;
> static void ofproto_unixctl_init(void);
> +static void ct_zone_config_init(struct dpif_backer *backer);
> +static void ct_zone_config_uninit(struct dpif_backer *backer);
> +static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
>
> static inline struct ofproto_dpif *
> ofproto_dpif_cast(const struct ofproto *ofproto)
> @@ -488,6 +510,7 @@ type_run(const char *type)
> }
>
> process_dpif_port_changes(backer);
> +ct_zone_timeout_policy_sweep(backer);
>
> return 0;
> }
> @@ -683,6 +706,7 @@ close_dpif_backer(struct dpif_backer *backer, bool del)
> }
> dpif_close(backer->dpif);
> id_pool_destroy(backer->meter_ids);
> +ct_zone_config_uninit(backer);
> free(backer);
> }
>
> @@ -694,6 +718,8 @@ struct odp_garbage {
>
> static void check_support(struct dpif_backer *backer);
>
> +#define MAX_TIMEOUT_POLICY_ID UINT32_MAX
>
seems like random placement; could be moved where it is used.
> +
> static int
> open_dpif_backer(const char *type, struct dpif_backer **backerp)
> {
> @@ -811,6 +837,8 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
> backer->meter_ids = NULL;
> }
>
> +ct_zone_config_init(backer);
> +
> /* Make a pristine snapshot of 'support' into 'boottime_support'.
> * 'boottime_support' can be checked to prevent 'support' to be
> changed
> * beyond the datapath capabilities. In case 'support' is changed by
> @@ -5086,6 +5114,269 @@ ct_flush(const struct ofproto *ofproto_, const
> uint16_t *zone)
[ovs-dev] [PATCH v3 6/9] ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy tables
This patch consumes the CT_Zone and CT_Timeout_Policy tables, maintains
the zone-based configuration in the vswitchd. Whenever there is a
database change, vswitchd will read the datapath, CT_Zone, and
CT_Timeout_Policy tables from ovsdb, builds an internal snapshot of the
database configuration in bridge.c, and pushes down the change into
ofproto and dpif layer.
If a new zone-based timeout policy is added, it updates the zone to
timeout policy mapping in the per datapath type datapath structure in
dpif-backer, and pushes down the timeout policy into the datapath via
dpif interface.
If a timeout policy is no longer used, for kernel datapath, vswitchd
may not be able to remove it from datapath immediately since
datapath flows can still reference the to-be-deleted timeout policies.
Thus, we keep an timeout policy kill list, that vswitchd will go
back to the list periodically and try to kill the unused timeout policies.
Signed-off-by: Yi-Hung Wei
---
ofproto/ofproto-dpif.c | 293 +
ofproto/ofproto-dpif.h | 10 ++
ofproto/ofproto-provider.h | 10 ++
ofproto/ofproto.c | 30 +
ofproto/ofproto.h | 5 +
vswitchd/bridge.c | 202 +++
6 files changed, 550 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 751535249e21..3013d83e96a0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -156,6 +156,25 @@ struct ofport_dpif {
size_t n_qdscp;
};
+struct ct_timeout_policy {
+int ref_count; /* The number of ct zones that use this
+ * timeout policy. */
+uint32_t tp_id; /* Timeout policy id in the datapath. */
+struct simap tp;/* A map from timeout policy attribute to
+ * timeout value. */
+struct hmap_node node; /* Element in struct dpif_backer's "ct_tps"
+ * cmap. */
+struct ovs_list list_node; /* Element in struct dpif_backer's
+ * "ct_tp_kill_list" list. */
+};
+
+struct ct_zone {
+uint16_t zone_id;
+struct ct_timeout_policy *ct_tp;
+struct cmap_node node; /* Element in struct dpif_backer's
+ * "ct_zones" cmap. */
+};
+
static odp_port_t ofp_port_to_odp_port(const struct ofproto_dpif *,
ofp_port_t);
@@ -196,6 +215,9 @@ static struct hmap all_ofproto_dpifs_by_uuid =
static bool ofproto_use_tnl_push_pop = true;
static void ofproto_unixctl_init(void);
+static void ct_zone_config_init(struct dpif_backer *backer);
+static void ct_zone_config_uninit(struct dpif_backer *backer);
+static void ct_zone_timeout_policy_sweep(struct dpif_backer *backer);
static inline struct ofproto_dpif *
ofproto_dpif_cast(const struct ofproto *ofproto)
@@ -488,6 +510,7 @@ type_run(const char *type)
}
process_dpif_port_changes(backer);
+ct_zone_timeout_policy_sweep(backer);
return 0;
}
@@ -683,6 +706,7 @@ close_dpif_backer(struct dpif_backer *backer, bool del)
}
dpif_close(backer->dpif);
id_pool_destroy(backer->meter_ids);
+ct_zone_config_uninit(backer);
free(backer);
}
@@ -694,6 +718,8 @@ struct odp_garbage {
static void check_support(struct dpif_backer *backer);
+#define MAX_TIMEOUT_POLICY_ID UINT32_MAX
+
static int
open_dpif_backer(const char *type, struct dpif_backer **backerp)
{
@@ -811,6 +837,8 @@ open_dpif_backer(const char *type, struct dpif_backer
**backerp)
backer->meter_ids = NULL;
}
+ct_zone_config_init(backer);
+
/* Make a pristine snapshot of 'support' into 'boottime_support'.
* 'boottime_support' can be checked to prevent 'support' to be changed
* beyond the datapath capabilities. In case 'support' is changed by
@@ -5086,6 +5114,269 @@ ct_flush(const struct ofproto *ofproto_, const uint16_t
*zone)
ct_dpif_flush(ofproto->backer->dpif, zone, NULL);
}
+static struct ct_timeout_policy *
+ct_timeout_policy_lookup(const struct hmap *ct_tps, struct simap *tp)
+{
+struct ct_timeout_policy *ct_tp;
+
+HMAP_FOR_EACH_WITH_HASH (ct_tp, node, simap_hash(tp), ct_tps) {
+if (simap_equal(_tp->tp, tp)) {
+return ct_tp;
+}
+}
+return NULL;
+}
+
+static struct ct_timeout_policy *
+ct_timeout_policy_alloc__(void)
+{
+struct ct_timeout_policy *ct_tp = xzalloc(sizeof *ct_tp);
+simap_init(_tp->tp);
+return ct_tp;
+}
+
+static struct ct_timeout_policy *
+ct_timeout_policy_alloc(struct simap *tp, struct id_pool *tp_ids)
+{
+struct simap_node *node;
+
+struct ct_timeout_policy *ct_tp = ct_timeout_policy_alloc__();
+SIMAP_FOR_EACH (node, tp) {
+simap_put(_tp->tp, node->name, node->data);
+}
+
+if (!id_pool_alloc_id(tp_ids, _tp->tp_id)) {
+VLOG_ERR_RL(, "failed to allocate timeout policy id.");
+
