Re: [ovs-dev] [PATCH] ovs-ctl: introuduce post start scripts
On Thu, Aug 25, 2022 at 12:29:36PM +0200, Ilya Maximets wrote:
> On 8/24/22 21:29, Aaron Conole wrote:
> > lic121 writes:
> >
> >> Support running a list of scripts after vswitchd start, so that we can
> >> make some runtime config persistence across ovs restart. For example,
> >> the config that `ovs-appctl dpctl` makes.
> >>
> >> Signed-off-by: lic121
> >> ---
> >
> > This is an interesting patch. I'm thinking that there is some value in
> > having a post-start mechanism like this because we talked years ago on
> > the list about having a way to do things like changing thread
> > priorities to RT[1], etc. These are things that OVS might not want to
> > have the complicated configuration knobs required to provide, but a
> > post-start script could perform and would be easy to see just by looking
> > at the post-start directory.
>
> Hmm. that might be useful, but I'm not sure if ovn-ctl is a right place.
> On most of the systems OVS is managed by systemd or some other init system.
> And most of such systems has post-start hooks. For example, users may
> add their scripts into ExecStartPost of a systemd service file. This
> should also give more visibility to the end user and, probably, more
> flexibility in the configuration by having also pre-start hooks and other
> tuning options.
>
> >
> > OTOH, I think that if there are ovs-appctl settings that we want to make
> > persistent, then it would make a lot more sense to just expose those
> > settings via the database (which is persistent). Maybe there is a
> > reason to keep some as post-start configurable only, but I can't think
> > of any that we wouldn't also want to just put into the DB.
>
> That's a good point. If you want something to be preserved across restarts,
> the database is exactly the place to store the configuration.
Ilya, thanks for your review on this patch. Would you mind have a look
at the patch that persistent max ct connection configureation.
https://patchwork.ozlabs.org/project/openvswitch/patch/1653105300-28434-1-git-send-email-lic...@chinatelecom.cn/
>
> For the mentioned vlog configuration, we already have ovs-ctl knobs that
> will place desired logging options into cmdline, IIRC.
>
> Best regards, Ilya Maximets.
>
> >
> > [1]:
> > https://lists.linuxfoundation.org/pipermail/ovs-dev/2017-January/327025.html
> >
> >> Documentation/ref/ovs-ctl.8.rst | 9 +
> >> NEWS| 2 ++
> >> utilities/ovs-ctl.in| 11 +++
> >> 3 files changed, 22 insertions(+)
> >>
> >> diff --git a/Documentation/ref/ovs-ctl.8.rst
> >> b/Documentation/ref/ovs-ctl.8.rst
> >> index 7cfc413..380ef06 100644
> >> --- a/Documentation/ref/ovs-ctl.8.rst
> >> +++ b/Documentation/ref/ovs-ctl.8.rst
> >> @@ -37,6 +37,15 @@ system administrators but to be called internally by
> >> system startup
> >> scripts.
> >>
> >>
> >> +Ovs has a lot of runtime configurations which can't survive from
> >> +service restart. For example, the vlog level config, dpctl/set
> >> +config and flows. To persistent runtime configurations, we now
> >> +support running a list of scripts after vswitchd process. Just
> >> +put your scripts under ``etcdir/post_scripts_vswitchd/`` and with
> >> +``.sh`` suffix, ovs-ctl start/restart will auto execute your
> >> +scripts one by one.
> >> +
> >> +
> >> Each ``ovs-ctl`` command is described separately below.
> >>
> >> The ``start`` command
> >> diff --git a/NEWS b/NEWS
> >> index 024fa44..9387d37 100644
> >> --- a/NEWS
> >> +++ b/NEWS
> >> @@ -81,6 +81,8 @@ v3.0.0 - xx xxx
> >> - Linux TC offload:
> >> * Add support for offloading meters via tc police.
> >> * Add support for offloading the check_pkt_len action.
> >> + - ovs-ctl:
> >> + * Support ovs post start scripts
> >> - New configuration knob 'other_config:all-members-active' for
> >> balance-slb bonds.
> >> - Previously deprecated Linux kernel module is now fully removed from
> >> diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
> >> index e6e07f4..a765ef3 100644
> >> --- a/utilities/ovs-ctl.in
> >> +++ b/utilities/ovs-ctl.in
> >> @@ -226,9 +226,18 @@ do_start_forwarding () {
> >> fi
> >> }
> >>
> >> +run_post_start_scripts () {
> >> +mkdir -p $POST_SCRIPTS_DIR_VSWITCHD
&
[ovs-dev] [PATCH] dpif-netdev: not skip cycle metric for no-packet rxqs
In PMD, cycle cost for each rxq is saved so that we know each rxq's
load. But rxq that doesn't receive a packet is skipped. In fact,
polling no-packet rxq costs cycles as well. In my test, 100w pps
rxq(vhostuser) cost cycles 87,553,850,086 while no-packet rxq costs
cycles 353,402,306.
In asign mode "group", ovs always pick the lowest load pmd. If we
have too many(let's say 100) no-packet rxqs, these rxqs will be
asigned to the same pmd. Because no load increase when a no-packet
rxq to pmd.
To avoid this, this patch count cycles for no-packet rxq as well.
Signed-off-by: lic121
---
lib/dpif-netdev.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index a45b460..b30a098 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -5335,8 +5335,8 @@ dp_netdev_process_rxq_port(struct dp_netdev_pmd_thread
*pmd,
dp_netdev_pmd_flush_output_packets(pmd, false);
} else {
-/* Discard cycles. */
-cycle_timer_stop(>perf_stats, );
+cycles = cycle_timer_stop(>perf_stats, );
+dp_netdev_rxq_add_cycles(rxq, RXQ_CYCLES_PROC_CURR, cycles);
if (error != EAGAIN && error != EOPNOTSUPP) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ovs-ctl: introuduce post start scripts
Support running a list of scripts after vswitchd start, so that we can
make some runtime config persistence across ovs restart. For example,
the config that `ovs-appctl dpctl` makes.
Signed-off-by: lic121
---
Documentation/ref/ovs-ctl.8.rst | 9 +
NEWS| 2 ++
utilities/ovs-ctl.in| 11 +++
3 files changed, 22 insertions(+)
diff --git a/Documentation/ref/ovs-ctl.8.rst b/Documentation/ref/ovs-ctl.8.rst
index 7cfc413..380ef06 100644
--- a/Documentation/ref/ovs-ctl.8.rst
+++ b/Documentation/ref/ovs-ctl.8.rst
@@ -37,6 +37,15 @@ system administrators but to be called internally by system
startup
scripts.
+Ovs has a lot of runtime configurations which can't survive from
+service restart. For example, the vlog level config, dpctl/set
+config and flows. To persistent runtime configurations, we now
+support running a list of scripts after vswitchd process. Just
+put your scripts under ``etcdir/post_scripts_vswitchd/`` and with
+``.sh`` suffix, ovs-ctl start/restart will auto execute your
+scripts one by one.
+
+
Each ``ovs-ctl`` command is described separately below.
The ``start`` command
diff --git a/NEWS b/NEWS
index 024fa44..9387d37 100644
--- a/NEWS
+++ b/NEWS
@@ -81,6 +81,8 @@ v3.0.0 - xx xxx
- Linux TC offload:
* Add support for offloading meters via tc police.
* Add support for offloading the check_pkt_len action.
+ - ovs-ctl:
+ * Support ovs post start scripts
- New configuration knob 'other_config:all-members-active' for
balance-slb bonds.
- Previously deprecated Linux kernel module is now fully removed from
diff --git a/utilities/ovs-ctl.in b/utilities/ovs-ctl.in
index e6e07f4..a765ef3 100644
--- a/utilities/ovs-ctl.in
+++ b/utilities/ovs-ctl.in
@@ -226,9 +226,18 @@ do_start_forwarding () {
fi
}
+run_post_start_scripts () {
+mkdir -p $POST_SCRIPTS_DIR_VSWITCHD
+for f in `ls $POST_SCRIPTS_DIR_VSWITCHD/*.sh 2>/dev/null|| true`
+do
+action "Running post script $f" bash $f
+done
+}
+
start_forwarding () {
if test X"$OVS_VSWITCHD" = Xyes; then
do_start_forwarding || return 1
+run_post_start_scripts || return 1
fi
if test X"$RECORD_HOSTNAME" = Xyes; then
set_hostname &
@@ -348,6 +357,8 @@ set_defaults () {
DB_SCHEMA=$datadir/vswitch.ovsschema
EXTRA_DBS=
+POST_SCRIPTS_DIR_VSWITCHD=$etcdir/post_scripts_vswitchd
+
PROTOCOL=gre
DPORT=
SPORT=
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4] dpif-netdev: Allow cross-NUMA polling on selected ports
On Wed, Jun 22, 2022 at 09:32:29AM +, Anurag Agarwal wrote:
> Thanks for your feedback. Please find my comments inline.
>
> Regards,
> Anurag
>
> > -Original Message-
> > From: lic...@chinatelecom.cn
> > Sent: Friday, June 17, 2022 2:29 PM
> > To: Anurag Agarwal
> > Cc: ovs-dev@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH v4] dpif-netdev: Allow cross-NUMA polling on
> > selected ports
> >
> > On Wed, Jun 08, 2022 at 03:58:31PM +0530, Anurag Agarwal wrote:
> > > From: Jan Scheurich
> > >
> > > Today dpif-netdev considers PMD threads on a non-local NUMA node for
> > > automatic assignment of the rxqs of a port only if there are no local,non-
> > isolated PMDs.
> > >
> > > On typical servers with both physical ports on one NUMA node, this
> > > often leaves the PMDs on the other NUMA node under-utilized, wasting CPU
> > resources.
> > > The alternative, to manually pin the rxqs to PMDs on remote NUMA
> > > nodes, also has drawbacks as it limits OVS' ability to auto load-balance
> > > the
> > rxqs.
> > >
> > > This patch introduces a new interface configuration option to allow
> > > ports to be automatically polled by PMDs on any NUMA node:
> > >
> > > ovs-vsctl set interface other_config:cross-numa-polling=true
> > >
> > > The group assignment algorithm now has the ability to select lowest
> > > loaded PMD on any NUMA, and not just the local NUMA on which the rxq
> > > of the port resides
> > >
> > > If this option is not present or set to false, legacy behaviour applies.
> > >
> > > Co-authored-by: Anurag Agarwal
> > > Signed-off-by: Jan Scheurich
> > > Signed-off-by: Anurag Agarwal
> > > ---
> > >
> > > Changes in this patch:
> > > - Addressed comments from Kevin Traynor
> > >
> > > Please refer this thread for an earlier discussion on this topic:
> > > https://protect2.fireeye.com/v1/url?k=31323334-501cfaf3-313273af-45444
> > > 5554331-165fefd0f093b353=1=d638a27f-b902-46d8-883e-
> > ed0c226be33c=
> > > https%3A%2F%2Fmail.openvswitch.org%2Fpipermail%2Fovs-dev%2F2022-
> > March%
> > > 2F392310.html
> > > ---
> > > Documentation/topics/dpdk/pmd.rst | 23 ++
> > > lib/dpif-netdev.c | 130 ++
> > > tests/pmd.at | 38 +
> > > vswitchd/vswitch.xml | 20 +
> > > 4 files changed, 177 insertions(+), 34 deletions(-)
> > >
> > > diff --git a/Documentation/topics/dpdk/pmd.rst
> > > b/Documentation/topics/dpdk/pmd.rst
> > > index b259cc8b3..387f962d1 100644
> > > --- a/Documentation/topics/dpdk/pmd.rst
> > > +++ b/Documentation/topics/dpdk/pmd.rst
> > > @@ -99,6 +99,25 @@ core cycles for each Rx queue::
> > >
> > > $ ovs-appctl dpif-netdev/pmd-rxq-show
> > >
> > > +Normally, Rx queues are assigned to PMD threads automatically. By
> > > +default OVS only assigns Rx queues to PMD threads executing on the
> > > +same NUMA node in order to avoid unnecessary latency for accessing
> > > +packet buffers across the NUMA boundary. Typically this overhead is
> > > +higher for vhostuser ports than for physical ports due to the packet
> > > +copy that is done for all rx packets.
> > > +
> > > +On NUMA servers with physical ports only on one NUMA node, the
> > > +NUMA-local polling policy can lead to an under-utilization of the PMD
> > > +threads on the remote NUMA node. For the overall OVS performance it
> > > +may in such cases be beneficial to utilize the spare capacity and
> > > +allow polling of a physical port's rxqs across NUMA nodes despite the
> > overhead involved.
> > > +The policy can be set per port with the following configuration option::
> > > +
> > > +$ ovs-vsctl set Interface \
> > > +other_config:cross-numa-polling=true|false
> > > +
> > > +The default value is false.
> > > +
> > > .. note::
> > >
> > > A history of one minute is recorded and shown for each Rx queue to
> > > allow for @@ -115,6 +134,10 @@ core cycles for each Rx queue::
> > > A ``overhead`` statistics is shown per PMD: it represents the number
> > > of
> > > cycles inherently consumed by the OVS PMD processing loop.
> > >
> > > +.. versionchanged:: 2.18.0
> > > +
> > > + Added the interface parameter ``other_config:cross-numa-polling``
> > > +
> > > Rx queue to PMD assignment takes place whenever there are
> > > configuration changes or can be triggered by using::
> > >
> > > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c index
> > > ff57b3961..86f88964b 100644
> > > --- a/lib/dpif-netdev.c
> > > +++ b/lib/dpif-netdev.c
> > > @@ -467,6 +467,7 @@ struct dp_netdev_port {
> > > char *type; /* Port type as requested by user. */
> > > char *rxq_affinity_list;/* Requested affinity of rx queues. */
> > > enum txq_req_mode txq_requested_mode;
> > > +bool cross_numa_polling;/* If true cross polling will be
> > > enabled. */
> > > };
> > >
> > > static bool dp_netdev_flow_ref(struct dp_netdev_flow *); @@ -2101,6
> > > +2102,7 @@
Re: [ovs-dev] [PATCH v4] dpif-netdev: Allow cross-NUMA polling on selected ports
On Wed, Jun 08, 2022 at 03:58:31PM +0530, Anurag Agarwal wrote:
> From: Jan Scheurich
>
> Today dpif-netdev considers PMD threads on a non-local NUMA node for automatic
> assignment of the rxqs of a port only if there are no local,non-isolated PMDs.
>
> On typical servers with both physical ports on one NUMA node, this often
> leaves the PMDs on the other NUMA node under-utilized, wasting CPU resources.
> The alternative, to manually pin the rxqs to PMDs on remote NUMA nodes, also
> has drawbacks as it limits OVS' ability to auto load-balance the rxqs.
>
> This patch introduces a new interface configuration option to allow ports to
> be automatically polled by PMDs on any NUMA node:
>
> ovs-vsctl set interface other_config:cross-numa-polling=true
>
> The group assignment algorithm now has the ability to select lowest loaded PMD
> on any NUMA, and not just the local NUMA on which the rxq of the port resides
>
> If this option is not present or set to false, legacy behaviour applies.
>
> Co-authored-by: Anurag Agarwal
> Signed-off-by: Jan Scheurich
> Signed-off-by: Anurag Agarwal
> ---
>
> Changes in this patch:
> - Addressed comments from Kevin Traynor
>
> Please refer this thread for an earlier discussion on this topic:
> https://mail.openvswitch.org/pipermail/ovs-dev/2022-March/392310.html
> ---
> Documentation/topics/dpdk/pmd.rst | 23 ++
> lib/dpif-netdev.c | 130 ++
> tests/pmd.at | 38 +
> vswitchd/vswitch.xml | 20 +
> 4 files changed, 177 insertions(+), 34 deletions(-)
>
> diff --git a/Documentation/topics/dpdk/pmd.rst
> b/Documentation/topics/dpdk/pmd.rst
> index b259cc8b3..387f962d1 100644
> --- a/Documentation/topics/dpdk/pmd.rst
> +++ b/Documentation/topics/dpdk/pmd.rst
> @@ -99,6 +99,25 @@ core cycles for each Rx queue::
>
> $ ovs-appctl dpif-netdev/pmd-rxq-show
>
> +Normally, Rx queues are assigned to PMD threads automatically. By default
> +OVS only assigns Rx queues to PMD threads executing on the same NUMA
> +node in order to avoid unnecessary latency for accessing packet buffers
> +across the NUMA boundary. Typically this overhead is higher for vhostuser
> +ports than for physical ports due to the packet copy that is done for all
> +rx packets.
> +
> +On NUMA servers with physical ports only on one NUMA node, the NUMA-local
> +polling policy can lead to an under-utilization of the PMD threads on the
> +remote NUMA node. For the overall OVS performance it may in such cases be
> +beneficial to utilize the spare capacity and allow polling of a physical
> +port's rxqs across NUMA nodes despite the overhead involved.
> +The policy can be set per port with the following configuration option::
> +
> +$ ovs-vsctl set Interface \
> +other_config:cross-numa-polling=true|false
> +
> +The default value is false.
> +
> .. note::
>
> A history of one minute is recorded and shown for each Rx queue to allow
> for
> @@ -115,6 +134,10 @@ core cycles for each Rx queue::
> A ``overhead`` statistics is shown per PMD: it represents the number of
> cycles inherently consumed by the OVS PMD processing loop.
>
> +.. versionchanged:: 2.18.0
> +
> + Added the interface parameter ``other_config:cross-numa-polling``
> +
> Rx queue to PMD assignment takes place whenever there are configuration
> changes
> or can be triggered by using::
>
> diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> index ff57b3961..86f88964b 100644
> --- a/lib/dpif-netdev.c
> +++ b/lib/dpif-netdev.c
> @@ -467,6 +467,7 @@ struct dp_netdev_port {
> char *type; /* Port type as requested by user. */
> char *rxq_affinity_list;/* Requested affinity of rx queues. */
> enum txq_req_mode txq_requested_mode;
> +bool cross_numa_polling;/* If true cross polling will be enabled. */
> };
>
> static bool dp_netdev_flow_ref(struct dp_netdev_flow *);
> @@ -2101,6 +2102,7 @@ port_create(const char *devname, const char *type,
> port->sf = NULL;
> port->emc_enabled = true;
> port->need_reconfigure = true;
> +port->cross_numa_polling = false;
> ovs_mutex_init(>txq_used_mutex);
>
> *portp = port;
> @@ -5013,6 +5015,7 @@ dpif_netdev_port_set_config(struct dpif *dpif,
> odp_port_t port_no,
> bool emc_enabled = smap_get_bool(cfg, "emc-enable", true);
> const char *tx_steering_mode = smap_get(cfg, "tx-steering");
> enum txq_req_mode txq_mode;
> +bool cross_numa_polling = smap_get_bool(cfg, "cross-numa-polling",
> false);
>
> ovs_rwlock_wrlock(>port_rwlock);
> error = get_port_by_number(dp, port_no, );
> @@ -5020,6 +5023,14 @@ dpif_netdev_port_set_config(struct dpif *dpif,
> odp_port_t port_no,
> goto unlock;
> }
>
> +if (cross_numa_polling != port->cross_numa_polling) {
> +port->cross_numa_polling = cross_numa_polling;
> +VLOG_INFO("%s:cross-numa-polling has
[ovs-dev] [PATCH v2 1/2] lldp: fix lldp memory leak
lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref
is supposed to free the memory regardless of hw->h_flags.
Signed-off-by: lic121
Acked-by: Eelco Chaudron
---
lib/lldp/lldpd.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/lib/lldp/lldpd.c b/lib/lldp/lldpd.c
index 403f1f5..4bff7b0 100644
--- a/lib/lldp/lldpd.c
+++ b/lib/lldp/lldpd.c
@@ -140,13 +140,9 @@ lldpd_cleanup(struct lldpd *cfg)
VLOG_DBG("cleanup all ports");
LIST_FOR_EACH_SAFE (hw, h_entries, >g_hardware) {
-if (!hw->h_flags) {
-ovs_list_remove(>h_entries);
-lldpd_remote_cleanup(hw, NULL, true);
-lldpd_hardware_cleanup(cfg, hw);
-} else {
-lldpd_remote_cleanup(hw, NULL, false);
-}
+ovs_list_remove(>h_entries);
+lldpd_remote_cleanup(hw, NULL, true);
+lldpd_hardware_cleanup(cfg, hw);
}
VLOG_DBG("cleanup all chassis");
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2 2/2] ofproto-dpif: avoid unneccesary backer revalidation
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Without this patch, bridge_reconfigure() always trigger udpif
revalidator because of lldp.
Signed-off-by: lic121
Signed-off-by: Eelco Chaudron
Co-authored-by: Eelco Chaudron
---
lib/ovs-lldp.c | 8
lib/ovs-lldp.h | 1 +
ofproto/ofproto-dpif.c | 7 +--
tests/ofproto-dpif.at | 33 +
4 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index a9d205e..2d13e97 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet
*packet,
ovs_mutex_unlock();
}
+/* Is LLDP enabled?
+ */
+bool
+lldp_is_enabled(struct lldp *lldp)
+{
+return lldp ? lldp->enabled : false;
+}
+
/* Configures the LLDP stack.
*/
bool
diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
index 0e536e8..661ac4e 100644
--- a/lib/ovs-lldp.h
+++ b/lib/ovs-lldp.h
@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
bool lldp_should_send_packet(struct lldp *cfg);
bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
+bool lldp_is_enabled(struct lldp *lldp);
void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
const struct eth_addr eth_src);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7a1bb0d..3ae44f0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2490,11 +2490,11 @@ set_lldp(struct ofport *ofport_,
{
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+bool old_enable = lldp_is_enabled(ofport->lldp);
int error = 0;
-if (cfg) {
+if (cfg && !smap_is_empty(cfg)) {
if (!ofport->lldp) {
-ofproto->backer->need_revalidate = REV_RECONFIGURE;
ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
}
@@ -2506,6 +2506,9 @@ set_lldp(struct ofport *ofport_,
} else if (ofport->lldp) {
lldp_unref(ofport->lldp);
ofport->lldp = NULL;
+}
+
+if (lldp_is_enabled(ofport->lldp) != old_enable) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index dbb3b6d..1c9a94c 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
+OVS_VSWITCHD_START(
+[add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
+)
+dnl first revalidation triggered by add interface
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+1
+])
+
+dnl enable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+2
+])
+
+dnl disable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+dnl remove lldp, no revalidation as lldp was disabled
+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2 0/2] fix revalidation triggers
v2: - split lldp patch into two This series fix revalidation trigger: - avoid revalidation if lldp config doesn't change I have these two patch in one series because the second patch may fails without the first one. lic121 (2): lldp: fix lldp memory leak ofproto-dpif: avoid unneccesary backer revalidation lib/lldp/lldpd.c | 10 +++--- lib/ovs-lldp.c | 8 lib/ovs-lldp.h | 1 + ofproto/ofproto-dpif.c | 7 +-- tests/ofproto-dpif.at | 33 + 5 files changed, 50 insertions(+), 9 deletions(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH 2/2] ofproto-dpif: avoid unneccesary backer revalidation
On Thu, May 26, 2022 at 11:51:31AM +0200, Ilya Maximets wrote: > On 4/5/22 12:16, lic121 wrote: > > If lldp didn't change, we are not supposed to trigger backer > > revalidation. > > Without this patch, bridge_reconfigure() always trigger udpif > > revalidator because of lldp. > > > > This patch also fix lldp memory leak bug: > > lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref > > is supposed to free the memory, but it doesn't. > > It looks like these are two separate changes. > Could you, please, split the patch in two? Sure, will do that. > > Best regards, Ilya Maximets. ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4] dpif-netdev: ct maxconns config persistence
Max allowed userspace dp conntrack entries is configurable with
'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
this configuration is expected to survive from host reboot, from
ovs service restart.
Signed-off-by: lic121
---
Notes:
v4:
- add '\n' for warning msg
v3:
- add a warning to dpctl_ct_set_maxconns
- add NEWS entry
v2:
- rename "ct-maxconns" to "userspace-ct-maxconns"
NEWS| 5 +
lib/dpctl.c | 3 +++
lib/dpctl.man | 4 +++-
lib/dpif-netdev.c | 11 +++
tests/system-traffic.at | 10 ++
vswitchd/vswitch.xml| 7 +++
6 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index eece0d0..e75b2af 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,11 @@ Post-v2.17.0
- Windows:
* Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6.
* IPv6 Geneve tunnel support.
+ - Userspace datapath:
+ * 'ovs-appctl dpctl/ct-set-maxconns' is deprecated for lack of persistence
+ capabilitiy.
+ * New configuration knob 'other_config:userspace-ct-maxconns' to set
+ maximum number of connection tracker entries for userspace datapath.
v2.17.0 - 17 Feb 2022
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 29041fa..ff64691 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1990,6 +1990,9 @@ dpctl_ct_set_maxconns(int argc, const char *argv[],
struct dpif *dpif;
int error = opt_dpif_open(argc, argv, dpctl_p, 3, );
if (!error) {
+dpctl_print(dpctl_p,
+"Warning: dpctl/ct-set-maxconns is deprecated by "
+"other_config:userspace-ct-maxconns\n");
uint32_t maxconns;
if (ovs_scan(argv[argc - 1], "%"SCNu32, )) {
error = ct_dpif_set_maxconns(dpif, maxconns);
diff --git a/lib/dpctl.man b/lib/dpctl.man
index c100d0a..4f08a3f 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -343,7 +343,9 @@ system due to connection tracking or simply limiting
connection
tracking. If the number of connections is already over the new maximum
limit request then the new maximum limit will be enforced when the
number of connections decreases to that limit, which normally happens
-due to connection expiry. Only supported for userspace datapath.
+due to connection expiry. Only supported for userspace datapath. This
+command is deprecated by ovsdb cfg other_config:userspace-ct-maxconns
+because of persistence capability.
.
.TP
\*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 21277b2..bfbe6db 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4828,6 +4828,17 @@ dpif_netdev_set_config(struct dpif *dpif, const struct
smap *other_config)
}
}
+uint32_t ct_maxconns, cur_maxconns;
+ct_maxconns = smap_get_int(other_config, "userspace-ct-maxconns",
+ UINT32_MAX);
+/* Leave runtime value as it is when cfg is removed. */
+if (ct_maxconns < UINT32_MAX) {
+conntrack_get_maxconns(dp->conntrack, _maxconns);
+if (ct_maxconns != cur_maxconns) {
+conntrack_set_maxconns(dp->conntrack, ct_maxconns);
+}
+}
+
bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
bool cur_smc;
atomic_read_relaxed(>smc_enable_db, _smc);
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 1d20366..cb1eb16 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2305,6 +2305,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
10
])
+AT_CHECK([ovs-vsctl set Open_vswitch . other_config:userspace-ct-maxconns=20],
[0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
+AT_CHECK([ovs-vsctl remove Open_vswitch . other_config userspace-ct-maxconns],
[0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index cc1dd77..f2324be 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -183,6 +183,13 @@
+
+The maximum number of connection tracker entries allowed in the
+userspace datapath. This deprecates "ovs-appctl dpctl/ct-set-maxconns"
+command.
+
+
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3] dpif-netdev: ct maxconns config persistence
Max allowed userspace dp conntrack entries is configurable with
'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
this configuration is expected to survive from host reboot, from
ovs service restart.
Signed-off-by: lic121
---
Notes:
v3:
- add a warning to dpctl_ct_set_maxconns
- add NEWS entry
v2:
- rename "ct-maxconns" to "userspace-ct-maxconns"
NEWS| 5 +
lib/dpctl.c | 3 +++
lib/dpctl.man | 4 +++-
lib/dpif-netdev.c | 11 +++
tests/system-traffic.at | 10 ++
vswitchd/vswitch.xml| 7 +++
6 files changed, 39 insertions(+), 1 deletion(-)
diff --git a/NEWS b/NEWS
index eece0d0..e75b2af 100644
--- a/NEWS
+++ b/NEWS
@@ -29,6 +29,11 @@ Post-v2.17.0
- Windows:
* Conntrack support for TCPv6, UDPv6, ICMPv6, FTPv6.
* IPv6 Geneve tunnel support.
+ - Userspace datapath:
+ * 'ovs-appctl dpctl/ct-set-maxconns' is deprecated for lack of persistence
+ capabilitiy.
+ * New configuration knob 'other_config:userspace-ct-maxconns' to set
+ maximum number of connection tracker entries for userspace datapath.
v2.17.0 - 17 Feb 2022
diff --git a/lib/dpctl.c b/lib/dpctl.c
index 29041fa..73cf14c 100644
--- a/lib/dpctl.c
+++ b/lib/dpctl.c
@@ -1990,6 +1990,9 @@ dpctl_ct_set_maxconns(int argc, const char *argv[],
struct dpif *dpif;
int error = opt_dpif_open(argc, argv, dpctl_p, 3, );
if (!error) {
+dpctl_print(dpctl_p,
+"Warning: dpctl/ct-set-maxconns is deprecated by "
+"other_config:userspace-ct-maxconns");
uint32_t maxconns;
if (ovs_scan(argv[argc - 1], "%"SCNu32, )) {
error = ct_dpif_set_maxconns(dpif, maxconns);
diff --git a/lib/dpctl.man b/lib/dpctl.man
index c100d0a..4f08a3f 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -343,7 +343,9 @@ system due to connection tracking or simply limiting
connection
tracking. If the number of connections is already over the new maximum
limit request then the new maximum limit will be enforced when the
number of connections decreases to that limit, which normally happens
-due to connection expiry. Only supported for userspace datapath.
+due to connection expiry. Only supported for userspace datapath. This
+command is deprecated by ovsdb cfg other_config:userspace-ct-maxconns
+because of persistence capability.
.
.TP
\*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 21277b2..bfbe6db 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4828,6 +4828,17 @@ dpif_netdev_set_config(struct dpif *dpif, const struct
smap *other_config)
}
}
+uint32_t ct_maxconns, cur_maxconns;
+ct_maxconns = smap_get_int(other_config, "userspace-ct-maxconns",
+ UINT32_MAX);
+/* Leave runtime value as it is when cfg is removed. */
+if (ct_maxconns < UINT32_MAX) {
+conntrack_get_maxconns(dp->conntrack, _maxconns);
+if (ct_maxconns != cur_maxconns) {
+conntrack_set_maxconns(dp->conntrack, ct_maxconns);
+}
+}
+
bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
bool cur_smc;
atomic_read_relaxed(>smc_enable_db, _smc);
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 1d20366..cb1eb16 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2305,6 +2305,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
10
])
+AT_CHECK([ovs-vsctl set Open_vswitch . other_config:userspace-ct-maxconns=20],
[0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
+AT_CHECK([ovs-vsctl remove Open_vswitch . other_config userspace-ct-maxconns],
[0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index cc1dd77..f2324be 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -183,6 +183,13 @@
+
+The maximum number of connection tracker entries allowed in the
+userspace datapath. This deprecates "ovs-appctl dpctl/ct-set-maxconns"
+command.
+
+
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2] dpif-netdev: ct maxconns config persistence
On Tue, May 17, 2022 at 09:06:24AM -0400, Aaron Conole wrote:
> lic121 writes:
>
> > Max allowed userspace dp conntrack entries is configurable with
> > 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
> > this configuration is expected to survive from host reboot, from
> > ovs service restart.
> >
> > Signed-off-by: lic121
> > ---
>
> Sorry - just one last comment, and then I think this can go in.
>
> Please add a warning to dpctl_ct_set_maxconns that also flags the
> deprecation, and add a NEWS entry as well. That way end users are aware
> of the change. If you want to add NEWS entry as a separate patch
> (because those can generate git-am related issues) that's probably okay.
No worry, and thanks for your careful review. I will add warning and add NEWS in
the next version.
>
> > Notes:
> > v2:
> > - rename "ct-maxconns" to "userspace-ct-maxconns"
> >
> > lib/dpctl.man | 4 +++-
> > lib/dpif-netdev.c | 11 +++
> > tests/system-traffic.at | 10 ++
> > vswitchd/vswitch.xml| 7 +++
> > 4 files changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dpctl.man b/lib/dpctl.man
> > index c100d0a..4f08a3f 100644
> > --- a/lib/dpctl.man
> > +++ b/lib/dpctl.man
> > @@ -343,7 +343,9 @@ system due to connection tracking or simply limiting
> > connection
> > tracking. If the number of connections is already over the new maximum
> > limit request then the new maximum limit will be enforced when the
> > number of connections decreases to that limit, which normally happens
> > -due to connection expiry. Only supported for userspace datapath.
> > +due to connection expiry. Only supported for userspace datapath. This
> > +command is deprecated by ovsdb cfg other_config:userspace-ct-maxconns
> > +because of persistence capability.
> > .
> > .TP
> > \*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 6764343..e2348a1 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -4827,6 +4827,17 @@ dpif_netdev_set_config(struct dpif *dpif, const
> > struct smap *other_config)
> > }
> > }
> >
> > +uint32_t ct_maxconns, cur_maxconns;
> > +ct_maxconns = smap_get_int(other_config, "userspace-ct-maxconns",
> > + UINT32_MAX);
> > +/* Leave runtime value as it is when cfg is removed. */
> > +if (ct_maxconns < UINT32_MAX) {
> > +conntrack_get_maxconns(dp->conntrack, _maxconns);
> > +if (ct_maxconns != cur_maxconns) {
> > +conntrack_set_maxconns(dp->conntrack, ct_maxconns);
> > +}
> > +}
> > +
> > bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
> > bool cur_smc;
> > atomic_read_relaxed(>smc_enable_db, _smc);
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 4a7fa49..82ea992 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -2258,6 +2258,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > 10
> > ])
> >
> > +AT_CHECK([ovs-vsctl set Open_vswitch .
> > other_config:userspace-ct-maxconns=20], [0])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > +20
> > +])
> > +
> > +AT_CHECK([ovs-vsctl remove Open_vswitch . other_config
> > userspace-ct-maxconns], [0])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > +20
> > +])
> > +
> > OVS_TRAFFIC_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 0c66326..48f05d7 100644
> > --- a/vswitchd/vswitch.xml
> > +++ b/vswitchd/vswitch.xml
> > @@ -183,6 +183,13 @@
> >
> >
> >
> > + > + type='{"type": "integer", "minInteger": 0}'>
> > +The maximum number of connection tracker entries allowed in the
> > +userspace datapath. This deprecates "ovs-appctl
> > dpctl/ct-set-maxconns"
> > +command.
> > +
> > +
> > >type='{"type": "integer", "minInteger": 500}'>
> >
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2] dpif-netdev: ct maxconns config persistence
Max allowed userspace dp conntrack entries is configurable with
'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
this configuration is expected to survive from host reboot, from
ovs service restart.
Signed-off-by: lic121
---
Notes:
v2:
- rename "ct-maxconns" to "userspace-ct-maxconns"
lib/dpctl.man | 4 +++-
lib/dpif-netdev.c | 11 +++
tests/system-traffic.at | 10 ++
vswitchd/vswitch.xml| 7 +++
4 files changed, 31 insertions(+), 1 deletion(-)
diff --git a/lib/dpctl.man b/lib/dpctl.man
index c100d0a..4f08a3f 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -343,7 +343,9 @@ system due to connection tracking or simply limiting
connection
tracking. If the number of connections is already over the new maximum
limit request then the new maximum limit will be enforced when the
number of connections decreases to that limit, which normally happens
-due to connection expiry. Only supported for userspace datapath.
+due to connection expiry. Only supported for userspace datapath. This
+command is deprecated by ovsdb cfg other_config:userspace-ct-maxconns
+because of persistence capability.
.
.TP
\*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 6764343..e2348a1 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4827,6 +4827,17 @@ dpif_netdev_set_config(struct dpif *dpif, const struct
smap *other_config)
}
}
+uint32_t ct_maxconns, cur_maxconns;
+ct_maxconns = smap_get_int(other_config, "userspace-ct-maxconns",
+ UINT32_MAX);
+/* Leave runtime value as it is when cfg is removed. */
+if (ct_maxconns < UINT32_MAX) {
+conntrack_get_maxconns(dp->conntrack, _maxconns);
+if (ct_maxconns != cur_maxconns) {
+conntrack_set_maxconns(dp->conntrack, ct_maxconns);
+}
+}
+
bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
bool cur_smc;
atomic_read_relaxed(>smc_enable_db, _smc);
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 4a7fa49..82ea992 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2258,6 +2258,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
10
])
+AT_CHECK([ovs-vsctl set Open_vswitch . other_config:userspace-ct-maxconns=20],
[0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
+AT_CHECK([ovs-vsctl remove Open_vswitch . other_config userspace-ct-maxconns],
[0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 0c66326..48f05d7 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -183,6 +183,13 @@
+
+The maximum number of connection tracker entries allowed in the
+userspace datapath. This deprecates "ovs-appctl dpctl/ct-set-maxconns"
+command.
+
+
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] tnl: fix multiple tunnel pop for native tnl
On Wed, May 04, 2022 at 12:53:34PM +0200, Ilya Maximets wrote:
> On 5/4/22 05:23, lic121 wrote:
> > On Tue, May 03, 2022 at 10:18:18PM +0200, Ilya Maximets wrote:
> >> On 5/3/22 15:31, Peng He wrote:
> >>> Hi，
> >>> this issue has been found and resolved by two independent patches:
> >>>
> >>> http://patchwork.ozlabs.org/project/openvswitch/patch/20190726121839.92497-1-wang...@139.com/
> >>>
> >>> <http://patchwork.ozlabs.org/project/openvswitch/patch/20190726121839.92497-1-wang...@139.com/>
> >>> http://patchwork.ozlabs.org/project/openvswitch/patch/20201009121503.43893-1-hepeng.0...@bytedance.com/
> >>>
> >>> <http://patchwork.ozlabs.org/project/openvswitch/patch/20201009121503.43893-1-hepeng.0...@bytedance.com/>
> >>>
> >>> but I recently found my patch is still not fixing all the cases.
> >>>
> >>> would like to have a discussion about this.
> >>
> >> Hrm. It seems to me that the problem is more genric then mirrors.
> >> The reason is that tnl_port_map_lookup() logic doesn't work for
> >> a bridge with more than one tunnel termination port.
> >> Before commit dc0bd12f5b04 ("userspace: Enable non-bridge port
> >> as tunnel endpoint."), only the bridge port was allowed to terminate
> >> the tunnel. MAC+IP lookup performed by the classifier inside the
> >> tnl_port_map_lookup() was enough to determine if termination is
> >> needed or if the packet has to be forwarded without tunnel_pop.
> >> Now we have multiple ports that may or may not terminate the tunnel.
> >>
> >> Assuming we have 2 different tunnel ports and two different normal
> >> ports with different IP addresses and we expect the first tunnel
> >> to be terminated on the first port and the second tunnel to be
> >> terminated on the second port. At the same time, the packet from
> >> the first tunnel should be forwarded to the second port without
> >> tunnel_pop and the packet from the second tunnel should be
> >> forwarded to the first port without tunnel_pop.
> >>
> >> Since the classifier in the tunnel-port.c will have rules for
> >> both tunnels and it only matches on MAC+IP (+ some other fileds,
> >> but we don't care much right now), we will have decapsulation
> >> triggered in all cases. The same as what happened with the
> >> mirrored traffic.
> >>
> >> Two patcehs mentioned above only focused on mirrors, so they
> >> will not fix the problem in a common case. The patch below
> >> is closer to a solution, but it will skip the matches on dl_dst,
> >> and, possibly, other tunnel header fileds, which we absolutely
> >> need in a non-mirror case with 2 tunnels.
> >
> > Hi Ilya, you mean we need to check dl_dst as well before tnl classifier
> > lookup? For the case that two ports have the same ip address?
>
> If two ports on the same bridge has the same IP, that sounds
> like a misconfiguration (I'd argue that having IP assigned
> to a bridge member is a misconfiguration in general, but we
> have what we have...), so I'm not sure if we need to check
> for IP additionally. But we must unwildcard (add to a flow
> match) fields that we're using to make a decision.
Thanks for the explanation, yes you are right. We do need to unwildcard.
> tnl_port_map_lookup() does that in a generic way. It has
> information about all the MACs, IPs, VLANs and other fileds
> that may or may not be significant for the lookup.
> Generic classifier lookup inside that function unwildcards
> all the fileds that was actually used during lookup.
>
> So, I'm not convinced if just checking the IP is sufficient,
An ovs port mainly has two propertyies: IP and MAC. MAC is not a property
for most tunnels. If two ports have the same IP but different MAC. We
can't tell which port is the right tunnel terminal port. So I checked
the IP only, but missed the unwildcard as you mentioned.
> but we definitely have to unwildcard it in a flow match if
> we're checking it.
>
> >
> >>
> >> The correct solution would be to add an output port match to
> >> the classifier, but clasifier is not designed to do that and
> >> 'struct match' doesn't have an appropriate field. It shoudl
> >> be a separate classifier per ipdev, or something like that.
> >>
> >> Thoughts?
> >>
> >> In any case, every part of the packet that was used to make a
> >> decision must be unwildcarded. IP on the port is not part of
> >> the packet, but dst_ip i
Re: [ovs-dev] [PATCH] tnl: fix multiple tunnel pop for native tnl
On Tue, May 03, 2022 at 10:18:18PM +0200, Ilya Maximets wrote:
> On 5/3/22 15:31, Peng He wrote:
> > Hi，
> > this issue has been found and resolved by two independent patches:
> >
> > http://patchwork.ozlabs.org/project/openvswitch/patch/20190726121839.92497-1-wang...@139.com/
> >
> > <http://patchwork.ozlabs.org/project/openvswitch/patch/20190726121839.92497-1-wang...@139.com/>
> > http://patchwork.ozlabs.org/project/openvswitch/patch/20201009121503.43893-1-hepeng.0...@bytedance.com/
> >
> > <http://patchwork.ozlabs.org/project/openvswitch/patch/20201009121503.43893-1-hepeng.0...@bytedance.com/>
> >
> > but I recently found my patch is still not fixing all the cases.
> >
> > would like to have a discussion about this.
>
> Hrm. It seems to me that the problem is more genric then mirrors.
> The reason is that tnl_port_map_lookup() logic doesn't work for
> a bridge with more than one tunnel termination port.
> Before commit dc0bd12f5b04 ("userspace: Enable non-bridge port
> as tunnel endpoint."), only the bridge port was allowed to terminate
> the tunnel. MAC+IP lookup performed by the classifier inside the
> tnl_port_map_lookup() was enough to determine if termination is
> needed or if the packet has to be forwarded without tunnel_pop.
> Now we have multiple ports that may or may not terminate the tunnel.
>
> Assuming we have 2 different tunnel ports and two different normal
> ports with different IP addresses and we expect the first tunnel
> to be terminated on the first port and the second tunnel to be
> terminated on the second port. At the same time, the packet from
> the first tunnel should be forwarded to the second port without
> tunnel_pop and the packet from the second tunnel should be
> forwarded to the first port without tunnel_pop.
>
> Since the classifier in the tunnel-port.c will have rules for
> both tunnels and it only matches on MAC+IP (+ some other fileds,
> but we don't care much right now), we will have decapsulation
> triggered in all cases. The same as what happened with the
> mirrored traffic.
>
> Two patcehs mentioned above only focused on mirrors, so they
> will not fix the problem in a common case. The patch below
> is closer to a solution, but it will skip the matches on dl_dst,
> and, possibly, other tunnel header fileds, which we absolutely
> need in a non-mirror case with 2 tunnels.
Hi Ilya, you mean we need to check dl_dst as well before tnl classifier
lookup? For the case that two ports have the same ip address?
>
> The correct solution would be to add an output port match to
> the classifier, but clasifier is not designed to do that and
> 'struct match' doesn't have an appropriate field. It shoudl
> be a separate classifier per ipdev, or something like that.
>
> Thoughts?
>
> In any case, every part of the packet that was used to make a
> decision must be unwildcarded. IP on the port is not part of
> the packet, but dst_ip is.
>
> Best regards, Ilya Maximets.
>
> >
> >
> >
> > lic121 mailto:lic...@chinatelecom.cn>>
> > 于2022年5月3日周二 20:55写道：
> >
> > In compose_output_action__(), terminate_native_tunnel() is called
> > to test if a pkt matches a tnl port. If yes, a TUNNEL_POP action
> > is appended. Instead of outputing the pkt to the origin port, pkt
> > is redirected into the tnl port.
> >
> > pkt metadata(i.e. nw_proto, dst_ip, dst_port) is considered when
> > testing a pkt regardless of the out port until Ilya's patch[1].
> > Patch[1] adds the condition that terminal ports must have IP
> > address. This patch is to make addtional port ip check that port
> > ip address must match the pkt dst ip. This check covers the mirror
> > port case:
> > To tcpdump underlay pkt, we may create mirror port of dpdk port.
> > When compose_output_action__() is called on the mirror port,
> > pkt is redirected into tnl port again. As a result, ping shows
> > DUP. fast path flow has two tnl_pop actions.
> >
> > ```
> > [root@ovs1 vagrant]# ping 172.16.100.2
> > PING 172.16.100.2 (172.16.100.2) 56(84) bytes of data.
> > 64 bytes from 172.16.100.2 <http://172.16.100.2>: icmp_seq=1 ttl=64
> > time=0.606 ms
> > 64 bytes from 172.16.100.2 <http://172.16.100.2>: icmp_seq=1 ttl=64
> > time=0.617 ms (DUP!)
> > ```
> >
> > ```
> > [root@ovs ovs]# ovs-appctl dpctl/dump-flows |grep tnl_pop
> >
> > recirc_id(0),in_port(3),packet_type(ns=0,id=0),eth(src=52:54:00:a6:c6:de,dst=ea:b4:09:72:44:4c),eth_type(0x0800),ipv4(dst=192
[ovs-dev] [PATCH] tnl: fix multiple tunnel pop for native tnl
In compose_output_action__(), terminate_native_tunnel() is called
to test if a pkt matches a tnl port. If yes, a TUNNEL_POP action
is appended. Instead of outputing the pkt to the origin port, pkt
is redirected into the tnl port.
pkt metadata(i.e. nw_proto, dst_ip, dst_port) is considered when
testing a pkt regardless of the out port until Ilya's patch[1].
Patch[1] adds the condition that terminal ports must have IP
address. This patch is to make addtional port ip check that port
ip address must match the pkt dst ip. This check covers the mirror
port case:
To tcpdump underlay pkt, we may create mirror port of dpdk port.
When compose_output_action__() is called on the mirror port,
pkt is redirected into tnl port again. As a result, ping shows
DUP. fast path flow has two tnl_pop actions.
```
[root@ovs1 vagrant]# ping 172.16.100.2
PING 172.16.100.2 (172.16.100.2) 56(84) bytes of data.
64 bytes from 172.16.100.2: icmp_seq=1 ttl=64 time=0.606 ms
64 bytes from 172.16.100.2: icmp_seq=1 ttl=64 time=0.617 ms (DUP!)
```
```
[root@ovs ovs]# ovs-appctl dpctl/dump-flows |grep tnl_pop
recirc_id(0),in_port(3),packet_type(ns=0,id=0),eth(src=52:54:00:a6:c6:de,dst=ea:b4:09:72:44:4c),eth_type(0x0800),ipv4(dst=192.168.124.33,proto=17,frag=no),udp(dst=8472),
packets:1, bytes:148, used:0.637s, actions:tnl_pop(2),tnl_pop(2)
```
[1] (d584bb2b6a1 "ofproto-dpif-xlate: Terminate native tunnels only on ports
with IP addresses.")
Signed-off-by: lic121
---
ofproto/ofproto-dpif-xlate.c | 45 +++-
tests/tunnel-push-pop.at | 49
2 files changed, 80 insertions(+), 14 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 17f7e28..30e636e 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -4107,18 +4107,26 @@ is_neighbor_reply_correct(const struct xlate_ctx *ctx,
const struct flow *flow)
}
static bool
-xport_has_ip(const struct xport *xport)
+xport_has_ip(const struct xport *xport, const struct in6_addr ip6)
{
struct in6_addr *ip_addr, *mask;
int n_in6 = 0;
+int i;
+bool hasip = false;
if (netdev_get_addr_list(xport->netdev, _addr, , _in6)) {
n_in6 = 0;
} else {
+for (i = 0; i < n_in6; i++) {
+if (IN6_ARE_ADDR_EQUAL(ip_addr + i, )) {
+hasip = true;
+break;
+}
+}
free(ip_addr);
free(mask);
}
-return n_in6 ? true : false;
+return hasip;
}
static bool
@@ -4127,26 +4135,35 @@ terminate_native_tunnel(struct xlate_ctx *ctx, const
struct xport *xport,
odp_port_t *tnl_port)
{
*tnl_port = ODPP_NONE;
+bool ip_pkt = true;
+struct in6_addr ip_dst;
+
+if (flow->dl_type == htons(ETH_TYPE_IP)) {
+ip_dst = in6_addr_mapped_ipv4(flow->nw_dst);
+} else if (flow->dl_type == htons(ETH_TYPE_IPV6)) {
+ip_dst = flow->ipv6_dst;
+} else {
+ip_pkt = false;
+}
/* XXX: Write better Filter for tunnel port. We can use in_port
* in tunnel-port flow to avoid these checks completely.
*
- * Port without an IP address cannot be a tunnel termination point.
- * Not performing a lookup in this case to avoid unwildcarding extra
- * flow fields (dl_dst). */
-if (ovs_native_tunneling_is_on(ctx->xbridge->ofproto)
-&& xport_has_ip(xport)) {
+ * Not performing a lookup if dst ip not match, to avoid unwildcarding
+ * extra flow fields (dl_dst). Also avoid duplicate tnl_pop. */
+if ((flow->dl_type == htons(ETH_TYPE_ARP) ||
+flow->nw_proto == IPPROTO_ICMPV6) &&
+is_neighbor_reply_correct(ctx, flow)) {
+tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
+ctx->xin->allow_side_effects);
+
+} else if (ovs_native_tunneling_is_on(ctx->xbridge->ofproto)
+&& ip_pkt && xport_has_ip(xport, ip_dst)) {
*tnl_port = tnl_port_map_lookup(flow, wc);
/* If no tunnel port was found and it's about an ARP or ICMPv6 packet,
* do tunnel neighbor snooping. */
-if (*tnl_port == ODPP_NONE &&
-(flow->dl_type == htons(ETH_TYPE_ARP) ||
- flow->nw_proto == IPPROTO_ICMPV6) &&
- is_neighbor_reply_correct(ctx, flow)) {
-tnl_neigh_snoop(flow, wc, ctx->xbridge->name,
-ctx->xin->allow_side_effects);
-} else if (*tnl_port != ODPP_NONE &&
+if (*tnl_port != ODPP_NONE &&
ctx->xin->allow_side_effects &&
dl_type_is_ip_any(flow->dl_type)) {
struct eth_addr mac = flow->dl_src;
diff --git a/tests/tunnel-push-pop.at b/tests/tunnel-push-pop.at
index c633441..b3a3bf9 100644
Re: [ovs-dev] [PATCH] dpif-netdev: ct maxconns config persistence
On Thu, Apr 28, 2022 at 10:04:17AM -0400, Aaron Conole wrote:
> lic121 writes:
>
> > On Mon, Apr 25, 2022 at 08:47:32AM -0400, Aaron Conole wrote:
> >> lic121 writes:
> >>
> >> > Max allowed conntrack entries is configurable with
> >> > 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
> >> > this configuration is expected to survive from host reboot.
> >> >
> >> > Signed-off-by: lic121
> >> > ---
> >>
> >> One complication is that there are 2 other conntrack implementations
> >> that OvS supports - one for the Windows Netlink, and the other for
> >> Linux kernel netlink. How do you deal with this parameter there?
> >
> > As linux kernel datapath leverages the kernel implementent of conntrack.
> > Besides ovs, the kernel conntrack is used by kernel network stack
> > itself. A proper way to set linux kernel datapath conntrack max conns
> > could be sysctl interface (nf_conntrack_max).
> > But I am not sure if a similar system level configuration exists for windows
> > datapath.
> >
> >>
> >> Something in the datapath for this needs to accommodate these. Maybe
> >> the DB should store the datapath name as well - that way each dp can
> >> lookup the configuration if supported (and if not we can either log the
> >> error, etc). That does start to look a bit unfriendly, but at least it
> >> prevents user confusion with this knob.
> >
> > Agree, to indiates the datapath name in configuration item name is
> > better than explanation in configuration detail description.
> > How about the name "userspace-ct-maxconns"?
>
> I guess that should be fine. I might prefer 'netdev-conntrack-maxconns'
> or something to that effect. "userspace" makes sense to developers, but
> we usually call the userspace side the "netdev" datapath.
When we say 'netdev', one may think it as 'netdev datapath'. This is what
we expect. But I wonder if someone else may think it as ovs 'netdev
library' that abstracts interacting with network devices.
>
> Ilya, might have some suggestion.
>
> >>
> >> Each CT dp interface does something slightly differently for
> >> configuration, and I'm not sure this knob as proposed is the best
> >> solution.
> >>
> >> The 'deprecated' command was only implemented for the userspace DP, but
> >> in theory it could work independently for all. I'm not sure this one
> >> can work though (for example, if we use AF_XDP for some bridge, but
> >> kernel DP for another, we would want to have two different max entry
> >> values, but this interface doesn't allow that).
> >>
> >> > lib/dpctl.man | 3 ++-
> >> > lib/dpif-netdev.c | 10 ++
> >> > tests/system-traffic.at | 10 ++
> >> > vswitchd/vswitch.xml| 7 +++
> >> > 4 files changed, 29 insertions(+), 1 deletion(-)
> >> >
> >> > diff --git a/lib/dpctl.man b/lib/dpctl.man
> >> > index c100d0a..2cfc4f2 100644
> >> > --- a/lib/dpctl.man
> >> > +++ b/lib/dpctl.man
> >> > @@ -343,7 +343,8 @@ system due to connection tracking or simply limiting
> >> > connection
> >> > tracking. If the number of connections is already over the new maximum
> >> > limit request then the new maximum limit will be enforced when the
> >> > number of connections decreases to that limit, which normally happens
> >> > -due to connection expiry. Only supported for userspace datapath.
> >> > +due to connection expiry. Only supported for userspace datapath. This
> >> > +command is deprecated by ovsdb cfg other_config:ct-maxconns.
> >> > .
> >> > .TP
> >> > \*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
> >> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> >> > index 6764343..c518d30 100644
> >> > --- a/lib/dpif-netdev.c
> >> > +++ b/lib/dpif-netdev.c
> >> > @@ -4827,6 +4827,16 @@ dpif_netdev_set_config(struct dpif *dpif, const
> >> > struct smap *other_config)
> >> > }
> >> > }
> >> >
> >> > +uint32_t ct_maxconns, cur_maxconns;
> >> > +ct_maxconns = smap_get_int(other_config, "ct-maxconns", UINT32_MAX);
> >> > +/* Leave runtime value as it is when cfg is removed. */
> >> > +if (ct_maxconns < UINT32_MAX) {
> >> > +conntrack_get_maxconns(dp->c
[ovs-dev] [PATCH v2] ipfix: trigger revalidation if ipfix options changes
ipfix cfg creation/deleting triggers revalidation. But this does
not cover the case where ipfix options changes, which also suppose
to trigger revalidation.
Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config
set.")
Signed-off-by: lic121
---
ofproto/ofproto-dpif-ipfix.c | 51 +---
ofproto/ofproto-dpif-ipfix.h | 2 +-
ofproto/ofproto-dpif.c | 7 +++---
tests/ofproto-dpif.at| 17 ++-
4 files changed, 54 insertions(+), 23 deletions(-)
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index fc927fe..742eed3 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -926,17 +926,21 @@ dpif_ipfix_bridge_exporter_destroy(struct
dpif_ipfix_bridge_exporter *exporter)
static void
dpif_ipfix_bridge_exporter_set_options(
struct dpif_ipfix_bridge_exporter *exporter,
-const struct ofproto_ipfix_bridge_exporter_options *options)
+const struct ofproto_ipfix_bridge_exporter_options *options,
+bool *options_changed)
{
-bool options_changed;
-
if (!options || sset_is_empty(>targets)) {
/* No point in doing any work if there are no targets. */
-dpif_ipfix_bridge_exporter_clear(exporter);
+if (exporter->options) {
+dpif_ipfix_bridge_exporter_clear(exporter);
+*options_changed = true;
+} else {
+*options_changed = false;
+}
return;
}
-options_changed = (
+*options_changed = (
!exporter->options
|| !ofproto_ipfix_bridge_exporter_options_equal(
options, exporter->options));
@@ -945,7 +949,7 @@ dpif_ipfix_bridge_exporter_set_options(
* shortchanged in collectors (which indicates that opening one or
* more of the configured collectors failed, so that we should
* retry). */
-if (options_changed
+if (*options_changed
|| collectors_count(exporter->exporter.collectors)
< sset_count(>targets)) {
if (!dpif_ipfix_exporter_set_options(
@@ -957,7 +961,7 @@ dpif_ipfix_bridge_exporter_set_options(
}
/* Avoid reconfiguring if options didn't change. */
-if (!options_changed) {
+if (!*options_changed) {
return;
}
@@ -1015,17 +1019,21 @@ dpif_ipfix_flow_exporter_destroy(struct
dpif_ipfix_flow_exporter *exporter)
static bool
dpif_ipfix_flow_exporter_set_options(
struct dpif_ipfix_flow_exporter *exporter,
-const struct ofproto_ipfix_flow_exporter_options *options)
+const struct ofproto_ipfix_flow_exporter_options *options,
+bool *options_changed)
{
-bool options_changed;
-
if (sset_is_empty(>targets)) {
/* No point in doing any work if there are no targets. */
-dpif_ipfix_flow_exporter_clear(exporter);
+if (exporter->options) {
+dpif_ipfix_flow_exporter_clear(exporter);
+*options_changed = true;
+} else {
+*options_changed = false;
+}
return true;
}
-options_changed = (
+*options_changed = (
!exporter->options
|| !ofproto_ipfix_flow_exporter_options_equal(
options, exporter->options));
@@ -1034,7 +1042,7 @@ dpif_ipfix_flow_exporter_set_options(
* shortchanged in collectors (which indicates that opening one or
* more of the configured collectors failed, so that we should
* retry). */
-if (options_changed
+if (*options_changed
|| collectors_count(exporter->exporter.collectors)
< sset_count(>targets)) {
if (!dpif_ipfix_exporter_set_options(
@@ -1046,7 +1054,7 @@ dpif_ipfix_flow_exporter_set_options(
}
/* Avoid reconfiguring if options didn't change. */
-if (!options_changed) {
+if (!*options_changed) {
return true;
}
@@ -1069,7 +1077,7 @@ remove_flow_exporter(struct dpif_ipfix *di,
free(node);
}
-void
+bool
dpif_ipfix_set_options(
struct dpif_ipfix *di,
const struct ofproto_ipfix_bridge_exporter_options
*bridge_exporter_options,
@@ -1077,16 +1085,19 @@ dpif_ipfix_set_options(
size_t n_flow_exporters_options) OVS_EXCLUDED(mutex)
{
int i;
+bool beo_changed, feo_changed, entry_changed;
struct ofproto_ipfix_flow_exporter_options *options;
struct dpif_ipfix_flow_exporter_map_node *node;
ovs_mutex_lock();
dpif_ipfix_bridge_exporter_set_options(>bridge_exporter,
- bridge_exporter_options);
+ bridge_exporter_options,
+ _changed);
/* Add new flow exporters and update current flow exporters. */
options = (struct ofproto_ipfix_flow_exporter_options *)
flow_exporters_options;
+feo_changed = false;
for (i = 0; i < n_flow_exporters_options; i++) {
node
[ovs-dev] [PATCH v2] ofproto-dpif-xlate: remove mirror assert
During the revalidation/upcall, mirror could be removed. Instead of crash
the process, we can simply skip the deleted mirror.
The issue had been triggered multiple times by ovs-tcpdump in my test.
Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.")
Signed-off-by: lic121
Tested-by: Adrian Moreno
---
ofproto/ofproto-dpif-xlate.c | 11 ---
1 file changed, 8 insertions(+), 3 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5a770f1..8a5632d 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2142,9 +2142,14 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle
*xbundle,
int snaplen;
/* Get the details of the mirror represented by the rightmost 1-bit. */
-ovs_assert(mirror_get(xbridge->mbridge, raw_ctz(mirrors),
- , _mirrors,
- , , _vlan));
+if (OVS_UNLIKELY(!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
+ , _mirrors,
+ , , _vlan))) {
+/* The mirror got reconfigured before we got to read it's
+ * configuration. */
+mirrors = zero_rightmost_1bit(mirrors);
+continue;
+}
/* If this mirror selects on the basis of VLAN, and it does not select
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: remove mirror assert
On Wed, Apr 27, 2022 at 01:36:48PM +0200, Adrian Moreno wrote:
>
>
> On 4/7/22 15:53, lic121 wrote:
> >During the revalidation, mirror could be removed. Instead of crash
> >the process, we can simply skip the deleted mirror.
> >
> >Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.")
> >Signed-off-by: lic121
> >---
> > ofproto/ofproto-dpif-xlate.c | 10 +++---
> > 1 file changed, 7 insertions(+), 3 deletions(-)
> >
> >diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >index 5a770f1..33e56c2 100644
> >--- a/ofproto/ofproto-dpif-xlate.c
> >+++ b/ofproto/ofproto-dpif-xlate.c
> >@@ -2142,9 +2142,13 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle
> >*xbundle,
> > int snaplen;
> > /* Get the details of the mirror represented by the rightmost
> > 1-bit. */
> >-ovs_assert(mirror_get(xbridge->mbridge, raw_ctz(mirrors),
> >- , _mirrors,
> >- , , _vlan));
> >+if (!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
> >+ , _mirrors,
> >+ , , _vlan)) {
> >+/* Mirror could be deleted during revalidation */
> >+mirrors = zero_rightmost_1bit(mirrors);
> >+continue;
> >+}
> > /* If this mirror selects on the basis of VLAN, and it does not
> > select
>
> I have reproduced the assert and verified this patch fixes it.
>
> The patch looks good to me. My only nit is about the comment, it's a
> bit confusing because this code is executed also in upcall handling,
> not only on revalidation. Something on the lines of the following
> would be a bit more clear IMHO:
>
> /* The mirror got reconfigured before we got to read it's configuration. */
>
> Also, maybe add OVS_UNLIKELY?
Thanks for the review, will address the comments in the next version.
>
> Thanks.
> --
> Adrián Moreno
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] dpif-netdev: ct maxconns config persistence
On Mon, Apr 25, 2022 at 08:47:32AM -0400, Aaron Conole wrote:
> lic121 writes:
>
> > Max allowed conntrack entries is configurable with
> > 'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
> > this configuration is expected to survive from host reboot.
> >
> > Signed-off-by: lic121
> > ---
>
> One complication is that there are 2 other conntrack implementations
> that OvS supports - one for the Windows Netlink, and the other for
> Linux kernel netlink. How do you deal with this parameter there?
As linux kernel datapath leverages the kernel implementent of conntrack.
Besides ovs, the kernel conntrack is used by kernel network stack
itself. A proper way to set linux kernel datapath conntrack max conns
could be sysctl interface (nf_conntrack_max).
But I am not sure if a similar system level configuration exists for windows
datapath.
>
> Something in the datapath for this needs to accommodate these. Maybe
> the DB should store the datapath name as well - that way each dp can
> lookup the configuration if supported (and if not we can either log the
> error, etc). That does start to look a bit unfriendly, but at least it
> prevents user confusion with this knob.
Agree, to indiates the datapath name in configuration item name is
better than explanation in configuration detail description.
How about the name "userspace-ct-maxconns"?
>
> Each CT dp interface does something slightly differently for
> configuration, and I'm not sure this knob as proposed is the best
> solution.
>
> The 'deprecated' command was only implemented for the userspace DP, but
> in theory it could work independently for all. I'm not sure this one
> can work though (for example, if we use AF_XDP for some bridge, but
> kernel DP for another, we would want to have two different max entry
> values, but this interface doesn't allow that).
>
> > lib/dpctl.man | 3 ++-
> > lib/dpif-netdev.c | 10 ++
> > tests/system-traffic.at | 10 ++
> > vswitchd/vswitch.xml| 7 +++
> > 4 files changed, 29 insertions(+), 1 deletion(-)
> >
> > diff --git a/lib/dpctl.man b/lib/dpctl.man
> > index c100d0a..2cfc4f2 100644
> > --- a/lib/dpctl.man
> > +++ b/lib/dpctl.man
> > @@ -343,7 +343,8 @@ system due to connection tracking or simply limiting
> > connection
> > tracking. If the number of connections is already over the new maximum
> > limit request then the new maximum limit will be enforced when the
> > number of connections decreases to that limit, which normally happens
> > -due to connection expiry. Only supported for userspace datapath.
> > +due to connection expiry. Only supported for userspace datapath. This
> > +command is deprecated by ovsdb cfg other_config:ct-maxconns.
> > .
> > .TP
> > \*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
> > diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
> > index 6764343..c518d30 100644
> > --- a/lib/dpif-netdev.c
> > +++ b/lib/dpif-netdev.c
> > @@ -4827,6 +4827,16 @@ dpif_netdev_set_config(struct dpif *dpif, const
> > struct smap *other_config)
> > }
> > }
> >
> > +uint32_t ct_maxconns, cur_maxconns;
> > +ct_maxconns = smap_get_int(other_config, "ct-maxconns", UINT32_MAX);
> > +/* Leave runtime value as it is when cfg is removed. */
> > +if (ct_maxconns < UINT32_MAX) {
> > +conntrack_get_maxconns(dp->conntrack, _maxconns);
> > +if (ct_maxconns != cur_maxconns) {
> > +conntrack_set_maxconns(dp->conntrack, ct_maxconns);
> > +}
> > +}
> > +
> > bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
> > bool cur_smc;
> > atomic_read_relaxed(>smc_enable_db, _smc);
> > diff --git a/tests/system-traffic.at b/tests/system-traffic.at
> > index 4a7fa49..00fefc5 100644
> > --- a/tests/system-traffic.at
> > +++ b/tests/system-traffic.at
> > @@ -2258,6 +2258,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > 10
> > ])
> >
> > +AT_CHECK([ovs-vsctl set Open_vswitch . other_config:ct-maxconns=20], [0])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > +20
> > +])
> > +
> > +AT_CHECK([ovs-vsctl remove Open_vswitch . other_config ct-maxconns], [0])
> > +AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
> > +20
> > +])
> > +
> > OVS_TRAFFIC_VSWITCHD_STOP
> > AT_CLEANUP
> >
> > diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
> > index 0c66326..ec634d5 100644
> > --- a/vswitchd/v
Re: [ovs-dev] [PATCH] ipfix: trigger revalidation if ipfix options changes
On Fri, Apr 22, 2022 at 11:41:25AM +0200, Eelco Chaudron wrote:
>
>
> On 16 Apr 2022, at 13:09, lic121 wrote:
>
> > ipfix cfg creation/deleting triggers revalidation. But this does
> > not cover the case where ipfix options changes, which also suppose
> > to trigger revalidation.
>
> Some small comments below, and I think we should add a test case to verify
> the cases that could yield a reconfigure.
Thanks for the review, will address the commnents in the next version.
>
> > Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config
> > set.")
> > Signed-off-by: lic121
> > ---
> > ofproto/ofproto-dpif-ipfix.c | 46
> >
> > ofproto/ofproto-dpif-ipfix.h | 2 +-
> > ofproto/ofproto-dpif.c | 7 +++
> > 3 files changed, 33 insertions(+), 22 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > index fc927fe..7d316ca 100644
> > --- a/ofproto/ofproto-dpif-ipfix.c
> > +++ b/ofproto/ofproto-dpif-ipfix.c
> > @@ -926,17 +926,19 @@ dpif_ipfix_bridge_exporter_destroy(struct
> > dpif_ipfix_bridge_exporter *exporter)
> > static void
> > dpif_ipfix_bridge_exporter_set_options(
> > struct dpif_ipfix_bridge_exporter *exporter,
> > -const struct ofproto_ipfix_bridge_exporter_options *options)
> > +const struct ofproto_ipfix_bridge_exporter_options *options,
> > +bool *options_changed)
> > {
> > -bool options_changed;
> > -
> > if (!options || sset_is_empty(>targets)) {
> > /* No point in doing any work if there are no targets. */
> > -dpif_ipfix_bridge_exporter_clear(exporter);
> > +if (exporter->options) {
> > +dpif_ipfix_bridge_exporter_clear(exporter);
> > +*options_changed = true;
> > +}
>
> This can return without the option changed being set, so it should become:
>
> } else {
>*options_changed = false;
> }
>
> > return;
> > }
> >
> > -options_changed = (
> > +*options_changed = (
> > !exporter->options
> > || !ofproto_ipfix_bridge_exporter_options_equal(
> > options, exporter->options));
> > @@ -945,7 +947,7 @@ dpif_ipfix_bridge_exporter_set_options(
> > * shortchanged in collectors (which indicates that opening one or
> > * more of the configured collectors failed, so that we should
> > * retry). */
> > -if (options_changed
> > +if (*options_changed
> > || collectors_count(exporter->exporter.collectors)
> > < sset_count(>targets)) {
> > if (!dpif_ipfix_exporter_set_options(
> > @@ -957,7 +959,7 @@ dpif_ipfix_bridge_exporter_set_options(
> > }
> >
> > /* Avoid reconfiguring if options didn't change. */
> > -if (!options_changed) {
> > +if (!*options_changed) {
> > return;
> > }
> >
> > @@ -1015,17 +1017,19 @@ dpif_ipfix_flow_exporter_destroy(struct
> > dpif_ipfix_flow_exporter *exporter)
> > static bool
> > dpif_ipfix_flow_exporter_set_options(
> > struct dpif_ipfix_flow_exporter *exporter,
> > -const struct ofproto_ipfix_flow_exporter_options *options)
> > +const struct ofproto_ipfix_flow_exporter_options *options,
> > +bool *options_changed)
> > {
> > -bool options_changed;
> > -
> > if (sset_is_empty(>targets)) {
> > /* No point in doing any work if there are no targets. */
> > -dpif_ipfix_flow_exporter_clear(exporter);
> > +if (exporter->options) {
> > +dpif_ipfix_flow_exporter_clear(exporter);
> > +*options_changed = true;
> > +}
>
> This can return without the option changed being set, so it should become:
>
> } else {
>*options_changed = false;
> }
>
> > return true;
> > }
> >
> > -options_changed = (
> > +*options_changed = (
> > !exporter->options
> > || !ofproto_ipfix_flow_exporter_options_equal(
> > options, exporter->options));
> > @@ -1034,7 +1038,7 @@ dpif_ipfix_flow_exporter_set_options(
> > * shortchanged in collectors (which indicates that opening one or
> > * more of the configured collectors failed, so that we should
> > * retry). */
> > -if (options_changed
> > +if (*options_changed
> > || collec
[ovs-dev] [PATCH] dpif-netdev: ct maxconns config persistence
Max allowed conntrack entries is configurable with
'ovs-appctl dpctl/ct-set-maxconns' command. In real scenarios,
this configuration is expected to survive from host reboot.
Signed-off-by: lic121
---
lib/dpctl.man | 3 ++-
lib/dpif-netdev.c | 10 ++
tests/system-traffic.at | 10 ++
vswitchd/vswitch.xml| 7 +++
4 files changed, 29 insertions(+), 1 deletion(-)
diff --git a/lib/dpctl.man b/lib/dpctl.man
index c100d0a..2cfc4f2 100644
--- a/lib/dpctl.man
+++ b/lib/dpctl.man
@@ -343,7 +343,8 @@ system due to connection tracking or simply limiting
connection
tracking. If the number of connections is already over the new maximum
limit request then the new maximum limit will be enforced when the
number of connections decreases to that limit, which normally happens
-due to connection expiry. Only supported for userspace datapath.
+due to connection expiry. Only supported for userspace datapath. This
+command is deprecated by ovsdb cfg other_config:ct-maxconns.
.
.TP
\*(DX\fBct\-get\-maxconns\fR [\fIdp\fR]
diff --git a/lib/dpif-netdev.c b/lib/dpif-netdev.c
index 6764343..c518d30 100644
--- a/lib/dpif-netdev.c
+++ b/lib/dpif-netdev.c
@@ -4827,6 +4827,16 @@ dpif_netdev_set_config(struct dpif *dpif, const struct
smap *other_config)
}
}
+uint32_t ct_maxconns, cur_maxconns;
+ct_maxconns = smap_get_int(other_config, "ct-maxconns", UINT32_MAX);
+/* Leave runtime value as it is when cfg is removed. */
+if (ct_maxconns < UINT32_MAX) {
+conntrack_get_maxconns(dp->conntrack, _maxconns);
+if (ct_maxconns != cur_maxconns) {
+conntrack_set_maxconns(dp->conntrack, ct_maxconns);
+}
+}
+
bool smc_enable = smap_get_bool(other_config, "smc-enable", false);
bool cur_smc;
atomic_read_relaxed(>smc_enable_db, _smc);
diff --git a/tests/system-traffic.at b/tests/system-traffic.at
index 4a7fa49..00fefc5 100644
--- a/tests/system-traffic.at
+++ b/tests/system-traffic.at
@@ -2258,6 +2258,16 @@ AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
10
])
+AT_CHECK([ovs-vsctl set Open_vswitch . other_config:ct-maxconns=20], [0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
+AT_CHECK([ovs-vsctl remove Open_vswitch . other_config ct-maxconns], [0])
+AT_CHECK([ovs-appctl dpctl/ct-get-maxconns], [], [dnl
+20
+])
+
OVS_TRAFFIC_VSWITCHD_STOP
AT_CLEANUP
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 0c66326..ec634d5 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -183,6 +183,13 @@
+
+The maximum number of connection tracker entries allowed in the
+datapath. Only supported for userspace datapath. This deprecates
+"ovs-appctl dpctl/ct-set-maxconns" command.
+
+
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] ipfix: trigger revalidation if ipfix options changes
ipfix cfg creation/deleting triggers revalidation. But this does
not cover the case where ipfix options changes, which also suppose
to trigger revalidation.
Fixes: a9f5ee1199e1 ("ofproto-dpif: Trigger revalidation when ipfix config
set.")
Signed-off-by: lic121
---
ofproto/ofproto-dpif-ipfix.c | 46
ofproto/ofproto-dpif-ipfix.h | 2 +-
ofproto/ofproto-dpif.c | 7 +++
3 files changed, 33 insertions(+), 22 deletions(-)
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index fc927fe..7d316ca 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -926,17 +926,19 @@ dpif_ipfix_bridge_exporter_destroy(struct
dpif_ipfix_bridge_exporter *exporter)
static void
dpif_ipfix_bridge_exporter_set_options(
struct dpif_ipfix_bridge_exporter *exporter,
-const struct ofproto_ipfix_bridge_exporter_options *options)
+const struct ofproto_ipfix_bridge_exporter_options *options,
+bool *options_changed)
{
-bool options_changed;
-
if (!options || sset_is_empty(>targets)) {
/* No point in doing any work if there are no targets. */
-dpif_ipfix_bridge_exporter_clear(exporter);
+if (exporter->options) {
+dpif_ipfix_bridge_exporter_clear(exporter);
+*options_changed = true;
+}
return;
}
-options_changed = (
+*options_changed = (
!exporter->options
|| !ofproto_ipfix_bridge_exporter_options_equal(
options, exporter->options));
@@ -945,7 +947,7 @@ dpif_ipfix_bridge_exporter_set_options(
* shortchanged in collectors (which indicates that opening one or
* more of the configured collectors failed, so that we should
* retry). */
-if (options_changed
+if (*options_changed
|| collectors_count(exporter->exporter.collectors)
< sset_count(>targets)) {
if (!dpif_ipfix_exporter_set_options(
@@ -957,7 +959,7 @@ dpif_ipfix_bridge_exporter_set_options(
}
/* Avoid reconfiguring if options didn't change. */
-if (!options_changed) {
+if (!*options_changed) {
return;
}
@@ -1015,17 +1017,19 @@ dpif_ipfix_flow_exporter_destroy(struct
dpif_ipfix_flow_exporter *exporter)
static bool
dpif_ipfix_flow_exporter_set_options(
struct dpif_ipfix_flow_exporter *exporter,
-const struct ofproto_ipfix_flow_exporter_options *options)
+const struct ofproto_ipfix_flow_exporter_options *options,
+bool *options_changed)
{
-bool options_changed;
-
if (sset_is_empty(>targets)) {
/* No point in doing any work if there are no targets. */
-dpif_ipfix_flow_exporter_clear(exporter);
+if (exporter->options) {
+dpif_ipfix_flow_exporter_clear(exporter);
+*options_changed = true;
+}
return true;
}
-options_changed = (
+*options_changed = (
!exporter->options
|| !ofproto_ipfix_flow_exporter_options_equal(
options, exporter->options));
@@ -1034,7 +1038,7 @@ dpif_ipfix_flow_exporter_set_options(
* shortchanged in collectors (which indicates that opening one or
* more of the configured collectors failed, so that we should
* retry). */
-if (options_changed
+if (*options_changed
|| collectors_count(exporter->exporter.collectors)
< sset_count(>targets)) {
if (!dpif_ipfix_exporter_set_options(
@@ -1046,7 +1050,7 @@ dpif_ipfix_flow_exporter_set_options(
}
/* Avoid reconfiguring if options didn't change. */
-if (!options_changed) {
+if (!*options_changed) {
return true;
}
@@ -1069,7 +1073,7 @@ remove_flow_exporter(struct dpif_ipfix *di,
free(node);
}
-void
+bool
dpif_ipfix_set_options(
struct dpif_ipfix *di,
const struct ofproto_ipfix_bridge_exporter_options
*bridge_exporter_options,
@@ -1077,12 +1081,14 @@ dpif_ipfix_set_options(
size_t n_flow_exporters_options) OVS_EXCLUDED(mutex)
{
int i;
+bool beo_changed, feo_changed, entry_changed;
struct ofproto_ipfix_flow_exporter_options *options;
struct dpif_ipfix_flow_exporter_map_node *node;
ovs_mutex_lock();
dpif_ipfix_bridge_exporter_set_options(>bridge_exporter,
- bridge_exporter_options);
+ bridge_exporter_options,
+ _changed);
/* Add new flow exporters and update current flow exporters. */
options = (struct ofproto_ipfix_flow_exporter_options *)
@@ -1095,10 +1101,14 @@ dpif_ipfix_set_options(
dpif_ipfix_flow_exporter_init(>exporter);
hmap_insert(>flow_exporter_map, >node,
hash_int(options->collector_set_id, 0));
+feo_changed = true;
}
-
[ovs-dev] [PATCH] ofproto-dpif-xlate: remove mirror assert
During the revalidation, mirror could be removed. Instead of crash
the process, we can simply skip the deleted mirror.
Fixes: ec7ceaed4f3e ("ofproto-dpif: Modularize mirror code.")
Signed-off-by: lic121
---
ofproto/ofproto-dpif-xlate.c | 10 +++---
1 file changed, 7 insertions(+), 3 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 5a770f1..33e56c2 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2142,9 +2142,13 @@ mirror_packet(struct xlate_ctx *ctx, struct xbundle
*xbundle,
int snaplen;
/* Get the details of the mirror represented by the rightmost 1-bit. */
-ovs_assert(mirror_get(xbridge->mbridge, raw_ctz(mirrors),
- , _mirrors,
- , , _vlan));
+if (!mirror_get(xbridge->mbridge, raw_ctz(mirrors),
+ , _mirrors,
+ , , _vlan)) {
+/* Mirror could be deleted during revalidation */
+mirrors = zero_rightmost_1bit(mirrors);
+continue;
+}
/* If this mirror selects on the basis of VLAN, and it does not select
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 1/2] ofproto-dpif: trigger revalidate if ct tp changes
Once ct_zone timeout policy changes, revalidator is supposed to be
triggered.
Fixes: 993cae678bca ("ofproto-dpif: Consume CT_Zone, and CT_Timeout_Policy
tables")
Signed-off-by: lic121
---
ofproto/ofproto-dpif.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 6601f23..7a1bb0d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5597,6 +5597,7 @@ ct_set_zone_timeout_policy(const char *datapath_type,
uint16_t zone_id,
ct_timeout_policy_unref(backer, ct_zone->ct_tp);
ct_zone->ct_tp = ct_tp;
ct_tp->ref_count++;
+backer->need_revalidate = REV_RECONFIGURE;
}
} else {
struct ct_zone *new_ct_zone = ct_zone_alloc(zone_id);
@@ -5604,6 +5605,7 @@ ct_set_zone_timeout_policy(const char *datapath_type,
uint16_t zone_id,
cmap_insert(>ct_zones, _ct_zone->node,
hash_int(zone_id, 0));
ct_tp->ref_count++;
+backer->need_revalidate = REV_RECONFIGURE;
}
}
@@ -5620,6 +5622,7 @@ ct_del_zone_timeout_policy(const char *datapath_type,
uint16_t zone_id)
if (ct_zone) {
ct_timeout_policy_unref(backer, ct_zone->ct_tp);
ct_zone_remove_and_destroy(backer, ct_zone);
+backer->need_revalidate = REV_RECONFIGURE;
}
}
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 2/2] ofproto-dpif: avoid unneccesary backer revalidation
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Without this patch, bridge_reconfigure() always trigger udpif
revalidator because of lldp.
This patch also fix lldp memory leak bug:
lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref
is supposed to free the memory, but it doesn't.
Signed-off-by: lic121
Signed-off-by: Eelco Chaudron
Co-authored-by: Eelco Chaudron
---
lib/lldp/lldpd.c | 10 +++---
lib/ovs-lldp.c | 8
lib/ovs-lldp.h | 1 +
ofproto/ofproto-dpif.c | 7 +--
tests/ofproto-dpif.at | 33 +
5 files changed, 50 insertions(+), 9 deletions(-)
diff --git a/lib/lldp/lldpd.c b/lib/lldp/lldpd.c
index 403f1f5..4bff7b0 100644
--- a/lib/lldp/lldpd.c
+++ b/lib/lldp/lldpd.c
@@ -140,13 +140,9 @@ lldpd_cleanup(struct lldpd *cfg)
VLOG_DBG("cleanup all ports");
LIST_FOR_EACH_SAFE (hw, h_entries, >g_hardware) {
-if (!hw->h_flags) {
-ovs_list_remove(>h_entries);
-lldpd_remote_cleanup(hw, NULL, true);
-lldpd_hardware_cleanup(cfg, hw);
-} else {
-lldpd_remote_cleanup(hw, NULL, false);
-}
+ovs_list_remove(>h_entries);
+lldpd_remote_cleanup(hw, NULL, true);
+lldpd_hardware_cleanup(cfg, hw);
}
VLOG_DBG("cleanup all chassis");
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index a9d205e..2d13e97 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet
*packet,
ovs_mutex_unlock();
}
+/* Is LLDP enabled?
+ */
+bool
+lldp_is_enabled(struct lldp *lldp)
+{
+return lldp ? lldp->enabled : false;
+}
+
/* Configures the LLDP stack.
*/
bool
diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
index 0e536e8..661ac4e 100644
--- a/lib/ovs-lldp.h
+++ b/lib/ovs-lldp.h
@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
bool lldp_should_send_packet(struct lldp *cfg);
bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
+bool lldp_is_enabled(struct lldp *lldp);
void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
const struct eth_addr eth_src);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7a1bb0d..3ae44f0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2490,11 +2490,11 @@ set_lldp(struct ofport *ofport_,
{
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+bool old_enable = lldp_is_enabled(ofport->lldp);
int error = 0;
-if (cfg) {
+if (cfg && !smap_is_empty(cfg)) {
if (!ofport->lldp) {
-ofproto->backer->need_revalidate = REV_RECONFIGURE;
ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
}
@@ -2506,6 +2506,9 @@ set_lldp(struct ofport *ofport_,
} else if (ofport->lldp) {
lldp_unref(ofport->lldp);
ofport->lldp = NULL;
+}
+
+if (lldp_is_enabled(ofport->lldp) != old_enable) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index dbb3b6d..1c9a94c 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
+OVS_VSWITCHD_START(
+[add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
+)
+dnl first revalidation triggered by add interface
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+1
+])
+
+dnl enable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+2
+])
+
+dnl disable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+dnl remove lldp, no revalidation as lldp was disabled
+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH 0/2] fix revalidation triggers
This series fix two revalidation triggers: - trigger revalidation if ct_zone timeout policy changes - avoid revalidation if lldp config doesn't change The second patch relies on the first one because of ct test cases lic121 (2): ofproto-dpif: trigger revalidate if ct tp changes ofproto-dpif: avoid unneccesary backer revalidation lib/lldp/lldpd.c | 10 +++--- lib/ovs-lldp.c | 8 lib/ovs-lldp.h | 1 + ofproto/ofproto-dpif.c | 10 -- tests/ofproto-dpif.at | 33 + 5 files changed, 53 insertions(+), 9 deletions(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 2/2] ofproto-dpif: avoid unneccesary backer revalidation
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Without this patch, bridge_reconfigure() always trigger udpif
revalidator because of lldp.
This patch also fix lldp memory leak bug:
lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref
is supposed to free the memory, but it doesn't.
Signed-off-by: lic121
Signed-off-by: Eelco Chaudron
Co-authored-by: Eelco Chaudron
---
lib/lldp/lldpd.c | 10 +++---
lib/ovs-lldp.c | 8
lib/ovs-lldp.h | 1 +
ofproto/ofproto-dpif.c | 7 +--
tests/ofproto-dpif.at | 33 +
5 files changed, 50 insertions(+), 9 deletions(-)
diff --git a/lib/lldp/lldpd.c b/lib/lldp/lldpd.c
index a024dc5..ee1051d 100644
--- a/lib/lldp/lldpd.c
+++ b/lib/lldp/lldpd.c
@@ -140,13 +140,9 @@ lldpd_cleanup(struct lldpd *cfg)
VLOG_DBG("cleanup all ports");
LIST_FOR_EACH_SAFE (hw, hw_next, h_entries, >g_hardware) {
-if (!hw->h_flags) {
-ovs_list_remove(>h_entries);
-lldpd_remote_cleanup(hw, NULL, true);
-lldpd_hardware_cleanup(cfg, hw);
-} else {
-lldpd_remote_cleanup(hw, NULL, false);
-}
+ovs_list_remove(>h_entries);
+lldpd_remote_cleanup(hw, NULL, true);
+lldpd_hardware_cleanup(cfg, hw);
}
VLOG_DBG("cleanup all chassis");
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index 162311f..bfc69a3 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet
*packet,
ovs_mutex_unlock();
}
+/* Is LLDP enabled?
+ */
+bool
+lldp_is_enabled(struct lldp *lldp)
+{
+return lldp ? lldp->enabled : false;
+}
+
/* Configures the LLDP stack.
*/
bool
diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
index 0e536e8..661ac4e 100644
--- a/lib/ovs-lldp.h
+++ b/lib/ovs-lldp.h
@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
bool lldp_should_send_packet(struct lldp *cfg);
bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
+bool lldp_is_enabled(struct lldp *lldp);
void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
const struct eth_addr eth_src);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ced67b0..a580fd4 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2499,11 +2499,11 @@ set_lldp(struct ofport *ofport_,
{
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+bool old_enable = lldp_is_enabled(ofport->lldp);
int error = 0;
-if (cfg) {
+if (cfg && !smap_is_empty(cfg)) {
if (!ofport->lldp) {
-ofproto->backer->need_revalidate = REV_RECONFIGURE;
ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
}
@@ -2515,6 +2515,9 @@ set_lldp(struct ofport *ofport_,
} else if (ofport->lldp) {
lldp_unref(ofport->lldp);
ofport->lldp = NULL;
+}
+
+if (lldp_is_enabled(ofport->lldp) != old_enable) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 7f273e5..edb6aa1 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
+OVS_VSWITCHD_START(
+[add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
+)
+dnl first revalidation triggered by add interface
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+1
+])
+
+dnl enable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+2
+])
+
+dnl disable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+dnl remove lldp, no revalidation as lldp was disabled
+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 1/2] ofproto-dpif: trigger revalidation when ipfix config set
Currently, ipfix conf creation/deletion don't trigger dpif backer
revalidation. This is not expected, as we need the revalidation
to commit ipfix into xlate. So that xlate can generate ipfix
actions.
This patch covers only new creation/deletion of ipfix config.
Will upload one more patch to cover ipfix option changes.
Signed-off-by: lic121
---
ofproto/ofproto-dpif.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8143dd9..ced67b0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2371,6 +2371,12 @@ set_ipfix(
dpif_ipfix_unref(di);
ofproto->ipfix = NULL;
}
+
+/* TODO: need to consider ipfix option changes more than
+ * enable/disable */
+if (new_di || !ofproto->ipfix) {
+ofproto->backer->need_revalidate = REV_RECONFIGURE;
+}
}
return 0;
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v5 0/2] fix dpif backer revalidation
v5: - merge patch #2 and patch #3 to retrigger CI, as it could be failed by anccident. v4: - fix lldp memory leak - add '!smap_is_empty(cfg)' condition v3: - fix lldp test case failure v2: - add TODO comments to clear ipfix patch coverage - test lldp enabling more than pointer - add test cases for lldp ovsdb change or netlink notification trigger bridge_reconfigure. In bridge_reconfigure, backer->need_revalidate flag is set if backer revalidation is needed. This series fix two places where need_revalidate flag is not set correctly. lic121 (2): ofproto-dpif: trigger revalidation when ipfix config set ofproto-dpif: avoid unneccesary backer revalidation lib/lldp/lldpd.c | 10 +++--- lib/ovs-lldp.c | 8 lib/ovs-lldp.h | 1 + ofproto/ofproto-dpif.c | 13 +++-- tests/ofproto-dpif.at | 33 + 5 files changed, 56 insertions(+), 9 deletions(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 2/2] ofproto-dpif: avoid unneccesary backer revalidation
>On 6 Feb 2022, at 4:58, lic121 wrote:
>
>>> If lldp didn't change, we are not supposed to trigger backer
>>> revalidation.
>>>
>>> Without this patch, bridge_reconfigure() always trigger udpif
>>> revalidator because of lldp.
>>>
>>> Signed-off-by: lic121
>>> Co-authored-by: Eelco Chaudron
>>> ---
>>> lib/ovs-lldp.c | 8
>>> lib/ovs-lldp.h | 1 +
>>> ofproto/ofproto-dpif.c | 5 -
>>> tests/ofproto-dpif.at | 33 +
>>> 4 files changed, 46 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
>>> index 162311f..bfc69a3 100644
>>> --- a/lib/ovs-lldp.c
>>> +++ b/lib/ovs-lldp.c
>>> @@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet
>>> *packet,
>>> ovs_mutex_unlock();
>>> }
>>>
>>> +/* Is LLDP enabled?
>>> + */
>>> +bool
>>> +lldp_is_enabled(struct lldp *lldp)
>>> +{
>>> +return lldp ? lldp->enabled : false;
>>> +}
>>> +
>>> /* Configures the LLDP stack.
>>> */
>>> bool
>>> diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
>>> index 0e536e8..661ac4e 100644
>>> --- a/lib/ovs-lldp.h
>>> +++ b/lib/ovs-lldp.h
>>> @@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
>>> bool lldp_should_send_packet(struct lldp *cfg);
>>> bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
>>> bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
>>> +bool lldp_is_enabled(struct lldp *lldp);
>>> void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
>>> void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
>>> const struct eth_addr eth_src);
>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>> index ced67b0..c2f5b12 100644
>>> --- a/ofproto/ofproto-dpif.c
>>> +++ b/ofproto/ofproto-dpif.c
>>> @@ -2499,11 +2499,11 @@ set_lldp(struct ofport *ofport_,
>>> {
>>> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
>>> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>>> +bool old_enable = lldp_is_enabled(ofport->lldp);
>>> int error = 0;
>>>
>>> if (cfg) {
>>> if (!ofport->lldp) {
>>> -ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>> ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu,
>>> cfg);
>>> }
>>>
>>> @@ -2515,6 +2515,9 @@ set_lldp(struct ofport *ofport_,
>>> } else if (ofport->lldp) {
>>> lldp_unref(ofport->lldp);
>>> ofport->lldp = NULL;
>>> +}
>>> +
>>> +if (lldp_is_enabled(ofport->lldp) != old_enable) {
>>> ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>> }
>>>
>>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>>> index 7f273e5..edb6aa1 100644
>>> --- a/tests/ofproto-dpif.at
>>> +++ b/tests/ofproto-dpif.at
>>> @@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
>>> OVS_VSWITCHD_STOP
>>> AT_CLEANUP
>>>
>>> +AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
>>> +OVS_VSWITCHD_START(
>>> +[add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
>>> +)
>>> +dnl first revalidation triggered by add interface
>>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>>> +1
>>> +])
>>> +
>>> +dnl enable lldp
>>> +AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
>>
>> CI test shows that setting lldp cause memory leaks, will try to find out the
>> places.
>
>
>Ack, I’ll wait for reviewing once you found/fix it.
v4 is uploaded
>
>PS: You can add me as a co-author and sign off in your next iteration for the
>changes I made.
>
Sure, please let me know if I haven't done it in the right way.
>>> +AT_CHECK([ovs-appctl revalidator/wait])
>>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>>> +2
>>> +])
>>> +
>>> +dnl disable lldp
>>> +AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
>>> +AT_CHECK([ovs-appctl revalidator/wait])
>>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>>> +3
>>> +])
>>> +
>>> +dnl remove lldp, no revalidation as lldp was disabled
>>> +AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
>>> +AT_CHECK([ovs-appctl revalidator/wait])
>>> +AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>>> +3
>>> +])
>>> +
>>> +OVS_VSWITCHD_STOP
>>> +AT_CLEANUP
>>> +
>>> AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
>>>
>>> dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
>>> --
>>> 1.8.3.1
>>>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 3/3] ofproto-dpif: avoid unneccesary backer revalidation
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Without this patch, bridge_reconfigure() always trigger udpif
revalidator because of lldp.
Signed-off-by: lic121
Signed-off-by: Eelco Chaudron
Co-authored-by: Eelco Chaudron
---
lib/ovs-lldp.c | 8
lib/ovs-lldp.h | 1 +
ofproto/ofproto-dpif.c | 7 +--
tests/ofproto-dpif.at | 33 +
4 files changed, 47 insertions(+), 2 deletions(-)
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index 162311f..bfc69a3 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet
*packet,
ovs_mutex_unlock();
}
+/* Is LLDP enabled?
+ */
+bool
+lldp_is_enabled(struct lldp *lldp)
+{
+return lldp ? lldp->enabled : false;
+}
+
/* Configures the LLDP stack.
*/
bool
diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
index 0e536e8..661ac4e 100644
--- a/lib/ovs-lldp.h
+++ b/lib/ovs-lldp.h
@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
bool lldp_should_send_packet(struct lldp *cfg);
bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
+bool lldp_is_enabled(struct lldp *lldp);
void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
const struct eth_addr eth_src);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ced67b0..a580fd4 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2499,11 +2499,11 @@ set_lldp(struct ofport *ofport_,
{
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+bool old_enable = lldp_is_enabled(ofport->lldp);
int error = 0;
-if (cfg) {
+if (cfg && !smap_is_empty(cfg)) {
if (!ofport->lldp) {
-ofproto->backer->need_revalidate = REV_RECONFIGURE;
ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
}
@@ -2515,6 +2515,9 @@ set_lldp(struct ofport *ofport_,
} else if (ofport->lldp) {
lldp_unref(ofport->lldp);
ofport->lldp = NULL;
+}
+
+if (lldp_is_enabled(ofport->lldp) != old_enable) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 7f273e5..edb6aa1 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
+OVS_VSWITCHD_START(
+[add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
+)
+dnl first revalidation triggered by add interface
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+1
+])
+
+dnl enable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+2
+])
+
+dnl disable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+dnl remove lldp, no revalidation as lldp was disabled
+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 2/3] lldp: fix memory leak
lldp_create() malloc memory for lldp->lldpd->g_hardware. lldp_unref
is supposed to free the memory, but it doesn't.
Signed-off-by: lic121
---
lib/lldp/lldpd.c | 10 +++---
1 file changed, 3 insertions(+), 7 deletions(-)
diff --git a/lib/lldp/lldpd.c b/lib/lldp/lldpd.c
index a024dc5..ee1051d 100644
--- a/lib/lldp/lldpd.c
+++ b/lib/lldp/lldpd.c
@@ -140,13 +140,9 @@ lldpd_cleanup(struct lldpd *cfg)
VLOG_DBG("cleanup all ports");
LIST_FOR_EACH_SAFE (hw, hw_next, h_entries, >g_hardware) {
-if (!hw->h_flags) {
-ovs_list_remove(>h_entries);
-lldpd_remote_cleanup(hw, NULL, true);
-lldpd_hardware_cleanup(cfg, hw);
-} else {
-lldpd_remote_cleanup(hw, NULL, false);
-}
+ovs_list_remove(>h_entries);
+lldpd_remote_cleanup(hw, NULL, true);
+lldpd_hardware_cleanup(cfg, hw);
}
VLOG_DBG("cleanup all chassis");
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 1/3] ofproto-dpif: trigger revalidation when ipfix config set
Currently, ipfix conf creation/deletion don't trigger dpif backer
revalidation. This is not expected, as we need the revalidation
to commit ipfix into xlate. So that xlate can generate ipfix
actions.
This patch covers only new creation/deletion of ipfix config.
Will upload one more patch to cover ipfix option changes.
Signed-off-by: lic121
---
ofproto/ofproto-dpif.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8143dd9..ced67b0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2371,6 +2371,12 @@ set_ipfix(
dpif_ipfix_unref(di);
ofproto->ipfix = NULL;
}
+
+/* TODO: need to consider ipfix option changes more than
+ * enable/disable */
+if (new_di || !ofproto->ipfix) {
+ofproto->backer->need_revalidate = REV_RECONFIGURE;
+}
}
return 0;
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 0/3] fix dpif backer revalidation
v4: - one patch is added to fix lldp memory leak - add '!smap_is_empty(cfg)' condition v3: - fix lldp test case failure v2: - add TODO comments to clear ipfix patch coverage - test lldp enabling more than pointer - add test cases for lldp ovsdb change or netlink notification trigger bridge_reconfigure. In bridge_reconfigure, backer->need_revalidate flag is set if backer revalidation is needed. This series fix two places where need_revalidate flag is not set correctly. lic121 (3): ofproto-dpif: trigger revalidation when ipfix config set lldp: fix memory leak ofproto-dpif: avoid unneccesary backer revalidation lib/lldp/lldpd.c | 10 +++--- lib/ovs-lldp.c | 8 lib/ovs-lldp.h | 1 + ofproto/ofproto-dpif.c | 13 +++-- tests/ofproto-dpif.at | 33 + 5 files changed, 56 insertions(+), 9 deletions(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 2/2] ofproto-dpif: avoid unneccesary backer revalidation
>If lldp didn't change, we are not supposed to trigger backer
>revalidation.
>
>Without this patch, bridge_reconfigure() always trigger udpif
>revalidator because of lldp.
>
>Signed-off-by: lic121
>Co-authored-by: Eelco Chaudron
>---
> lib/ovs-lldp.c | 8
> lib/ovs-lldp.h | 1 +
> ofproto/ofproto-dpif.c | 5 -
> tests/ofproto-dpif.at | 33 +
> 4 files changed, 46 insertions(+), 1 deletion(-)
>
>diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
>index 162311f..bfc69a3 100644
>--- a/lib/ovs-lldp.c
>+++ b/lib/ovs-lldp.c
>@@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet
>*packet,
> ovs_mutex_unlock();
> }
>
>+/* Is LLDP enabled?
>+ */
>+bool
>+lldp_is_enabled(struct lldp *lldp)
>+{
>+return lldp ? lldp->enabled : false;
>+}
>+
> /* Configures the LLDP stack.
> */
> bool
>diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
>index 0e536e8..661ac4e 100644
>--- a/lib/ovs-lldp.h
>+++ b/lib/ovs-lldp.h
>@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
> bool lldp_should_send_packet(struct lldp *cfg);
> bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
> bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
>+bool lldp_is_enabled(struct lldp *lldp);
> void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
> void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
> const struct eth_addr eth_src);
>diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>index ced67b0..c2f5b12 100644
>--- a/ofproto/ofproto-dpif.c
>+++ b/ofproto/ofproto-dpif.c
>@@ -2499,11 +2499,11 @@ set_lldp(struct ofport *ofport_,
> {
> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>+bool old_enable = lldp_is_enabled(ofport->lldp);
> int error = 0;
>
> if (cfg) {
> if (!ofport->lldp) {
>-ofproto->backer->need_revalidate = REV_RECONFIGURE;
> ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
> }
>
>@@ -2515,6 +2515,9 @@ set_lldp(struct ofport *ofport_,
> } else if (ofport->lldp) {
> lldp_unref(ofport->lldp);
> ofport->lldp = NULL;
>+}
>+
>+if (lldp_is_enabled(ofport->lldp) != old_enable) {
> ofproto->backer->need_revalidate = REV_RECONFIGURE;
> }
>
>diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>index 7f273e5..edb6aa1 100644
>--- a/tests/ofproto-dpif.at
>+++ b/tests/ofproto-dpif.at
>@@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
> OVS_VSWITCHD_STOP
> AT_CLEANUP
>
>+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
>+OVS_VSWITCHD_START(
>+[add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
>+)
>+dnl first revalidation triggered by add interface
>+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>+1
>+])
>+
>+dnl enable lldp
>+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
CI test shows that setting lldp cause memory leaks, will try to find out the
places.
>+AT_CHECK([ovs-appctl revalidator/wait])
>+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>+2
>+])
>+
>+dnl disable lldp
>+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
>+AT_CHECK([ovs-appctl revalidator/wait])
>+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>+3
>+])
>+
>+dnl remove lldp, no revalidation as lldp was disabled
>+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
>+AT_CHECK([ovs-appctl revalidator/wait])
>+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>+3
>+])
>+
>+OVS_VSWITCHD_STOP
>+AT_CLEANUP
>+
> AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
>
> dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
>--
>1.8.3.1
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 2/2] ofproto-dpif: avoid unneccesary backer revalidation
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Without this patch, bridge_reconfigure() always trigger udpif
revalidator because of lldp.
Signed-off-by: lic121
Co-authored-by: Eelco Chaudron
---
lib/ovs-lldp.c | 8
lib/ovs-lldp.h | 1 +
ofproto/ofproto-dpif.c | 5 -
tests/ofproto-dpif.at | 33 +
4 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index 162311f..bfc69a3 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet
*packet,
ovs_mutex_unlock();
}
+/* Is LLDP enabled?
+ */
+bool
+lldp_is_enabled(struct lldp *lldp)
+{
+return lldp ? lldp->enabled : false;
+}
+
/* Configures the LLDP stack.
*/
bool
diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
index 0e536e8..661ac4e 100644
--- a/lib/ovs-lldp.h
+++ b/lib/ovs-lldp.h
@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
bool lldp_should_send_packet(struct lldp *cfg);
bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
+bool lldp_is_enabled(struct lldp *lldp);
void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
const struct eth_addr eth_src);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index ced67b0..c2f5b12 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2499,11 +2499,11 @@ set_lldp(struct ofport *ofport_,
{
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+bool old_enable = lldp_is_enabled(ofport->lldp);
int error = 0;
if (cfg) {
if (!ofport->lldp) {
-ofproto->backer->need_revalidate = REV_RECONFIGURE;
ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
}
@@ -2515,6 +2515,9 @@ set_lldp(struct ofport *ofport_,
} else if (ofport->lldp) {
lldp_unref(ofport->lldp);
ofport->lldp = NULL;
+}
+
+if (lldp_is_enabled(ofport->lldp) != old_enable) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 7f273e5..edb6aa1 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
+OVS_VSWITCHD_START(
+[add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
+)
+dnl first revalidation triggered by add interface
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+1
+])
+
+dnl enable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+2
+])
+
+dnl disable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+dnl remove lldp, no revalidation as lldp was disabled
+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 1/2] ofproto-dpif: trigger revalidation when ipfix config set
Currently, ipfix conf creation/deletion don't trigger dpif backer
revalidation. This is not expected, as we need the revalidation
to commit ipfix into xlate. So that xlate can generate ipfix
actions.
This patch covers only new creation/deletion of ipfix config.
Will upload one more patch to cover ipfix option changes.
Signed-off-by: lic121
---
ofproto/ofproto-dpif.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 8143dd9..ced67b0 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2371,6 +2371,12 @@ set_ipfix(
dpif_ipfix_unref(di);
ofproto->ipfix = NULL;
}
+
+/* TODO: need to consider ipfix option changes more than
+ * enable/disable */
+if (new_di || !ofproto->ipfix) {
+ofproto->backer->need_revalidate = REV_RECONFIGURE;
+}
}
return 0;
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 0/2] fix dpif backer revalidation
v3: - fix lldp test case failure v2: - add TODO comments to clear ipfix patch coverage - test lldp enabling more than pointer - add test cases for lldp ovsdb change or netlink notification trigger bridge_reconfigure. In bridge_reconfigure, backer->need_revalidate flag is set if backer revalidation is needed. This series fix two places where need_revalidate flag is not set correctly. lic121 (2): ofproto-dpif: trigger revalidation when ipfix config set ofproto-dpif: avoid unneccesary backer revalidation lib/ovs-lldp.c | 8 lib/ovs-lldp.h | 1 + ofproto/ofproto-dpif.c | 11 ++- tests/ofproto-dpif.at | 33 + 4 files changed, 52 insertions(+), 1 deletion(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/2] ofproto-dpif: trigger revalidation when ipfix config set
>On 4 Feb 2022, at 15:25, lic121 wrote:
>
>>> On 28 Jan 2022, at 7:40, lic121 wrote:
>>>
>>>> Currently, ipfix conf creation/deletion don't trigger dpif backer
>>>> revalidation. This is not expected, as we need the revalidation
>>>> to commit ipfix into xlate. So that xlate can generate ipfix
>>>> actions.
>>>>
>>>> This patch covers only new creation/deletion of ipfix config.
>>>> Will upload one more patch to cover ipfix option changes.
>>>
>>> I think you could drop this patch from the series until you have the full
>>> fix, but I let the maintainers decide how they would like to handle this.
>>
>> Without this patch, two ipfix test cases fails with my second patch.
>
>Which test cases are they, as I see no newly introduced test cases for ipfix?
They are not new test cases: "Bridge IPFIX sanity check" and "Bridge IPFIX
statistics check".
I copied the first test case failure log:
## test log #
--- - 2022-02-05 02:07:11.901471095 +
+++ /root/ovs/tests/testsuite.dir/at-groups/1190/stdout 2022-02-05
02:07:11.899001638 +
@@ -1,3 +1,3 @@
flow-dump from the main thread:
-packets:2, bytes:68, used:0.001s,
actions:userspace(pid=0,ipfix(output_port=4294967295))
+packets:2, bytes:68, used:0.001s, actions:drop
## log end #
It fails because ipfix creating doesn't trigger revalidator. So no ipfix action
is generated.
It didn't fail becase lldp always trigger revalidator in bridge_reconfigure()
even if lldp config is empty
In my second patch, lldp issue is fixed, which avoid unneccesary revalidator
triggered by lldp.
>
>> With my second patch, revalidator is not triggerd when no lldp changes.
>> As the result, ipfix config changes is not commited into xlate layer,
>> because ipfix doesn't trigger revalidator by itself.
>>
>>>
>>> Ilya/Ian/William?
>>>
>>>> Signed-off-by: lic121
>>>> ---
>>>> ofproto/ofproto-dpif.c | 6 ++
>>>> 1 file changed, 6 insertions(+)
>>>>
>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>> index bc3df8e..5737615 100644
>>>> --- a/ofproto/ofproto-dpif.c
>>>> +++ b/ofproto/ofproto-dpif.c
>>>> @@ -2333,6 +2333,12 @@ set_ipfix(
>>>> dpif_ipfix_unref(di);
>>>> ofproto->ipfix = NULL;
>>>> }
>>>> +
>>>> +/* TODO: need to test ipfix option changes more than
>>>> + * enable/disable */
>>>> +if (new_di || !ofproto->ipfix) {
>>>> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>>> +}
>>>> }
>>>>
>>>> return 0;
>>>> --
>>>> 1.8.3.1
>>>>
>>>> ___
>>>> dev mailing list
>>>> d...@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 1/2] ofproto-dpif: trigger revalidation when ipfix config set
>On 28 Jan 2022, at 7:40, lic121 wrote:
>
>> Currently, ipfix conf creation/deletion don't trigger dpif backer
>> revalidation. This is not expected, as we need the revalidation
>> to commit ipfix into xlate. So that xlate can generate ipfix
>> actions.
>>
>> This patch covers only new creation/deletion of ipfix config.
>> Will upload one more patch to cover ipfix option changes.
>
>I think you could drop this patch from the series until you have the full fix,
>but I let the maintainers decide how they would like to handle this.
Without this patch, two ipfix test cases fails with my second patch.
With my second patch, revalidator is not triggerd when no lldp changes.
As the result, ipfix config changes is not commited into xlate layer, because
ipfix doesn't trigger revalidator by itself.
>
>Ilya/Ian/William?
>
>> Signed-off-by: lic121
>> ---
>> ofproto/ofproto-dpif.c | 6 ++
>> 1 file changed, 6 insertions(+)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index bc3df8e..5737615 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -2333,6 +2333,12 @@ set_ipfix(
>> dpif_ipfix_unref(di);
>> ofproto->ipfix = NULL;
>> }
>> +
>> +/* TODO: need to test ipfix option changes more than
>> + * enable/disable */
>> +if (new_di || !ofproto->ipfix) {
>> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> +}
>> }
>>
>> return 0;
>> --
>> 1.8.3.1
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v2 2/2] ofproto-dpif: avoid unneccesary backer revalidation
>On 28 Jan 2022, at 7:41, lic121 wrote:
>
>> If lldp didn't change, we are not supposed to trigger backer
>> revalidation.
>>
>> Without this patch, bridge_reconfigure() always trigger udpif
>> revalidator because of lldp.
>>
>> Signed-off-by: lic121
>> ---
>> lib/ovs-lldp.c | 8
>> lib/ovs-lldp.h | 1 +
>> ofproto/ofproto-dpif.c | 5 -
>> tests/ofproto-dpif.at | 33 +
>> 4 files changed, 46 insertions(+), 1 deletion(-)
>
>The patch itself looks good to me, however the selftest is failing each time
>on the second case:
>
>./ofproto-dpif.at:37: ovs-appctl coverage/read-counter rev_reconfigure
>./ofproto-dpif.at:42: ovs-vsctl set interface p1 lldp:enable=yes
>./ofproto-dpif.at:44: ovs-appctl revalidator/wait
>./ofproto-dpif.at:45: ovs-appctl coverage/read-counter rev_reconfigure
>--- - 2022-02-04 13:12:54.116010047 +0100
>+++
>/home/echaudron/Documents/review/ovs_lic_trigger_revalidation/tests/testsuite.dir/at-groups/1036/stdout
>2022-02-04 13:12:54.114089445 +0100
>@@ -1,2 +1,2 @@
>-2
>+1
>
>I even tried to add a 5-second delay, but same problem:
Hi Eelco, very appreciate your careful review of this patch. About the test, I
am very sure I had run the test and passed at my local environment.
But after my local test before I upload the test, I made a rebase on the
upstream master. I will update the patch to make sure it works.
>
>dnl enable lldp
>AT_CHECK([ovs-vsctl set interface p1 lldp:enable=yes])
>sleep 5
>AT_CHECK([ovs-appctl revalidator/wait])
>AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
>2
>])
>
>
>So it made me wonder if you ever tested your patch? Especially as I figured
>out the script was wrong.
>Here is the diff to make it work:
>
>diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>index 1ba0aac85..7de27fe7c 100644
>--- a/tests/ofproto-dpif.at
>+++ b/tests/ofproto-dpif.at
>@@ -39,14 +39,14 @@ AT_CHECK([ovs-appctl coverage/read-counter
>rev_reconfigure], [0], [dnl
> ])
>
> dnl enable lldp
>-AT_CHECK([ovs-vsctl set interface p1 lldp:enable=yes])
>+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=true])
> AT_CHECK([ovs-appctl revalidator/wait])
> AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
> 2
> ])
>
> dnl disable lldp
>-AT_CHECK([ovs-vsctl set interface p1 lldp:enable=no])
>+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=false])
> AT_CHECK([ovs-appctl revalidator/wait])
> AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
> 3
>
>
>Guess you need a v3 of this.
>
>>
>> diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
>> index 162311f..bfc69a3 100644
>> --- a/lib/ovs-lldp.c
>> +++ b/lib/ovs-lldp.c
>> @@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet
>> *packet,
>> ovs_mutex_unlock();
>> }
>>
>> +/* Is LLDP enabled?
>> + */
>> +bool
>> +lldp_is_enabled(struct lldp *lldp)
>> +{
>> +return lldp ? lldp->enabled : false;
>> +}
>> +
>> /* Configures the LLDP stack.
>> */
>> bool
>> diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
>> index 0e536e8..661ac4e 100644
>> --- a/lib/ovs-lldp.h
>> +++ b/lib/ovs-lldp.h
>> @@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
>> bool lldp_should_send_packet(struct lldp *cfg);
>> bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
>> bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
>> +bool lldp_is_enabled(struct lldp *lldp);
>> void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
>> void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
>> const struct eth_addr eth_src);
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 5737615..c3c1922 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -2461,11 +2461,11 @@ set_lldp(struct ofport *ofport_,
>> {
>> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
>> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>> +bool old_enable = lldp_is_enabled(ofport->lldp);
>> int error = 0;
>>
>> if (cfg) {
>> if (!ofport->lldp) {
>> -ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu,
>> cfg);
>> }
>>
>> @@ -2477,6 +2477,9 @@ set_lldp(struct ofport *ofport_,
>> }
[ovs-dev] [PATCH v2 2/2] ofproto-dpif: avoid unneccesary backer revalidation
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Without this patch, bridge_reconfigure() always trigger udpif
revalidator because of lldp.
Signed-off-by: lic121
---
lib/ovs-lldp.c | 8
lib/ovs-lldp.h | 1 +
ofproto/ofproto-dpif.c | 5 -
tests/ofproto-dpif.at | 33 +
4 files changed, 46 insertions(+), 1 deletion(-)
diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
index 162311f..bfc69a3 100644
--- a/lib/ovs-lldp.c
+++ b/lib/ovs-lldp.c
@@ -738,6 +738,14 @@ lldp_put_packet(struct lldp *lldp, struct dp_packet
*packet,
ovs_mutex_unlock();
}
+/* Is LLDP enabled?
+ */
+bool
+lldp_is_enabled(struct lldp *lldp)
+{
+return lldp ? lldp->enabled : false;
+}
+
/* Configures the LLDP stack.
*/
bool
diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
index 0e536e8..661ac4e 100644
--- a/lib/ovs-lldp.h
+++ b/lib/ovs-lldp.h
@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
bool lldp_should_send_packet(struct lldp *cfg);
bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
+bool lldp_is_enabled(struct lldp *lldp);
void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
const struct eth_addr eth_src);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 5737615..c3c1922 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2461,11 +2461,11 @@ set_lldp(struct ofport *ofport_,
{
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
+bool old_enable = lldp_is_enabled(ofport->lldp);
int error = 0;
if (cfg) {
if (!ofport->lldp) {
-ofproto->backer->need_revalidate = REV_RECONFIGURE;
ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
}
@@ -2477,6 +2477,9 @@ set_lldp(struct ofport *ofport_,
} else if (ofport->lldp) {
lldp_unref(ofport->lldp);
ofport->lldp = NULL;
+}
+
+if (lldp_is_enabled(ofport->lldp) != old_enable) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 1660b08..1ba0aac 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -29,6 +29,39 @@ AT_CHECK([ovs-appctl revalidator/wait])
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - lldp revalidator event(REV_RECONFIGURE)])
+OVS_VSWITCHD_START(
+[add-port br0 p1 -- set interface p1 ofport_request=1 type=dummy]
+)
+dnl first revalidation triggered by add interface
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+1
+])
+
+dnl enable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=yes])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+2
+])
+
+dnl disable lldp
+AT_CHECK([ovs-vsctl set interface p1 lldp:enable=no])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+dnl remove lldp
+AT_CHECK([ovs-vsctl remove interface p1 lldp enable])
+AT_CHECK([ovs-appctl revalidator/wait])
+AT_CHECK([ovs-appctl coverage/read-counter rev_reconfigure], [0], [dnl
+3
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - active-backup bonding (with primary)])
dnl Create br0 with members p1, p2 and p7, creating bond0 with p1 and
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2 1/2] ofproto-dpif: trigger revalidation when ipfix config set
Currently, ipfix conf creation/deletion don't trigger dpif backer
revalidation. This is not expected, as we need the revalidation
to commit ipfix into xlate. So that xlate can generate ipfix
actions.
This patch covers only new creation/deletion of ipfix config.
Will upload one more patch to cover ipfix option changes.
Signed-off-by: lic121
---
ofproto/ofproto-dpif.c | 6 ++
1 file changed, 6 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index bc3df8e..5737615 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2333,6 +2333,12 @@ set_ipfix(
dpif_ipfix_unref(di);
ofproto->ipfix = NULL;
}
+
+/* TODO: need to test ipfix option changes more than
+ * enable/disable */
+if (new_di || !ofproto->ipfix) {
+ofproto->backer->need_revalidate = REV_RECONFIGURE;
+}
}
return 0;
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v2 0/2] fix dpif backer revalidation
v2: - add TODO comments to clear ipfix patch coverage - test lldp enabling more than pointer - add test cases for lldp ovsdb change or netlink notification trigger bridge_reconfigure. In bridge_reconfigure, backer->need_revalidate flag is set if backer revalidation is needed. This series fix two places where need_revalidate flag is not set correctly. lic121 (2): ofproto-dpif: trigger revalidation when ipfix config set ofproto-dpif: avoid unneccesary backer revalidation lib/ovs-lldp.c | 8 lib/ovs-lldp.h | 1 + ofproto/ofproto-dpif.c | 11 ++- tests/ofproto-dpif.at | 33 + 4 files changed, 52 insertions(+), 1 deletion(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] fix dpif backer revalidation
v2: - add TODO comments to clear ipfix patch coverage - test lldp enabling more than pointer - add test cases for lldp ovsdb change or netlink notification trigger bridge_reconfigure. In bridge_reconfigure, backer->need_revalidate flag is set if backer revalidation is needed. This series fix two places where need_revalidate flag is not set correctly. lic121 (2): ofproto-dpif: trigger revalidation when ipfix config set ofproto-dpif: avoid unneccesary backer revalidation lib/ovs-lldp.c | 8 lib/ovs-lldp.h | 1 + ofproto/ofproto-dpif.c | 11 ++- tests/ofproto-dpif.at | 33 + 4 files changed, 52 insertions(+), 1 deletion(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 1/2] ofproto-dpif: trigger revalidation when ipfix changes
>
>
>On 14 Jan 2022, at 10:38, lic121 wrote:
>
>>>
>>>
>>> On 14 Jan 2022, at 9:58, lic121 wrote:
>>>
>>>>>
>>>>>
>>>>> On 9 Jan 2022, at 14:44, lic121 wrote:
>>>>>
>>>>>> Currently, ipfix creation/delection don't trigger dpif backer
>>>>>> revalidation. This is not expected, as we need the revalidation
>>>>>> to commit ipfix into xlate. So that xlate can generate ipfix
>>>>>> actions.
>>>>>>
>>>>>> Signed-off-by: lic121
>>>>>> ---
>>>>>> ofproto/ofproto-dpif.c | 5 +
>>>>>> 1 file changed, 5 insertions(+)
>>>>>>
>>>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>>>> index bc3df8e..1cdef18 100644
>>>>>> --- a/ofproto/ofproto-dpif.c
>>>>>> +++ b/ofproto/ofproto-dpif.c
>>>>>> @@ -2306,6 +2306,7 @@ set_ipfix(
>>>>>> {
>>>>>> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>>>>>> struct dpif_ipfix *di = ofproto->ipfix;
>>>>>> +struct dpif_ipfix *old_ipfix = ofproto->ipfix;
>>>>>> bool has_options = bridge_exporter_options ||
>>>>>> flow_exporters_options;
>>>>>> bool new_di = false;
>>>>>>
>>>>>> @@ -2335,6 +2336,10 @@ set_ipfix(
>>>>>> }
>>>>>> }
>>>>>>
>>>>>> +if (old_ipfix != ofproto->ipfix) {
>>>>>
>>>>> This only works if ipfix was not configured earlier or disabled, i.e.,
>>>>> ofproto->ipfix was/is NULL.
>>>>> If this was your intention, you could just have done “if (new_di ||
>>>>> !ofproto->ipfix)”.
>>>>>
>>>>> However, I think there can also be changed in the configuration that
>>>>> requires a revalidate, what do you think? For example, enabling/disabling
>>>>> ingress/egress sampling.
>>>>> In this case dpif_ipfix_set_options() can be changed so it will return
>>>>> true if any configuration changes.
>>>>>
>>>> Actually, I had ever thought the same thing as you. But at last I didn't
>>>> do as that, for 3 reasons:
>>>> 1. I checked the history commit of ipfix, seems its not active in last 2-3
>>>> years. So I guess not so many ovs users are using ipfix feature.
>>>> 2. In xlate_xbridge_set, the place where checks the change flag, it
>>>> doesn't check the ipfix options changes as well.
>>>>
>>>> https://github.com/openvswitch/ovs/blob/eb1ab5357b6f3755f0ef1ee6d341ce24398d3bc1/ofproto/ofproto-dpif-xlate.c#L975-L978
>>>
>>> But here it just set the pointer as a reference, it does not take care of
>>> acting on configuration changes, or do I miss something?
>>>
>> Assume that we checks the ipfix options changes in set_ipfix():
>> 1. set_ipfix(ofproto, new_option,...) {
>>if (ofproto->ipfix->options != new_options) {
>> ipfix->options = new_options;
>> ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>}
>>}
>>
>> 2. in xlate_xbridge_set, which is under revalidate context.
>> xlate_xbridge_set() {
>> ...
>> if (xbridge->ipfix != ipfix) { // here the ipfix options has changed,
>> but the "if test" will not aware that
>> dpif_ipfix_unref(xbridge->ipfix);
>> xbridge->ipfix = dpif_ipfix_ref(ipfix);
>
>But here the pointer does not change, so no need to update the reference to it.
>The actual configuration is taken in
>xlate_sample_action()/xlate_sample_action() used when creating/updating rules
>by the revalidator, kicked in by the succeeding udpif_revalidate() call.
>
After reading the code carefully again, I think you are right.
I would like to divide the things into two parts.
Part #1 is to resolve unneccesary revalidate cased by set_lldp(). Part #2 is
the to take care of the ipfix/lldp content change more that enabling/diabling.
My team uses neither ipfix nor lldp, the problem we met is that
bridge_reconfigure() already trigger udpif revalidate because of lldp
problem(set_lldp() always trigger udpif revalidate).
So I would like to focus on the first part, but leave the second part for users
who really want to use ipfix/lldp features.
Please let me know your think, thanks.
>> }
>> }
>>
>>>> 3. My main patch is the second one in this series, this patch is here
>>>> because it breaks two test cases. So I didn't spend muhc time on the ipfix
>>>> issue.
>>>
>>> See comments on the second patch.
>>>
>>>>>> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>>>>> +}
>>>>>> +
>>>>>> return 0;
>>>>>> }
>>>>>>
>>>>>> --
>>>>>> 1.8.3.1
>>>>>>
>>>>>> ___
>>>>>> dev mailing list
>>>>>> d...@openvswitch.org
>>>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>>>
>>>>>
>>>
>>>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 1/2] ofproto-dpif: trigger revalidation when ipfix changes
>
>
>On 14 Jan 2022, at 9:58, lic121 wrote:
>
>>>
>>>
>>> On 9 Jan 2022, at 14:44, lic121 wrote:
>>>
>>>> Currently, ipfix creation/delection don't trigger dpif backer
>>>> revalidation. This is not expected, as we need the revalidation
>>>> to commit ipfix into xlate. So that xlate can generate ipfix
>>>> actions.
>>>>
>>>> Signed-off-by: lic121
>>>> ---
>>>> ofproto/ofproto-dpif.c | 5 +
>>>> 1 file changed, 5 insertions(+)
>>>>
>>>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>>>> index bc3df8e..1cdef18 100644
>>>> --- a/ofproto/ofproto-dpif.c
>>>> +++ b/ofproto/ofproto-dpif.c
>>>> @@ -2306,6 +2306,7 @@ set_ipfix(
>>>> {
>>>> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>>>> struct dpif_ipfix *di = ofproto->ipfix;
>>>> +struct dpif_ipfix *old_ipfix = ofproto->ipfix;
>>>> bool has_options = bridge_exporter_options || flow_exporters_options;
>>>> bool new_di = false;
>>>>
>>>> @@ -2335,6 +2336,10 @@ set_ipfix(
>>>> }
>>>> }
>>>>
>>>> +if (old_ipfix != ofproto->ipfix) {
>>>
>>> This only works if ipfix was not configured earlier or disabled, i.e.,
>>> ofproto->ipfix was/is NULL.
>>> If this was your intention, you could just have done “if (new_di ||
>>> !ofproto->ipfix)”.
>>>
>>> However, I think there can also be changed in the configuration that
>>> requires a revalidate, what do you think? For example, enabling/disabling
>>> ingress/egress sampling.
>>> In this case dpif_ipfix_set_options() can be changed so it will return true
>>> if any configuration changes.
>>>
>> Actually, I had ever thought the same thing as you. But at last I didn't do
>> as that, for 3 reasons:
>> 1. I checked the history commit of ipfix, seems its not active in last 2-3
>> years. So I guess not so many ovs users are using ipfix feature.
>> 2. In xlate_xbridge_set, the place where checks the change flag, it doesn't
>> check the ipfix options changes as well.
>>
>> https://github.com/openvswitch/ovs/blob/eb1ab5357b6f3755f0ef1ee6d341ce24398d3bc1/ofproto/ofproto-dpif-xlate.c#L975-L978
>
>But here it just set the pointer as a reference, it does not take care of
>acting on configuration changes, or do I miss something?
>
Assume that we checks the ipfix options changes in set_ipfix():
1. set_ipfix(ofproto, new_option,...) {
if (ofproto->ipfix->options != new_options) {
ipfix->options = new_options;
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
}
2. in xlate_xbridge_set, which is under revalidate context.
xlate_xbridge_set() {
...
if (xbridge->ipfix != ipfix) { // here the ipfix options has changed, but
the "if test" will not aware that
dpif_ipfix_unref(xbridge->ipfix);
xbridge->ipfix = dpif_ipfix_ref(ipfix);
}
}
>> 3. My main patch is the second one in this series, this patch is here
>> because it breaks two test cases. So I didn't spend muhc time on the ipfix
>> issue.
>
>See comments on the second patch.
>
>>>> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
>>>> +}
>>>> +
>>>> return 0;
>>>> }
>>>>
>>>> --
>>>> 1.8.3.1
>>>>
>>>> ___
>>>> dev mailing list
>>>> d...@openvswitch.org
>>>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>>>
>>>
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 2/2] ofproto-dpif: avoid unneccesary backer revalidation
>
>
>On 9 Jan 2022, at 14:44, lic121 wrote:
>
>> If lldp didn't change, we are not supposed to trigger backer
>> revalidation.
>>
>> Signed-off-by: lic121
>> ---
>> ofproto/ofproto-dpif.c | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index 1cdef18..1bc9ec7 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -2461,10 +2461,10 @@ set_lldp(struct ofport *ofport_,
>> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
>> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>> int error = 0;
>> +struct lldp *old_lldp = ofport->lldp;
>>
>> if (cfg) {
>> if (!ofport->lldp) {
>> -ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu,
>> cfg);
>> }
>>
>> @@ -2476,6 +2476,8 @@ set_lldp(struct ofport *ofport_,
>> } else if (ofport->lldp) {
>> lldp_unref(ofport->lldp);
>> ofport->lldp = NULL;
>> +}
>> +if (old_lldp != ofport->lldp) {
>
>Same as for your first patch in the series. Here you only check if the pointer
>changes, so if you enable and then disable LLDP it will not trigger a
>REV_RECONFIGURE?
Good catch, lldp_configure returns true even the cfg has eabled=false
Will address this in the next version
>Maybe something like this will work (not tested):
>
>diff --git a/lib/ovs-lldp.c b/lib/ovs-lldp.c
>index 162311fa4..0f374df67 100644
>--- a/lib/ovs-lldp.c
>+++ b/lib/ovs-lldp.c
>@@ -761,6 +761,14 @@ lldp_configure(struct lldp *lldp, const struct smap *cfg)
>OVS_EXCLUDED(mutex)
> return true;
> }
>
>+/* Is LLDP enabled?
>+ */
>+bool
>+lldp_is_enabled(struct lldp *lldp)
>+{
>+return lldp ? lldp->enabled : false;
>+}
>+
> /* Create an LLDP stack instance. At the moment there is one per bridge port.
> */
> struct lldp *
>diff --git a/lib/ovs-lldp.h b/lib/ovs-lldp.h
>index 0e536e8c2..661ac4e18 100644
>--- a/lib/ovs-lldp.h
>+++ b/lib/ovs-lldp.h
>@@ -86,6 +86,7 @@ void lldp_run(struct lldpd *cfg);
> bool lldp_should_send_packet(struct lldp *cfg);
> bool lldp_should_process_flow(struct lldp *lldp, const struct flow *flow);
> bool lldp_configure(struct lldp *lldp, const struct smap *cfg);
>+bool lldp_is_enabled(struct lldp *lldp);
> void lldp_process_packet(struct lldp *cfg, const struct dp_packet *);
> void lldp_put_packet(struct lldp *lldp, struct dp_packet *packet,
> const struct eth_addr eth_src);
>diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>index 1bc9ec7e4..edfe7e123 100644
>--- a/ofproto/ofproto-dpif.c
>+++ b/ofproto/ofproto-dpif.c
>@@ -2461,7 +2461,7 @@ set_lldp(struct ofport *ofport_,
> struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
>+bool old_enable = lldp_is_enabled(ofport->lldp);
> int error = 0;
>-struct lldp *old_lldp = ofport->lldp;
>
> if (cfg) {
> if (!ofport->lldp) {
>@@ -2477,7 +2477,8 @@ set_lldp(struct ofport *ofport_,
> lldp_unref(ofport->lldp);
> ofport->lldp = NULL;
> }
>-if (old_lldp != ofport->lldp) {
>+
>+if (lldp_is_enabled(ofport->lldp) != old_enable) {
> ofproto->backer->need_revalidate = REV_RECONFIGURE;
> }
>
>
>Maybe you could also add some test cases to make sure this works?
>
>> ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> }
>>
>> --
>> 1.8.3.1
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v1 1/2] ofproto-dpif: trigger revalidation when ipfix changes
>
>
>On 9 Jan 2022, at 14:44, lic121 wrote:
>
>> Currently, ipfix creation/delection don't trigger dpif backer
>> revalidation. This is not expected, as we need the revalidation
>> to commit ipfix into xlate. So that xlate can generate ipfix
>> actions.
>>
>> Signed-off-by: lic121
>> ---
>> ofproto/ofproto-dpif.c | 5 +
>> 1 file changed, 5 insertions(+)
>>
>> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
>> index bc3df8e..1cdef18 100644
>> --- a/ofproto/ofproto-dpif.c
>> +++ b/ofproto/ofproto-dpif.c
>> @@ -2306,6 +2306,7 @@ set_ipfix(
>> {
>> struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
>> struct dpif_ipfix *di = ofproto->ipfix;
>> +struct dpif_ipfix *old_ipfix = ofproto->ipfix;
>> bool has_options = bridge_exporter_options || flow_exporters_options;
>> bool new_di = false;
>>
>> @@ -2335,6 +2336,10 @@ set_ipfix(
>> }
>> }
>>
>> +if (old_ipfix != ofproto->ipfix) {
>
>This only works if ipfix was not configured earlier or disabled, i.e.,
>ofproto->ipfix was/is NULL.
>If this was your intention, you could just have done “if (new_di ||
>!ofproto->ipfix)”.
>
>However, I think there can also be changed in the configuration that requires
>a revalidate, what do you think? For example, enabling/disabling
>ingress/egress sampling.
>In this case dpif_ipfix_set_options() can be changed so it will return true if
>any configuration changes.
>
Actually, I had ever thought the same thing as you. But at last I didn't do as
that, for 3 reasons:
1. I checked the history commit of ipfix, seems its not active in last 2-3
years. So I guess not so many ovs users are using ipfix feature.
2. In xlate_xbridge_set, the place where checks the change flag, it doesn't
check the ipfix options changes as well.
https://github.com/openvswitch/ovs/blob/eb1ab5357b6f3755f0ef1ee6d341ce24398d3bc1/ofproto/ofproto-dpif-xlate.c#L975-L978
3. My main patch is the second one in this series, this patch is here because
it breaks two test cases. So I didn't spend muhc time on the ipfix issue.
>> +ofproto->backer->need_revalidate = REV_RECONFIGURE;
>> +}
>> +
>> return 0;
>> }
>>
>> --
>> 1.8.3.1
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH] ofproto-dpif-upcall:Fix for OVS route cache re-add
>Problem Statement:
>OVS flushes and subsequently repopulates its route
>cache whenever it receives a netlink notification
>about kernel interface change. At the same time the
>port addition triggers a revalidation of all
>datapath flow cache entries. The revalidation of
>egress tunnel flows depends on correct routing
>information and can fail during the rebuild of
>the route cache, which leads to temporary insertion
>of drop flows.
>
>Solution:
>There is already a route_table_mutex that OVS seizes
>when it rebuilds the route cache.
>To ensure that flow revalidation cannot collide with
>route cache rebuild, seize that route_table_mutex also
>during revalidation. Since revalidation is multi-threaded,
>we seize the lock only on the leader thread. As a
>revalidation run can last hundreds of milliseconds,
>replace the lock() with trylock() in the route table code
>to avoid blocking the main OVS thread during revalidation.
>
>Signed-off-by: Cheekatamarla Eswara Venkata Pavan Kumar
>
>---
> lib/route-table.c | 34 --
> lib/route-table.h | 2 ++
> ofproto/ofproto-dpif-upcall.c | 14 ++
> 3 files changed, 40 insertions(+), 10 deletions(-)
>
>diff --git a/lib/route-table.c b/lib/route-table.c
>index ac82cf262..e1b17f1b8 100644
>--- a/lib/route-table.c
>+++ b/lib/route-table.c
>@@ -88,6 +88,21 @@ static void route_map_clear(void);
> static void name_table_init(void);
> static void name_table_change(const struct rtnetlink_change *, void *);
>
>+inline void route_table_lock(void)
>+{
>+ ovs_mutex_lock(_table_mutex);
>+}
>+
>+inline void route_table_unlock(void)
>+{
>+ ovs_mutex_unlock(_table_mutex);
>+}
>+
>+inline int route_table_trylock(void)
>+{
>+ return ovs_mutex_trylock(_table_mutex);
>+}
>+
> uint64_t
> route_table_get_change_seq(void)
> {
>@@ -100,7 +115,7 @@ void
> route_table_init(void)
> OVS_EXCLUDED(route_table_mutex)
> {
>-ovs_mutex_lock(_table_mutex);
>+route_table_lock();
> ovs_assert(!nln);
> ovs_assert(!route_notifier);
> ovs_assert(!route6_notifier);
>@@ -119,7 +134,7 @@ route_table_init(void)
> route_table_reset();
> name_table_init();
>
>-ovs_mutex_unlock(_table_mutex);
>+route_table_unlock();
> }
>
> /* Run periodically to update the locally maintained routing table. */
>@@ -127,7 +142,11 @@ void
> route_table_run(void)
> OVS_EXCLUDED(route_table_mutex)
> {
>-ovs_mutex_lock(_table_mutex);
>+/* Skip route table updates when the table is locked. */
>+if (route_table_trylock()) {
>+return;
>+}
>+
> if (nln) {
> rtnetlink_run();
> nln_run(nln);
>@@ -136,7 +155,7 @@ route_table_run(void)
> route_table_reset();
> }
> }
>-ovs_mutex_unlock(_table_mutex);
>+route_table_unlock();
> }
>
> /* Causes poll_block() to wake up when route_table updates are required. */
>@@ -144,12 +163,15 @@ void
> route_table_wait(void)
> OVS_EXCLUDED(route_table_mutex)
> {
>-ovs_mutex_lock(_table_mutex);
>+if (route_table_trylock()) {
>+return;
>+}
>+
> if (nln) {
> rtnetlink_wait();
> nln_wait(nln);
> }
>-ovs_mutex_unlock(_table_mutex);
>+route_table_unlock();
> }
>
> static int
>diff --git a/lib/route-table.h b/lib/route-table.h
>index 3a02d737a..58418bd3f 100644
>--- a/lib/route-table.h
>+++ b/lib/route-table.h
>@@ -33,4 +33,6 @@ void route_table_wait(void);
> bool route_table_fallback_lookup(const struct in6_addr *ip6_dst,
> char name[],
> struct in6_addr *gw6);
>+void route_table_lock(void);
>+void route_table_unlock(void);
> #endif /* route-table.h */
>diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
>index 1c9c720f0..ab2d20833 100644
>--- a/ofproto/ofproto-dpif-upcall.c
>+++ b/ofproto/ofproto-dpif-upcall.c
>@@ -23,6 +23,7 @@
> #include "coverage.h"
> #include "cmap.h"
> #include "lib/dpif-provider.h"
>+#include "lib/route-table.h"
> #include "dpif.h"
> #include "openvswitch/dynamic-string.h"
> #include "fail-open.h"
>@@ -587,7 +588,8 @@ udpif_start_threads(struct udpif *udpif, uint32_t
>n_handlers_,
> dpif_enable_upcall(udpif->dpif);
>
> ovs_barrier_init(>reval_barrier, udpif->n_revalidators);
>-ovs_barrier_init(>pause_barrier, udpif->n_revalidators + 1);
>+/* For main thread and leader*/
>+ovs_barrier_init(>pause_barrier, 2);
> udpif->reval_exit = false;
> udpif->pause = false;
> udpif->offload_rebalance_time = time_msec();
>@@ -953,6 +955,10 @@ udpif_revalidator(void *arg)
> * on the pause_barrier */
> udpif->pause = latch_is_set(>pause_latch);
>
>+if (udpif->pause) {
>+revalidator_pause(revalidator);
>+}
>+
> /* Only the leader checks the exit latch to prevent a race where
> * some threads think it's true and exit and others
[ovs-dev] [PATCH v1 2/2] ofproto-dpif: avoid unneccesary backer revalidation
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Signed-off-by: lic121
---
ofproto/ofproto-dpif.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 1cdef18..1bc9ec7 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2461,10 +2461,10 @@ set_lldp(struct ofport *ofport_,
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
int error = 0;
+struct lldp *old_lldp = ofport->lldp;
if (cfg) {
if (!ofport->lldp) {
-ofproto->backer->need_revalidate = REV_RECONFIGURE;
ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
}
@@ -2476,6 +2476,8 @@ set_lldp(struct ofport *ofport_,
} else if (ofport->lldp) {
lldp_unref(ofport->lldp);
ofport->lldp = NULL;
+}
+if (old_lldp != ofport->lldp) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1 1/2] ofproto-dpif: trigger revalidation when ipfix changes
Currently, ipfix creation/delection don't trigger dpif backer
revalidation. This is not expected, as we need the revalidation
to commit ipfix into xlate. So that xlate can generate ipfix
actions.
Signed-off-by: lic121
---
ofproto/ofproto-dpif.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index bc3df8e..1cdef18 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2306,6 +2306,7 @@ set_ipfix(
{
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofproto_);
struct dpif_ipfix *di = ofproto->ipfix;
+struct dpif_ipfix *old_ipfix = ofproto->ipfix;
bool has_options = bridge_exporter_options || flow_exporters_options;
bool new_di = false;
@@ -2335,6 +2336,10 @@ set_ipfix(
}
}
+if (old_ipfix != ofproto->ipfix) {
+ofproto->backer->need_revalidate = REV_RECONFIGURE;
+}
+
return 0;
}
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1 0/2] fix dpif backer revalidation
ovsdb change or netlink notification trigger bridge_reconfigure. In bridge_reconfigure, backer->need_revalidate flag is set if backer revalidation is needed. This series fix two places where need_revalidate flag is not set correctly. lic121 (2): ofproto-dpif: trigger revalidation when ipfix changes ofproto-dpif: avoid unneccesary backer revalidation ofproto/ofproto-dpif.c | 9 - 1 file changed, 8 insertions(+), 1 deletion(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v1] ofproto-dpif: avoid unneccesary backer revalidation
If lldp didn't change, we are not supposed to trigger backer
revalidation.
Signed-off-by: lic121
---
ofproto/ofproto-dpif.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index bc3df8e..eb0e412 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2456,10 +2456,10 @@ set_lldp(struct ofport *ofport_,
struct ofport_dpif *ofport = ofport_dpif_cast(ofport_);
struct ofproto_dpif *ofproto = ofproto_dpif_cast(ofport->up.ofproto);
int error = 0;
+struct lldp *old_lldp = ofport->lldp;
if (cfg) {
if (!ofport->lldp) {
-ofproto->backer->need_revalidate = REV_RECONFIGURE;
ofport->lldp = lldp_create(ofport->up.netdev, ofport_->mtu, cfg);
}
@@ -2471,6 +2471,8 @@ set_lldp(struct ofport *ofport_,
} else if (ofport->lldp) {
lldp_unref(ofport->lldp);
ofport->lldp = NULL;
+}
+if (old_lldp != ofport->lldp) {
ofproto->backer->need_revalidate = REV_RECONFIGURE;
}
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v4 0/3] bug fix: avoid install bad dp flow
>On 11/21/21 16:19, lic121 wrote:
>> Version 4:
>> - Cover case where tcp_hdrlen > tcp_pkt_size
>> - Other small adjustments
>>
>> ovs may install bad datapath flow when meet malformed pkts. As a
>> result, it may allows some unwanted pkts pass. This could be a point
>> of attack.
>>
>> lic121 (3):
>> upcall: prevent from installing flows when inconsistence
>> tests: fix packet data endianness
>> upcall: considering dataofs when parsing tcp pkt
>>
>> lib/flow.c | 20 ++---
>> ofproto/ofproto-dpif-upcall.c | 26 +++---
>> tests/flowgen.py | 2 +-
>> tests/ofproto-dpif.at | 50
>>+++
>> 4 files changed, 86 insertions(+), 12 deletions(-)
>>
>
>Thanks for the new version! With some small cosmetic changes
>I applied patches #2 and #3. Also backported down to all
>supported branches (down to 2.13). This should fix the main
>parsing problem.
>
>And I found the patch that actually broke the parsing more
>than 7 years ago:
> 5a51b2cd3483 ("lib/ofpbuf: Remove 'l7' pointer.")
>
>I added it to the 'Fixes' tag in the patch #3.
>
>I didn't actually look closely to the patch #1 yet, so I'll
>need to get back to it a bit later.
>
Thanks for review the patches. Yes, the #3 patch fixs the main problem.
The #1 patch is important as well. Without patch #1,
we will have to keep an key on every patch, which affects flow parsing, to
prevent it from breaking the consistentance between kmod and vswitchd.
>Best regards, Ilya Maximets.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
Re: [ovs-dev] [PATCH v3 3/3] upcall: considering dataofs when parsing tcp pkt
>On 11/1/21 12:03, lic121 wrote:
>> dataofs field of tcp header indicates the tcp header len. The len
>> should be >= 20 bytes/4. This patch is to test dataofs, and don't
>> parse layer 4 fields when meet ba dataofs. This behave is the consistent
>> with openvswitch kenrel module.
>>
>> Signed-off-by: lic121
>> ---
>> lib/flow.c | 18 ++
>> tests/ofproto-dpif.at | 31 +++
>> 2 files changed, 41 insertions(+), 8 deletions(-)
>
>Hi. Thanks for the patch! And sorry for the late response.
>See some comments inline.
>
Hi Ilya, it's OK and thanks for your review.
>Bets regards, Ilya Maximets.
>
>>
>> diff --git a/lib/flow.c b/lib/flow.c
>> index 89837de..f117490 100644
>> --- a/lib/flow.c
>> +++ b/lib/flow.c
>> @@ -1006,14 +1006,16 @@ miniflow_extract(struct dp_packet *packet, struct
>> miniflow *dst)
>> if (OVS_LIKELY(nw_proto == IPPROTO_TCP)) {
>> if (OVS_LIKELY(size >= TCP_HEADER_LEN)) {
>> const struct tcp_header *tcp = data;
>> -
>> - miniflow_push_be32(mf, arp_tha.ea[2], 0);
>> - miniflow_push_be32(mf, tcp_flags,
>> - TCP_FLAGS_BE32(tcp->tcp_ctl));
>> - miniflow_push_be16(mf, tp_src, tcp->tcp_src);
>> - miniflow_push_be16(mf, tp_dst, tcp->tcp_dst);
>> - miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
>> - miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
>> + size_t tcp_hdr_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
>
>Please, add an empty line between the declarations and the code below.
Will address this in v4
>
>> + if (tcp_hdr_len >= TCP_HEADER_LEN) {
>
>This check seems fine, but, IIUC, it doesn't check the case where
>the TCP header length declared larger than the remaining space in
>a packet. Looking at the kernel implementation of tcphdr_ok(),
>there are, basically, 3 checks that make it fail:
>
>1. pskb_may_pull(skb, th_ofs + sizeof(struct tcphdr))) == false
>
> This one is similar to (size >= TCP_HEADER_LEN) check here.
>
>2. tcp_len < sizeof(struct tcphdr)
>
> This check you added as (tcp_hdr_len >= TCP_HEADER_LEN)
>
>3. skb->len < th_ofs + tcp_len
>
> But this one is not covered by the patch. IIUC, it should
> look like:
> if (tcp_hdr_len >= TCP_HEADER_LEN && size >= tcp_hdr_len) {
>
>Without the third check, if TCP header length is larger than
>the remaining packet size, kernel will not parse it, but
>userspace will, leading to the similar issue.
>
>Could you add this check to the patch and a test case for this
>condition?
Agree, will address this in v4
>
>> + miniflow_push_be32(mf, arp_tha.ea[2], 0);
>> + miniflow_push_be32(mf, tcp_flags,
>> + TCP_FLAGS_BE32(tcp->tcp_ctl));
>
>Please, shift this line 3 spaces to the right.
good catch. Will do
>
>> + miniflow_push_be16(mf, tp_src, tcp->tcp_src);
>> + miniflow_push_be16(mf, tp_dst, tcp->tcp_dst);
>> + miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
>> + miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
>> + }
>> }
>> } else if (OVS_LIKELY(nw_proto == IPPROTO_UDP)) {
>> if (OVS_LIKELY(size >= UDP_HEADER_LEN)) {
>> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
>> index 31fb163..0f372ae 100644
>> --- a/tests/ofproto-dpif.at
>> +++ b/tests/ofproto-dpif.at
>> @@ -4862,6 +4862,37 @@
>> recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,fr
>> OVS_VSWITCHD_STOP
>> AT_CLEANUP
>>
>> +AT_SETUP([ofproto-dpif - malformed packets handling - upcall])
>> +OVS_VSWITCHD_START
>> +add_of_ports br0 1 90
>> +dnl drop packet has tcp port 0-f but allow other tcp packets
>> +AT_DATA([flows.txt], [dnl
>> +priority=75 tcp tp_dst=0/0xfff0 actions=drop
>> +priority=50 tcp actions=output:1
>> +])
>> +AT_CHECK([ovs-ofctl replace-flows br0 flows.txt])
>> +dnl good tcp pkt, tcp(sport=100,dpor=16)
>> +pkt1="be95df40fb57fa163e5ee35
[ovs-dev] [PATCH v4 3/3] upcall: considering dataofs when parsing tcp pkt
dataofs field of tcp header indicates the tcp header len. The len
should be >= 20 bytes/4 and be <= tcp data len. This patch is to
test dataofs, and don't parse layer 4 fields when meet bad dataofs.
This behave is consistent with openvswitch kenrel module.
Signed-off-by: lic121
---
lib/flow.c| 20
tests/ofproto-dpif.at | 50 ++
2 files changed, 62 insertions(+), 8 deletions(-)
diff --git a/lib/flow.c b/lib/flow.c
index 89837de..a021bc0 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1006,14 +1006,18 @@ miniflow_extract(struct dp_packet *packet, struct
miniflow *dst)
if (OVS_LIKELY(nw_proto == IPPROTO_TCP)) {
if (OVS_LIKELY(size >= TCP_HEADER_LEN)) {
const struct tcp_header *tcp = data;
-
-miniflow_push_be32(mf, arp_tha.ea[2], 0);
-miniflow_push_be32(mf, tcp_flags,
- TCP_FLAGS_BE32(tcp->tcp_ctl));
-miniflow_push_be16(mf, tp_src, tcp->tcp_src);
-miniflow_push_be16(mf, tp_dst, tcp->tcp_dst);
-miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
-miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
+size_t tcp_hdr_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
+
+if (OVS_LIKELY(tcp_hdr_len >= TCP_HEADER_LEN)
+&& OVS_LIKELY(size >= tcp_hdr_len)) {
+miniflow_push_be32(mf, arp_tha.ea[2], 0);
+miniflow_push_be32(mf, tcp_flags,
+ TCP_FLAGS_BE32(tcp->tcp_ctl));
+miniflow_push_be16(mf, tp_src, tcp->tcp_src);
+miniflow_push_be16(mf, tp_dst, tcp->tcp_dst);
+miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
+miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
+}
}
} else if (OVS_LIKELY(nw_proto == IPPROTO_UDP)) {
if (OVS_LIKELY(size >= UDP_HEADER_LEN)) {
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 31fb163..637bc89 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -4862,6 +4862,56 @@
recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,fr
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - malformed packets handling - upcall])
+OVS_VSWITCHD_START
+add_of_ports br0 1 90
+
+dnl drop packet has tcp port 0-f but allow other tcp packets
+AT_DATA([flows.txt], [dnl
+priority=75 tcp tp_dst=0/0xfff0 actions=drop
+priority=50 tcp actions=output:1
+])
+AT_CHECK([ovs-ofctl replace-flows br0 flows.txt])
+
+dnl good tcp pkt, tcp(sport=100,dpor=16)
+pkt1="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010500220005333"
+
+dnl malformed tcp pkt(tcp_hdr < 20 byte), tcp(sport=100,dport=16,dataofs=1)
+pkt2="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010100220009333"
+
+dnl malformed tcp pkt(tcp_hdr > pkt_len), tcp(sport=100,dport=16,dataofs=15)
+pkt3="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010f00220009333"
+
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+
+mode=normal
+
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt1"], [0], [stdout])
+dnl for good tcp pkt, ovs can extract the tp_dst=16
+AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=16/0xfff0),
packets:0, bytes:0, used:never, actions:1
+])
+
+AT_CHECK([ovs-appctl revalidator/purge], [0], [stdout])
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt2"], [0], [stdout])
+dnl for malformed tcp pkt(tcp_hdr < 20 byte), ovs uses default value tp_dst=0
+AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=0/0xfff0),
packets:0, bytes:0, used:never, actions:drop
+])
+
+AT_CHECK([ovs-appctl revalidator/purge], [0], [stdout])
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt3"], [0], [stdout])
+dnl for malformed tcp pkt(tcp_hdr > pkt_len), ovs uses default value tp_dst=0
+AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=0/0xfff0),
packets:0, bytes:0, used:never, actions:drop
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - exit])
OVS_VSWITCHD_START
add_of_ports br0 1 2 3 10 11 12 13 14
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 2/3] tests: fix packet data endianness
Without this fix, flowgen.py generates bad tcp pkts.
tcpdump reports "bad hdr length 4 - too short" with the pcap
generated by flowgen.py
This patch is to correct pkt data endianness
Signed-off-by: lic121
---
tests/flowgen.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/flowgen.py b/tests/flowgen.py
index 7ef32d1..9823167 100755
--- a/tests/flowgen.py
+++ b/tests/flowgen.py
@@ -135,7 +135,7 @@ def output(attrs):
12893) # urgent pointer
if attrs['TP_PROTO'] == 'TCP+options':
tcp = (tcp[:12]
- + struct.pack('H', (6 << 12) | 0x02 | 0x10)
+ + struct.pack('>H', (6 << 12) | 0x02 | 0x10)
+ tcp[14:])
tcp += struct.pack('>BBH', 2, 4, 1975) # MSS option
tcp += b'payload'
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v4 1/3] upcall: prevent from installing flows when inconsistence
In ovs kernel datapath upcall, the *key* and packet are passed to
userspace. The key contains the fields/meta extracted from packet.
Once the ovs-vswitchd receives the upcall, the packet is extracted
again into *flow*. Next, the flow is used to match openflow rules to
generate the wildcard(wc). At last, vswitchd installs a mega_flow in
datapath(mega_flow = key/wc,action)
We can see that vswitchd generate wc from flow while it installs dp
flow with key. If the key is not consistent with the flow [1], we get
bad mega_flow.
Let's assume we have the flowing rules, means to block tcp port 0-0xf,
but allow other ports.
"table=0,priority=100,tcp,tp_dst=0x0/0xfff0 actions=drop"
"table=0,priority=90,tcp actions=p1"
good case:
If a packet has tcp dst=0x10, generated `mega_flow=0x10/0xfff0,out:p1`,
this is expected.
bad case:
If a packet has tcp dst=0x10 but not pass tcphdr_ok [1], generated wc
and action are `0xfff0,out:p1`. The mega_flow will be
`0x0/0xfff0,out:p1`, bacause mega_flow=key/wc,action. This allows
packets with tcp port 0-0xf pass by mistake.
The following scapy3 script triggers the issue:
```py
eth=Ether(src="fa:16:3e:5e:e3:57",dst="be:95:df:40:fb:57")
ip=IP(src="10.10.10.10",dst="20.20.20.20")
tcp=TCP(sport=100,dport=16,dataofs=1)
sendp(eth/ip/tcp)
```
This patch is to prevent from installing datapath flow if the key is
not consistant with the flow.
[1] https://github.com/openvswitch/ovs/blob/v2.16.1/datapath/flow.c#L601
Signed-off-by: lic121
---
ofproto/ofproto-dpif-upcall.c | 26 +++---
1 file changed, 23 insertions(+), 3 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 1c9c720..81f297d 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -244,6 +244,7 @@ struct upcall {
size_t key_len;/* Datapath flow key length. */
const struct nlattr *out_tun_key; /* Datapath output tunnel key. */
+const struct flow *key_as_flow; /* converted from key. */
struct user_action_cookie cookie;
uint64_t odp_actions_stub[1024 / 8]; /* Stub for odp_actions. */
@@ -810,6 +811,7 @@ recv_upcalls(struct handler *handler)
struct dpif_upcall dupcalls[UPCALL_MAX_BATCH];
struct upcall upcalls[UPCALL_MAX_BATCH];
struct flow flows[UPCALL_MAX_BATCH];
+struct flow key_as_flows[UPCALL_MAX_BATCH];
size_t n_upcalls, i;
n_upcalls = 0;
@@ -818,6 +820,7 @@ recv_upcalls(struct handler *handler)
struct dpif_upcall *dupcall = [n_upcalls];
struct upcall *upcall = [n_upcalls];
struct flow *flow = [n_upcalls];
+struct flow *key_as_flow = _as_flows[n_upcalls];
unsigned int mru = 0;
uint64_t hash = 0;
int error;
@@ -830,7 +833,7 @@ recv_upcalls(struct handler *handler)
}
upcall->fitness = odp_flow_key_to_flow(dupcall->key, dupcall->key_len,
- flow, NULL);
+ key_as_flow, NULL);
if (upcall->fitness == ODP_FIT_ERROR) {
goto free_dupcall;
}
@@ -843,6 +846,9 @@ recv_upcalls(struct handler *handler)
hash = nl_attr_get_u64(dupcall->hash);
}
+/* Fill flow with key_as_flow as upcall_receive needs
+ * packet flow info. */
+*flow = *key_as_flow;
error = upcall_receive(upcall, udpif->backer, >packet,
dupcall->type, dupcall->userdata, flow, mru,
>ufid, PMD_ID_NULL);
@@ -856,20 +862,21 @@ recv_upcalls(struct handler *handler)
dupcall->key_len, NULL, 0, NULL, 0,
>ufid, PMD_ID_NULL, NULL);
VLOG_INFO_RL(, "received packet on unassociated datapath "
- "port %"PRIu32, flow->in_port.odp_port);
+ "port %"PRIu32, key_as_flow->in_port.odp_port);
}
goto free_dupcall;
}
upcall->key = dupcall->key;
upcall->key_len = dupcall->key_len;
+upcall->key_as_flow = key_as_flow;
upcall->ufid = >ufid;
upcall->hash = hash;
upcall->out_tun_key = dupcall->out_tun_key;
upcall->actions = dupcall->actions;
-pkt_metadata_from_flow(>packet.md, flow);
+pkt_metadata_from_flow(>packet.md, key_as_flow);
flow_extract(>packet, flow);
error = process_upcall(udpif, upcall,
@@ -1332,6 +1339,19 @@ should_install_flow(struct udpif *udpif, struct upcall
*upcall)
return false;
}
+/* For linux kernel datapath, the "key" extracted by kernel may be
+ * inconsistent with the flow extracted from packet by ovs. If that
+ *
[ovs-dev] [PATCH v4 0/3] bug fix: avoid install bad dp flow
Version 4: - Cover case where tcp_hdrlen > tcp_pkt_size - Other small adjustments ovs may install bad datapath flow when meet malformed pkts. As a result, it may allows some unwanted pkts pass. This could be a point of attack. lic121 (3): upcall: prevent from installing flows when inconsistence tests: fix packet data endianness upcall: considering dataofs when parsing tcp pkt lib/flow.c| 20 ++--- ofproto/ofproto-dpif-upcall.c | 26 +++--- tests/flowgen.py | 2 +- tests/ofproto-dpif.at | 50 +++ 4 files changed, 86 insertions(+), 12 deletions(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH] doc: fix install doc
remove bad quotes Signed-off-by: lic121 --- Documentation/intro/install/dpdk.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Documentation/intro/install/dpdk.rst b/Documentation/intro/install/dpdk.rst index 296ec4f..d554409 100644 --- a/Documentation/intro/install/dpdk.rst +++ b/Documentation/intro/install/dpdk.rst @@ -219,7 +219,7 @@ To verify hugepage configuration:: Mount the hugepages, if not already mounted by default:: -$ mount -t hugetlbfs none /dev/hugepages`` +$ mount -t hugetlbfs none /dev/hugepages .. note:: -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 3/3] upcall: considering dataofs when parsing tcp pkt
dataofs field of tcp header indicates the tcp header len. The len
should be >= 20 bytes/4. This patch is to test dataofs, and don't
parse layer 4 fields when meet ba dataofs. This behave is the consistent
with openvswitch kenrel module.
Signed-off-by: lic121
---
lib/flow.c| 18 ++
tests/ofproto-dpif.at | 31 +++
2 files changed, 41 insertions(+), 8 deletions(-)
diff --git a/lib/flow.c b/lib/flow.c
index 89837de..f117490 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1006,14 +1006,16 @@ miniflow_extract(struct dp_packet *packet, struct
miniflow *dst)
if (OVS_LIKELY(nw_proto == IPPROTO_TCP)) {
if (OVS_LIKELY(size >= TCP_HEADER_LEN)) {
const struct tcp_header *tcp = data;
-
-miniflow_push_be32(mf, arp_tha.ea[2], 0);
-miniflow_push_be32(mf, tcp_flags,
- TCP_FLAGS_BE32(tcp->tcp_ctl));
-miniflow_push_be16(mf, tp_src, tcp->tcp_src);
-miniflow_push_be16(mf, tp_dst, tcp->tcp_dst);
-miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
-miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
+size_t tcp_hdr_len = TCP_OFFSET(tcp->tcp_ctl) * 4;
+if (tcp_hdr_len >= TCP_HEADER_LEN) {
+miniflow_push_be32(mf, arp_tha.ea[2], 0);
+miniflow_push_be32(mf, tcp_flags,
+TCP_FLAGS_BE32(tcp->tcp_ctl));
+miniflow_push_be16(mf, tp_src, tcp->tcp_src);
+miniflow_push_be16(mf, tp_dst, tcp->tcp_dst);
+miniflow_push_be16(mf, ct_tp_src, ct_tp_src);
+miniflow_push_be16(mf, ct_tp_dst, ct_tp_dst);
+}
}
} else if (OVS_LIKELY(nw_proto == IPPROTO_UDP)) {
if (OVS_LIKELY(size >= UDP_HEADER_LEN)) {
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 31fb163..0f372ae 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -4862,6 +4862,37 @@
recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,fr
OVS_VSWITCHD_STOP
AT_CLEANUP
+AT_SETUP([ofproto-dpif - malformed packets handling - upcall])
+OVS_VSWITCHD_START
+add_of_ports br0 1 90
+dnl drop packet has tcp port 0-f but allow other tcp packets
+AT_DATA([flows.txt], [dnl
+priority=75 tcp tp_dst=0/0xfff0 actions=drop
+priority=50 tcp actions=output:1
+])
+AT_CHECK([ovs-ofctl replace-flows br0 flows.txt])
+dnl good tcp pkt, tcp(sport=100,dpor=16)
+pkt1="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010500220005333"
+dnl malformed tcp pkt, tcp(sport=100,dport=16,dataofs=1)
+pkt2="be95df40fb57fa163e5ee35708004528000140063e940a0a0a0a1414141400640010100220009333"
+AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
+mode=normal
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt1"], [0], [stdout])
+dnl for good tcp pkt, ovs can extract the tp_dst=16
+AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=16/0xfff0),
packets:0, bytes:0, used:never, actions:1
+])
+AT_CHECK([ovs-appctl dpctl/del-flows], [0], [stdout])
+AT_CHECK([ovs-appctl netdev-dummy/receive p90 "$pkt2"], [0], [stdout])
+dnl for malformed tcp pkt, ovs can use default value tp_dst=0
+AT_CHECK([ovs-appctl dpctl/dump-flows filter=in_port\(90\),tcp], [0], [dnl
+flow-dump from the main thread:
+recirc_id(0),in_port(90),packet_type(ns=0,id=0),eth_type(0x0800),ipv4(proto=6,frag=no),tcp(dst=0/0xfff0),
packets:0, bytes:0, used:never, actions:drop
+])
+OVS_VSWITCHD_STOP
+AT_CLEANUP
+
AT_SETUP([ofproto-dpif - exit])
OVS_VSWITCHD_START
add_of_ports br0 1 2 3 10 11 12 13 14
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 2/3] tests: fix packet data endianness
Without this fix, flowgen.py generates bad tcp pkts.
tcpdump reports "bad hdr length 4 - too short" with the pcap
generated by flowgen.py
This patch is to correct pkt data endianness
Signed-off-by: lic121
---
tests/flowgen.py | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/tests/flowgen.py b/tests/flowgen.py
index 7ef32d1..9823167 100755
--- a/tests/flowgen.py
+++ b/tests/flowgen.py
@@ -135,7 +135,7 @@ def output(attrs):
12893) # urgent pointer
if attrs['TP_PROTO'] == 'TCP+options':
tcp = (tcp[:12]
- + struct.pack('H', (6 << 12) | 0x02 | 0x10)
+ + struct.pack('>H', (6 << 12) | 0x02 | 0x10)
+ tcp[14:])
tcp += struct.pack('>BBH', 2, 4, 1975) # MSS option
tcp += b'payload'
--
1.8.3.1
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev
[ovs-dev] [PATCH v3 1/3] upcall: prevent from installing flows when inconsistence
In ovs kernel datapath upcall, the *key* and packet are passed to
userspace. The key contains the fields/meta extracted from packet.
Once the ovs-vswitchd receives the upcall, the packet is extracted
again into *flow*. Next, the flow is used to match openflow rules to
generate the wildcard(wc). At last, vswitchd installs a mega_flow in
datapath(mega_flow = key/wc,action)
We can see that vswitchd generate wc from flow while it installs dp
flow with key. If the key is not consistent with the flow [1], we get
bad mega_flow.
Let's assume we have the flowing rules, means to block tcp port 0-0xf,
but allow other ports.
"table=0,priority=100,tcp,tp_dst=0x0/0xfff0 actions=drop"
"table=0,priority=90,tcp actions=p1"
good case:
If a packet has tcp dst=0x10, generated `mega_flow=0x10/0xfff0,out:p1`,
this is expected.
bad case:
If a packet has tcp dst=0x10 but not pass tcphdr_ok [1], generated wc
and action are `0xfff0,out:p1`. The mega_flow will be
`0x0/0xfff0,out:p1`, bacause mega_flow=key/wc,action. This allows
packets with tcp port 0-0xf pass by mistake.
The following scapy3 script triggers the issue:
```py
eth=Ether(src="fa:16:3e:5e:e3:57",dst="be:95:df:40:fb:57")
ip=IP(src="10.10.10.10",dst="20.20.20.20")
tcp=TCP(sport=100,dport=16,dataofs=1)
sendp(eth/ip/tcp)
```
This patch is to prevent from installing datapath flow if the key is
not consistant with the flow.
[1] https://github.com/openvswitch/ovs/blob/v2.16.1/datapath/flow.c#L601
Signed-off-by: lic121
---
ofproto/ofproto-dpif-upcall.c | 29 +
1 file changed, 25 insertions(+), 4 deletions(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 1c9c720..93c750d 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -244,6 +244,7 @@ struct upcall {
size_t key_len;/* Datapath flow key length. */
const struct nlattr *out_tun_key; /* Datapath output tunnel key. */
+const struct flow *key_as_flow; /* converted from key. */
struct user_action_cookie cookie;
uint64_t odp_actions_stub[1024 / 8]; /* Stub for odp_actions. */
@@ -810,6 +811,7 @@ recv_upcalls(struct handler *handler)
struct dpif_upcall dupcalls[UPCALL_MAX_BATCH];
struct upcall upcalls[UPCALL_MAX_BATCH];
struct flow flows[UPCALL_MAX_BATCH];
+struct flow key_as_flows[UPCALL_MAX_BATCH];
size_t n_upcalls, i;
n_upcalls = 0;
@@ -818,6 +820,7 @@ recv_upcalls(struct handler *handler)
struct dpif_upcall *dupcall = [n_upcalls];
struct upcall *upcall = [n_upcalls];
struct flow *flow = [n_upcalls];
+struct flow *key_as_flow = _as_flows[n_upcalls];
unsigned int mru = 0;
uint64_t hash = 0;
int error;
@@ -830,7 +833,7 @@ recv_upcalls(struct handler *handler)
}
upcall->fitness = odp_flow_key_to_flow(dupcall->key, dupcall->key_len,
- flow, NULL);
+ key_as_flow, NULL);
if (upcall->fitness == ODP_FIT_ERROR) {
goto free_dupcall;
}
@@ -842,7 +845,9 @@ recv_upcalls(struct handler *handler)
if (dupcall->hash) {
hash = nl_attr_get_u64(dupcall->hash);
}
-
+/* Fill flow with key_as_flow as upcall_receive needs
+ * packet flow info. */
+*flow = *key_as_flow;
error = upcall_receive(upcall, udpif->backer, >packet,
dupcall->type, dupcall->userdata, flow, mru,
>ufid, PMD_ID_NULL);
@@ -856,20 +861,21 @@ recv_upcalls(struct handler *handler)
dupcall->key_len, NULL, 0, NULL, 0,
>ufid, PMD_ID_NULL, NULL);
VLOG_INFO_RL(, "received packet on unassociated datapath "
- "port %"PRIu32, flow->in_port.odp_port);
+ "port %"PRIu32, key_as_flow->in_port.odp_port);
}
goto free_dupcall;
}
upcall->key = dupcall->key;
upcall->key_len = dupcall->key_len;
+upcall->key_as_flow = key_as_flow;
upcall->ufid = >ufid;
upcall->hash = hash;
upcall->out_tun_key = dupcall->out_tun_key;
upcall->actions = dupcall->actions;
-pkt_metadata_from_flow(>packet.md, flow);
+pkt_metadata_from_flow(>packet.md, key_as_flow);
flow_extract(>packet, flow);
error = process_upcall(udpif, upcall,
@@ -1332,6 +1338,21 @@ should_install_flow(struct udpif *udpif, struct upcall
*upcall)
return false;
}
+/* For linux kernel datapath, the "key" extracted by kernel may be
+inconsistent with the flow extrac
[ovs-dev] [PATCH v3 0/3] bug fix: avoid install bad dp flow
ovs may install bad datapath flow when meet malformed pkts. As a result, it may allows some unwanted pkts pass. This could be a point of attack. lic121 (3): upcall: prevent from installing flows when inconsistence tests: fix packet data endianness upcall: considering dataofs when parsing tcp pkt lib/flow.c| 18 ++ ofproto/ofproto-dpif-upcall.c | 29 + tests/flowgen.py | 2 +- tests/ofproto-dpif.at | 31 +++ 4 files changed, 67 insertions(+), 13 deletions(-) -- 1.8.3.1 ___ dev mailing list d...@openvswitch.org https://mail.openvswitch.org/mailman/listinfo/ovs-dev
