Re: [ovs-dev] [PATCH v7 2/2] ofproto-dpif-mirror: Add support for pre-selection filter.
On 2/26/24 22:16, Mike Pattrick wrote:
> Currently a bridge mirror will collect all packets and tools like
> ovs-tcpdump can apply additional filters after they have already been
> duplicated by vswitchd. This can result in inefficient collection.
>
> This patch adds support to apply pre-selection to bridge mirrors, which
> can limit which packets are mirrored based on flow metadata. This
> significantly improves overall vswitchd performance during mirroring if
> only a subset of traffic is required.
>
> Signed-off-by: Mike Pattrick
> ---
> v7:
> - Make sure filter mask is added to masks of non-matching flows.
> - Added additional tests.
Hi, Mike. Thanks for v7!
I see Elco made some comments, so I'll just throw in a few from me.
See inline.
Best regards, Ilya Maixmets.
> ---
> Documentation/ref/ovs-tcpdump.8.rst | 8 +-
> NEWS| 3 +
> lib/flow.c | 21 +++-
> lib/flow.h | 12 +++
> ofproto/ofproto-dpif-mirror.c | 78 ++-
> ofproto/ofproto-dpif-mirror.h | 12 ++-
> ofproto/ofproto-dpif-xlate.c| 26 -
> ofproto/ofproto-dpif.c | 9 +-
> ofproto/ofproto-dpif.h | 6 ++
> ofproto/ofproto.c | 4 +-
All the changes for ofproto.c are not related to this patch.
> ofproto/ofproto.h | 3 +
> tests/ofproto-dpif.at | 142
> utilities/ovs-tcpdump.in| 13 ++-
> vswitchd/bridge.c | 13 ++-
> vswitchd/vswitch.ovsschema | 5 +-
> vswitchd/vswitch.xml| 13 +++
> 16 files changed, 343 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/ref/ovs-tcpdump.8.rst
> b/Documentation/ref/ovs-tcpdump.8.rst
> index b9f8cdf6f..e21e61211 100644
> --- a/Documentation/ref/ovs-tcpdump.8.rst
> +++ b/Documentation/ref/ovs-tcpdump.8.rst
> @@ -61,8 +61,14 @@ Options
>
>If specified, mirror all ports (optional).
>
> +* ``--filter ``
> +
> + If specified, only mirror flows that match the provided OpenFlow filter.
> + The available fields are documented in ``ovs-fields(7)``.
> +
> See Also
>
>
> ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``,
> -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``.
> +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``,
> +``wireshark(8)``.
> diff --git a/NEWS b/NEWS
> index c9e4064e6..35f7eb0c7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ Post-v3.3.0
> * Conntrack now supports 'random' flag for selecting ports in a range
> while natting and 'persistent' flag for selection of the IP address
> from a range.
> + - OVSDB:
'OVSDB' section is usually for changes in ovsdb-server itself.
OVS configuration changes can be in the ovs-vsctl section, if
you're describing a new option for a mirror in ovs-vsctl. Or
just a separate entry outsie of any sections. ovs-vsctl might
be a better option here, but you may want to rephrase the entry
in terms of ovs-vsctl configuration.
You may also add an entry for ovs-tcpdump since you're changing
it as well.
> + * Added a new filter column in the Mirror table which can be used to
> + apply filters to mirror ports.
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/lib/flow.c b/lib/flow.c
> index 8e3402388..a088bdc86 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -3569,7 +3569,7 @@ miniflow_equal_in_minimask(const struct miniflow *a,
> const struct miniflow *b,
> return true;
> }
>
> -/* Returns true if 'a' and 'b' are equal at the places where there are 1-bits
> +/* Returns true if 'a' and 'b' are equal at the places where there are 0-bits
> * in 'mask', false if they differ. */
> bool
> miniflow_equal_flow_in_minimask(const struct miniflow *a, const struct flow
> *b,
> @@ -3587,6 +3587,25 @@ miniflow_equal_flow_in_minimask(const struct miniflow
> *a, const struct flow *b,
> return true;
> }
>
> +/* Returns false if 'a' and 'b' differ in places where there are 1-bits in
> + * 'wc', true otherwise. */
> +bool
> +miniflow_equal_flow_in_flow_wc(const struct miniflow *a, const struct flow
> *b,
> + const struct flow_wildcards *wc)
> +{
> +const struct flow *wc_masks = >masks;
> +size_t idx;
> +
> +FLOWMAP_FOR_EACH_INDEX (idx, a->map) {
> +if ((miniflow_get(a, idx) ^ flow_u64_value(b, idx)) &
> +flow_u64_value(wc_masks, idx)) {
> +return false;
> +}
> +}
> +
> +return true;
> +}
I don't see this function being used anywhere. Do we need it?
> +
>
> void
> minimask_init(struct minimask *mask, const struct flow_wildcards *wc)
> diff --git a/lib/flow.h b/lib/flow.h
> index 75a9be3c1..a644be39d 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -748,6 +748,9 @@ bool miniflow_equal_in_minimask(const struct miniflow *a,
> bool miniflow_equal_flow_in_minimask(const struct
Re: [ovs-dev] [PATCH v7 2/2] ofproto-dpif-mirror: Add support for pre-selection filter.
On 26 Feb 2024, at 22:16, Mike Pattrick wrote:
> Currently a bridge mirror will collect all packets and tools like
> ovs-tcpdump can apply additional filters after they have already been
> duplicated by vswitchd. This can result in inefficient collection.
>
> This patch adds support to apply pre-selection to bridge mirrors, which
> can limit which packets are mirrored based on flow metadata. This
> significantly improves overall vswitchd performance during mirroring if
> only a subset of traffic is required.
>
> Signed-off-by: Mike Pattrick
Hi Mike,
Thanks for the patch revision. Some small comments below.
Cheers,
Eelco
> ---
> v7:
> - Make sure filter mask is added to masks of non-matching flows.
> - Added additional tests.
> ---
> Documentation/ref/ovs-tcpdump.8.rst | 8 +-
> NEWS| 3 +
> lib/flow.c | 21 +++-
> lib/flow.h | 12 +++
> ofproto/ofproto-dpif-mirror.c | 78 ++-
> ofproto/ofproto-dpif-mirror.h | 12 ++-
> ofproto/ofproto-dpif-xlate.c| 26 -
> ofproto/ofproto-dpif.c | 9 +-
> ofproto/ofproto-dpif.h | 6 ++
> ofproto/ofproto.c | 4 +-
> ofproto/ofproto.h | 3 +
> tests/ofproto-dpif.at | 142
> utilities/ovs-tcpdump.in| 13 ++-
> vswitchd/bridge.c | 13 ++-
> vswitchd/vswitch.ovsschema | 5 +-
> vswitchd/vswitch.xml| 13 +++
> 16 files changed, 343 insertions(+), 25 deletions(-)
>
> diff --git a/Documentation/ref/ovs-tcpdump.8.rst
> b/Documentation/ref/ovs-tcpdump.8.rst
> index b9f8cdf6f..e21e61211 100644
> --- a/Documentation/ref/ovs-tcpdump.8.rst
> +++ b/Documentation/ref/ovs-tcpdump.8.rst
> @@ -61,8 +61,14 @@ Options
>
>If specified, mirror all ports (optional).
>
> +* ``--filter ``
> +
> + If specified, only mirror flows that match the provided OpenFlow filter.
> + The available fields are documented in ``ovs-fields(7)``.
> +
> See Also
>
>
> ``ovs-appctl(8)``, ``ovs-vswitchd(8)``, ``ovs-pcap(1)``,
> -``ovs-tcpundump(1)``, ``tcpdump(8)``, ``wireshark(8)``.
> +``ovs-fields(7)``, ``ovs-tcpundump(1)``, ``tcpdump(8)``,
> +``wireshark(8)``.
> diff --git a/NEWS b/NEWS
> index c9e4064e6..35f7eb0c7 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -4,6 +4,9 @@ Post-v3.3.0
> * Conntrack now supports 'random' flag for selecting ports in a range
> while natting and 'persistent' flag for selection of the IP address
> from a range.
> + - OVSDB:
> + * Added a new filter column in the Mirror table which can be used to
> + apply filters to mirror ports.
>
>
> v3.3.0 - 16 Feb 2024
> diff --git a/lib/flow.c b/lib/flow.c
> index 8e3402388..a088bdc86 100644
> --- a/lib/flow.c
> +++ b/lib/flow.c
> @@ -3569,7 +3569,7 @@ miniflow_equal_in_minimask(const struct miniflow *a,
> const struct miniflow *b,
> return true;
> }
>
> -/* Returns true if 'a' and 'b' are equal at the places where there are 1-bits
> +/* Returns true if 'a' and 'b' are equal at the places where there are 0-bits
I think the logic is that only the differences on the masked data will return
false. So the original text is right.
> * in 'mask', false if they differ. */
> bool
> miniflow_equal_flow_in_minimask(const struct miniflow *a, const struct flow
> *b,
> @@ -3587,6 +3587,25 @@ miniflow_equal_flow_in_minimask(const struct miniflow
> *a, const struct flow *b,
> return true;
> }
>
> +/* Returns false if 'a' and 'b' differ in places where there are 1-bits in
> + * 'wc', true otherwise. */
Maybe wright this the same as above to avoid confusion, i.e.
Returns true if 'a' and 'b' are equal at the places where there are 1-bits in
‘wc’, false if they differ [at the places where there are 1-bits in ‘wc’].
> +bool
> +miniflow_equal_flow_in_flow_wc(const struct miniflow *a, const struct flow
> *b,
> + const struct flow_wildcards *wc)
> +{
> +const struct flow *wc_masks = >masks;
> +size_t idx;
> +
> +FLOWMAP_FOR_EACH_INDEX (idx, a->map) {
> +if ((miniflow_get(a, idx) ^ flow_u64_value(b, idx)) &
> +flow_u64_value(wc_masks, idx)) {
> +return false;
> +}
> +}
> +
> +return true;
> +}
> +
>
> void
> minimask_init(struct minimask *mask, const struct flow_wildcards *wc)
> diff --git a/lib/flow.h b/lib/flow.h
> index 75a9be3c1..a644be39d 100644
> --- a/lib/flow.h
> +++ b/lib/flow.h
> @@ -748,6 +748,9 @@ bool miniflow_equal_in_minimask(const struct miniflow *a,
> bool miniflow_equal_flow_in_minimask(const struct miniflow *a,
> const struct flow *b,
> const struct minimask *);
> +bool miniflow_equal_flow_in_flow_wc(const struct miniflow *a,
> +const struct flow *b,
> +
