| 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. >
On 5/18/24 17:51, Chandler Wu wrote: > Hi Ilya, > > I would prefer to keep ct_zone_alloc staying as it is now semantically, > for its existence dates back to like 5 years ago This would be a good argument if this function was part of public API, but it is just a simple static function with just one user, so there is no point in trying to preserve the behavior. Also, I think, the fact that the faction not only allocates, but also fills in the structure is a primary cause of the issue you're fixing. The function is called 'alloc', but does something unexpected (populating the hash map) as well. > To void unnecessary checks, I add a jump. I'd still prefer just removing the line form the function instead. There is no need to add more complexity where it can be reduced. > > Btw, this is my first commit to the opensource community, and for a > 'v2 commit' I don't know if it's ok to reply to this thread directly. > > So I just sent it to `dev@openvswitch.org` in another mail (cc to you). That's the right way to post new versions of the patch. All good. :) For some reason the Fixes tag became a Subject of the patch, that will need to be adjusted though. I'll reply to v2 separately. And please keep the mailing list in CC while replying to emails (i.e. use Reply-All), so the thread is visible in the mailing list archives. 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);