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.
_______________________________________________
dev mailing list
[email protected]
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
