| Message ID | E4EE54FD-E480-4153-8D73-0D13EC28DF4D@gmail.com |
|---|---|
| State | Changes Requested |
| Delegated to: | Ilya Maximets |
| Headers | show |
| Series | [ovs-dev] fix ct tp policy when create zone. | expand |
On 5/6/24 06:05, Chandler Wu wrote: > From 5b4d479a9083272e56c51ef9521e6ef665dd534d Mon Sep 17 00:00:00 2001 > From: chandlerwu <chandler0149@gmail.com> > Date: Mon, 6 May 2024 11:58:21 +0800 > Subject: [PATCH] Subject:[PATCH] fix ct tp policy when create zone. > > Signed-off-by: chandlerwu <chandler0149@gmail.com> > --- > vswitchd/bridge.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > index 95a65fcdc..e60359b59 100644 > --- a/vswitchd/bridge.c > +++ b/vswitchd/bridge.c > @@ -766,6 +766,8 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg) > if (!ct_zone) { > ct_zone = ct_zone_alloc(zone_id, tp_cfg); > hmap_insert(&dp->ct_zones, &ct_zone->node, hash_int(zone_id, 0)); > + ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id, > + &ct_zone->tp); > } > > struct simap new_tp = SIMAP_INITIALIZER(&new_tp); Hi, Chandler Wu. Thanks for the patch! But could you, please, explain what is the issue you're trying to fix? Reading the code, it seems that update_timeout_policy() call later in the function should correctly set the policy. Is that not happening? Best regards, Ilya Maximets.
Hi, IIya, thanks for the attention. If I create a zone for the first time, the new tp_cfg will be copied to the zone, see `ct_zone_alloc`. Then update_timeout_policy will check that the new copied tp equals to tp_cfg, so ofproto_ct_set_zone_timeout_policy will not got called. Or you can check tag v3.0.0 or before. There's no such issue for these versions. Best regards. On Tue, May 14, 2024 at 5:11 AM Ilya Maximets <i.maximets@ovn.org> wrote: > On 5/6/24 06:05, Chandler Wu wrote: > > From 5b4d479a9083272e56c51ef9521e6ef665dd534d Mon Sep 17 00:00:00 2001 > > From: chandlerwu <chandler0149@gmail.com> > > Date: Mon, 6 May 2024 11:58:21 +0800 > > Subject: [PATCH] Subject:[PATCH] fix ct tp policy when create zone. > > > > Signed-off-by: chandlerwu <chandler0149@gmail.com> > > --- > > vswitchd/bridge.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > > index 95a65fcdc..e60359b59 100644 > > --- a/vswitchd/bridge.c > > +++ b/vswitchd/bridge.c > > @@ -766,6 +766,8 @@ ct_zones_reconfigure(struct datapath *dp, struct > ovsrec_datapath *dp_cfg) > > if (!ct_zone) { > > ct_zone = ct_zone_alloc(zone_id, tp_cfg); > > hmap_insert(&dp->ct_zones, &ct_zone->node, > hash_int(zone_id, 0)); > > + ofproto_ct_set_zone_timeout_policy(dp->type, > ct_zone->zone_id, > > + &ct_zone->tp); > > } > > > > struct simap new_tp = SIMAP_INITIALIZER(&new_tp); > > Hi, Chandler Wu. Thanks for the patch! > > But could you, please, explain what is the issue you're trying to fix? > > Reading the code, it seems that update_timeout_policy() call later in > the function should correctly set the policy. Is that not happening? > > Best regards, Ilya Maximets. >
On 5/14/24 09:50, Chandler Wu wrote: > Hi, IIya, thanks for the attention. > > If I create a zone for the first time, the new tp_cfg will be copied > to the zone, see `ct_zone_alloc`. Then update_timeout_policy will check > that the new copied tp equals to tp_cfg, so ofproto_ct_set_zone_timeout_policy > will not got called. Or you can check tag v3.0.0 or before. There's no such > issue for these versions. Ah, that makes sense. Please, add this information to the commit message before sending v2. Please, also add the following tag: Fixes: 1b3557f53dbc ("vswitchd, ofproto-dpif: Propagate the CT limit from database.") For the code itself: Maybe we should remove the get_timeout_policy_from_ovsrec() call from ct_zone_alloc() instead of adding ofproto_ct_set_zone_timeout_policy()? If we do not initialize it, the update_timeout_policy() later should sync it correctly. What do you think? Best regards, Ilya Maximets. > > Best regards. > > On Tue, May 14, 2024 at 5:11 AM Ilya Maximets <i.maximets@ovn.org <mailto:i.maximets@ovn.org>> wrote: > > On 5/6/24 06:05, Chandler Wu wrote: > > From 5b4d479a9083272e56c51ef9521e6ef665dd534d Mon Sep 17 00:00:00 2001 > > From: chandlerwu <chandler0149@gmail.com <mailto:chandler0149@gmail.com>> > > Date: Mon, 6 May 2024 11:58:21 +0800 > > Subject: [PATCH] Subject:[PATCH] fix ct tp policy when create zone. > > > > Signed-off-by: chandlerwu <chandler0149@gmail.com <mailto:chandler0149@gmail.com>> > > --- > > vswitchd/bridge.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c > > index 95a65fcdc..e60359b59 100644 > > --- a/vswitchd/bridge.c > > +++ b/vswitchd/bridge.c > > @@ -766,6 +766,8 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg) > > if (!ct_zone) { > > ct_zone = ct_zone_alloc(zone_id, tp_cfg); > > hmap_insert(&dp->ct_zones, &ct_zone->node, hash_int(zone_id, 0)); > > + ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id, > > + &ct_zone->tp); > > } > > > > struct simap new_tp = SIMAP_INITIALIZER(&new_tp); > > Hi, Chandler Wu. Thanks for the patch! > > But could you, please, explain what is the issue you're trying to fix? > > Reading the code, it seems that update_timeout_policy() call later in > the function should correctly set the policy. Is that not happening? > > Best regards, Ilya Maximets. >
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c index 95a65fcdc..e60359b59 100644 --- a/vswitchd/bridge.c +++ b/vswitchd/bridge.c @@ -766,6 +766,8 @@ ct_zones_reconfigure(struct datapath *dp, struct ovsrec_datapath *dp_cfg) if (!ct_zone) { ct_zone = ct_zone_alloc(zone_id, tp_cfg); hmap_insert(&dp->ct_zones, &ct_zone->node, hash_int(zone_id, 0)); + ofproto_ct_set_zone_timeout_policy(dp->type, ct_zone->zone_id, + &ct_zone->tp); } struct simap new_tp = SIMAP_INITIALIZER(&new_tp);