On 10/8/23 06:21, Mike Pattrick wrote:
> Previously the mirror_set() and mirror_get() functions took a large
> number of parameters, which was inefficient and difficult to read and
> extend. This patch moves most of the parameters into a struct.
>
> Signed-off-by: Mike Pattrick <m...@redhat.com>
> Acked-by: Simon Horman <ho...@ovn.org>
> Acked-by: Eelco Chaudron <echau...@redhat.com>
>
Hi, Mike. Thanks for the patch!
See some comments inline.
Best regards, Ilya Maximets.
> ---
>
> v6:
> - Changed a comment to be more grammatically correct
> ---
> ofproto/ofproto-dpif-mirror.c | 61 ++++++++++++++++++-----------------
> ofproto/ofproto-dpif-mirror.h | 31 +++++++++++++-----
> ofproto/ofproto-dpif-xlate.c | 25 ++++++--------
> ofproto/ofproto-dpif.c | 6 ++--
> 4 files changed, 67 insertions(+), 56 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-mirror.c b/ofproto/ofproto-dpif-mirror.c
> index 343b75f0e..17ba6f799 100644
> --- a/ofproto/ofproto-dpif-mirror.c
> +++ b/ofproto/ofproto-dpif-mirror.c
> @@ -207,19 +207,23 @@ mirror_bundle_dst(struct mbridge *mbridge, struct
> ofbundle *ofbundle)
> }
>
> int
> -mirror_set(struct mbridge *mbridge, void *aux, const char *name,
> - struct ofbundle **srcs, size_t n_srcs,
> - struct ofbundle **dsts, size_t n_dsts,
> - unsigned long *src_vlans, struct ofbundle *out_bundle,
> - uint16_t snaplen,
> - uint16_t out_vlan)
> +mirror_set(struct mbridge *mbridge, void *aux,
> + const struct ofproto_mirror_settings *ms, struct ofbundle **srcs,
> + struct ofbundle **dsts, struct ofbundle *bundle)
> +
> {
> struct mbundle *mbundle, *out;
> mirror_mask_t mirror_bit;
> struct mirror *mirror;
> struct hmapx srcs_map; /* Contains "struct ofbundle *"s. */
> struct hmapx dsts_map; /* Contains "struct ofbundle *"s. */
> + uint16_t out_vlan;
> +
> + if (!ms || !mbridge) {
> + return EINVAL;
> + }
>
> + out_vlan = ms->out_vlan;
> mirror = mirror_lookup(mbridge, aux);
> if (!mirror) {
> int idx;
> @@ -227,7 +231,7 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
> *name,
> idx = mirror_scan(mbridge);
> if (idx < 0) {
> VLOG_WARN("maximum of %d port mirrors reached, cannot create %s",
> - MAX_MIRRORS, name);
> + MAX_MIRRORS, ms->name);
> return EFBIG;
> }
>
> @@ -242,8 +246,8 @@ mirror_set(struct mbridge *mbridge, void *aux, const char
> *name,
> unsigned long *vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
>
> /* Get the new configuration. */
> - if (out_bundle) {
> - out = mbundle_lookup(mbridge, out_bundle);
> + if (bundle) {
> + out = mbundle_lookup(mbridge, bundle);
> if (!out) {
> mirror_destroy(mbridge, mirror->aux);
> return EINVAL;
> @@ -252,16 +256,16 @@ mirror_set(struct mbridge *mbridge, void *aux, const
> char *name,
> } else {
> out = NULL;
> }
> - mbundle_lookup_multiple(mbridge, srcs, n_srcs, &srcs_map);
> - mbundle_lookup_multiple(mbridge, dsts, n_dsts, &dsts_map);
> + mbundle_lookup_multiple(mbridge, srcs, ms->n_srcs, &srcs_map);
> + mbundle_lookup_multiple(mbridge, dsts, ms->n_dsts, &dsts_map);
This doesn't look right. ms->n_srcs is the number of entries
in ms->srcs. mirror_set__() function knows that the sizes of these
arrays are the same, but we do not know that here in this function.
Looks very prone to errors.
>
> /* If the configuration has not changed, do nothing. */
> if (hmapx_equals(&srcs_map, &mirror->srcs)
> && hmapx_equals(&dsts_map, &mirror->dsts)
> - && vlan_bitmap_equal(vlans, src_vlans)
> + && vlan_bitmap_equal(vlans, ms->src_vlans)
> && mirror->out == out
> && mirror->out_vlan == out_vlan
> - && mirror->snaplen == snaplen)
> + && mirror->snaplen == ms->snaplen)
> {
> hmapx_destroy(&srcs_map);
> hmapx_destroy(&dsts_map);
> @@ -275,15 +279,15 @@ mirror_set(struct mbridge *mbridge, void *aux, const
> char *name,
> hmapx_swap(&dsts_map, &mirror->dsts);
> hmapx_destroy(&dsts_map);
>
> - if (vlans || src_vlans) {
> + if (vlans || ms->src_vlans) {
> ovsrcu_postpone(free, vlans);
> - vlans = vlan_bitmap_clone(src_vlans);
> + vlans = vlan_bitmap_clone(ms->src_vlans);
> ovsrcu_set(&mirror->vlans, vlans);
> }
>
> mirror->out = out;
> mirror->out_vlan = out_vlan;
> - mirror->snaplen = snaplen;
> + mirror->snaplen = ms->snaplen;
>
> /* Update mbundles. */
> mirror_bit = MIRROR_MASK_C(1) << mirror->idx;
> @@ -406,23 +410,22 @@ mirror_update_stats(struct mbridge *mbridge,
> mirror_mask_t mirrors,
> /* Retrieves the mirror numbered 'index' in 'mbridge'. Returns true if such
> a
> * mirror exists, false otherwise.
> *
> - * If successful, '*vlans' receives the mirror's VLAN membership information,
> + * If successful 'mc->vlans' receives the mirror's VLAN membership
> information,
> * either a null pointer if the mirror includes all VLANs or a 4096-bit
> bitmap
> * in which a 1-bit indicates that the mirror includes a particular VLAN,
> - * '*dup_mirrors' receives a bitmap of mirrors whose output duplicates mirror
> - * 'index', '*out' receives the output ofbundle (if any), and '*out_vlan'
> - * receives the output VLAN (if any).
> + * 'mc->dup_mirrors' receives a bitmap of mirrors whose output duplicates
> + * mirror 'index', 'mc->out' receives the output ofbundle (if any),
> + * and 'mc->out_vlan' receives the output VLAN (if any).
> *
> * Everything returned here is assumed to be RCU protected.
> */
> bool
> -mirror_get(struct mbridge *mbridge, int index, const unsigned long **vlans,
> - mirror_mask_t *dup_mirrors, struct ofbundle **out,
> - int *snaplen, int *out_vlan)
> +mirror_get(struct mbridge *mbridge, int index,
> + struct mirror_config *mc)
> {
> struct mirror *mirror;
>
> - if (!mbridge) {
> + if (!mc || !mbridge) {
> return false;
> }
>
> @@ -433,11 +436,11 @@ mirror_get(struct mbridge *mbridge, int index, const
> unsigned long **vlans,
> /* Assume 'mirror' is RCU protected, i.e., it will not be freed until
> this
> * thread quiesces. */
>
> - *vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
> - *dup_mirrors = mirror->dup_mirrors;
> - *out = mirror->out ? mirror->out->ofbundle : NULL;
> - *out_vlan = mirror->out_vlan;
> - *snaplen = mirror->snaplen;
> + mc->vlans = ovsrcu_get(unsigned long *, &mirror->vlans);
> + mc->dup_mirrors = mirror->dup_mirrors;
> + mc->bundle = mirror->out ? mirror->out->ofbundle : NULL;
> + mc->out_vlan = mirror->out_vlan;
> + mc->snaplen = mirror->snaplen;
> return true;
> }
>
> diff --git a/ofproto/ofproto-dpif-mirror.h b/ofproto/ofproto-dpif-mirror.h
> index eed63ec4a..9d5a6d680 100644
> --- a/ofproto/ofproto-dpif-mirror.h
> +++ b/ofproto/ofproto-dpif-mirror.h
> @@ -22,9 +22,26 @@
> #define MAX_MIRRORS 32
> typedef uint32_t mirror_mask_t;
>
> +struct ofproto_mirror_settings;
> struct ofproto_dpif;
> struct ofbundle;
>
> +struct mirror_config {
> + /* A bitmap of mirrors that duplicate the current mirror */
Period at the end of a comment.
> + mirror_mask_t dup_mirrors;
> +
> + /* VLANs of packets to select for mirroring. */
> + unsigned long *vlans; /* vlan_bitmap, NULL selects all VLANs. */
> +
> + /* Output (mutually exclusive). */
'snaplen' is not mutually exclusive with bundle or out_vlan.
> + struct ofbundle *bundle; /* A registered ofbundle handle or NULL. */
Maybe call this 'out_bundle'? Alternatively, we could create
an anonymous struct here named 'out' with two fields - 'vlan'
and 'bundle'.
> + uint16_t out_vlan; /* Output VLAN, only if out_bundle is NULL.
> */
What excalty 'only if' means here? The mirror structure is differentiating
unset value as -1, but here we have an unsigned field. Will it be UNIT16_MAX
if not specified? I'm a bit worried about type conversions.
> + uint16_t snaplen; /* Max size of a mirrored packet in bytes,
> + if set to zero then no truncation will
> + occur. */
Each line in a block comment should start with a '*'.
> +};
> +
> +
> /* The following functions are used by handler threads without any locking,
> * assuming RCU protection. */
>
> @@ -38,9 +55,8 @@ mirror_mask_t mirror_bundle_dst(struct mbridge *, struct
> ofbundle *);
>
> void mirror_update_stats(struct mbridge*, mirror_mask_t, uint64_t packets,
> uint64_t bytes);
> -bool mirror_get(struct mbridge *, int index, const unsigned long **vlans,
> - mirror_mask_t *dup_mirrors, struct ofbundle **out,
> - int *snaplen, int *out_vlan);
> +bool mirror_get(struct mbridge *mbridge, int index,
> + struct mirror_config *mc);
>
> /* The remaining functions are assumed to be called by the main thread only.
> */
>
> @@ -50,11 +66,10 @@ bool mbridge_need_revalidate(struct mbridge *);
> void mbridge_register_bundle(struct mbridge *, struct ofbundle *);
> void mbridge_unregister_bundle(struct mbridge *, struct ofbundle *);
>
> -int mirror_set(struct mbridge *, void *aux, const char *name,
> - struct ofbundle **srcs, size_t n_srcs,
> - struct ofbundle **dsts, size_t n_dsts,
> - unsigned long *src_vlans, struct ofbundle *out_bundle,
> - uint16_t snaplen, uint16_t out_vlan);
> +int mirror_set(struct mbridge *mbridge, void *aux,
> + const struct ofproto_mirror_settings *ms,
> + struct ofbundle **srcs, struct ofbundle **dsts,
> + struct ofbundle *bundle);
> void mirror_destroy(struct mbridge *, void *aux);
> int mirror_get_stats(struct mbridge *, void *aux, uint64_t *packets,
> uint64_t *bytes);
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index be4bd6657..3746edf06 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -2247,16 +2247,11 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle
> *xbundle,
> * 'used_mirrors', as long as some candidates remain. */
> mirror_mask_t used_mirrors = 0;
> while (mirrors) {
> - const unsigned long *vlans;
> - mirror_mask_t dup_mirrors;
> - struct ofbundle *out;
> - int out_vlan;
> - int snaplen;
> + struct mirror_config mc;
>
> /* Get the details of the mirror represented by the rightmost 1-bit.
> */
> if (OVS_UNLIKELY(!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
> - &vlans, &dup_mirrors,
> - &out, &snaplen, &out_vlan))) {
> + &mc))) {
Might be better to split the line after the first argument.
> /* The mirror got reconfigured before we got to read it's
> * configuration. */
> mirrors = zero_rightmost_1bit(mirrors);
> @@ -2266,10 +2261,10 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle
> *xbundle,
>
> /* If this mirror selects on the basis of VLAN, and it does not
> select
> * 'vlan', then discard this mirror and go on to the next one. */
> - if (vlans) {
> + if (mc.vlans) {
> ctx->wc->masks.vlans[0].tci |= htons(VLAN_CFI | VLAN_VID_MASK);
> }
> - if (vlans && !bitmap_is_set(vlans, xvlan.v[0].vid)) {
> + if (mc.vlans && !bitmap_is_set(mc.vlans, xvlan.v[0].vid)) {
> mirrors = zero_rightmost_1bit(mirrors);
> continue;
> }
> @@ -2281,21 +2276,21 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle
> *xbundle,
> * destination, so that we don't mirror to them again. This must be
> * done now to ensure that output_normal(), below, doesn't
> recursively
> * output to the same mirrors. */
> - ctx->mirrors |= dup_mirrors;
> - ctx->mirror_snaplen = snaplen;
> + ctx->mirrors |= mc.dup_mirrors;
> + ctx->mirror_snaplen = mc.snaplen;
>
> /* Send the packet to the mirror. */
> - if (out) {
> - struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg, out);
> + if (mc.bundle) {
> + struct xbundle *out_xbundle = xbundle_lookup(ctx->xcfg,
> mc.bundle);
> if (out_xbundle) {
> output_normal(ctx, out_xbundle, &xvlan);
> }
> - } else if (xvlan.v[0].vid != out_vlan
> + } else if (xvlan.v[0].vid != mc.out_vlan
> && !eth_addr_is_reserved(ctx->xin->flow.dl_dst)) {
> struct xbundle *xb;
> uint16_t old_vid = xvlan.v[0].vid;
>
> - xvlan.v[0].vid = out_vlan;
> + xvlan.v[0].vid = mc.out_vlan;
> LIST_FOR_EACH (xb, list_node, &xbridge->xbundles) {
> if (xbundle_includes_vlan(xb, &xvlan)
> && !xbundle_mirror_out(xbridge, xb)) {
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index ba5706f6a..5a5a8ea86 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -3663,10 +3663,8 @@ mirror_set__(struct ofproto *ofproto_, void *aux,
> dsts[i] = bundle_lookup(ofproto, s->dsts[i]);
> }
>
> - error = mirror_set(ofproto->mbridge, aux, s->name, srcs, s->n_srcs, dsts,
> - s->n_dsts, s->src_vlans,
> - bundle_lookup(ofproto, s->out_bundle),
> - s->snaplen, s->out_vlan);
> + error = mirror_set(ofproto->mbridge, aux, s, srcs, dsts,
> + bundle_lookup(ofproto, s->out_bundle));
> free(srcs);
> free(dsts);
> return error;
> --
> 2.39.3
_______________________________________________
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
