Hi Numan, Dumitru Thanks for your feedback and sorry for the confusion. Comment embedded below.
Thanks Xavier On Mon, Dec 23, 2024 at 12:58 PM Dumitru Ceara <[email protected]> wrote: > On 12/20/24 10:31 PM, Numan Siddique wrote: > > On Thu, Nov 14, 2024 at 8:58 AM Xavier Simonart <[email protected]> > wrote: > >> Signed-off-by: Xavier Simonart <[email protected]> > >> --- > >> controller/physical.c | 15 ++++++++++++--- > >> 1 file changed, 12 insertions(+), 3 deletions(-) > >> > >> diff --git a/controller/physical.c b/controller/physical.c > >> index 8d5065098..6b93696fa 100644 > >> --- a/controller/physical.c > >> +++ b/controller/physical.c > >> @@ -2194,15 +2194,15 @@ consider_mc_group(struct ovsdb_idl_index > *sbrec_port_binding_by_name, > >> } > >> > >> int zone_id = ct_zone_find_zone(ct_zones, port->logical_port); > >> - if (zone_id) { > >> - put_load(zone_id, MFF_LOG_CT_ZONE, 0, 16, &ofpacts); > >> - } > > Hi Xavier, > > > > I'm a little confused with this patch. Can you please explain what is > > the improvement here ? > The goal of the patch is minor: it avoids some differences between flows created by I+P and recompute. Fixing this will let us (in a later patch) automatically compare I+P and recompute flows and catch issues. Without the patch I+P might produce flows as: ... table=43 ... priority=100,reg15=0x8000,metadata=0x1 actions=load:0->NXM_NX_REG6[], load:0x3->NXM_NX_REG13[0..15],load:0x1->NXM_NX_REG15[],resubmit(,44),load:0x8000->NXM_NX_REG15[] and recompute: ... table=43 ... priority=100,reg15=0x8000,metadata=0x1 actions=load:0->NXM_NX_REG6[]*,load:0x4->NXM_NX_REG13[0..15]* ,load:0x3->NXM_NX_REG13[0..15],load:0x1->NXM_NX_REG15[],resubmit(,44),load:0x8000->NXM_NX_REG15[] So, in one case we set register REG13 twice: first it is set to 4, and then overwritten to 3; in the other case we set it directly to 3. Goal of the patch is to avoid the extra load:0x4->NXM_NX_REG13[0..15]. > > > Also can you please update the commit message with more details ? > I'll do it if we agree that the patch makes sense. > > > > From what I understand prior to this patch, for all the port bindings > > in a multicast group, > > zone id of the pb (if present) is loaded to the ct_zone register. > > > > How is this patch avoiding setting the same register again ? > The issue happens for some port types, such as virtual or localport. Those ports usually have a ct-zone assigned, so we were issuing "put_load(zone_id, MFF_LOG_CT_ZONE, 0, 16, &ofpacts);" for those ports. However, we were not calling "local_output_pb" for those ports (so, no resubmit), and hence the ct_zone register was either overwritten when handling another port in the loop, or was useless (no resubmit after setting the ct_zone register). With the patch, we do not set the ct_zone register for those ports. > > > Thanks > > Numan > > Hi Numan, Xavier, > > I marked the rest of the series as "changes requested" in patchwork. > > Thanks Xavier > Regards, > Dumitru > > _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
