Re: [ovs-dev] [PATCH v4 1/4] lib: Add CRC32C Implementation
On 30 July 2013 20:31, Joe Stringer wrote: > This implementation was derived from FreeBSD: > http://code.google.com/p/freebsd-head/source/browse/sys/libkern/crc32.c The canonical FreeBSD web repo location for this file would be: http://svnweb.freebsd.org/base/head/sys/libkern/crc32.c (It doesn't matter much, but mirrors like the one on code.google.com may not be updated regularly, or may disappear in the future.) X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4 1/4] lib: Add CRC32C Implementation
Thanks, didn't know about that. I'll use that link instead. On Mon, Aug 12, 2013 at 11:08 PM, Ed Maste wrote: > On 30 July 2013 20:31, Joe Stringer wrote: > > This implementation was derived from FreeBSD: > > http://code.google.com/p/freebsd-head/source/browse/sys/libkern/crc32.c > > The canonical FreeBSD web repo location for this file would be: > http://svnweb.freebsd.org/base/head/sys/libkern/crc32.c > > (It doesn't matter much, but mirrors like the one on code.google.com > may not be updated regularly, or may disappear in the future.) > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 5/5 v2] dpif-netdev: Avoid races on queue and port changes using seq objects.
Here is an example I can think of:
Thread A wants to do a seq_change(), Thread B wants to do seq_wait(), they
both compete for dp_netdev_mutex at the same time.
Assume Thread A wins, seq_change changed the seq->value to +1 and releases
the dp_netdev_mutex. Thread B now waits for the +1 value to change, thus
missing Thread A's +1 event.
Is this possible?
On Sat, Aug 10, 2013 at 8:57 PM, Ben Pfaff wrote:
> On Sat, Aug 10, 2013 at 04:39:09PM -0700, Andy Zhou wrote:
> > On Wed, Aug 7, 2013 at 1:31 PM, Ben Pfaff wrote:
> > > dpif_upcall *upcall,
> > > static void
> > > dpif_netdev_recv_wait(struct dpif *dpif)
> > > {
> > > -/* XXX In a multithreaded process, there is a race window between
> this
> > > - * function and the poll_block() in one thread and a packet being
> > > queued in
> > > - * another thread. */
> > > +struct dp_netdev *dp = get_dp_netdev(dpif);
> > > +uint64_t seq;
> > >
> > > ovs_mutex_lock(&dp_netdev_mutex);
> > > +seq = seq_read(dp->queue_seq);
> > >
> >
> > Is there a risk of lossing events in case seq_change() is executed in
> > another thread while this thread is waiting
> > on the dp_netdev_mutex lock?
>
> I guess by "losing events" you mean sleeping too long, beyond the time
> at which an upcall is queued? I don't think we can lose events here,
> then. There are two cases. The first case, where we call
> poll_immediate_wake(), is obviously OK. In the other case, we know that
> the queues were empty at a sequence number of 'seq' or later, so calling
> seq_wait() passing 'seq' will ensure that we wake up when the sequence
> number becomes greater than 'seq', which will definitely happen whenever
> an upcall gets queued.
>
> Do you see a hole in the logic?
>
> Thanks,
>
> Ben.
>
> > > if (find_nonempty_queue(dpif)) {
> > > poll_immediate_wake();
> > > +} else {
> > > +seq_wait(dp->queue_seq, seq);
> > > }
> > > ovs_mutex_unlock(&dp_netdev_mutex);
> > > }
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [seq 4/5] seq: New module for race-free, pollable, thread-safe sequence number.
On Sat, Aug 10, 2013 at 8:51 PM, Ben Pfaff wrote: > On Sat, Aug 10, 2013 at 04:32:11PM -0700, Andy Zhou wrote: > > Sorry for the delay in review. It took me a while to understand the > design > > intent to be able to understand the logic. > > Thanks for the review. > > If the design intent is unclear, then maybe I could make the code > clearer. I want my code to be easy to understand. Can you think of > some change that would make it more obvious? > One aspect of the design took me while to understand is that when thread woke up, it does not necessarily know which 'seq'(s) caused it to wake up and it is intentional. > > > A design question: It seems 'seq' API expects seq numbers to be > > monotonically increasing. is this true? If yes, would it make sense to > add > > some asserts? > > The seq numbers should be monotonically increasing, but I am not sure > what asserts would help. What do you suggest? > The code is fine. I did not realize that the seq_wait() API can handle value less than seq->value correctly. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] datapath: Do not set dev->master for XEN.
XEN dom0 networking assumes dev->master is bond device
and it tries to access bond private structure from dev->master ptr
on receive path. This causes panic, Therefore do not set dev->master
for XEN.
Signed-off-by: Pravin B Shelar
Bug #18920
---
datapath/linux/compat/include/linux/netdevice.h | 12
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/datapath/linux/compat/include/linux/netdevice.h
b/datapath/linux/compat/include/linux/netdevice.h
index 3f66d3a..9f49c26 100644
--- a/datapath/linux/compat/include/linux/netdevice.h
+++ b/datapath/linux/compat/include/linux/netdevice.h
@@ -190,6 +190,18 @@ static inline struct sk_buff *__skb_gso_segment(struct
sk_buff *skb,
#endif
#if LINUX_VERSION_CODE < KERNEL_VERSION(3,9,0)
+
+#if defined(CONFIG_XEN)
+/* XEN assumes dev->master as bond device which causes netif_receive_skb()
+ * panic, Therefore ignore upper device setting on XEN.
+ * ref: netif_receive_skb() for XEN dom0. */
+#define netdev_set_master rpl_netdev_set_master
+static inline int netdev_set_master(struct net_device *slave, struct
net_device *master)
+{
+ return 0;
+}
+#endif
+
static inline int netdev_master_upper_dev_link(struct net_device *dev,
struct net_device *upper_dev)
{
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath: Support for Linux kernel 3.10
On Tue, Aug 6, 2013 at 7:42 PM, Jesse Gross wrote: > On Fri, Aug 2, 2013 at 11:38 AM, Pravin B Shelar wrote: >> Changes are mostly related API changes in vlan, GRE >> restructuring. >> >> Signed-off-by: Pravin B Shelar > > Acked-by: Jesse Gross > > I agree with Kyle's comment about the FAQ and NEWS - I think this > probably belongs in 1.12 since it seems to affect so many people and > we're early on in the process. Thanks, I updated NEWS and FAQ and pushed patch to master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Use parallel_ops genl.
On Tue, Aug 6, 2013 at 7:44 PM, Jesse Gross wrote: > On Thu, Aug 1, 2013 at 3:36 PM, Pravin B Shelar wrote: >> OVS locking was recently changed to have private OVS lock which >> simplified overall locking. Therefore there is no need to have >> another global genl lock to protect OVS data structures. Following >> patch uses of parallel_ops genl family for OVS. This also allows >> more granual OVS locking using ovs_mutex for protecting OVS data >> structures, which gives more concurrencey. E.g multiple genl >> operations OVS_PACKET_CMD_EXECUTE can run in parallel, etc. >> >> Signed-off-by: Pravin B Shelar > > Acked-by: Jesse Gross Thanks, I pushed patch to master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] added locking to IPFIX exporter cache
Hi, I updated my IPFIX caching patch to add mutex locking to the run() and wait() functions. Thanks, -- Romain Lenglet ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ipfix: implement flow caching and aggregation in exporter
Implement a per-exporter flow cache with active timeout expiration.
Add columns "cache_active_timeout" and "cache_max_flows" into table
"IPFIX" to configure each cache.
Add per-flow elements "octetDeltaSumOfSquares",
"minimumIpTotalLength", and "maximumIpTotalLength" to replace
"ethernetTotalLength". Add per-flow element "flowEndReason" to
indicate whether a flow has expired because of an active timeout, the
cache size limit being reached, or the exporter being stopped.
Signed-off-by: Romain Lenglet
---
ofproto/ofproto-dpif-ipfix.c | 764 +-
ofproto/ofproto-dpif-ipfix.h |3 +
ofproto/ofproto-dpif.c | 23 +-
ofproto/ofproto.h|4 +
utilities/ovs-vsctl.8.in |5 +-
vswitchd/bridge.c| 10 +
vswitchd/vswitch.ovsschema | 12 +-
vswitchd/vswitch.xml | 12 +
8 files changed, 733 insertions(+), 100 deletions(-)
diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
index 8e8e7a2..c37da5f 100644
--- a/ofproto/ofproto-dpif-ipfix.c
+++ b/ofproto/ofproto-dpif-ipfix.c
@@ -15,15 +15,18 @@
*/
#include
+#include
#include "ofproto-dpif-ipfix.h"
#include "byte-order.h"
#include "collectors.h"
#include "flow.h"
#include "hash.h"
#include "hmap.h"
+#include "list.h"
#include "ofpbuf.h"
#include "ofproto.h"
#include "packets.h"
+#include "poll-loop.h"
#include "sset.h"
#include "util.h"
#include "timeval.h"
@@ -41,7 +44,11 @@ static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
struct dpif_ipfix_exporter {
struct collectors *collectors;
uint32_t seq_number;
-time_t last_template_set_time;
+__kernel_time_t last_template_set_time;
+struct hmap cache_flow_key_map; /* ipfix_flow_cache_entry. */
+struct list cache_flow_start_timestamp_list; /* ipfix_flow_cache_entry. */
+uint32_t cache_active_timeout; /* In seconds. */
+uint32_t cache_max_flows;
};
struct dpif_ipfix_bridge_exporter {
@@ -62,7 +69,7 @@ struct dpif_ipfix_flow_exporter_map_node {
struct dpif_ipfix {
struct dpif_ipfix_bridge_exporter bridge_exporter;
-struct hmap flow_exporter_map; /* dpif_ipfix_flow_exporter_map_nodes. */
+struct hmap flow_exporter_map; /* dpif_ipfix_flow_exporter_map_node. */
atomic_int ref_cnt;
};
@@ -143,32 +150,30 @@ struct ipfix_template_field_specifier {
});
BUILD_ASSERT_DECL(sizeof(struct ipfix_template_field_specifier) == 4);
-/* Part of data record for common metadata and Ethernet entities. */
+/* Part of data record flow key for common metadata and Ethernet entities. */
OVS_PACKED(
-struct ipfix_data_record_common {
+struct ipfix_data_record_flow_key_common {
ovs_be32 observation_point_id; /* OBSERVATION_POINT_ID */
-ovs_be64 packet_delta_count; /* PACKET_DELTA_COUNT */
-ovs_be64 layer2_octet_delta_count; /* LAYER2_OCTET_DELTA_COUNT */
uint8_t source_mac_address[6]; /* SOURCE_MAC_ADDRESS */
uint8_t destination_mac_address[6]; /* DESTINATION_MAC_ADDRESS */
ovs_be16 ethernet_type; /* ETHERNET_TYPE */
-ovs_be16 ethernet_total_length; /* ETHERNET_TOTAL_LENGTH */
uint8_t ethernet_header_length; /* ETHERNET_HEADER_LENGTH */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_common) == 37);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_common) == 19);
-/* Part of data record for VLAN entities. */
+/* Part of data record flow key for VLAN entities. */
OVS_PACKED(
-struct ipfix_data_record_vlan {
+struct ipfix_data_record_flow_key_vlan {
ovs_be16 vlan_id; /* VLAN_ID */
ovs_be16 dot1q_vlan_id; /* DOT1Q_VLAN_ID */
uint8_t dot1q_priority; /* DOT1Q_PRIORITY */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_vlan) == 5);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_vlan) == 5);
-/* Part of data record for IP entities. */
+/* Part of data record flow key for IP entities. */
+/* XXX: Replace IP_TTL with MINIMUM_TTL and MAXIMUM_TTL? */
OVS_PACKED(
-struct ipfix_data_record_ip {
+struct ipfix_data_record_flow_key_ip {
uint8_t ip_version; /* IP_VERSION */
uint8_t ip_ttl; /* IP_TTL */
uint8_t protocol_identifier; /* PROTOCOL_IDENTIFIER */
@@ -176,32 +181,122 @@ struct ipfix_data_record_ip {
uint8_t ip_precedence; /* IP_PRECEDENCE */
uint8_t ip_class_of_service; /* IP_CLASS_OF_SERVICE */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_ip) == 6);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_ip) == 6);
-/* Part of data record for IPv4 entities. */
+/* Part of data record flow key for IPv4 entities. */
OVS_PACKED(
-struct ipfix_data_record_ipv4 {
+struct ipfix_data_record_flow_key_ipv4 {
ovs_be32 source_ipv4_address; /* SOURCE_IPV4_ADDRESS */
ovs_be32 destination_ipv4_address; /* DESTINATION_IPV4_ADDRESS */
});
-BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_ipv4) == 8);
+BUILD_ASSERT_DECL(sizeof(struct ipfix_data_record_flow_key_ipv4) == 8);
-/* Part of data record
Re: [ovs-dev] [PATCH 4/5] datapath lisp: use iptunnel_pull_header().
On Wed, Aug 7, 2013 at 1:54 PM, Lori Jakab wrote:
> On 7/30/13 1:47 AM, Pravin B Shelar wrote:
>>
>> CC: Lori Jakab
>> Signed-off-by: Pravin B Shelar
>> ---
>> datapath/vport-lisp.c | 48
>> ++--
>> 1 files changed, 2 insertions(+), 46 deletions(-)
>>
>> diff --git a/datapath/vport-lisp.c b/datapath/vport-lisp.c
>> index 6e37b2f..847cb39 100644
>> --- a/datapath/vport-lisp.c
>> +++ b/datapath/vport-lisp.c
>> @@ -206,48 +206,6 @@ static void lisp_build_header(const struct vport
>> *vport,
>> lisph->u2.word2.locator_status_bits = 1;
>> }
>> -/**
>> - * ovs_tnl_rcv - ingress point for generic tunnel code
>> - *
>> - * @vport: port this packet was received on
>> - * @skb: received packet
>> - * @tos: ToS from encapsulating IP packet, used to copy ECN bits
>> - *
>> - * Must be called with rcu_read_lock.
>> - *
>> - * Packets received by this function are in the following state:
>> - * - skb->data points to the inner Ethernet header.
>> - * - The inner Ethernet header is in the linear data area.
>> - * - skb->csum does not include the inner Ethernet header.
>> - * - The layer pointers are undefined.
>> - */
>> -static void ovs_tnl_rcv(struct vport *vport, struct sk_buff *skb,
>> - struct ovs_key_ipv4_tunnel *tun_key)
>> -{
>> - struct ethhdr *eh;
>> -
>> - skb_reset_mac_header(skb);
>> - eh = eth_hdr(skb);
>> -
>> - if (likely(ntohs(eh->h_proto) >= ETH_P_802_3_MIN))
>> - skb->protocol = eh->h_proto;
>> - else
>> - skb->protocol = htons(ETH_P_802_2);
>> -
>> - skb_dst_drop(skb);
>> - nf_reset(skb);
>> - skb_clear_rxhash(skb);
>> - secpath_reset(skb);
>> - vlan_set_tci(skb, 0);
>> -
>> - if (unlikely(compute_ip_summed(skb, false))) {
>> - kfree_skb(skb);
>> - return;
>> - }
>> -
>> - ovs_vport_receive(vport, skb, tun_key);
>> -}
>> -
>> /* Called with rcu_read_lock and BH disabled. */
>> static int lisp_rcv(struct sock *sk, struct sk_buff *skb)
>> {
>> @@ -263,13 +221,11 @@ static int lisp_rcv(struct sock *sk, struct sk_buff
>> *skb)
>> if (unlikely(!lisp_port))
>> goto error;
>> - if (unlikely(!pskb_may_pull(skb, LISP_HLEN)))
>> + if (iptunnel_pull_header(skb, LISP_HLEN, htons(ETH_P_TEB)))
>
>
> Why use ETH_P_TEB for the inner_proto ? That will make
> iptunnel_pull_header() assume there is an Ethernet header after the LISP
> header and try to extract the inner Ethertype. It does not affect
> functionality, since we explicitly determine and set it later on, but if we
> use an arbitrary value here, we might as well use ETH_P_IP. The other
> option would be to determine the inner Ethertype before the
> iptunnel_pull_header() call, and use the correct one.
>
>
ok, I will pass zero as undefined protocol. Then will set protocol
later once it is determined.
>> goto error;
>> lisph = lisp_hdr(skb);
>> - skb_pull_rcsum(skb, LISP_HLEN);
>> -
>> if (lisph->instance_id_present != 1)
>> key = 0;
>> else
>> @@ -301,7 +257,7 @@ static int lisp_rcv(struct sock *sk, struct sk_buff
>> *skb)
>> ovs_skb_postpush_rcsum(skb, skb->data, ETH_HLEN);
>> - ovs_tnl_rcv(vport_from_priv(lisp_port), skb, &tun_key);
>> + ovs_vport_receive(vport_from_priv(lisp_port), skb, &tun_key);
>> goto out;
>> error:
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] bond.c: Fix a type.
This commit fixes a typo in "lib/bond.c" which causes the high CPU
utilization after adding bond.
Signed-off-by: Alex Wang
---
lib/bond.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/lib/bond.c b/lib/bond.c
index 06dd362..a9278db 100644
--- a/lib/bond.c
+++ b/lib/bond.c
@@ -475,7 +475,7 @@ bond_wait(struct bond *bond)
poll_timer_wait_until(bond->next_fake_iface_update);
}
-if (!bond->bond_revalidate) {
+if (bond->bond_revalidate) {
poll_immediate_wake();
}
ovs_rwlock_unlock(&rwlock);
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Adds way to determine db changes from Idl.run()
On Fri, Aug 9, 2013 at 2:50 PM, Reid Price wrote: > Or you could keep the original function behavior the same and expose this > as a separate function > > def foo(...): > > > def run(...): > return self.foo(...)[0] > > where foo is a better function name - update? run_details? > run_with_changes? run_diff? _run? No opinion there. > > -Reid > > > On Fri, Aug 9, 2013 at 2:30 PM, Aaron Rosen wrote: > >> Right, this would break things for anyone checking the return value of >> idl.run(). The only alternative I see to that is if we pass an optional arg >> to run() (i.e: def run(self, return_changes=False)). Would you prefer this >> instead? >> >> Thanks, >> >> Aaron >> >> >> On Fri, Aug 9, 2013 at 1:02 PM, Ben Pfaff wrote: >> >>> On Tue, Aug 06, 2013 at 02:45:35PM -0700, Aaron Rosen wrote: >>> > This patch changes what is being returned from Idl.run() to a tuple >>> > (changed, changes) so one can determine what changes have occurred to >>> > the database without having to read the entire table. >>> > >>> > Signed-off-by: Aaron Rosen >>> >>> It seems like a reasonable idea but I suspect it doesn't fix up all >>> the users. Also the patch is wordwrapped so I can't apply it. >>> >>> Thanks, >>> >>> Ben. >>> >> >> >> ___ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >> >> > 0001-Add-run_with_changes-method-to-Idl.patch Description: Binary data ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/3] netdev-dummy: Include all dummy classes in iterations.
Commit 86f1d0326bd0 (netdev-dummy: Use netdev_get_devices() instead of a
local shash.) caused netdev-dummy functions that iterate over all dummy
devices to iterate only over the ones that have class 'dummy_class'. This
seemed to obviously include all the ones that we want, but in fact
when ovs-vswitch is invoked with --enable-dummy=override, there are more
dummy classes than just dummy_class, which this new form of iteration
skipped over, with various negative consequences that showed up in some
testing.
This commit switches netdev-dummy back to internally tracking its own
dummy devices. It fixes the tests for me.
Signed-off-by: Ben Pfaff
---
lib/netdev-dummy.c | 61 ++--
1 file changed, 31 insertions(+), 30 deletions(-)
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 7c43f12..5c31210 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -44,11 +44,21 @@ struct dummy_stream {
struct list txq;
};
+/* Protects 'dummy_list'. */
+static struct ovs_mutex dummy_list_mutex = OVS_MUTEX_INITIALIZER;
+
+/* Contains all 'struct dummy_dev's. */
+static struct list dummy_list OVS_GUARDED_BY(dummy_list_mutex)
+= LIST_INITIALIZER(&dummy_list);
+
struct netdev_dummy {
struct netdev up;
+/* In dummy_list. */
+struct list list_node OVS_GUARDED_BY(dummy_list_mutex);
+
/* Protects all members below. */
-struct ovs_mutex mutex;
+struct ovs_mutex mutex OVS_ACQ_AFTER(dummy_list_mutex);
uint8_t hwaddr[ETH_ADDR_LEN];
int mtu;
@@ -64,8 +74,6 @@ struct netdev_dummy {
struct list rxes; /* List of child "netdev_rx_dummy"s. */
};
-static const struct netdev_class dummy_class;
-
/* Max 'recv_queue_len' in struct netdev_dummy. */
#define NETDEV_DUMMY_MAX_QUEUE 100
@@ -108,13 +116,10 @@ netdev_rx_dummy_cast(const struct netdev_rx *rx)
static void
netdev_dummy_run(void)
{
-struct shash dummy_netdevs;
-struct shash_node *node;
+struct netdev_dummy *dev;
-shash_init(&dummy_netdevs);
-netdev_get_devices(&dummy_class, &dummy_netdevs);
-SHASH_FOR_EACH (node, &dummy_netdevs) {
-struct netdev_dummy *dev = node->data;
+ovs_mutex_lock(&dummy_list_mutex);
+LIST_FOR_EACH (dev, list_node, &dummy_list) {
size_t i;
ovs_mutex_lock(&dev->mutex);
@@ -211,9 +216,8 @@ netdev_dummy_run(void)
}
ovs_mutex_unlock(&dev->mutex);
-netdev_close(&dev->up);
}
-shash_destroy(&dummy_netdevs);
+ovs_mutex_unlock(&dummy_list_mutex);
}
static void
@@ -227,13 +231,10 @@ dummy_stream_close(struct dummy_stream *s)
static void
netdev_dummy_wait(void)
{
-struct shash dummy_netdevs;
-struct shash_node *node;
+struct netdev_dummy *dev;
-shash_init(&dummy_netdevs);
-netdev_get_devices(&dummy_class, &dummy_netdevs);
-SHASH_FOR_EACH (node, &dummy_netdevs) {
-struct netdev_dummy *dev = node->data;
+ovs_mutex_lock(&dummy_list_mutex);
+LIST_FOR_EACH (dev, list_node, &dummy_list) {
size_t i;
ovs_mutex_lock(&dev->mutex);
@@ -250,9 +251,8 @@ netdev_dummy_wait(void)
stream_recv_wait(s->stream);
}
ovs_mutex_unlock(&dev->mutex);
-netdev_close(&dev->up);
}
-shash_destroy(&dummy_netdevs);
+ovs_mutex_unlock(&dummy_list_mutex);
}
static struct netdev *
@@ -289,6 +289,10 @@ netdev_dummy_construct(struct netdev *netdev_)
list_init(&netdev->rxes);
+ovs_mutex_lock(&dummy_list_mutex);
+list_push_back(&dummy_list, &netdev->list_node);
+ovs_mutex_unlock(&dummy_list_mutex);
+
return 0;
}
@@ -298,6 +302,10 @@ netdev_dummy_destruct(struct netdev *netdev_)
struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
size_t i;
+ovs_mutex_lock(&dummy_list_mutex);
+list_remove(&netdev->list_node);
+ovs_mutex_unlock(&dummy_list_mutex);
+
pstream_close(netdev->pstream);
for (i = 0; i < netdev->n_streams; i++) {
dummy_stream_close(&netdev->streams[i]);
@@ -863,22 +871,15 @@ netdev_dummy_set_admin_state(struct unixctl_conn *conn,
int argc,
return;
}
} else {
-struct shash dummy_netdevs;
-struct shash_node *node;
-
-shash_init(&dummy_netdevs);
-netdev_get_devices(&dummy_class, &dummy_netdevs);
-SHASH_FOR_EACH (node, &dummy_netdevs) {
-struct netdev *netdev_ = node->data;
-struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
+struct netdev_dummy *netdev;
+ovs_mutex_lock(&dummy_list_mutex);
+LIST_FOR_EACH (netdev, list_node, &dummy_list) {
ovs_mutex_lock(&netdev->mutex);
netdev_dummy_set_admin_state__(netdev, up);
ovs_mutex_unlock(&netdev->mutex);
-
-netdev_close(netdev_);
}
-shash_destroy(&dummy_netdevs);
+ovs_mutex_unlock(&dummy_list_mutex);
}
unixctl_command_reply(conn
[ovs-dev] [PATCH 3/3] netdev-dummy: Add Clang lock annotations.
The locks and unlocks added in this commit are superfluous but should not
hurt performance and make Clang happy.
Signed-off-by: Ben Pfaff
---
lib/netdev-dummy.c | 31 +++
1 file changed, 19 insertions(+), 12 deletions(-)
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 32b0943..e17ef9d 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -60,18 +60,18 @@ struct netdev_dummy {
/* Protects all members below. */
struct ovs_mutex mutex OVS_ACQ_AFTER(dummy_list_mutex);
-uint8_t hwaddr[ETH_ADDR_LEN];
-int mtu;
-struct netdev_stats stats;
-enum netdev_flags flags;
-unsigned int change_seq;
-int ifindex;
-
-struct pstream *pstream;
-struct dummy_stream *streams;
-size_t n_streams;
-
-struct list rxes; /* List of child "netdev_rx_dummy"s. */
+uint8_t hwaddr[ETH_ADDR_LEN] OVS_GUARDED;
+int mtu OVS_GUARDED;
+struct netdev_stats stats OVS_GUARDED;
+enum netdev_flags flags OVS_GUARDED;
+unsigned int change_seq OVS_GUARDED;
+int ifindex OVS_GUARDED;
+
+struct pstream *pstream OVS_GUARDED;
+struct dummy_stream *streams OVS_GUARDED;
+size_t n_streams OVS_GUARDED;
+
+struct list rxes OVS_GUARDED; /* List of child "netdev_rx_dummy"s. */
};
/* Max 'recv_queue_len' in struct netdev_dummy. */
@@ -272,6 +272,7 @@ netdev_dummy_construct(struct netdev *netdev_)
atomic_add(&next_n, 1, &n);
ovs_mutex_init(&netdev->mutex, PTHREAD_MUTEX_NORMAL);
+ovs_mutex_lock(&netdev->mutex);
netdev->hwaddr[0] = 0xaa;
netdev->hwaddr[1] = 0x55;
netdev->hwaddr[2] = n >> 24;
@@ -288,6 +289,7 @@ netdev_dummy_construct(struct netdev *netdev_)
netdev->n_streams = 0;
list_init(&netdev->rxes);
+ovs_mutex_unlock(&netdev->mutex);
ovs_mutex_lock(&dummy_list_mutex);
list_push_back(&dummy_list, &netdev->list_node);
@@ -306,11 +308,13 @@ netdev_dummy_destruct(struct netdev *netdev_)
list_remove(&netdev->list_node);
ovs_mutex_unlock(&dummy_list_mutex);
+ovs_mutex_lock(&netdev->mutex);
pstream_close(netdev->pstream);
for (i = 0; i < netdev->n_streams; i++) {
dummy_stream_close(&netdev->streams[i]);
}
free(netdev->streams);
+ovs_mutex_unlock(&netdev->mutex);
ovs_mutex_destroy(&netdev->mutex);
}
@@ -483,7 +487,10 @@ netdev_dummy_send(struct netdev *netdev, const void
*buffer, size_t size)
const struct eth_header *eth = buffer;
int max_size;
+ovs_mutex_lock(&dev->mutex);
max_size = dev->mtu + ETH_HEADER_LEN;
+ovs_mutex_unlock(&dev->mutex);
+
if (eth->eth_type == htons(ETH_TYPE_VLAN)) {
max_size += VLAN_HEADER_LEN;
}
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/3] netdev-dummy: Fix synchronization error in netdev_dummy_get_config().
Found by Clang.
Signed-off-by: Ben Pfaff
---
lib/netdev-dummy.c |3 +++
1 file changed, 3 insertions(+)
diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
index 5c31210..32b0943 100644
--- a/lib/netdev-dummy.c
+++ b/lib/netdev-dummy.c
@@ -327,12 +327,15 @@ netdev_dummy_get_config(const struct netdev *netdev_,
struct smap *args)
{
struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
+ovs_mutex_lock(&netdev->mutex);
if (netdev->ifindex >= 0) {
smap_add_format(args, "ifindex", "%d", netdev->ifindex);
}
if (netdev->pstream) {
smap_add(args, "pstream", pstream_get_name(netdev->pstream));
}
+ovs_mutex_unlock(&netdev->mutex);
+
return 0;
}
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] bond.c: Fix a type.
On Mon, Aug 12, 2013 at 12:01:46PM -0700, Alex Wang wrote: > This commit fixes a typo in "lib/bond.c" which causes the high CPU > utilization after adding bond. > > Signed-off-by: Alex Wang Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 5/5 v2] dpif-netdev: Avoid races on queue and port changes using seq objects.
To be specific, I guess we are talking about
dp_netdev_output_userspace() calling seq_change() and
dpif_netdev_recv_wait() calling seq_wait(). As you say, they are
serialized by dp_netdev_mutex, so there are two possible orders:
- If dp_netdev_output_userspace() runs first, then queue_seq does
not matter at all, because find_nonempty_queue() will return
true and poll_immediate_wake() will be called.
- If dpif_netdev_recv_wait() runs first, then it will read some
sequence number N and pass that to seq_wait().
dp_netdev_output_userspace() will then increment the sequence
number, which (barring a bug inside the seq module) will prevent
the subsequent poll_block() from sleeping.
On Mon, Aug 12, 2013 at 09:31:36AM -0700, Andy Zhou wrote:
> Here is an example I can think of:
>
> Thread A wants to do a seq_change(), Thread B wants to do seq_wait(), they
> both compete for dp_netdev_mutex at the same time.
>
> Assume Thread A wins, seq_change changed the seq->value to +1 and releases
> the dp_netdev_mutex. Thread B now waits for the +1 value to change, thus
> missing Thread A's +1 event.
>
> Is this possible?
>
>
>
> On Sat, Aug 10, 2013 at 8:57 PM, Ben Pfaff wrote:
>
> > On Sat, Aug 10, 2013 at 04:39:09PM -0700, Andy Zhou wrote:
> > > On Wed, Aug 7, 2013 at 1:31 PM, Ben Pfaff wrote:
> > > > dpif_upcall *upcall,
> > > > static void
> > > > dpif_netdev_recv_wait(struct dpif *dpif)
> > > > {
> > > > -/* XXX In a multithreaded process, there is a race window between
> > this
> > > > - * function and the poll_block() in one thread and a packet being
> > > > queued in
> > > > - * another thread. */
> > > > +struct dp_netdev *dp = get_dp_netdev(dpif);
> > > > +uint64_t seq;
> > > >
> > > > ovs_mutex_lock(&dp_netdev_mutex);
> > > > +seq = seq_read(dp->queue_seq);
> > > >
> > >
> > > Is there a risk of lossing events in case seq_change() is executed in
> > > another thread while this thread is waiting
> > > on the dp_netdev_mutex lock?
> >
> > I guess by "losing events" you mean sleeping too long, beyond the time
> > at which an upcall is queued? I don't think we can lose events here,
> > then. There are two cases. The first case, where we call
> > poll_immediate_wake(), is obviously OK. In the other case, we know that
> > the queues were empty at a sequence number of 'seq' or later, so calling
> > seq_wait() passing 'seq' will ensure that we wake up when the sequence
> > number becomes greater than 'seq', which will definitely happen whenever
> > an upcall gets queued.
> >
> > Do you see a hole in the logic?
> >
> > Thanks,
> >
> > Ben.
> >
> > > > if (find_nonempty_queue(dpif)) {
> > > > poll_immediate_wake();
> > > > +} else {
> > > > +seq_wait(dp->queue_seq, seq);
> > > > }
> > > > ovs_mutex_unlock(&dp_netdev_mutex);
> > > > }
> >
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 5/5 v2] dpif-netdev: Avoid races on queue and port changes using seq objects.
I was thinking of the first case. You are right, it is covered by
find_nonempty_queue().
Then this patch looks good to me.
On Mon, Aug 12, 2013 at 1:06 PM, Ben Pfaff wrote:
> To be specific, I guess we are talking about
> dp_netdev_output_userspace() calling seq_change() and
> dpif_netdev_recv_wait() calling seq_wait(). As you say, they are
> serialized by dp_netdev_mutex, so there are two possible orders:
>
> - If dp_netdev_output_userspace() runs first, then queue_seq does
> not matter at all, because find_nonempty_queue() will return
> true and poll_immediate_wake() will be called.
>
> - If dpif_netdev_recv_wait() runs first, then it will read some
> sequence number N and pass that to seq_wait().
> dp_netdev_output_userspace() will then increment the sequence
> number, which (barring a bug inside the seq module) will prevent
> the subsequent poll_block() from sleeping.
>
> On Mon, Aug 12, 2013 at 09:31:36AM -0700, Andy Zhou wrote:
> > Here is an example I can think of:
> >
> > Thread A wants to do a seq_change(), Thread B wants to do seq_wait(),
> they
> > both compete for dp_netdev_mutex at the same time.
> >
> > Assume Thread A wins, seq_change changed the seq->value to +1 and
> releases
> > the dp_netdev_mutex. Thread B now waits for the +1 value to change, thus
> > missing Thread A's +1 event.
> >
> > Is this possible?
> >
> >
> >
> > On Sat, Aug 10, 2013 at 8:57 PM, Ben Pfaff wrote:
> >
> > > On Sat, Aug 10, 2013 at 04:39:09PM -0700, Andy Zhou wrote:
> > > > On Wed, Aug 7, 2013 at 1:31 PM, Ben Pfaff wrote:
> > > > > dpif_upcall *upcall,
> > > > > static void
> > > > > dpif_netdev_recv_wait(struct dpif *dpif)
> > > > > {
> > > > > -/* XXX In a multithreaded process, there is a race window
> between
> > > this
> > > > > - * function and the poll_block() in one thread and a packet
> being
> > > > > queued in
> > > > > - * another thread. */
> > > > > +struct dp_netdev *dp = get_dp_netdev(dpif);
> > > > > +uint64_t seq;
> > > > >
> > > > > ovs_mutex_lock(&dp_netdev_mutex);
> > > > > +seq = seq_read(dp->queue_seq);
> > > > >
> > > >
> > > > Is there a risk of lossing events in case seq_change() is executed in
> > > > another thread while this thread is waiting
> > > > on the dp_netdev_mutex lock?
> > >
> > > I guess by "losing events" you mean sleeping too long, beyond the time
> > > at which an upcall is queued? I don't think we can lose events here,
> > > then. There are two cases. The first case, where we call
> > > poll_immediate_wake(), is obviously OK. In the other case, we know
> that
> > > the queues were empty at a sequence number of 'seq' or later, so
> calling
> > > seq_wait() passing 'seq' will ensure that we wake up when the sequence
> > > number becomes greater than 'seq', which will definitely happen
> whenever
> > > an upcall gets queued.
> > >
> > > Do you see a hole in the logic?
> > >
> > > Thanks,
> > >
> > > Ben.
> > >
> > > > > if (find_nonempty_queue(dpif)) {
> > > > > poll_immediate_wake();
> > > > > +} else {
> > > > > +seq_wait(dp->queue_seq, seq);
> > > > > }
> > > > > ovs_mutex_unlock(&dp_netdev_mutex);
> > > > > }
> > >
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2] datapath: Do not set dev->master for XEN.
XEN dom0 networking assumes dev->master is bond device
and it tries to access bond private structure from dev->master
ptr on receive path. This causes panic.
Following patch removes compat code that is setting master
device.
Signed-off-by: Pravin B Shelar
Bug #18920
---
v1-v2:
Fixed according to comments from Jesse.
---
datapath/linux/compat/include/linux/netdevice.h |3 +--
1 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/datapath/linux/compat/include/linux/netdevice.h
b/datapath/linux/compat/include/linux/netdevice.h
index 3f66d3a..cf03821 100644
--- a/datapath/linux/compat/include/linux/netdevice.h
+++ b/datapath/linux/compat/include/linux/netdevice.h
@@ -193,13 +193,12 @@ static inline struct sk_buff *__skb_gso_segment(struct
sk_buff *skb,
static inline int netdev_master_upper_dev_link(struct net_device *dev,
struct net_device *upper_dev)
{
- return netdev_set_master(dev, upper_dev);
+ return 0;
}
static inline void netdev_upper_dev_unlink(struct net_device *dev,
struct net_device *upper_dev)
{
- netdev_set_master(dev, NULL);
}
#endif
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] seq: Add some comments.
Signed-off-by: Ben Pfaff
---
lib/seq.c | 12 ++--
lib/seq.h | 40 +++-
2 files changed, 49 insertions(+), 3 deletions(-)
diff --git a/lib/seq.c b/lib/seq.c
index abe1ad8..36e5065 100644
--- a/lib/seq.c
+++ b/lib/seq.c
@@ -106,7 +106,11 @@ seq_change(struct seq *seq)
ovs_mutex_unlock(&seq_mutex);
}
-/* Returns 'seq''s current sequence number (which could change immediately). */
+/* Returns 'seq''s current sequence number (which could change immediately).
+ *
+ * seq_read() and seq_wait() can be used together to yield a race-free wakeup
+ * when an object changes, even without an ability to lock the object. See
+ * Usage in seq.h for details. */
uint64_t
seq_read(const struct seq *seq)
OVS_EXCLUDED(seq_mutex)
@@ -156,7 +160,11 @@ seq_wait__(struct seq *seq, uint64_t value)
/* Causes the following poll_block() to wake up when 'seq''s sequence number
* changes from 'value'. (If 'seq''s sequence number isn't 'value', then
- * poll_block() won't block at all.) */
+ * poll_block() won't block at all.)
+ *
+ * seq_read() and seq_wait() can be used together to yield a race-free wakeup
+ * when an object changes, even without an ability to lock the object. See
+ * Usage in seq.h for details. */
void
seq_wait(const struct seq *seq_, uint64_t value)
OVS_EXCLUDED(seq_mutex)
diff --git a/lib/seq.h b/lib/seq.h
index 3423e21..c764809 100644
--- a/lib/seq.h
+++ b/lib/seq.h
@@ -20,7 +20,7 @@
/* Thread-safe, pollable sequence number.
*
*
- * Background
+ * Motivation
* ==
*
* It is sometimes desirable to take an action whenever an object changes.
@@ -66,6 +66,44 @@
*poll_block();
*
*
+ * Alternate Usage
+ * ===
+ *
+ * struct seq can also be used as a sort of pollable condition variable.
+ * Suppose that we want a thread to process items in a queue, and thus to be
+ * able to wake up whenever the queue is nonempty. This requires a lock to
+ * protect the queue and a seq to signal that the queue has become nonempty,
+ * e.g.:
+ *
+ *struct ovs_mutex mutex;
+ *struct list queue OVS_GUARDED_BY(mutex);
+ *struct seq nonempty_seq;
+ *
+ * To add an element to the queue:
+ *
+ *ovs_mutex_lock(&mutex);
+ *list_push_back(&queue, ...element...);
+ *if (list_is_singleton(&queue)) { // The 'if' test here is optional.
+ *seq_change(&nonempty_seq);
+ *}
+ *ovs_mutex_unlock(&mutex);
+ *
+ * To wait for the queue to become nonempty:
+ *
+ *ovs_mutex_lock(&mutex);
+ *if (list_is_empty(&queue)) {
+ *seq_wait(&nonempty_seq, seq_read(&nonempty_seq));
+ *} else {
+ *poll_immediate_wake();
+ *}
+ *ovs_mutex_unlock(&mutex);
+ *
+ * (In the above code 'mutex' prevents the queue from changing between
+ * seq_read() and seq_wait(). Otherwise, it would be necessary to seq_read(),
+ * check for a nonempty queue, and then seq_wait() on the previously read
+ * sequence number, as under Usage above.)
+ *
+ *
* Thread-safety
* =
*
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] netdev-dummy: Include all dummy classes in iterations.
Acked-by: Andy Zhou
On Mon, Aug 12, 2013 at 12:54 PM, Ben Pfaff wrote:
> Commit 86f1d0326bd0 (netdev-dummy: Use netdev_get_devices() instead of a
> local shash.) caused netdev-dummy functions that iterate over all dummy
> devices to iterate only over the ones that have class 'dummy_class'. This
> seemed to obviously include all the ones that we want, but in fact
> when ovs-vswitch is invoked with --enable-dummy=override, there are more
> dummy classes than just dummy_class, which this new form of iteration
> skipped over, with various negative consequences that showed up in some
> testing.
>
> This commit switches netdev-dummy back to internally tracking its own
> dummy devices. It fixes the tests for me.
>
> Signed-off-by: Ben Pfaff
> ---
> lib/netdev-dummy.c | 61
> ++--
> 1 file changed, 31 insertions(+), 30 deletions(-)
>
> diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> index 7c43f12..5c31210 100644
> --- a/lib/netdev-dummy.c
> +++ b/lib/netdev-dummy.c
> @@ -44,11 +44,21 @@ struct dummy_stream {
> struct list txq;
> };
>
> +/* Protects 'dummy_list'. */
> +static struct ovs_mutex dummy_list_mutex = OVS_MUTEX_INITIALIZER;
> +
> +/* Contains all 'struct dummy_dev's. */
> +static struct list dummy_list OVS_GUARDED_BY(dummy_list_mutex)
> += LIST_INITIALIZER(&dummy_list);
> +
> struct netdev_dummy {
> struct netdev up;
>
> +/* In dummy_list. */
> +struct list list_node OVS_GUARDED_BY(dummy_list_mutex);
> +
> /* Protects all members below. */
> -struct ovs_mutex mutex;
> +struct ovs_mutex mutex OVS_ACQ_AFTER(dummy_list_mutex);
>
> uint8_t hwaddr[ETH_ADDR_LEN];
> int mtu;
> @@ -64,8 +74,6 @@ struct netdev_dummy {
> struct list rxes; /* List of child "netdev_rx_dummy"s. */
> };
>
> -static const struct netdev_class dummy_class;
> -
> /* Max 'recv_queue_len' in struct netdev_dummy. */
> #define NETDEV_DUMMY_MAX_QUEUE 100
>
> @@ -108,13 +116,10 @@ netdev_rx_dummy_cast(const struct netdev_rx *rx)
> static void
> netdev_dummy_run(void)
> {
> -struct shash dummy_netdevs;
> -struct shash_node *node;
> +struct netdev_dummy *dev;
>
> -shash_init(&dummy_netdevs);
> -netdev_get_devices(&dummy_class, &dummy_netdevs);
> -SHASH_FOR_EACH (node, &dummy_netdevs) {
> -struct netdev_dummy *dev = node->data;
> +ovs_mutex_lock(&dummy_list_mutex);
> +LIST_FOR_EACH (dev, list_node, &dummy_list) {
> size_t i;
>
> ovs_mutex_lock(&dev->mutex);
> @@ -211,9 +216,8 @@ netdev_dummy_run(void)
> }
>
> ovs_mutex_unlock(&dev->mutex);
> -netdev_close(&dev->up);
> }
> -shash_destroy(&dummy_netdevs);
> +ovs_mutex_unlock(&dummy_list_mutex);
> }
>
> static void
> @@ -227,13 +231,10 @@ dummy_stream_close(struct dummy_stream *s)
> static void
> netdev_dummy_wait(void)
> {
> -struct shash dummy_netdevs;
> -struct shash_node *node;
> +struct netdev_dummy *dev;
>
> -shash_init(&dummy_netdevs);
> -netdev_get_devices(&dummy_class, &dummy_netdevs);
> -SHASH_FOR_EACH (node, &dummy_netdevs) {
> -struct netdev_dummy *dev = node->data;
> +ovs_mutex_lock(&dummy_list_mutex);
> +LIST_FOR_EACH (dev, list_node, &dummy_list) {
> size_t i;
>
> ovs_mutex_lock(&dev->mutex);
> @@ -250,9 +251,8 @@ netdev_dummy_wait(void)
> stream_recv_wait(s->stream);
> }
> ovs_mutex_unlock(&dev->mutex);
> -netdev_close(&dev->up);
> }
> -shash_destroy(&dummy_netdevs);
> +ovs_mutex_unlock(&dummy_list_mutex);
> }
>
> static struct netdev *
> @@ -289,6 +289,10 @@ netdev_dummy_construct(struct netdev *netdev_)
>
> list_init(&netdev->rxes);
>
> +ovs_mutex_lock(&dummy_list_mutex);
> +list_push_back(&dummy_list, &netdev->list_node);
> +ovs_mutex_unlock(&dummy_list_mutex);
> +
> return 0;
> }
>
> @@ -298,6 +302,10 @@ netdev_dummy_destruct(struct netdev *netdev_)
> struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
> size_t i;
>
> +ovs_mutex_lock(&dummy_list_mutex);
> +list_remove(&netdev->list_node);
> +ovs_mutex_unlock(&dummy_list_mutex);
> +
> pstream_close(netdev->pstream);
> for (i = 0; i < netdev->n_streams; i++) {
> dummy_stream_close(&netdev->streams[i]);
> @@ -863,22 +871,15 @@ netdev_dummy_set_admin_state(struct unixctl_conn
> *conn, int argc,
> return;
> }
> } else {
> -struct shash dummy_netdevs;
> -struct shash_node *node;
> -
> -shash_init(&dummy_netdevs);
> -netdev_get_devices(&dummy_class, &dummy_netdevs);
> -SHASH_FOR_EACH (node, &dummy_netdevs) {
> -struct netdev *netdev_ = node->data;
> -struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
> +struct netdev_dummy *netdev;
>
> +ovs_mutex_lock(&dummy_list_mutex);
> +LIST_FOR_EACH (netdev, list_node,
Re: [ovs-dev] [PATCH 1/3] netdev-dummy: Include all dummy classes in iterations.
Thanks, applied.
On Mon, Aug 12, 2013 at 01:46:15PM -0700, Andy Zhou wrote:
> Acked-by: Andy Zhou
>
>
>
> On Mon, Aug 12, 2013 at 12:54 PM, Ben Pfaff wrote:
>
> > Commit 86f1d0326bd0 (netdev-dummy: Use netdev_get_devices() instead of a
> > local shash.) caused netdev-dummy functions that iterate over all dummy
> > devices to iterate only over the ones that have class 'dummy_class'. This
> > seemed to obviously include all the ones that we want, but in fact
> > when ovs-vswitch is invoked with --enable-dummy=override, there are more
> > dummy classes than just dummy_class, which this new form of iteration
> > skipped over, with various negative consequences that showed up in some
> > testing.
> >
> > This commit switches netdev-dummy back to internally tracking its own
> > dummy devices. It fixes the tests for me.
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > lib/netdev-dummy.c | 61
> > ++--
> > 1 file changed, 31 insertions(+), 30 deletions(-)
> >
> > diff --git a/lib/netdev-dummy.c b/lib/netdev-dummy.c
> > index 7c43f12..5c31210 100644
> > --- a/lib/netdev-dummy.c
> > +++ b/lib/netdev-dummy.c
> > @@ -44,11 +44,21 @@ struct dummy_stream {
> > struct list txq;
> > };
> >
> > +/* Protects 'dummy_list'. */
> > +static struct ovs_mutex dummy_list_mutex = OVS_MUTEX_INITIALIZER;
> > +
> > +/* Contains all 'struct dummy_dev's. */
> > +static struct list dummy_list OVS_GUARDED_BY(dummy_list_mutex)
> > += LIST_INITIALIZER(&dummy_list);
> > +
> > struct netdev_dummy {
> > struct netdev up;
> >
> > +/* In dummy_list. */
> > +struct list list_node OVS_GUARDED_BY(dummy_list_mutex);
> > +
> > /* Protects all members below. */
> > -struct ovs_mutex mutex;
> > +struct ovs_mutex mutex OVS_ACQ_AFTER(dummy_list_mutex);
> >
> > uint8_t hwaddr[ETH_ADDR_LEN];
> > int mtu;
> > @@ -64,8 +74,6 @@ struct netdev_dummy {
> > struct list rxes; /* List of child "netdev_rx_dummy"s. */
> > };
> >
> > -static const struct netdev_class dummy_class;
> > -
> > /* Max 'recv_queue_len' in struct netdev_dummy. */
> > #define NETDEV_DUMMY_MAX_QUEUE 100
> >
> > @@ -108,13 +116,10 @@ netdev_rx_dummy_cast(const struct netdev_rx *rx)
> > static void
> > netdev_dummy_run(void)
> > {
> > -struct shash dummy_netdevs;
> > -struct shash_node *node;
> > +struct netdev_dummy *dev;
> >
> > -shash_init(&dummy_netdevs);
> > -netdev_get_devices(&dummy_class, &dummy_netdevs);
> > -SHASH_FOR_EACH (node, &dummy_netdevs) {
> > -struct netdev_dummy *dev = node->data;
> > +ovs_mutex_lock(&dummy_list_mutex);
> > +LIST_FOR_EACH (dev, list_node, &dummy_list) {
> > size_t i;
> >
> > ovs_mutex_lock(&dev->mutex);
> > @@ -211,9 +216,8 @@ netdev_dummy_run(void)
> > }
> >
> > ovs_mutex_unlock(&dev->mutex);
> > -netdev_close(&dev->up);
> > }
> > -shash_destroy(&dummy_netdevs);
> > +ovs_mutex_unlock(&dummy_list_mutex);
> > }
> >
> > static void
> > @@ -227,13 +231,10 @@ dummy_stream_close(struct dummy_stream *s)
> > static void
> > netdev_dummy_wait(void)
> > {
> > -struct shash dummy_netdevs;
> > -struct shash_node *node;
> > +struct netdev_dummy *dev;
> >
> > -shash_init(&dummy_netdevs);
> > -netdev_get_devices(&dummy_class, &dummy_netdevs);
> > -SHASH_FOR_EACH (node, &dummy_netdevs) {
> > -struct netdev_dummy *dev = node->data;
> > +ovs_mutex_lock(&dummy_list_mutex);
> > +LIST_FOR_EACH (dev, list_node, &dummy_list) {
> > size_t i;
> >
> > ovs_mutex_lock(&dev->mutex);
> > @@ -250,9 +251,8 @@ netdev_dummy_wait(void)
> > stream_recv_wait(s->stream);
> > }
> > ovs_mutex_unlock(&dev->mutex);
> > -netdev_close(&dev->up);
> > }
> > -shash_destroy(&dummy_netdevs);
> > +ovs_mutex_unlock(&dummy_list_mutex);
> > }
> >
> > static struct netdev *
> > @@ -289,6 +289,10 @@ netdev_dummy_construct(struct netdev *netdev_)
> >
> > list_init(&netdev->rxes);
> >
> > +ovs_mutex_lock(&dummy_list_mutex);
> > +list_push_back(&dummy_list, &netdev->list_node);
> > +ovs_mutex_unlock(&dummy_list_mutex);
> > +
> > return 0;
> > }
> >
> > @@ -298,6 +302,10 @@ netdev_dummy_destruct(struct netdev *netdev_)
> > struct netdev_dummy *netdev = netdev_dummy_cast(netdev_);
> > size_t i;
> >
> > +ovs_mutex_lock(&dummy_list_mutex);
> > +list_remove(&netdev->list_node);
> > +ovs_mutex_unlock(&dummy_list_mutex);
> > +
> > pstream_close(netdev->pstream);
> > for (i = 0; i < netdev->n_streams; i++) {
> > dummy_stream_close(&netdev->streams[i]);
> > @@ -863,22 +871,15 @@ netdev_dummy_set_admin_state(struct unixctl_conn
> > *conn, int argc,
> > return;
> > }
> > } else {
> > -struct shash dummy_netdevs;
> > -struct shash_node *node;
> > -
> > -shash_init(&dummy_netdevs);
[ovs-dev] [PATCH] band.c: Fix error in bond_choose_output_slave() function.
This commit fixes the error introduced by commit 4a1b8f30e59 (bond: Stop using tags.). The error is caused by mistakenly returning 'slave' where 'slave->aux' should be returned. Signed-off-by: Alex Wang --- lib/bond.c |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/bond.c b/lib/bond.c index a9278db..ebf2ee0 100644 --- a/lib/bond.c +++ b/lib/bond.c @@ -665,7 +665,7 @@ bond_choose_output_slave(struct bond *bond, const struct flow *flow, ovs_rwlock_rdlock(&rwlock); slave = choose_output_slave(bond, flow, wc, vlan); ovs_rwlock_unlock(&rwlock); -return slave; +return slave ? slave->aux : NULL; } /* Rebalancing. */ -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] seq: Add some comments.
Thanks for the added comments. Looks good.
Coding style says we should not use "//", but not sure if this applies to
comments.
On Mon, Aug 12, 2013 at 1:45 PM, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff
> ---
> lib/seq.c | 12 ++--
> lib/seq.h | 40 +++-
> 2 files changed, 49 insertions(+), 3 deletions(-)
>
> diff --git a/lib/seq.c b/lib/seq.c
> index abe1ad8..36e5065 100644
> --- a/lib/seq.c
> +++ b/lib/seq.c
> @@ -106,7 +106,11 @@ seq_change(struct seq *seq)
> ovs_mutex_unlock(&seq_mutex);
> }
>
> -/* Returns 'seq''s current sequence number (which could change
> immediately). */
> +/* Returns 'seq''s current sequence number (which could change
> immediately).
> + *
> + * seq_read() and seq_wait() can be used together to yield a race-free
> wakeup
> + * when an object changes, even without an ability to lock the object.
> See
> + * Usage in seq.h for details. */
> uint64_t
> seq_read(const struct seq *seq)
> OVS_EXCLUDED(seq_mutex)
> @@ -156,7 +160,11 @@ seq_wait__(struct seq *seq, uint64_t value)
>
> /* Causes the following poll_block() to wake up when 'seq''s sequence
> number
> * changes from 'value'. (If 'seq''s sequence number isn't 'value', then
> - * poll_block() won't block at all.) */
> + * poll_block() won't block at all.)
> + *
> + * seq_read() and seq_wait() can be used together to yield a race-free
> wakeup
> + * when an object changes, even without an ability to lock the object.
> See
> + * Usage in seq.h for details. */
> void
> seq_wait(const struct seq *seq_, uint64_t value)
> OVS_EXCLUDED(seq_mutex)
> diff --git a/lib/seq.h b/lib/seq.h
> index 3423e21..c764809 100644
> --- a/lib/seq.h
> +++ b/lib/seq.h
> @@ -20,7 +20,7 @@
> /* Thread-safe, pollable sequence number.
> *
> *
> - * Background
> + * Motivation
> * ==
> *
> * It is sometimes desirable to take an action whenever an object changes.
> @@ -66,6 +66,44 @@
> *poll_block();
> *
> *
> + * Alternate Usage
> + * ===
> + *
> + * struct seq can also be used as a sort of pollable condition variable.
> + * Suppose that we want a thread to process items in a queue, and thus to
> be
> + * able to wake up whenever the queue is nonempty. This requires a lock
> to
> + * protect the queue and a seq to signal that the queue has become
> nonempty,
> + * e.g.:
> + *
> + *struct ovs_mutex mutex;
> + *struct list queue OVS_GUARDED_BY(mutex);
> + *struct seq nonempty_seq;
> + *
> + * To add an element to the queue:
> + *
> + *ovs_mutex_lock(&mutex);
> + *list_push_back(&queue, ...element...);
> + *if (list_is_singleton(&queue)) { // The 'if' test here is
> optional.
> + *seq_change(&nonempty_seq);
> + *}
> + *ovs_mutex_unlock(&mutex);
> + *
> + * To wait for the queue to become nonempty:
> + *
> + *ovs_mutex_lock(&mutex);
> + *if (list_is_empty(&queue)) {
> + *seq_wait(&nonempty_seq, seq_read(&nonempty_seq));
> + *} else {
> + *poll_immediate_wake();
> + *}
> + *ovs_mutex_unlock(&mutex);
> + *
> + * (In the above code 'mutex' prevents the queue from changing between
> + * seq_read() and seq_wait(). Otherwise, it would be necessary to
> seq_read(),
> + * check for a nonempty queue, and then seq_wait() on the previously read
> + * sequence number, as under Usage above.)
> + *
> + *
> * Thread-safety
> * =
> *
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] Reinterpret base for meter band types bitmap.
OpenFlow 1.3 says that the band_types member of struct ofp_meter_features
is a bitmap of OFPMBT_* values. The OFPMBT_* values are 1-based, so
until now, to avoid wasting bit 0, OVS mapped an OFPMBT_* with value 1 to
bit 0, value 2 to bit 1, and so on. However, according to
http://openvswitch.org/pipermail/dev/2013-July/029666.html,
other OpenFlow implementations directly translate value 1 to bit 1,
value 2 to bit 2, and so on. This commit changes Open vSwitch to use this
more common interpretation.
A request for clarification of this issue in the OpenFlow standard has been
filed with the ONF Extensibility Working Group as issue EXT-345.
Reported-by: YAMAMOTO Takashi
Signed-off-by: Ben Pfaff
---
I'm pretty sure I sent this out before but I can't find it in
patchwork. Reviews welcome.
lib/ofp-print.c|8 ++--
tests/ofp-print.at |2 +-
2 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 1a4dd9c..e914b0a 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -1102,13 +1102,9 @@ ofputil_meter_capabilities_to_name(uint32_t bit)
static const char *
ofputil_meter_band_types_to_name(uint32_t bit)
{
-/*
- * Note: Meter band types start from 1. We assume that the lowest bit
- * in the band_types corresponds to DROP band type (1).
- */
switch (bit) {
-case 1 << (OFPMBT13_DROP - 1): return "drop";
-case 1 << (OFPMBT13_DSCP_REMARK - 1): return "dscp_remark";
+case 1 << OFPMBT13_DROP: return "drop";
+case 1 << OFPMBT13_DSCP_REMARK: return "dscp_remark";
}
return NULL;
diff --git a/tests/ofp-print.at b/tests/ofp-print.at
index 986b931..266af6c 100644
--- a/tests/ofp-print.at
+++ b/tests/ofp-print.at
@@ -1670,7 +1670,7 @@ AT_SETUP([OFPST_METER_FEATURES reply - OF1.3])
AT_KEYWORDS([ofp-print OFPT_STATS_REPLY])
AT_CHECK([ovs-ofctl ofp-print "\
04 13 00 20 00 00 00 02 00 0b 00 00 00 00 00 00 \
-00 01 00 00 00 00 00 03 00 00 00 0F 10 02 00 00 \
+00 01 00 00 00 00 00 06 00 00 00 0F 10 02 00 00 \
"], [0], [dnl
OFPST_METER_FEATURES reply (OF1.3) (xid=0x2):
max_meter:65536 max_bands:16 max_color:2
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] seq: Add some comments.
On Mon, Aug 12, 2013 at 01:56:02PM -0700, Andy Zhou wrote: > Thanks for the added comments. Looks good. Thanks. I applied this to master. > Coding style says we should not use "//", but not sure if this applies to > comments. Well, you can't use a /**/ comment inside a comment because that ends the comment, so I use // inside /**/. It works even if the compiler doesn't support // comments, since it's inside a comment. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] seq: Add some comments.
Make sense. This may be most reasonable solution. On Mon, Aug 12, 2013 at 1:59 PM, Ben Pfaff wrote: > On Mon, Aug 12, 2013 at 01:56:02PM -0700, Andy Zhou wrote: > > Thanks for the added comments. Looks good. > > Thanks. I applied this to master. > > > Coding style says we should not use "//", but not sure if this applies to > > comments. > > Well, you can't use a /**/ comment inside a comment because that ends > the comment, so I use // inside /**/. It works even if the compiler > doesn't support // comments, since it's inside a comment. > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] band.c: Fix error in bond_choose_output_slave() function.
On Mon, Aug 12, 2013 at 02:14:52PM -0700, Alex Wang wrote:
> This commit fixes the error introduced by commit 4a1b8f30e59 (bond:
> Stop using tags.). The error is caused by mistakenly returning 'slave'
> where 'slave->aux' should be returned.
>
> Signed-off-by: Alex Wang
I am nervous about accessing anything in 'slave' outside the lock.
What do you think of this variant?
Thanks,
Ben.
--8<--cut here-->8--
From: Alex Wang
Date: Mon, 12 Aug 2013 14:14:52 -0700
Subject: [PATCH] band: Fix error in bond_choose_output_slave() function.
This commit fixes the error introduced by commit 4a1b8f30e59 (bond:
Stop using tags.). The error is caused by mistakenly returning 'slave'
where 'slave->aux' should be returned.
Signed-off-by: Alex Wang
Signed-off-by: Ben Pfaff
---
lib/bond.c |5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/lib/bond.c b/lib/bond.c
index a9278db..3834774 100644
--- a/lib/bond.c
+++ b/lib/bond.c
@@ -661,11 +661,14 @@ bond_choose_output_slave(struct bond *bond, const struct
flow *flow,
struct flow_wildcards *wc, uint16_t vlan)
{
struct bond_slave *slave;
+void *aux;
ovs_rwlock_rdlock(&rwlock);
slave = choose_output_slave(bond, flow, wc, vlan);
+aux = slave ? slave->aux : NULL;
ovs_rwlock_unlock(&rwlock);
-return slave;
+
+return aux;
}
/* Rebalancing. */
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] band.c: Fix error in bond_choose_output_slave() function.
On Mon, Aug 12, 2013 at 2:04 PM, Ben Pfaff wrote: > > I am nervous about accessing anything in 'slave' outside the lock. > What do you think of this variant? > > Thanks, > > Ben. Thanks Ben, for pointing this out, Yours looks good and safer! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Adds way to determine db changes from Idl.run()
This is pretty crude (nothing else in the IDL interface requires the caller to understand the JSON-RPC protocol) and seems difficult for the callers to use. The changes that it returns can also be incomplete, at least in one corner case: if the database connection drops and reconnects, then the returned changes will not mention any rows that were deleted from the database while the connection was down. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] band.c: Fix error in bond_choose_output_slave() function.
On Mon, Aug 12, 2013 at 02:06:54PM -0700, Alex Wang wrote: > On Mon, Aug 12, 2013 at 2:04 PM, Ben Pfaff wrote: > > > > > I am nervous about accessing anything in 'slave' outside the lock. > > What do you think of this variant? > > > > Thanks, > > > > Ben. > > > Thanks Ben, for pointing this out, > > Yours looks good and safer! Thanks, applied. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Bug#701760: Bug#701760: openvswitch-switch: problem interacting with libvirt
Dear Ernesto, I had the same problem. Just add this line to /etc/init.d/openvswitch-switch: # X-Stop-After: libvirt-guests and reconfigure boot/shutdown order: update-rc.d -f openvswitch-switch remove insserv openvswitch-switch Now openvswitch-switch waits to libvirt-guests shutdown all VMs. -- Best regards, Lukas ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [debian 0/2] Fix lintian warnings
These patches fix some lintian warnings for the Debian packaging. I intend to apply them both to master and branch-1.9 for upload of that branch to Debian. Thanks, Ben. Ben Pfaff (2): debian: Apply hardening options to build. ovs-ofctl: Avoid groff warning due to too-long line. debian/rules |1 + utilities/ovs-ofctl.8.in |2 +- 2 files changed, 2 insertions(+), 1 deletion(-) -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [debian 1/2] debian: Apply hardening options to build.
Debian now encourages building every program with various GCC hardening options. This commit implements that recommendation for Open vSwitch. See https://wiki.debian.org/Hardening for details. Found by lintian. Signed-off-by: Ben Pfaff --- debian/rules |1 + 1 file changed, 1 insertion(+) diff --git a/debian/rules b/debian/rules index d34bdb3..1ee7048 100755 --- a/debian/rules +++ b/debian/rules @@ -40,6 +40,7 @@ configure-stamp: test -e Makefile || \ ../configure --prefix=/usr --localstatedir=/var --enable-ssl \ --sysconfdir=/etc CFLAGS="$(CFLAGS)" \ + $(shell dpkg-buildflags --export=configure) \ $(DATAPATH_CONFIGURE_OPTS)) touch configure-stamp -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [debian 2/2] ovs-ofctl: Avoid groff warning due to too-long line.
Avoids these warnings from groff: :1037: warning [p 14, 6.0i]: cannot adjust line :1037: warning [p 14, 6.2i]: can't break line Found by lintian. Signed-off-by: Ben Pfaff --- utilities/ovs-ofctl.8.in |2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in index 3e6c7fe..6fd03f8 100644 --- a/utilities/ovs-ofctl.8.in +++ b/utilities/ovs-ofctl.8.in @@ -1107,7 +1107,7 @@ be specified as a name used for matching. (This is similar to Open Flow 1.2 and above.) . .IP -Example: \fBset_field:fe80:0123:4567:890a:a6ba:dbff:fefe:59fa\->ipv6_src\fR +Example: \fBset_field:00:11:22:33:44:55->eth_src\fR. . .IP "\fBmultipath(\fIfields\fB, \fIbasis\fB, \fIalgorithm\fB, \fIn_links\fB, \fIarg\fB, \fIdst\fB[\fIstart\fB..\fIend\fB])\fR" Hashes \fIfields\fR using \fIbasis\fR as a universal hash parameter, -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/2] ofproto-dpif-upcall: don't forget to initialize mutexes
From: YAMAMOTO Takashi Signed-off-by: YAMAMOTO Takashi --- ofproto/ofproto-dpif-upcall.c | 3 +++ 1 file changed, 3 insertions(+) diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c index 54d3c21..55ada89 100644 --- a/ofproto/ofproto-dpif-upcall.c +++ b/ofproto/ofproto-dpif-upcall.c @@ -123,6 +123,9 @@ udpif_create(struct dpif_backer *backer, struct dpif *dpif) list_init(&udpif->upcalls); list_init(&udpif->fmbs); atomic_init(&udpif->reval_seq, 0); +ovs_mutex_init(&udpif->drop_key_mutex, PTHREAD_MUTEX_NORMAL); +ovs_mutex_init(&udpif->upcall_mutex, PTHREAD_MUTEX_NORMAL); +ovs_mutex_init(&udpif->fmb_mutex, PTHREAD_MUTEX_NORMAL); return udpif; } -- 1.8.3.1 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/2] netdev-bsd: ioctl "cmd" is unsigned long, not int
From: YAMAMOTO Takashi
Signed-off-by: YAMAMOTO Takashi
---
lib/netdev-bsd.c | 9 +
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
index f6d066b..50fb520 100644
--- a/lib/netdev-bsd.c
+++ b/lib/netdev-bsd.c
@@ -133,7 +133,7 @@ static void destroy_tap(int fd, const char *name);
static int get_flags(const struct netdev *, int *flagsp);
static int set_flags(const char *, int flags);
static int do_set_addr(struct netdev *netdev,
- int ioctl_nr, const char *ioctl_name,
+ unsigned long ioctl_nr, const char *ioctl_name,
struct in_addr addr);
static int get_etheraddr(const char *netdev_name, uint8_t ea[ETH_ADDR_LEN]);
static int set_etheraddr(const char *netdev_name, int hwaddr_family,
@@ -144,7 +144,7 @@ static int ifr_get_flags(const struct ifreq *);
static void ifr_set_flags(struct ifreq *, int flags);
#ifdef __NetBSD__
-static int af_link_ioctl(int command, const void *arg);
+static int af_link_ioctl(unsigned long command, const void *arg);
#endif
static void netdev_bsd_run(void);
@@ -1347,7 +1347,8 @@ make_in4_sockaddr(struct sockaddr *sa, struct in_addr
addr)
static int
do_set_addr(struct netdev *netdev,
-int ioctl_nr, const char *ioctl_name, struct in_addr addr)
+unsigned long ioctl_nr, const char *ioctl_name,
+struct in_addr addr)
{
struct ifreq ifr;
make_in4_sockaddr(&ifr.ifr_addr, addr);
@@ -1716,7 +1717,7 @@ ifr_set_flags(struct ifreq *ifr, int flags)
/* Calls ioctl() on an AF_LINK sock, passing the specified 'command' and
* 'arg'. Returns 0 if successful, otherwise a positive errno value. */
int
-af_link_ioctl(int command, const void *arg)
+af_link_ioctl(unsigned long command, const void *arg)
{
static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
static int sock;
--
1.8.3.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] netdev-bsd: ioctl "cmd" is unsigned long, not int
On Tue, Aug 13, 2013 at 07:20:22AM +0900, y...@mwd.biglobe.ne.jp wrote: > From: YAMAMOTO Takashi > > Signed-off-by: YAMAMOTO Takashi Both patch applied, thank you. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath: Do not set dev->master for XEN.
On Mon, Aug 12, 2013 at 1:28 PM, Pravin B Shelar wrote: > XEN dom0 networking assumes dev->master is bond device > and it tries to access bond private structure from dev->master > ptr on receive path. This causes panic. > Following patch removes compat code that is setting master > device. > > Signed-off-by: Pravin B Shelar > Bug #18920 Can you also add some of the description from the commit message to a comment? That way in the future we know why we don't backport it. Acked-by: Jesse Gross X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [sparse 0/3] Bring sparse annotations up to date.
I've been building with Clang lately and didn't notice that the sparse annotations had bit-rotted. The first two patches bring them up to date. The final patch is a style update. Ben Pfaff (3): ofproto-dpif-upcall: Fix sparse warning. sparse: Remove support for thread-safety annotations. ovs-thread: Use mutex instead of its address in thread-safety annotation. include/sparse/pthread.h | 38 -- lib/compiler.h| 29 + lib/ovs-thread.h | 11 +++ ofproto/ofproto-dpif-upcall.c |2 +- ofproto/ofproto-dpif.c|1 + ofproto/ofproto.c |3 ++- 6 files changed, 8 insertions(+), 76 deletions(-) -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [sparse 2/3] sparse: Remove support for thread-safety annotations.
The Clang support for thread-safety annotations is much more effective
than "sparse" support. I found that I was unable to make the annotations
warning-free under sparse.
Signed-off-by: Ben Pfaff
---
include/sparse/pthread.h | 38 --
lib/compiler.h | 29 +
lib/ovs-thread.h |5 -
ofproto/ofproto-dpif.c |1 +
ofproto/ofproto.c|3 ++-
5 files changed, 4 insertions(+), 72 deletions(-)
diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
index aa4652e..40c5ca3 100644
--- a/include/sparse/pthread.h
+++ b/include/sparse/pthread.h
@@ -21,18 +21,6 @@
/* Get actual definitions for us to annotate and build on. */
#include_next
-#include "compiler.h"
-
-int pthread_mutex_lock(pthread_mutex_t *mutex) OVS_ACQUIRES(mutex);
-int pthread_mutex_unlock(pthread_mutex_t *mutex) OVS_RELEASES(mutex);
-
-int pthread_rwlock_rdlock(pthread_rwlock_t *rwlock) OVS_ACQ_RDLOCK(rwlock);
-int pthread_rwlock_wrlock(pthread_rwlock_t *rwlock) OVS_ACQ_WRLOCK(rwlock);
-int pthread_rwlock_unlock(pthread_rwlock_t *rwlock) OVS_RELEASES(rwlock);
-
-int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex)
-OVS_REQUIRES(mutex);
-
/* Sparse complains about the proper PTHREAD_*_INITIALIZER definitions.
* Luckily, it's not a real compiler so we can overwrite it with something
* simple. */
@@ -47,29 +35,3 @@ int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t
*mutex)
#undef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
#define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP {}
-
-#define pthread_mutex_trylock(MUTEX)\
-({ \
-int retval = pthread_mutex_trylock(mutex); \
-if (!retval) { \
-OVS_MACRO_LOCK(MUTEX); \
-} \
-retval; \
-})
-
-#define pthread_rwlock_tryrdlock(RWLOCK)\
-({ \
-int retval = pthread_rwlock_tryrdlock(rwlock); \
-if (!retval) { \
-OVS_MACRO_LOCK(RWLOCK); \
-} \
-retval; \
-})
-#define pthread_rwlock_trywrlock(RWLOCK)\
-({ \
-int retval = pthread_rwlock_trywrlock(rwlock); \
-if (!retval) { \
-OVS_MACRO_LOCK(RWLOCK); \
-} \
-retval; \
-})
diff --git a/lib/compiler.h b/lib/compiler.h
index 2ca81bd..519b832 100644
--- a/lib/compiler.h
+++ b/lib/compiler.h
@@ -128,32 +128,7 @@
#define OVS_EXCLUDED(...) __attribute__((locks_excluded(__VA_ARGS__)))
#define OVS_ACQ_BEFORE(...) __attribute__((acquired_before(__VA_ARGS__)))
#define OVS_ACQ_AFTER(...) __attribute__((acquired_after(__VA_ARGS__)))
-#elif __CHECKER__
-/* "sparse" annotations for mutexes and mutex-like constructs.
- *
- * Change the thread-safety check annotations to use "context" attribute.
- *
- * OVS_MACRO_LOCK and OVS_MACRO_RELEASE are suitable for use within macros,
- * where there is no function prototype to annotate. */
-#define OVS_LOCKABLE
-#define OVS_REQ_RDLOCK(...) __attribute__((context(MUTEX, 1, 1)))
-#define OVS_ACQ_RDLOCK(...) __attribute__((context(MUTEX, 0, 1)))
-#define OVS_REQ_WRLOCK(...) __attribute__((context(MUTEX, 1, 1)))
-#define OVS_ACQ_WRLOCK(...) __attribute__((context(MUTEX, 0, 1)))
-#define OVS_REQUIRES(...) __attribute__((context(MUTEX, 1, 1)))
-#define OVS_ACQUIRES(...) __attribute__((context(MUTEX, 0, 1)))
-#define OVS_TRY_WRLOCK(RETVAL, ...)
-#define OVS_TRY_RDLOCK(RETVAL, ...)
-#define OVS_TRY_LOCK(REVAL, ...)
-#define OVS_GUARDED
-#define OVS_GUARDED_BY(...)
-#define OVS_EXCLUDED(...)
-#define OVS_RELEASES(...) __attribute__((context(MUTEX, 1, 0)))
-#define OVS_ACQ_BEFORE(...)
-#define OVS_ACQ_AFTER(...)
-#define OVS_MACRO_LOCK(...) __context__(MUTEX, 0, 1)
-#define OVS_MACRO_RELEASE(...) __context__(MUTEX, 1, 0)
-#else
+#else /* not Clang */
#define OVS_LOCKABLE
#define OVS_REQ_RDLOCK(...)
#define OVS_ACQ_RDLOCK(...)
@@ -170,8 +145,6 @@
#define OVS_RELEASES(...)
#define OVS_ACQ_BEFORE(...)
#define OVS_ACQ_AFTER(...)
-#define OVS_MACRO_LOCK(...)
-#define OVS_MACRO_RELEASE(...)
#endif
/* ISO C says that a C implementation may choose any integer type for an enum
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 3547686..5d3964a 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -496,11 +496,6 @@ ovsthread_once_start(struct ovsthread_once *once)
return OVS_UNLIKELY(!ovsthread_once_is_done__(once)
[ovs-dev] [sparse 1/3] ofproto-dpif-upcall: Fix sparse warning.
Signed-off-by: Ben Pfaff
---
ofproto/ofproto-dpif-upcall.c |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 55ada89..b451f1d 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -547,7 +547,7 @@ recv_upcalls(struct udpif *udpif)
|| type == OVS_KEY_ATTR_UDP) {
if (nl_attr_get_size(nla) == 4) {
ovs_be32 attr = nl_attr_get_be32(nla);
-hash = mhash_add(hash, (uint32_t) attr);
+hash = mhash_add(hash, (OVS_FORCE uint32_t) attr);
n_bytes += 4;
} else {
VLOG_WARN("Netlink attribute with incorrect size.");
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [sparse 3/3] ovs-thread: Use mutex instead of its address in thread-safety annotation.
CodingStyle says that this is preferred.
Signed-off-by: Ben Pfaff
---
lib/ovs-thread.h |6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
index 5d3964a..b7bc5d1 100644
--- a/lib/ovs-thread.h
+++ b/lib/ovs-thread.h
@@ -467,12 +467,12 @@ struct ovsthread_once {
}
static inline bool ovsthread_once_start(struct ovsthread_once *once)
-OVS_TRY_LOCK(true, &once->mutex);
+OVS_TRY_LOCK(true, once->mutex);
void ovsthread_once_done(struct ovsthread_once *once)
-OVS_RELEASES(&once->mutex);
+OVS_RELEASES(once->mutex);
bool ovsthread_once_start__(struct ovsthread_once *once)
-OVS_TRY_LOCK(false, &once->mutex);
+OVS_TRY_LOCK(false, once->mutex);
static inline bool
ovsthread_once_is_done__(const struct ovsthread_once *once)
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v2] datapath: Do not set dev->master for XEN.
On Mon, Aug 12, 2013 at 3:46 PM, Jesse Gross wrote: > On Mon, Aug 12, 2013 at 1:28 PM, Pravin B Shelar wrote: >> XEN dom0 networking assumes dev->master is bond device >> and it tries to access bond private structure from dev->master >> ptr on receive path. This causes panic. >> Following patch removes compat code that is setting master >> device. >> >> Signed-off-by: Pravin B Shelar >> Bug #18920 > > Can you also add some of the description from the commit message to a > comment? That way in the future we know why we don't backport it. > > Acked-by: Jesse Gross Thanks, I pushed updated patch. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto: make oftable_remove_rule__ release evict lock
From: YAMAMOTO Takashi
according to the OVS_RELEASES annotation, oftable_remove_rule__ is
expected to release rule->evict lock. make it actually do so.
this fixes pthread_rwlock_destroy failures observed on NetBSD,
where destroying a held lock, which is specwise undefined behaviour,
actually fails. i guess it doesn't fail on linux but it's better
not to rely on an undefined behavior.
Signed-off-by: YAMAMOTO Takashi
---
ofproto/ofproto.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/ofproto/ofproto.c b/ofproto/ofproto.c
index c7fc09b..44a6f9d 100644
--- a/ofproto/ofproto.c
+++ b/ofproto/ofproto.c
@@ -5475,6 +5475,7 @@ oftable_remove_rule__(struct ofproto *ofproto, struct
classifier *cls,
if (!list_is_empty(&rule->meter_list_node)) {
list_remove(&rule->meter_list_node);
}
+ovs_rwlock_unlock(&rule->evict);
}
static void
--
1.8.3.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Adds way to determine db changes from Idl.run()
I am also a bit concerned by issues that might arise from a user thinking that this is always accurate, rather than hints. Aaron, I think you had said something regarding this when we chatted off-list, but I don't recall the details. -Reid On Fri, Aug 9, 2013 at 2:50 PM, Reid Price wrote: > Or you could keep the original function behavior the same and expose this > as a separate function > > def foo(...): > > > def run(...): > return self.foo(...)[0] > > where foo is a better function name - update? run_details? > run_with_changes? run_diff? _run? No opinion there. > > -Reid > > > On Fri, Aug 9, 2013 at 2:30 PM, Aaron Rosen wrote: > >> Right, this would break things for anyone checking the return value of >> idl.run(). The only alternative I see to that is if we pass an optional arg >> to run() (i.e: def run(self, return_changes=False)). Would you prefer this >> instead? >> >> Thanks, >> >> Aaron >> >> >> On Fri, Aug 9, 2013 at 1:02 PM, Ben Pfaff wrote: >> >>> On Tue, Aug 06, 2013 at 02:45:35PM -0700, Aaron Rosen wrote: >>> > This patch changes what is being returned from Idl.run() to a tuple >>> > (changed, changes) so one can determine what changes have occurred to >>> > the database without having to read the entire table. >>> > >>> > Signed-off-by: Aaron Rosen >>> >>> It seems like a reasonable idea but I suspect it doesn't fix up all >>> the users. Also the patch is wordwrapped so I can't apply it. >>> >>> Thanks, >>> >>> Ben. >>> >> >> >> ___ >> dev mailing list >> dev@openvswitch.org >> http://openvswitch.org/mailman/listinfo/dev >> >> > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] Adds way to determine db changes from Idl.run()
Hi, I believe I've addressed the corner case in this patch set by returning a dict() that represents the current state of the database on connection reset/initial sync, otherwise a list of changes. This should allow the user to have a consistent view of the database. I've also changed the response to be the dict() that contains the changes rather than the full json-rpc message. Let me know what you guys thing about this change. Thanks, Aaron On Mon, Aug 12, 2013 at 4:00 PM, Reid Price wrote: > I am also a bit concerned by issues that might arise > from a user thinking that this is always accurate, rather > than hints. Aaron, I think you had said something > regarding this when we chatted off-list, but I don't > recall the details. > > -Reid > > > On Fri, Aug 9, 2013 at 2:50 PM, Reid Price wrote: > >> Or you could keep the original function behavior the same and expose this >> as a separate function >> >> def foo(...): >> >> >> def run(...): >> return self.foo(...)[0] >> >> where foo is a better function name - update? run_details? >> run_with_changes? run_diff? _run? No opinion there. >> >> -Reid >> >> >> On Fri, Aug 9, 2013 at 2:30 PM, Aaron Rosen wrote: >> >>> Right, this would break things for anyone checking the return value of >>> idl.run(). The only alternative I see to that is if we pass an optional arg >>> to run() (i.e: def run(self, return_changes=False)). Would you prefer this >>> instead? >>> >>> Thanks, >>> >>> Aaron >>> >>> >>> On Fri, Aug 9, 2013 at 1:02 PM, Ben Pfaff wrote: >>> On Tue, Aug 06, 2013 at 02:45:35PM -0700, Aaron Rosen wrote: > This patch changes what is being returned from Idl.run() to a tuple > (changed, changes) so one can determine what changes have occurred to > the database without having to read the entire table. > > Signed-off-by: Aaron Rosen It seems like a reasonable idea but I suspect it doesn't fix up all the users. Also the patch is wordwrapped so I can't apply it. Thanks, Ben. >>> >>> >>> ___ >>> dev mailing list >>> dev@openvswitch.org >>> http://openvswitch.org/mailman/listinfo/dev >>> >>> >> > 0001-Add-run_with_changes-method-to-Idl.patch Description: Binary data ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto: make oftable_remove_rule__ release evict lock
On Tue, Aug 13, 2013 at 08:00:05AM +0900, y...@mwd.biglobe.ne.jp wrote: > From: YAMAMOTO Takashi > > according to the OVS_RELEASES annotation, oftable_remove_rule__ is > expected to release rule->evict lock. make it actually do so. > > this fixes pthread_rwlock_destroy failures observed on NetBSD, > where destroying a held lock, which is specwise undefined behaviour, > actually fails. i guess it doesn't fail on linux but it's better > not to rely on an undefined behavior. > > Signed-off-by: YAMAMOTO Takashi Thank you! Applied to master. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [rule evict v2] ofproto-dpif: Lock rules to prevent eviction.
On Mon, Aug 12, 2013 at 01:23:38PM +0900, YAMAMOTO Takashi wrote: > > This patch uses a read-write lock to prevent rules from being evicted > > while they're used by child threads. It also changes the prototypes > > of the various rule lookup functions so that the thread safety > > analysis can be used to ensure that the locking is handled properly. > > destroying a rwlock which is currently held is an undefined behaviour. > on NetBSD, pthread_rwlock_destroy fails in that case. I'm aware it's undefined. I let this slip in by mistake. Sorry about that, and thanks for the fix. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [netdev]Internal interface 'br0' can't be created
On Mon, Aug 12, 2013 at 11:04:39AM +0800, ZhengLingyun wrote: > I only know 'br0' can be used as an interface to linux host. Can someone > explain me the function of 'br0'? In fact I don't understand how 'br0' works > and why this bug does't affect me at all. If you're not putting an IP address on br0, then it probably doesn't matter. (We should still address the bug.) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto: Include classifier wildcards in trace output.
On Mon, Aug 05, 2013 at 04:00:03PM -0700, Jesse Gross wrote: > When tracing a flow, it shows the "relevant fields" that were used > to determine the results. However, this currently only includes fields > that are used for computing the actions but not the flow lookup. This > can be confusing so this patch includes the wildcards from the classifer > lookup as well. > > Signed-off-by: Jesse Gross Acked-by: Ben Pfaff ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto: Include classifier wildcards in trace output.
On Mon, Aug 12, 2013 at 4:59 PM, Ben Pfaff wrote: > On Mon, Aug 05, 2013 at 04:00:03PM -0700, Jesse Gross wrote: >> When tracing a flow, it shows the "relevant fields" that were used >> to determine the results. However, this currently only includes fields >> that are used for computing the actions but not the flow lookup. This >> can be confusing so this patch includes the wildcards from the classifer >> lookup as well. >> >> Signed-off-by: Jesse Gross > > Acked-by: Ben Pfaff Applied, thanks. X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [rule evict v2] ofproto-dpif: Lock rules to prevent eviction.
At any rate this isn't an issue. I have simply unlocked and destroyed the rule after it's been removed from the classifier. Ethan On Tue, Aug 13, 2013 at 7:48 AM, Ben Pfaff wrote: > On Mon, Aug 12, 2013 at 01:23:38PM +0900, YAMAMOTO Takashi wrote: >> > This patch uses a read-write lock to prevent rules from being evicted >> > while they're used by child threads. It also changes the prototypes >> > of the various rule lookup functions so that the thread safety >> > analysis can be used to ensure that the locking is handled properly. >> >> destroying a rwlock which is currently held is an undefined behaviour. >> on NetBSD, pthread_rwlock_destroy fails in that case. > > I'm aware it's undefined. I let this slip in by mistake. Sorry about > that, and thanks for the fix. X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [debian 1/2] debian: Apply hardening options to build.
Looks good to me, I ran the "fakeroot debian/rules binary" and saw the change of CFLAGS attribute in "_debian/Makefile" ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [debian 2/2] ovs-ofctl: Avoid groff warning due to too-long line.
This change looks good to me, During my run of "fakeroot debian/rules binary", there is no warning. Could you hint me how to reproduce it? On Mon, Aug 12, 2013 at 3:13 PM, Ben Pfaff wrote: > Avoids these warnings from groff: > > :1037: warning [p 14, 6.0i]: cannot adjust line > :1037: warning [p 14, 6.2i]: can't break line > > Found by lintian. > > Signed-off-by: Ben Pfaff > --- > utilities/ovs-ofctl.8.in |2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/utilities/ovs-ofctl.8.in b/utilities/ovs-ofctl.8.in > index 3e6c7fe..6fd03f8 100644 > --- a/utilities/ovs-ofctl.8.in > +++ b/utilities/ovs-ofctl.8.in > @@ -1107,7 +1107,7 @@ be specified as a name used for matching. (This is > similar to > Open Flow 1.2 and above.) > . > .IP > -Example: > \fBset_field:fe80:0123:4567:890a:a6ba:dbff:fefe:59fa\->ipv6_src\fR > +Example: \fBset_field:00:11:22:33:44:55->eth_src\fR. > . > .IP "\fBmultipath(\fIfields\fB, \fIbasis\fB, \fIalgorithm\fB, > \fIn_links\fB, \fIarg\fB, \fIdst\fB[\fIstart\fB..\fIend\fB])\fR" > Hashes \fIfields\fR using \fIbasis\fR as a universal hash parameter, > -- > 1.7.10.4 > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [sparse 1/3] ofproto-dpif-upcall: Fix sparse warning.
Acked-by: Ethan Jackson
We've really got to get sparse to work with clang, this sort of thing
is going to get annoying. C'est la vie.
Ethan
On Tue, Aug 13, 2013 at 6:54 AM, Ben Pfaff wrote:
> Signed-off-by: Ben Pfaff
> ---
> ofproto/ofproto-dpif-upcall.c |2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 55ada89..b451f1d 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -547,7 +547,7 @@ recv_upcalls(struct udpif *udpif)
> || type == OVS_KEY_ATTR_UDP) {
> if (nl_attr_get_size(nla) == 4) {
> ovs_be32 attr = nl_attr_get_be32(nla);
> -hash = mhash_add(hash, (uint32_t) attr);
> +hash = mhash_add(hash, (OVS_FORCE uint32_t) attr);
> n_bytes += 4;
> } else {
> VLOG_WARN("Netlink attribute with incorrect size.");
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [sparse 3/3] ovs-thread: Use mutex instead of its address in thread-safety annotation.
Acked-by: Ethan Jackson
On Tue, Aug 13, 2013 at 6:54 AM, Ben Pfaff wrote:
> CodingStyle says that this is preferred.
>
> Signed-off-by: Ben Pfaff
> ---
> lib/ovs-thread.h |6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/lib/ovs-thread.h b/lib/ovs-thread.h
> index 5d3964a..b7bc5d1 100644
> --- a/lib/ovs-thread.h
> +++ b/lib/ovs-thread.h
> @@ -467,12 +467,12 @@ struct ovsthread_once {
> }
>
> static inline bool ovsthread_once_start(struct ovsthread_once *once)
> -OVS_TRY_LOCK(true, &once->mutex);
> +OVS_TRY_LOCK(true, once->mutex);
> void ovsthread_once_done(struct ovsthread_once *once)
> -OVS_RELEASES(&once->mutex);
> +OVS_RELEASES(once->mutex);
>
> bool ovsthread_once_start__(struct ovsthread_once *once)
> -OVS_TRY_LOCK(false, &once->mutex);
> +OVS_TRY_LOCK(false, once->mutex);
>
> static inline bool
> ovsthread_once_is_done__(const struct ovsthread_once *once)
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [sparse 2/3] sparse: Remove support for thread-safety annotations.
Acked-by: Ethan Jackson
Agreed thank you.
Ethan
On Tue, Aug 13, 2013 at 6:54 AM, Ben Pfaff wrote:
> The Clang support for thread-safety annotations is much more effective
> than "sparse" support. I found that I was unable to make the annotations
> warning-free under sparse.
>
> Signed-off-by: Ben Pfaff
> ---
> include/sparse/pthread.h | 38 --
> lib/compiler.h | 29 +
> lib/ovs-thread.h |5 -
> ofproto/ofproto-dpif.c |1 +
> ofproto/ofproto.c|3 ++-
> 5 files changed, 4 insertions(+), 72 deletions(-)
>
> diff --git a/include/sparse/pthread.h b/include/sparse/pthread.h
> index aa4652e..40c5ca3 100644
> --- a/include/sparse/pthread.h
> +++ b/include/sparse/pthread.h
> @@ -21,18 +21,6 @@
> /* Get actual definitions for us to annotate and build on. */
> #include_next
>
> -#include "compiler.h"
> -
> -int pthread_mutex_lock(pthread_mutex_t *mutex) OVS_ACQUIRES(mutex);
> -int pthread_mutex_unlock(pthread_mutex_t *mutex) OVS_RELEASES(mutex);
> -
> -int pthread_rwlock_rdlock(pthread_rwlock_t *rwlock) OVS_ACQ_RDLOCK(rwlock);
> -int pthread_rwlock_wrlock(pthread_rwlock_t *rwlock) OVS_ACQ_WRLOCK(rwlock);
> -int pthread_rwlock_unlock(pthread_rwlock_t *rwlock) OVS_RELEASES(rwlock);
> -
> -int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t *mutex)
> -OVS_REQUIRES(mutex);
> -
> /* Sparse complains about the proper PTHREAD_*_INITIALIZER definitions.
> * Luckily, it's not a real compiler so we can overwrite it with something
> * simple. */
> @@ -47,29 +35,3 @@ int pthread_cond_wait(pthread_cond_t *, pthread_mutex_t
> *mutex)
>
> #undef PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP
> #define PTHREAD_ERRORCHECK_MUTEX_INITIALIZER_NP {}
> -
> -#define pthread_mutex_trylock(MUTEX)\
> -({ \
> -int retval = pthread_mutex_trylock(mutex); \
> -if (!retval) { \
> -OVS_MACRO_LOCK(MUTEX); \
> -} \
> -retval; \
> -})
> -
> -#define pthread_rwlock_tryrdlock(RWLOCK)\
> -({ \
> -int retval = pthread_rwlock_tryrdlock(rwlock); \
> -if (!retval) { \
> -OVS_MACRO_LOCK(RWLOCK); \
> -} \
> -retval; \
> -})
> -#define pthread_rwlock_trywrlock(RWLOCK)\
> -({ \
> -int retval = pthread_rwlock_trywrlock(rwlock); \
> -if (!retval) { \
> -OVS_MACRO_LOCK(RWLOCK); \
> -} \
> -retval; \
> -})
> diff --git a/lib/compiler.h b/lib/compiler.h
> index 2ca81bd..519b832 100644
> --- a/lib/compiler.h
> +++ b/lib/compiler.h
> @@ -128,32 +128,7 @@
> #define OVS_EXCLUDED(...) __attribute__((locks_excluded(__VA_ARGS__)))
> #define OVS_ACQ_BEFORE(...) __attribute__((acquired_before(__VA_ARGS__)))
> #define OVS_ACQ_AFTER(...) __attribute__((acquired_after(__VA_ARGS__)))
> -#elif __CHECKER__
> -/* "sparse" annotations for mutexes and mutex-like constructs.
> - *
> - * Change the thread-safety check annotations to use "context" attribute.
> - *
> - * OVS_MACRO_LOCK and OVS_MACRO_RELEASE are suitable for use within macros,
> - * where there is no function prototype to annotate. */
> -#define OVS_LOCKABLE
> -#define OVS_REQ_RDLOCK(...) __attribute__((context(MUTEX, 1, 1)))
> -#define OVS_ACQ_RDLOCK(...) __attribute__((context(MUTEX, 0, 1)))
> -#define OVS_REQ_WRLOCK(...) __attribute__((context(MUTEX, 1, 1)))
> -#define OVS_ACQ_WRLOCK(...) __attribute__((context(MUTEX, 0, 1)))
> -#define OVS_REQUIRES(...) __attribute__((context(MUTEX, 1, 1)))
> -#define OVS_ACQUIRES(...) __attribute__((context(MUTEX, 0, 1)))
> -#define OVS_TRY_WRLOCK(RETVAL, ...)
> -#define OVS_TRY_RDLOCK(RETVAL, ...)
> -#define OVS_TRY_LOCK(REVAL, ...)
> -#define OVS_GUARDED
> -#define OVS_GUARDED_BY(...)
> -#define OVS_EXCLUDED(...)
> -#define OVS_RELEASES(...) __attribute__((context(MUTEX, 1, 0)))
> -#define OVS_ACQ_BEFORE(...)
> -#define OVS_ACQ_AFTER(...)
> -#define OVS_MACRO_LOCK(...) __context__(MUTEX, 0, 1)
> -#define OVS_MACRO_RELEASE(...) __context__(MUTEX, 1, 0)
> -#else
> +#else /* not Clang */
> #define OVS_LOCKABLE
> #define OVS_REQ_RDLOCK(...)
> #define OVS_ACQ_RDLOCK(...)
> @@ -170,8 +145,6 @@
> #define OVS_RELEASES(...)
> #define OVS_ACQ_BEFORE(...)
> #define OVS_ACQ_AFTER(...)
> -#define OVS_MACRO_LOCK(...)
> -#define OVS_MACRO_RELEASE(...)
> #endif
>
> /* ISO C says that a
Re: [ovs-dev] [PATCH V4 1/2] bfd: Implement BFD decay.
Sorry for such a delayed review of this patch. I have bit more time now that multithreading is in, so let's try to get this merged this week. Is this v4 the most recent version? Since I was so delayed reviewing it, it no longer applies to master. There's one major problem with this patch which will require some re-architecture. So let's get that cleared up before I dig to deeply into the review. In this version of the patch, there are several places where you set bfd->min_rx directly. This is very much not allowed. If you set bfd->min_rx without giving the remote BFD session a change to learn of the new configuration, you'll cause tunnel flapping. Imagine if min_rx is 1 second and you suddenly set it to 100ms. There's going to be a lot of 100ms intervals with no received BFD packets before the remote session updates. I think as a high level architecture we should do the following. 1) Get rid of the second half of bfd_decay that attempts to set the min_rx. Instead if we need to either enter or exit decay mode, simply do a bfd_poll(). 2) In bfd_poll change this line: +bfd->poll_min_rx = bfd->min_rx == bfd->decay_min_rx + ? bfd->decay_min_rx : bfd->cfg_min_rx; To something like: bfd->poll_min_rx = in_decay_mode ? bfd->decay_min-rx : bfd->cfg_min_rx 3) change the configuration code to simply call a bfd_poll() if the decay_min_rx changes instead of handling it directly. Does that all make sense? I'm 12 hours off so if you could send out a new version I'll have a look at it in the morning. Thanks, Ethan X-CudaMail-Whitelist-To: dev@openvswitch.org ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2 1/4] bfd: Implement BFD decay.
Hey Ethan,
This is the newest version,
I think I address most of issues, mentioned in your comments,
Kind Regards,
Alex Wang,
On Tue, Aug 6, 2013 at 8:51 AM, Alex Wang wrote:
> When there is no incoming data traffic at the interface for a period,
> BFD decay allows the bfd session to increase the min_rx. This is
> helpful in that some interfaces may be idle for long a time. And cpu
> consumption can be reduced by processing fewer bfd control packets.
>
> Signed-off-by: Alex Wang
>
> ---
>
> v1 -> v2:
> - add 'in_decay' boolean variable to bfd struct
> and simplify code.
> - include unit test to this patch.
>
> ---
> lib/bfd.c | 103 +--
> lib/bfd.h |5 +-
> ofproto/ofproto-dpif.c |7 +-
> tests/bfd.at | 211
> +---
> vswitchd/vswitch.xml | 10 +++
> 5 files changed, 319 insertions(+), 17 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index d4ac489..8a0f5e1 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -26,6 +26,7 @@
> #include "hash.h"
> #include "hmap.h"
> #include "list.h"
> +#include "netdev.h"
> #include "netlink.h"
> #include "odp-util.h"
> #include "ofpbuf.h"
> @@ -149,6 +150,9 @@ struct bfd {
> bool cpath_down; /* Concatenated Path Down. */
> uint8_t mult; /* bfd.DetectMult. */
>
> +struct netdev *netdev;
> +uint64_t rx_packets; /* Packets received by 'netdev'. */
> +
> enum state state; /* bfd.SessionState. */
> enum state rmt_state; /* bfd.RemoteSessionState. */
>
> @@ -184,6 +188,11 @@ struct bfd {
>
> atomic_bool check_tnl_key;/* Verify tunnel key of inbound
> packets? */
> atomic_int ref_cnt;
> +
> +/* BFD decay related variables. */
> +bool in_decay;
> +int decay_min_rx;
> +long long int decay_detect_time; /* Decay detection time. */
> };
>
> static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
> @@ -206,6 +215,8 @@ static void bfd_set_state(struct bfd *, enum state,
> enum diag)
> static uint32_t generate_discriminator(void) OVS_REQ_WRLOCK(&mutex);
> static void bfd_put_details(struct ds *, const struct bfd *)
> OVS_REQ_WRLOCK(&mutex);
> +static uint64_t bfd_rx_packets(const struct bfd *) OVS_REQ_WRLOCK(&mutex);
> +static void bfd_try_decay(struct bfd *) OVS_REQ_WRLOCK(&mutex);
> static void bfd_unixctl_show(struct unixctl_conn *, int argc,
> const char *argv[], void *aux OVS_UNUSED);
> static void bfd_unixctl_set_forwarding_override(struct unixctl_conn *,
> @@ -253,12 +264,13 @@ bfd_get_status(const struct bfd *bfd, struct smap
> *smap)
> * handle for the session, or NULL if BFD is not enabled according to
> 'cfg'.
> * Also returns NULL if cfg is NULL. */
> struct bfd *
> -bfd_configure(struct bfd *bfd, const char *name, const struct smap *cfg)
> -OVS_EXCLUDED(mutex)
> +bfd_configure(struct bfd *bfd, const char *name, const struct smap *cfg,
> + struct netdev *netdev) OVS_EXCLUDED(mutex)
> {
> static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
> static atomic_uint16_t udp_src = ATOMIC_VAR_INIT(0);
>
> +int decay_min_rx;
> long long int min_tx, min_rx;
> bool cpath_down;
> const char *hwaddr;
> @@ -290,6 +302,10 @@ bfd_configure(struct bfd *bfd, const char *name,
> const struct smap *cfg)
> bfd->min_tx = 1000;
> bfd->mult = 3;
> atomic_init(&bfd->ref_cnt, 1);
> +bfd->netdev = netdev_ref(netdev);
> +bfd->rx_packets = bfd_rx_packets(bfd);
> +bfd->in_decay = false;
> +bfd->decay_detect_time = 0;
>
> /* RFC 5881 section 4
> * The source port MUST be in the range 49152 through 65535. The
> same
> @@ -325,6 +341,24 @@ bfd_configure(struct bfd *bfd, const char *name,
> const struct smap *cfg)
> || (!bfd_in_poll(bfd) && bfd->cfg_min_rx > bfd->min_rx)) {
> bfd->min_rx = bfd->cfg_min_rx;
> }
> +/* Always resets decay_min_rx when cfg_min_rx is updated. */
> +bfd->decay_min_rx = 0;
> +bfd_poll(bfd);
> +}
> +
> +decay_min_rx = smap_get_int(cfg, "decay_min_rx", 0);
> +if (bfd->decay_min_rx != decay_min_rx ) {
> +if (decay_min_rx > 0 && decay_min_rx < bfd->cfg_min_rx) {
> +VLOG_WARN("%s: decay_min_rx cannot be less than %lld ms",
> + bfd->name, bfd->cfg_min_rx);
> +bfd->decay_min_rx = 0;
> +} else {
> +bfd->decay_min_rx = decay_min_rx;
> +}
> +/* Resets current decay. */
> +bfd->in_decay = false;
> +bfd->decay_detect_time = (bfd->decay_min_rx < 2000 ?
> + 2000 : bfd->decay_min_rx) + time_msec();
> bfd_poll(bfd);
> }
>
> @@ -373,6 +407,7 @@ bfd_unref(struct bfd *bfd) OVS_EXCLUDED(mutex)
> if (orig == 1) {
> ovs_mutex_lock(&mutex);
>
Re: [ovs-dev] [PATCH V2 1/4] bfd: Implement BFD decay.
> This is the newest version,
>
> I think I address most of issues, mentioned in your comments,
Ah ok I'll have a look at this one. Thanks.
Ethan
>
>
> On Tue, Aug 6, 2013 at 8:51 AM, Alex Wang wrote:
>>
>> When there is no incoming data traffic at the interface for a period,
>> BFD decay allows the bfd session to increase the min_rx. This is
>> helpful in that some interfaces may be idle for long a time. And cpu
>> consumption can be reduced by processing fewer bfd control packets.
>>
>> Signed-off-by: Alex Wang
>>
>> ---
>>
>> v1 -> v2:
>> - add 'in_decay' boolean variable to bfd struct
>> and simplify code.
>> - include unit test to this patch.
>>
>> ---
>> lib/bfd.c | 103 +--
>> lib/bfd.h |5 +-
>> ofproto/ofproto-dpif.c |7 +-
>> tests/bfd.at | 211
>> +---
>> vswitchd/vswitch.xml | 10 +++
>> 5 files changed, 319 insertions(+), 17 deletions(-)
>>
>> diff --git a/lib/bfd.c b/lib/bfd.c
>> index d4ac489..8a0f5e1 100644
>> --- a/lib/bfd.c
>> +++ b/lib/bfd.c
>> @@ -26,6 +26,7 @@
>> #include "hash.h"
>> #include "hmap.h"
>> #include "list.h"
>> +#include "netdev.h"
>> #include "netlink.h"
>> #include "odp-util.h"
>> #include "ofpbuf.h"
>> @@ -149,6 +150,9 @@ struct bfd {
>> bool cpath_down; /* Concatenated Path Down. */
>> uint8_t mult; /* bfd.DetectMult. */
>>
>> +struct netdev *netdev;
>> +uint64_t rx_packets; /* Packets received by 'netdev'. */
>> +
>> enum state state; /* bfd.SessionState. */
>> enum state rmt_state; /* bfd.RemoteSessionState. */
>>
>> @@ -184,6 +188,11 @@ struct bfd {
>>
>> atomic_bool check_tnl_key;/* Verify tunnel key of inbound
>> packets? */
>> atomic_int ref_cnt;
>> +
>> +/* BFD decay related variables. */
>> +bool in_decay;
>> +int decay_min_rx;
>> +long long int decay_detect_time; /* Decay detection time. */
>> };
>>
>> static struct ovs_mutex mutex = OVS_MUTEX_INITIALIZER;
>> @@ -206,6 +215,8 @@ static void bfd_set_state(struct bfd *, enum state,
>> enum diag)
>> static uint32_t generate_discriminator(void) OVS_REQ_WRLOCK(&mutex);
>> static void bfd_put_details(struct ds *, const struct bfd *)
>> OVS_REQ_WRLOCK(&mutex);
>> +static uint64_t bfd_rx_packets(const struct bfd *)
>> OVS_REQ_WRLOCK(&mutex);
>> +static void bfd_try_decay(struct bfd *) OVS_REQ_WRLOCK(&mutex);
>> static void bfd_unixctl_show(struct unixctl_conn *, int argc,
>> const char *argv[], void *aux OVS_UNUSED);
>> static void bfd_unixctl_set_forwarding_override(struct unixctl_conn *,
>> @@ -253,12 +264,13 @@ bfd_get_status(const struct bfd *bfd, struct smap
>> *smap)
>> * handle for the session, or NULL if BFD is not enabled according to
>> 'cfg'.
>> * Also returns NULL if cfg is NULL. */
>> struct bfd *
>> -bfd_configure(struct bfd *bfd, const char *name, const struct smap *cfg)
>> -OVS_EXCLUDED(mutex)
>> +bfd_configure(struct bfd *bfd, const char *name, const struct smap *cfg,
>> + struct netdev *netdev) OVS_EXCLUDED(mutex)
>> {
>> static struct ovsthread_once once = OVSTHREAD_ONCE_INITIALIZER;
>> static atomic_uint16_t udp_src = ATOMIC_VAR_INIT(0);
>>
>> +int decay_min_rx;
>> long long int min_tx, min_rx;
>> bool cpath_down;
>> const char *hwaddr;
>> @@ -290,6 +302,10 @@ bfd_configure(struct bfd *bfd, const char *name,
>> const struct smap *cfg)
>> bfd->min_tx = 1000;
>> bfd->mult = 3;
>> atomic_init(&bfd->ref_cnt, 1);
>> +bfd->netdev = netdev_ref(netdev);
>> +bfd->rx_packets = bfd_rx_packets(bfd);
>> +bfd->in_decay = false;
>> +bfd->decay_detect_time = 0;
>>
>> /* RFC 5881 section 4
>> * The source port MUST be in the range 49152 through 65535. The
>> same
>> @@ -325,6 +341,24 @@ bfd_configure(struct bfd *bfd, const char *name,
>> const struct smap *cfg)
>> || (!bfd_in_poll(bfd) && bfd->cfg_min_rx > bfd->min_rx)) {
>> bfd->min_rx = bfd->cfg_min_rx;
>> }
>> +/* Always resets decay_min_rx when cfg_min_rx is updated. */
>> +bfd->decay_min_rx = 0;
>> +bfd_poll(bfd);
>> +}
>> +
>> +decay_min_rx = smap_get_int(cfg, "decay_min_rx", 0);
>> +if (bfd->decay_min_rx != decay_min_rx ) {
>> +if (decay_min_rx > 0 && decay_min_rx < bfd->cfg_min_rx) {
>> +VLOG_WARN("%s: decay_min_rx cannot be less than %lld ms",
>> + bfd->name, bfd->cfg_min_rx);
>> +bfd->decay_min_rx = 0;
>> +} else {
>> +bfd->decay_min_rx = decay_min_rx;
>> +}
>> +/* Resets current decay. */
>> +bfd->in_decay = false;
>> +bfd->decay_detect_time = (bfd->decay_min_rx < 2000 ?
>> + 2000 : bfd->decay_min_rx) +
>> time_msec();
>> bfd_poll
Re: [ovs-dev] [PATCH V2 1/4] bfd: Implement BFD decay.
Thanks this version is much closer. Just some minor points which
should be pretty easy to clean up. Ben, so we don't have a 12 hour
review cycle on this, would you mind looking at the next version of
this patch (which should be pretty much there) and applying it if it's
ready?
>>> +/* Always resets decay_min_rx when cfg_min_rx is updated. */
>>> +bfd->decay_min_rx = 0;
>>> +bfd_poll(bfd);
I think the intention is right here, but it's a bit odd. For example,
if in_decay is true, we don't drop out of it with this logic in all
cases. How about we add a boolean to this function
"cfm_min_rx_changed" and set it to true here. If that's true, then in
the next if statement we can run through the decay_min_rx changed
logic which we know is correct.
>>> +if (bfd->state == STATE_UP && bfd->decay_min_rx > 0
>>> +&& now >= bfd->decay_detect_time) {
>>> +bfd_try_decay(bfd);
>>> +}
I'm not sure the bfd_try_decay() function is giving us a lot. This is
the only caller and it's fairly short. What do you think about simply
moving it's logic into this if block? If you feel strongly about it,
it's fine as is, just a style nit.
One thing, we should probably reset the decay_detect_time when we
first enter STATE_UP. Otherwise our time in other states counts to
the detect time.
>>> -ovs_strlcpy(flag_str, ds_cstr(&ds), sizeof flag_str);
>>> +/* Do not copy the trailing whitespace. */
>>> +ovs_strlcpy(flag_str, ds_cstr(&ds), ds.length);
This probably belongs in a different patch. Also have a look at the
ds_chomp() function which I think is a bit cleaner for this sort of
thing.
>>> +measure = (bfd->decay_min_rx < 2000 ? 2000 : bfd->decay_min_rx)
>>> + / bfd->min_rx + 5;
This is awfully complicated. Any particular reason we don't simply do
something like MAX(bfd->decay_min_rx, 2000)? It certainly would be
easier to explain to people. If not, could we add a comment
explaining this formula and how we arrived at it?
Thanks,
Ethan
X-CudaMail-Whitelist-To: dev@openvswitch.org
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
