On 2023/09/20 21:31, Ilya Maximets wrote: > On 9/15/23 05:02, Nobuhiro MIKI wrote: >> In subsequent patches, flows need to be grouped in >> a list and treated as a conjunctive flow. >> >> Signed-off-by: Nobuhiro MIKI <[email protected]> > > Hi. Thanks for the patch! > See some comments below. > >> --- >> include/openvswitch/flow.h | 9 ++++++--- >> lib/dpif-netdev-extract-avx512.c | 2 +- >> lib/flow.c | 20 ++++++++++---------- >> lib/flow.h | 2 +- >> lib/match.c | 2 +- >> lib/nx-match.c | 2 +- >> lib/odp-util.c | 6 +++--- >> lib/odp-util.h | 2 +- >> lib/ofp-match.c | 2 +- >> ofproto/ofproto-dpif-rid.h | 2 +- >> ofproto/ofproto-dpif-xlate.c | 2 +- >> 11 files changed, 27 insertions(+), 24 deletions(-) >> >> diff --git a/include/openvswitch/flow.h b/include/openvswitch/flow.h >> index df10cf579e2f..dc0cf500bb9b 100644 >> --- a/include/openvswitch/flow.h >> +++ b/include/openvswitch/flow.h >> @@ -19,6 +19,7 @@ >> #include "openflow/nicira-ext.h" >> #include "openvswitch/packets.h" >> #include "openvswitch/util.h" >> +#include "openvswitch/list.h" >> >> #ifdef __cplusplus >> extern "C" { >> @@ -27,7 +28,7 @@ extern "C" { >> /* This sequence number should be incremented whenever anything involving >> flows >> * or the wildcarding of flows changes. This will cause build assertion >> * failures in places which likely need to be updated. */ >> -#define FLOW_WC_SEQ 42 >> +#define FLOW_WC_SEQ 43 >> >> /* Number of Open vSwitch extension 32-bit registers. */ >> #define FLOW_N_REGS 16 >> @@ -116,6 +117,7 @@ struct flow { >> ovs_u128 ct_label; /* Connection label. */ >> uint32_t conj_id; /* Conjunction ID. */ >> ofp_port_t actset_output; /* Output port in action set. */ >> + struct ovs_list list_node; /* In struct xlate_out's "conj_flows" list. >> */ > > I'd recommend to generally avoid changes in struct flow. > It's one of the central structures in OVS and many parts > of OVS depend on a certain layout of this structure (we > have lots of build assertions for that reason). And even > if the change is correct it has a potential to affect > performance due to shifts in cache lines and other less > visible side effects. > > IIUC, the only use for the list entry here is to be able > to store multiple flows during tracing. Can we just > store them in array in this case? I see you're copying > large chunks of data and allocating new structures in > the next patch anyway, so re-sizing an array if necessary > should not be a problem. Alternatively, we also have > a hmapx datastructure that can hold unique pointers > without need for a structure modification. > > Best regards, Ilya Maximets.
Thanks for the review. I don't want to make a change that could potentially degrade performance as well. As you said, this change just needs to be able to treat the items as a group, so hmapx or just an array is enough. I would rewrite it that way. Best Regards, Nobuhiro Miki _______________________________________________ dev mailing list [email protected] https://mail.openvswitch.org/mailman/listinfo/ovs-dev
