Re: [ovs-dev] [PATCH 0/5] Backport facet wildcarding to 1.11.
On Jun 11, 2013, at 5:02 PM, Ben Pfaff wrote: > On Tue, Jun 11, 2013 at 03:17:17PM -0700, Justin Pettit wrote: >> This is a backport of the facet wildcarding code to 1.11. The patch that >> should be the most closely looked at is "ofproto-dpif: Consolidate facet >> stat logic.", since I had to modify the facet_flush_stats() and >> rule_get_stats() to prevent double-counting. The others were very >> straight-forward. > > It looks like the most likely risk, if the backport had a bug not in > the original series, is that we get some stats wrong. I'm OK with > that, so I'm happy with the backport. I forgot to reply to this earlier, but I pushed the series to branch-1.11. Thanks for looking at it. --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 0/3] skbuff: pskb_expand_head changes
This patchset applies against David Miller's net-next tree. This is my first patch, so please let me know if I've done something wrong or could do something better (e.g. splitting the patch in a different way). I think there is a problem with memory accounting in pskb_expand_head. Some call sites update truesize while others don't. This means that the skbuff can change in size without truesize being updated. This seems incorrect to me. So I changed it. Perhaps I don't understand something, but I thought it best to generate the change and then ask. So is this correct? The other patches are from looking at all the call sites to pskb_expand_head and since I was doing that I cleaned some of them up. Dave Wiltshire (3): skbuff: Update truesize in pskb_expand_head skbuff: skb_cow_head not used in some drivers. skbuff: Added new helper function skb_cow_clone_head. drivers/net/ethernet/atheros/atl1c/atl1c_main.c |8 ++-- drivers/net/ethernet/atheros/atl1e/atl1e_main.c |8 ++-- drivers/net/ethernet/atheros/atlx/atl1.c |8 ++-- drivers/net/ethernet/broadcom/tg3.c |3 +-- drivers/net/ethernet/brocade/bna/bnad.c | 11 +++ drivers/net/ethernet/intel/e1000/e1000_main.c |8 ++-- drivers/net/ethernet/intel/e1000e/netdev.c|8 ++-- drivers/net/ethernet/intel/igb/igb_main.c |7 ++- drivers/net/ethernet/intel/igbvf/netdev.c | 11 +++ drivers/net/ethernet/intel/ixgb/ixgb_main.c |8 ++-- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 10 +++--- drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |7 ++- drivers/net/ethernet/jme.c|3 +-- drivers/net/ethernet/qlogic/qlge/qlge_main.c |8 ++-- drivers/net/wimax/i2400m/netdev.c |3 +-- drivers/net/wireless/mwl8k.c |1 - include/linux/skbuff.h| 14 + kernel/audit.c|2 -- net/core/dev.c|8 ++-- net/core/skbuff.c |1 + net/netlink/af_netlink.c |3 +-- net/openvswitch/actions.c | 22 +++-- net/sched/act_csum.c |8 ++-- net/sched/act_nat.c | 18 + net/wireless/util.c |2 -- 25 files changed, 62 insertions(+), 128 deletions(-) -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 1/3] skbuff: Update truesize in pskb_expand_head
Some call sites to pskb_expand_head subsequently update the skb truesize
and others don't (even with non-zero arguments). This is likely a memory
audit leak. Fixed this up by moving the memory accounting to the
skbuff.c file and removing it from the calling sites.
Signed-off-by: Dave Wiltshire
---
drivers/net/wireless/mwl8k.c |1 -
kernel/audit.c |2 --
net/core/skbuff.c|1 +
net/netlink/af_netlink.c |3 +--
net/wireless/util.c |2 --
5 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/mwl8k.c b/drivers/net/wireless/mwl8k.c
index 6820fce..802c8d7 100644
--- a/drivers/net/wireless/mwl8k.c
+++ b/drivers/net/wireless/mwl8k.c
@@ -845,7 +845,6 @@ mwl8k_add_dma_header(struct mwl8k_priv *priv, struct
sk_buff *skb,
"Failed to reallocate TX buffer\n");
return;
}
- skb->truesize += REDUCED_TX_HEADROOM;
}
reqd_hdrlen = sizeof(*tr) + head_pad;
diff --git a/kernel/audit.c b/kernel/audit.c
index 21c7fa6..e05b57b 100644
--- a/kernel/audit.c
+++ b/kernel/audit.c
@@ -1157,7 +1157,6 @@ struct audit_buffer *audit_log_start(struct audit_context
*ctx, gfp_t gfp_mask,
static inline int audit_expand(struct audit_buffer *ab, int extra)
{
struct sk_buff *skb = ab->skb;
- int oldtail = skb_tailroom(skb);
int ret = pskb_expand_head(skb, 0, extra, ab->gfp_mask);
int newtail = skb_tailroom(skb);
@@ -1166,7 +1165,6 @@ static inline int audit_expand(struct audit_buffer *ab,
int extra)
return 0;
}
- skb->truesize += newtail - oldtail;
return newtail;
}
diff --git a/net/core/skbuff.c b/net/core/skbuff.c
index edf3757..125bb7e 100644
--- a/net/core/skbuff.c
+++ b/net/core/skbuff.c
@@ -1061,6 +1061,7 @@ int pskb_expand_head(struct sk_buff *skb, int nhead, int
ntail,
if (!data)
goto nodata;
size = SKB_WITH_OVERHEAD(ksize(data));
+ skb->truesize += size - skb_end_offset(skb);
/* Copy only real data... and, alas, header. This should be
* optimized for the cases when header is void.
diff --git a/net/netlink/af_netlink.c b/net/netlink/af_netlink.c
index 9b6b115..77fd986 100644
--- a/net/netlink/af_netlink.c
+++ b/net/netlink/af_netlink.c
@@ -1565,8 +1565,7 @@ static struct sk_buff *netlink_trim(struct sk_buff *skb,
gfp_t allocation)
skb = nskb;
}
- if (!pskb_expand_head(skb, 0, -delta, allocation))
- skb->truesize -= delta;
+ pskb_expand_head(skb, 0, -delta, allocation);
return skb;
}
diff --git a/net/wireless/util.c b/net/wireless/util.c
index f5ad4d9..5710aa2 100644
--- a/net/wireless/util.c
+++ b/net/wireless/util.c
@@ -533,8 +533,6 @@ int ieee80211_data_from_8023(struct sk_buff *skb, const u8
*addr,
if (pskb_expand_head(skb, head_need, 0, GFP_ATOMIC))
return -ENOMEM;
-
- skb->truesize += head_need;
}
if (encaps_data) {
--
1.7.10.4
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] skbuff: Update truesize in pskb_expand_head
On Wed, 2013-06-12 at 19:05 +1000, Dave Wiltshire wrote: > Some call sites to pskb_expand_head subsequently update the skb truesize > and others don't (even with non-zero arguments). This is likely a memory > audit leak. Fixed this up by moving the memory accounting to the > skbuff.c file and removing it from the calling sites. > > Signed-off-by: Dave Wiltshire > --- > drivers/net/wireless/mwl8k.c |1 - > kernel/audit.c |2 -- > net/core/skbuff.c|1 + > net/netlink/af_netlink.c |3 +-- > net/wireless/util.c |2 -- > 5 files changed, 2 insertions(+), 7 deletions(-) Ouch. Sorry, you cannot do that. skb->truesize is really complex, because there is a strong relation between skb->truesize and memory accounting on sockets. So pskb_expand_head() should not touch skb->truesize. Only callers can do that when needed, and if possible. An example of very careful truesize manipulation can be found in tcp_tso_segment() ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] Actions in OVS
Hi, I am trying to determine how actions are stored and applied in OVS for each flow. When I use the ovs-ofctl add-flow command (specifying only the source & destination IP and protocol with actions=output:1), I see that 8 "ofpact" structures are created inside the ofproto_add_flow function. The first of these holds the action I specify in the command, but what is the significance of the remaining actions? Thanks, Muhammad Sohaib Ayub ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] datapath: Mega flow implementation
On Jun 10, 2013, at 20:04 , ext Jesse Gross wrote:
>
+static bool __flow_cmp_key_mask(struct sw_flow *flow, u32 hash, u8
*key,
+ int key_start, int key_len,
+ struct sw_flow_mask *mfm)
+{
+ return (flow->hash == hash && (flow->mfm == mfm) &&
+ !memcmp((u8 *)&flow->key + key_start, key, (key_len -
key_start)));
+}
>>>
>>> Since the flows are non-overlapping, do we need to compare the masks?
>>
>> Yes, we are comparing masked key, which is no longer unique without
>> comparing the mask.
>
> Well, more to the point, I think we want to make sure that they are
> unique. The ordering of the masks has no inherent meaning so in order
> to avoid surprising behavior I think we need to enforce that there is
> no possibility of overlap. This should happen naturally when we start
> looking up exact matches on flow setup.
The masks could be added to the list so as to keep a partial ordering between
the masks, where masks completely covering (have all the same 1-bits + some)
other masks would be closer to the front. This way a full mask (if any) would
be at front, and all-wildcards mask (if any) would be at the back. It would be
safe to stop at first match from the beginning, and maybe more importantly,
there would be no confusion nor any performance penalty for any exact (full
mask) matches, as they would match at the first try. Any additional cycles
would be spent only on flows that currently all go to userspace.
Jarno
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Possible Regression in 'netdev: Add new "struct netdev_rx" for capturing packets from a netdev.'
(Sorry this isn't an actual reply and is missing context -- I wasn't on the list when it was originally posted.) Simon and I have been in touch about this, and I thought I'd share my findings for what they're worth. The problem is from the commit Simon mentioned (796223f5bc3a4896e6398733c798390158479400). Specifically, it's in netdev-linux.c in netdev_linux_send(). The new version always sends using the "sender" socket made by af_packet_sock() unless the interface is a tap, in which case it sends it using the tap fd. This differs from the old version which sent using whatever was in the fd field of the netdev if it was available. For tap interfaces, this was the tap fd, so the result was the same as it is now. But for other interfaces, this held the socket opened for receiving if the interface was listening (which was maybe never "right" in some sense and isn't convenient anymore since this socket descriptor is no longer stored in the non-rx netdev). The comments indicate that the exception is made for tap interfaces since writing to a tap interface with an AF_PACKET socket results in receiving the packet you just wrote. However, I don't think this behavior is limited to taps. Since the old version of the code sent and received with the same socket descriptor, I think the loop was fixed by the check in dev_queue_xmit_nit() in net/core/dev.c. Since they're two different socket descriptors now, this no longer works and you get the loop. I fixed it (I think) by adding a BPF packet filter on the rx socket so that it only receives incoming packets. There's probably a better fix, but you're welcome to the patch if you want it. -- Murphy ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 3/3] skbuff: Added new helper function skb_cow_clone_head.
In a few different drivers there is a check of (skb_cloned &&
!skb_clone_writable) before then using pskb_expand_head to copy the skb
if that is required. There are already some skb_cow_* functions for
other conditions, so added this one and changed the call sites.
Signed-off-by: Dave Wiltshire
---
include/linux/skbuff.h| 14 ++
net/core/dev.c|8 ++--
net/openvswitch/actions.c | 22 +++---
net/sched/act_csum.c |8 ++--
net/sched/act_nat.c | 18 +-
5 files changed, 30 insertions(+), 40 deletions(-)
diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
index a7393ad..7d18541 100644
--- a/include/linux/skbuff.h
+++ b/include/linux/skbuff.h
@@ -2154,6 +2154,20 @@ static inline int skb_cow_head(struct sk_buff *skb,
unsigned int headroom)
}
/**
+ * skb_cow_clone_head
+ * @skb: buffer to cow
+ * @len: length up to which to write
+ *
+ * This function is identical to skb_cow and sb_cow_head except that we
+ * replace the skb_cloned check by skb_cloned && !skb_clone_writable.
+ *
+ */
+static inline int skb_cow_clone_head(struct sk_buff *skb, unsigned int len)
+{
+ return __skb_cow(skb, 0, skb_cloned(skb) && !skb_clone_writable(skb,
len));
+}
+
+/**
* skb_padto - pad an skbuff up to a minimal size
* @skb: buffer to pad
* @len: minimal length
diff --git a/net/core/dev.c b/net/core/dev.c
index fa007db..1fb1cf5 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -,12 +,8 @@ int skb_checksum_help(struct sk_buff *skb)
offset += skb->csum_offset;
BUG_ON(offset + sizeof(__sum16) > skb_headlen(skb));
- if (skb_cloned(skb) &&
- !skb_clone_writable(skb, offset + sizeof(__sum16))) {
- ret = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
- if (ret)
- goto out;
- }
+ if (skb_cow_clone_head(skb, offset + sizeof(__sum16)))
+ goto out;
*(__sum16 *)(skb->data + offset) = csum_fold(csum);
out_set_summed:
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 894b6cb..3d3a9d0 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -38,21 +38,13 @@
static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
const struct nlattr *attr, int len, bool keep_skb);
-static int make_writable(struct sk_buff *skb, int write_len)
-{
- if (!skb_cloned(skb) || skb_clone_writable(skb, write_len))
- return 0;
-
- return pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
-}
-
/* remove VLAN header from packet and update csum accordingly. */
static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
{
struct vlan_hdr *vhdr;
int err;
- err = make_writable(skb, VLAN_ETH_HLEN);
+ err = skb_cow_clone_head(skb, VLAN_ETH_HLEN);
if (unlikely(err))
return err;
@@ -126,7 +118,7 @@ static int set_eth_addr(struct sk_buff *skb,
const struct ovs_key_ethernet *eth_key)
{
int err;
- err = make_writable(skb, ETH_HLEN);
+ err = skb_cow_clone_head(skb, ETH_HLEN);
if (unlikely(err))
return err;
@@ -221,7 +213,7 @@ static int set_ipv4(struct sk_buff *skb, const struct
ovs_key_ipv4 *ipv4_key)
struct iphdr *nh;
int err;
- err = make_writable(skb, skb_network_offset(skb) +
+ err = skb_cow_clone_head(skb, skb_network_offset(skb) +
sizeof(struct iphdr));
if (unlikely(err))
return err;
@@ -250,7 +242,7 @@ static int set_ipv6(struct sk_buff *skb, const struct
ovs_key_ipv6 *ipv6_key)
__be32 *saddr;
__be32 *daddr;
- err = make_writable(skb, skb_network_offset(skb) +
+ err = skb_cow_clone_head(skb, skb_network_offset(skb) +
sizeof(struct ipv6hdr));
if (unlikely(err))
return err;
@@ -284,7 +276,7 @@ static int set_ipv6(struct sk_buff *skb, const struct
ovs_key_ipv6 *ipv6_key)
return 0;
}
-/* Must follow make_writable() since that can move the skb data. */
+/* Must follow skb_cow_clone_head() since that can move the skb data. */
static void set_tp_port(struct sk_buff *skb, __be16 *port,
__be16 new_port, __sum16 *check)
{
@@ -313,7 +305,7 @@ static int set_udp(struct sk_buff *skb, const struct
ovs_key_udp *udp_port_key)
struct udphdr *uh;
int err;
- err = make_writable(skb, skb_transport_offset(skb) +
+ err = skb_cow_clone_head(skb, skb_transport_offset(skb) +
sizeof(struct udphdr));
if (unlikely(err))
return err;
@@ -333,7 +325,7 @@ static int set_tcp(struct sk_buff *skb, const struct
ovs_key_tcp *tcp_port_key)
struct tcphdr *th;
int err;
- err = make_writable(skb, skb_transport_off
[ovs-dev] [PATCH 2/3] skbuff: skb_cow_head not used in some drivers.
There is a helper function skb_cow_head which checks if a skb needs to
be expanded. This was unused in some drivers. Fixing that leads to
cleaner code in the drivers.
Signed-off-by: Dave Wiltshire
---
drivers/net/ethernet/atheros/atl1c/atl1c_main.c |8 ++--
drivers/net/ethernet/atheros/atl1e/atl1e_main.c |8 ++--
drivers/net/ethernet/atheros/atlx/atl1.c |8 ++--
drivers/net/ethernet/broadcom/tg3.c |3 +--
drivers/net/ethernet/brocade/bna/bnad.c | 11 +++
drivers/net/ethernet/intel/e1000/e1000_main.c |8 ++--
drivers/net/ethernet/intel/e1000e/netdev.c|8 ++--
drivers/net/ethernet/intel/igb/igb_main.c |7 ++-
drivers/net/ethernet/intel/igbvf/netdev.c | 11 +++
drivers/net/ethernet/intel/ixgb/ixgb_main.c |8 ++--
drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 10 +++---
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c |7 ++-
drivers/net/ethernet/jme.c|3 +--
drivers/net/ethernet/qlogic/qlge/qlge_main.c |8 ++--
drivers/net/wimax/i2400m/netdev.c |3 +--
15 files changed, 30 insertions(+), 81 deletions(-)
diff --git a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
index 786a874..8c69cda 100644
--- a/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
+++ b/drivers/net/ethernet/atheros/atl1c/atl1c_main.c
@@ -1931,14 +1931,10 @@ static int atl1c_tso_csum(struct atl1c_adapter *adapter,
u8 hdr_len;
u32 real_len;
unsigned short offload_type;
- int err;
if (skb_is_gso(skb)) {
- if (skb_header_cloned(skb)) {
- err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
- if (unlikely(err))
- return -1;
- }
+ if (unlikely(skb_cow_head(skb, 0)))
+ return -1;
offload_type = skb_shinfo(skb)->gso_type;
if (offload_type & SKB_GSO_TCPV4) {
diff --git a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
index 895f537..ff10d03 100644
--- a/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
+++ b/drivers/net/ethernet/atheros/atl1e/atl1e_main.c
@@ -1599,14 +1599,10 @@ static int atl1e_tso_csum(struct atl1e_adapter *adapter,
u8 hdr_len;
u32 real_len;
unsigned short offload_type;
- int err;
if (skb_is_gso(skb)) {
- if (skb_header_cloned(skb)) {
- err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
- if (unlikely(err))
- return -1;
- }
+ if (unlikely(skb_cow_head(skb, 0)))
+ return -1;
offload_type = skb_shinfo(skb)->gso_type;
if (offload_type & SKB_GSO_TCPV4) {
diff --git a/drivers/net/ethernet/atheros/atlx/atl1.c
b/drivers/net/ethernet/atheros/atlx/atl1.c
index 538211d..da7169c 100644
--- a/drivers/net/ethernet/atheros/atlx/atl1.c
+++ b/drivers/net/ethernet/atheros/atlx/atl1.c
@@ -2115,14 +2115,10 @@ static int atl1_tso(struct atl1_adapter *adapter,
struct sk_buff *skb,
{
u8 hdr_len, ip_off;
u32 real_len;
- int err;
if (skb_shinfo(skb)->gso_size) {
- if (skb_header_cloned(skb)) {
- err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
- if (unlikely(err))
- return -1;
- }
+ if (unlikely(skb_cow_head(skb, 0)))
+ return -1;
if (skb->protocol == htons(ETH_P_IP)) {
struct iphdr *iph = ip_hdr(skb);
diff --git a/drivers/net/ethernet/broadcom/tg3.c
b/drivers/net/ethernet/broadcom/tg3.c
index 28a645f..6daf8f9 100644
--- a/drivers/net/ethernet/broadcom/tg3.c
+++ b/drivers/net/ethernet/broadcom/tg3.c
@@ -7803,8 +7803,7 @@ static netdev_tx_t tg3_start_xmit(struct sk_buff *skb,
struct net_device *dev)
struct iphdr *iph;
u32 tcp_opt_len, hdr_len;
- if (skb_header_cloned(skb) &&
- pskb_expand_head(skb, 0, 0, GFP_ATOMIC))
+ if (skb_cow_head(skb, 0))
goto drop;
iph = ip_hdr(skb);
diff --git a/drivers/net/ethernet/brocade/bna/bnad.c
b/drivers/net/ethernet/brocade/bna/bnad.c
index b78e69e..ba5325a 100644
--- a/drivers/net/ethernet/brocade/bna/bnad.c
+++ b/drivers/net/ethernet/brocade/bna/bnad.c
@@ -2303,14 +2303,9 @@ bnad_mbox_irq_sync(struct bnad *bnad)
static int
bnad_tso_prepare(struct bnad *bnad, struct sk_buff *skb)
{
- int err;
-
- if (skb_header_cloned(skb)) {
- err = pskb_expand_head(skb, 0, 0, GFP_ATOMIC);
- if (err) {
- BNAD_
Re: [ovs-dev] Possible Regression in 'netdev: Add new "struct netdev_rx" for capturing packets from a netdev.'
On 12 June 2013 07:04, Murphy McCauley wrote: > (Sorry this isn't an actual reply and is missing context -- I wasn't on the > list when it was originally posted.) > > Simon and I have been in touch about this, and I thought I'd share my > findings for what they're worth. > > The problem is from the commit Simon mentioned > (796223f5bc3a4896e6398733c798390158479400). Specifically, it's in > netdev-linux.c in netdev_linux_send(). > > The new version always sends using the "sender" socket made by > af_packet_sock() unless the interface is a tap, in which case it sends it > using the tap fd. This differs from the old version which sent using > whatever was in the fd field of the netdev if it was available. For tap > interfaces, this was the tap fd, so the result was the same as it is now. > But for other interfaces, this held the socket opened for receiving if the > interface was listening (which was maybe never "right" in some sense and > isn't convenient anymore since this socket descriptor is no longer stored in > the non-rx netdev). > > The comments indicate that the exception is made for tap interfaces since > writing to a tap interface with an AF_PACKET socket results in receiving the > packet you just wrote. However, I don't think this behavior is limited to > taps. Since the old version of the code sent and received with the same > socket descriptor, I think the loop was fixed by the check in > dev_queue_xmit_nit() in net/core/dev.c. Since they're two different socket > descriptors now, this no longer works and you get the loop. Ahh, it turns out Ben explained this to me when I ran into a related issue with the FreeBSD userspace implementation. Ben's message in the thread is at http://openvswitch.org/pipermail/dev/2012-July/018806.html . > I fixed it (I think) by adding a BPF packet filter on the rx socket so that > it only receives incoming packets. There's probably a better fix, but you're > welcome to the patch if you want it. I think it's worth taking a look. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Possible Regression in 'netdev: Add new "struct netdev_rx" for capturing packets from a netdev.'
On Jun 12, 2013, at 6:28 AM, Ed Maste wrote:
> On 12 June 2013 07:04, Murphy McCauley wrote:
>> (Sorry this isn't an actual reply and is missing context -- I wasn't on the
>> list when it was originally posted.)
>>
>> Simon and I have been in touch about this, and I thought I'd share my
>> findings for what they're worth.
>>
>> The problem is from the commit Simon mentioned
>> (796223f5bc3a4896e6398733c798390158479400). Specifically, it's in
>> netdev-linux.c in netdev_linux_send().
>>
>> The new version always sends using the "sender" socket made by
>> af_packet_sock() unless the interface is a tap, in which case it sends it
>> using the tap fd. This differs from the old version which sent using
>> whatever was in the fd field of the netdev if it was available. For tap
>> interfaces, this was the tap fd, so the result was the same as it is now.
>> But for other interfaces, this held the socket opened for receiving if the
>> interface was listening (which was maybe never "right" in some sense and
>> isn't convenient anymore since this socket descriptor is no longer stored in
>> the non-rx netdev).
>>
>> The comments indicate that the exception is made for tap interfaces since
>> writing to a tap interface with an AF_PACKET socket results in receiving the
>> packet you just wrote. However, I don't think this behavior is limited to
>> taps. Since the old version of the code sent and received with the same
>> socket descriptor, I think the loop was fixed by the check in
>> dev_queue_xmit_nit() in net/core/dev.c. Since they're two different socket
>> descriptors now, this no longer works and you get the loop.
>
> Ahh, it turns out Ben explained this to me when I ran into a related
> issue with the FreeBSD userspace implementation. Ben's message in the
> thread is at http://openvswitch.org/pipermail/dev/2012-July/018806.html
> .
>
>> I fixed it (I think) by adding a BPF packet filter on the rx socket so that
>> it only receives incoming packets. There's probably a better fix, but
>> you're welcome to the patch if you want it.
>
> I think it's worth taking a look.
I think this is more or less against master.
diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
index d73115b..cc47a6b 100644
--- a/lib/netdev-linux.c
+++ b/lib/netdev-linux.c
@@ -744,6 +744,14 @@ netdev_linux_rx_open(struct netdev *netdev_, struct
netdev_rx **rxp)
} else {
struct sockaddr_ll sll;
int ifindex;
+/* Result of tcpdump -dd inbound */
+static struct sock_filter filt[] = {
+{ 0x28, 0, 0, 0xf004 },
+{ 0x15, 0, 1, 0x0004 },
+{ 0x6, 0, 0, 0x },
+{ 0x6, 0, 0, 0x }
+};
+static struct sock_fprog fprog = {ARRAY_SIZE(filt), filt};
/* Create file descriptor. */
fd = socket(PF_PACKET, SOCK_RAW, 0);
@@ -776,6 +784,16 @@ netdev_linux_rx_open(struct netdev *netdev_, struct
netdev_rx **rxp)
netdev_get_name(netdev_), strerror(error));
goto error;
}
+
+/* Filter for only incoming packets. */
+error = setsockopt(fd, SOL_SOCKET, SO_ATTACH_FILTER, &fprog,
+ sizeof fprog);
+if (error) {
+error = errno;
+VLOG_ERR("%s: failed attach filter (%s)",
+ netdev_get_name(netdev_), strerror(error));
+goto error;
+}
}
rx = xmalloc(sizeof *rx);
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Possible Regression in 'netdev: Add new "struct netdev_rx" for capturing packets from a netdev.'
On Wed, Jun 12, 2013 at 07:11:50AM -0700, Murphy McCauley wrote:
>
> On Jun 12, 2013, at 6:28 AM, Ed Maste wrote:
>
> > On 12 June 2013 07:04, Murphy McCauley wrote:
> >> (Sorry this isn't an actual reply and is missing context -- I wasn't on
> >> the list when it was originally posted.)
> >>
> >> Simon and I have been in touch about this, and I thought I'd share my
> >> findings for what they're worth.
> >>
> >> The problem is from the commit Simon mentioned
> >> (796223f5bc3a4896e6398733c798390158479400). Specifically, it's in
> >> netdev-linux.c in netdev_linux_send().
> >>
> >> The new version always sends using the "sender" socket made by
> >> af_packet_sock() unless the interface is a tap, in which case it sends it
> >> using the tap fd. This differs from the old version which sent using
> >> whatever was in the fd field of the netdev if it was available. For tap
> >> interfaces, this was the tap fd, so the result was the same as it is now.
> >> But for other interfaces, this held the socket opened for receiving if the
> >> interface was listening (which was maybe never "right" in some sense and
> >> isn't convenient anymore since this socket descriptor is no longer stored
> >> in the non-rx netdev).
> >>
> >> The comments indicate that the exception is made for tap interfaces since
> >> writing to a tap interface with an AF_PACKET socket results in receiving
> >> the packet you just wrote. However, I don't think this behavior is
> >> limited to taps. Since the old version of the code sent and received with
> >> the same socket descriptor, I think the loop was fixed by the check in
> >> dev_queue_xmit_nit() in net/core/dev.c. Since they're two different
> >> socket descriptors now, this no longer works and you get the loop.
> >
> > Ahh, it turns out Ben explained this to me when I ran into a related
> > issue with the FreeBSD userspace implementation. Ben's message in the
> > thread is at http://openvswitch.org/pipermail/dev/2012-July/018806.html
> > .
> >
> >> I fixed it (I think) by adding a BPF packet filter on the rx socket so
> >> that it only receives incoming packets. There's probably a better fix,
> >> but you're welcome to the patch if you want it.
> >
> > I think it's worth taking a look.
>
>
> I think this is more or less against master.
>
> diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> index d73115b..cc47a6b 100644
> --- a/lib/netdev-linux.c
> +++ b/lib/netdev-linux.c
> @@ -744,6 +744,14 @@ netdev_linux_rx_open(struct netdev *netdev_, struct
> netdev_rx **rxp)
> } else {
> struct sockaddr_ll sll;
> int ifindex;
> +/* Result of tcpdump -dd inbound */
> +static struct sock_filter filt[] = {
> +{ 0x28, 0, 0, 0xf004 },
> +{ 0x15, 0, 1, 0x0004 },
> +{ 0x6, 0, 0, 0x },
> +{ 0x6, 0, 0, 0x }
> +};
> +static struct sock_fprog fprog = {ARRAY_SIZE(filt), filt};
>
> /* Create file descriptor. */
> fd = socket(PF_PACKET, SOCK_RAW, 0);
> @@ -776,6 +784,16 @@ netdev_linux_rx_open(struct netdev *netdev_, struct
> netdev_rx **rxp)
> netdev_get_name(netdev_), strerror(error));
> goto error;
> }
> +
> +/* Filter for only incoming packets. */
> +error = setsockopt(fd, SOL_SOCKET, SO_ATTACH_FILTER, &fprog,
> + sizeof fprog);
> +if (error) {
> +error = errno;
> +VLOG_ERR("%s: failed attach filter (%s)",
> + netdev_get_name(netdev_), strerror(error));
> +goto error;
> +}
> }
>
> rx = xmalloc(sizeof *rx);
Thanks, Murphy.
Simon, does this solve the problem you see?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Pull xlate_actions() into its own file.
On Tue, Jun 11, 2013 at 06:24:54PM -0700, Ethan Jackson wrote: > Ideally, this patch would move xlate_actions() into it's own module > with a clearly defined regular interface which is minimally > dependent on ofproto-dpif. While, I've done this in a prototype, > moving large amounts of code into a new file while simultaneously > changing the logic and keeping up with changes to master has proved > nearly impossible. > > This patch takes a different approach. It simply copies the logic > directly from ofproto-dpif with no changes. Once this is in, > future patches can begin breaking the ties between > ofproto-dpif-xlate and ofproto-dpif proper. > > Signed-off-by: Ethan Jackson I'm fine with this. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Actions in OVS
On Wed, Jun 12, 2013 at 03:36:02PM +0500, Sohaib Ayub wrote: > I am trying to determine how actions are stored and applied in OVS for each > flow. When I use the ovs-ofctl add-flow command (specifying only the source > & destination IP and protocol with actions=output:1), I see that 8 > "ofpact" structures are created inside the ofproto_add_flow function. > The first of these holds the action I specify in the command, but what is > the significance of the remaining actions? Are you Muhammad Sohaib Ayub or Qasim Maqbool? The latter person asked the word-for-word identical question on June 6: http://openvswitch.org/pipermail/discuss/2013-June/010224.html The question doesn't make any sense. An output action uses one ofpact. There won't be 8 ofpacts. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OFConfig
On Thu, Jun 06, 2013 at 03:59:59PM -0500, Gurkan, Deniz wrote: > Our OFConfig code and initial development efforts are posted here: > https://github.com/UH-SDN/softconf.d This is a collaboration with > Infoblox. > > We included a configuration description under /doc. All YANG modules > from OFConfig standard 1.1.1 are in. However, only the controller IP > address update on OVS is available as a configuration action > (demonstrated during GEC16: > http://groups.geni.net/geni/wiki/GEC16Agenda/EveningDemoSession#LEARN:OFConfigonGENI). It's interesting. I have always contended that layering OF-CONFIG over OVSDB would be a reasonable approach. I guess this prototype demonstrates that it is possible. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/3] datapath: Mega flow implementation
The user space logic ensures no key will overlap (in the kernel), so the
order of masks are not important for correctness.
Ordering masks to improve efficiency is an interesting idea. However, it is
not clear ordering by mask specificity will gain efficiency -- there is no
guarantee that the frequency of exact match will be higher than that of
wildcarded match. One possible solution is to order them by the 'hit
rate' collected at run time -- but may not work as well when traffic cycles
through masks with rapid bursts. Any other ideas?
Andy
On Wed, Jun 12, 2013 at 3:40 AM, Rajahalme, Jarno (NSN - FI/Espoo) <
jarno.rajaha...@nsn.com> wrote:
>
> On Jun 10, 2013, at 20:04 , ext Jesse Gross wrote:
> >
> +static bool __flow_cmp_key_mask(struct sw_flow *flow, u32 hash, u8
> *key,
> + int key_start, int key_len,
> + struct sw_flow_mask *mfm)
> +{
> + return (flow->hash == hash && (flow->mfm == mfm) &&
> + !memcmp((u8 *)&flow->key + key_start, key, (key_len -
> key_start)));
> +}
> >>>
> >>> Since the flows are non-overlapping, do we need to compare the masks?
> >>
> >> Yes, we are comparing masked key, which is no longer unique without
> >> comparing the mask.
> >
> > Well, more to the point, I think we want to make sure that they are
> > unique. The ordering of the masks has no inherent meaning so in order
> > to avoid surprising behavior I think we need to enforce that there is
> > no possibility of overlap. This should happen naturally when we start
> > looking up exact matches on flow setup.
>
> The masks could be added to the list so as to keep a partial ordering
> between the masks, where masks completely covering (have all the same
> 1-bits + some) other masks would be closer to the front. This way a full
> mask (if any) would be at front, and all-wildcards mask (if any) would be
> at the back. It would be safe to stop at first match from the beginning,
> and maybe more importantly, there would be no confusion nor any performance
> penalty for any exact (full mask) matches, as they would match at the first
> try. Any additional cycles would be spent only on flows that currently all
> go to userspace.
>
> Jarno
>
>
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4] datapath: make skb->csum consistent with rest of networking stack.
On Tue, Jun 11, 2013 at 6:59 PM, Pravin B Shelar wrote: > As suggested by Jesse in the comment for patch "gre: Restructure > tunneling", following patch keeps skb->csum correct across ovs. > > Signed-off-by: Pravin B Shelar Acked-by: Jesse Gross ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow v3 1/2] datapath: Mega flow implementation
On Tue, Jun 11, 2013 at 8:49 PM, Andy Zhou wrote: > Did not realize that the __CHECK_ENDIAN__ flags has to be passed by hand to > enable those checks. This is what I use to verify the fix. Any better way > to enable this check? > > make C=2 CF="-D__CHECK_ENDIAN__" That's how run it (actually I do a slightly extended version): make C=2 CF="-Wsparse-all -D__CHECK_ENDIAN__" You could put this in a script or environment variable if you want. > The following incremental patch fixes those warnings. Would you mind sending out a new version with these changes rolled in? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow v3 1/2] datapath: Mega flow implementation
Sure, will send out v4 of the patch soon. On Wed, Jun 12, 2013 at 9:42 AM, Jesse Gross wrote: > On Tue, Jun 11, 2013 at 8:49 PM, Andy Zhou wrote: > > Did not realize that the __CHECK_ENDIAN__ flags has to be passed by hand > to > > enable those checks. This is what I use to verify the fix. Any better > way > > to enable this check? > > > > make C=2 CF="-D__CHECK_ENDIAN__" > > That's how run it (actually I do a slightly extended version): > make C=2 CF="-Wsparse-all -D__CHECK_ENDIAN__" > > You could put this in a script or environment variable if you want. > > > The following incremental patch fixes those warnings. > > Would you mind sending out a new version with these changes rolled in? > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] OFConfig
Since we have implemented the prototype on OVS, it had to be layered on OVSDB. Otherwise, OVS does not have any other way to be configured other than OVSDB interface when applicable - we would also have to use vsctl commands for some configuration data. Other OpenFlow-capable virtual switch implementations (such as LINC - no OVSDB or any DB) may have native support for OFConfig to work. Deniz -Original Message- From: Ben Pfaff [mailto:b...@nicira.com] Sent: Wednesday, June 12, 2013 10:38 AM To: Gurkan, Deniz Cc: dev@openvswitch.org Subject: Re: [ovs-dev] OFConfig On Thu, Jun 06, 2013 at 03:59:59PM -0500, Gurkan, Deniz wrote: > Our OFConfig code and initial development efforts are posted here: > https://github.com/UH-SDN/softconf.d This is a collaboration with > Infoblox. > > We included a configuration description under /doc. All YANG modules > from OFConfig standard 1.1.1 are in. However, only the controller IP > address update on OVS is available as a configuration action > (demonstrated during GEC16: > http://groups.geni.net/geni/wiki/GEC16Agenda/EveningDemoSession#LEARN:OFConfigonGENI). It's interesting. I have always contended that layering OF-CONFIG over OVSDB would be a reasonable approach. I guess this prototype demonstrates that it is possible. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow v4 2/2] ovs-dpctl: Add mega flow support
On Wed, Jun 12, 2013 at 10:00:03AM -0700, Andy Zhou wrote: > Added --mega option to ovs-dpctl command to allow mega flow to be > specified and displayed. ovs-dpctl tool is mainly used as debugging > tool. > > This patch also implements the low level user space routines to send > and receive mega flow netlink messages. Those netlink support > routines are required for forthcoming user space mega flow patches. > > Ethan contributed current version of ovs-dpctl mega flow output > function. > > Co-authored-by: Ethan Jackson > Signed-off-by: Ethan Jackson > Signed-off-by: Andy Zhou I sent my feedback for this patch as a followup to v3 2/2, because you said there hadn't been any userspace changes. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow v3 2/2] ovs-dpctl: Add mega flow support
On Tue, Jun 11, 2013 at 04:45:10PM -0700, Andy Zhou wrote:
> Added --mega option to ovs-dpctl command to allow mega flow to be
> specified and displayed. ovs-dpctl tool is mainly used as debugging
> tool.
>
> This patch also implements the low level user space routines to send
> and receive mega flow netlink messages. Those netlink support
> routines are required for forthcoming user space mega flow patches.
>
> Ethan contributed current version of ovs-dpctl mega flow output
> function.
>
> Co-authored-by: Ethan Jackson
> Signed-off-by: Ethan Jackson
> Signed-off-by: Andy Zhou
Thanks for doing this! I have a few comments.
First I have to echo Ethan's comments about --mega and --micro. Are
they needed?
> --- a/lib/dpif-linux.c
> +++ b/lib/dpif-linux.c
> @@ -105,6 +105,8 @@ struct dpif_linux_flow {
> * the Netlink version of the command, even if actions_len is zero. */
> const struct nlattr *key; /* OVS_FLOW_ATTR_KEY. */
> size_t key_len;
> +const struct nlattr *mask; /* OVS_FLOW_ATTR_KEY. */
OVS_FLOW_ATTR_MASK?
The indentation of the comment is off by one space.
It looks like dpif-netdev does not support masks. This will reduce
code coverage for "make check", because we will not be able to test
ofproto-dpif behavior with masks.
The comment on 'flow_dump_next' in lib/dpif-provider.h needs an
update.
dpif_flow_put() needs a comment update.
I don't see anything anywhere that says that a datapath may ignore a
field's wildcards, instead fixing the field's values. That's
important but not at all obvious from the interface, so I would think
that we should document this in the dpif interface and possibly in
datapath/README (check with Jesse on that).
format_odp_key_attr() has a loop that can be replaced by a call to
is_all_ones(), declared in util.h.
format_odp_key_attr() doesn't seem to check the length of the mask
attribute. I think it needs the same check as the key attribute.
The OVS_KEY_ATTR_PRIORITY and OVS_KEY_ATTR_SKB_MARK cases in
format_odp_key_attr() look identical. I think that you can combine
them.
In the OVS_KEY_ATTR_TUNNEL case in format_odp_key_attr(), I think that
we need to memset() tun_mask to zeros before we parse it, as we do for
tun_key just above.
In the same case, I think that the two casts to uint32_t are
unnecessary. I see that they were there before, but I do not know
why.
In the same case, the bit-mask for the tunnel flags is going to be
hard to interpret, because no one knows the bit assignments. I
suggest using format_flags() for the mask too. Maybe a format like
this: flags(df)/(df).
In format_odp_key_attr(), for the OVS_KEY_ATTR_VLAN case, the mask is
a be16, so it needs to be retrieved that way and byte-swapped before
printing. Similarly for OVS_KEY_ATTR_MPLS, except that there the
mask is encapsulated inside a struct ovs_key_mpls and should be
retrieved through that.
In the OVS_KEY_ATTR_IPV6 case in format_odp_key_attr(), nothing
initializes src_mask or dst_mask.
parse_odp_key_attr() has many instances of constants like 0x.
Please use UINT32_MAX (etc.) instead to make it easier to recognize
that the values are correct.
parse_odp_key_mask_attr() uses %x for many of the masks. Our usual
practice is to use %i, to allow users to specify input in any base.
This also will allow us later, if we choose, to switch to using a
tokenizing parser that doesn't have to guess based on context which
radix a number is in.
In parse_odp_key_mask_attr(), the tun_id parser looks like it doesn't
accept a mask.
Please add test cases for masks to tests/odp.at (this would have
caught the tun_id oversight).
In the tunnel parser, I don't think that any of the casts to uint16_t
are needed.
For vlan parsing, I don't think we need "unsigned long long int" for
vlan_mask. A vlan is 16 bits so an "int" will do (see the big comment
at the top of parse_odp_key_mask_attr()). Similarly for many of the
fields in the IPv4, IPv6, TCP, UDP, and other fields' parsing (look at
the sizes of the similar fields for the key).
I don't see support for parsing masked MPLS in
parse_odp_key_mask_attr().
parse_odp_key_attr() duplicates half the code in
parse_odp_key_mask_attr(). Couldn't both sets of functionality be
implemented together, without duplication, by passing a boolean flag
that, if not set, disallows masking? e.g. something like this for
skb_priority:
{
unsigned long long int priority;
unsigned long long int priority_mask;
int n = -1;
if (maskable && sscanf(s, "skb_priority(%llx/%llx)%n", &priority,
&priority_mask, &n) > 0 && n > 0) {
nl_msg_put_u32(key, OVS_KEY_ATTR_PRIORITY, priority);
nl_msg_put_u32(mask, OVS_KEY_ATTR_PRIORITY, priority_mask);
return n;
} else if (sscanf(s, "skb_priority(%llx)%n",
&priority, &n) > 0 && n > 0) {
nl_msg_put_u32(key, OVS_KEY_ATTR_PRIORITY, priority
[ovs-dev] [PATCH V1 1/3] coverage: Reimplement the "ovs-appclt coverage/show" command
This commit changes the "ovs-appclt coverage/show" command to show the
per-second, per-minute and per-hour rates of function invocation. More
importantly, this makes using coverage counter an easy way to monitor
the execution of specific functions.
Signed-off-by: Alex Wang
---
Possible Issue:
The "coverage_run" runs every second. And the number of addition operations is
not very large. If it is still considered to be time-consuming, I'll adjust
further.
---
lib/coverage-unixctl.man |3 +-
lib/coverage.c | 76 +-
lib/coverage.h | 33 +---
lib/timeval.c|1 -
vswitchd/ovs-vswitchd.c |3 ++
5 files changed, 96 insertions(+), 20 deletions(-)
diff --git a/lib/coverage-unixctl.man b/lib/coverage-unixctl.man
index 9718894..2d283dc 100644
--- a/lib/coverage-unixctl.man
+++ b/lib/coverage-unixctl.man
@@ -8,4 +8,5 @@ main loop takes unusually long to run.
Coverage counters are useful mainly for performance analysis and
debugging.
.IP "\fBcoverage/show\fR"
-Displays the values of all of the coverage counters.
+Displays the per-second, per-minute and per-hour rates, and total count
+of all of the coverage counters.
diff --git a/lib/coverage.c b/lib/coverage.c
index f152474..814c741 100644
--- a/lib/coverage.c
+++ b/lib/coverage.c
@@ -47,7 +47,19 @@ struct coverage_counter *coverage_counters[] = {
#define n_coverage_counters ARRAY_SIZE(coverage_counters)
#endif /* !USE_LINKER_SECTIONS */
-static unsigned int epoch;
+/* Makes coverage_run run every second. */
+#define COVERAGE_WAKEUP_INTERVAL1000
+static long long int coverage_wakeup_timer = LLONG_MIN;
+
+#define HR_TIME 3600/* 3600 seconds in one hour. */
+
+/* Defines the moving average array index variables. */
+static unsigned int mov_avg_sec_idx = 0;
+static unsigned int mov_avg_min_idx = 0;
+static unsigned int mov_avg_hr_idx = 0;
+
+/* Index counter used to compute the moving average array's index. */
+static unsigned int idx_count = 0;
static void coverage_read(struct svec *);
@@ -170,7 +182,7 @@ coverage_log(void)
uint32_t hash = coverage_hash();
if (coverage_hit(hash)) {
VLOG_INFO("Skipping details of duplicate event coverage for "
- "hash=%08"PRIx32" in epoch %u", hash, epoch);
+ "hash=%08"PRIx32, hash);
} else {
struct svec lines;
const char *line;
@@ -189,8 +201,17 @@ coverage_log(void)
static void
coverage_read_counter(struct svec *lines, const struct coverage_counter *c)
{
-svec_add_nocopy(lines, xasprintf("%-24s %5u / %9llu",
- c->name, c->count, c->count + c->total));
+/* The per second rate is calculated via averaging the counts in recent
+ * five seconds. */
+svec_add_nocopy(lines,
+xasprintf("%-24s %5.1f/sec %7u/min "
+ "%9u/hr total: %llu",
+ c->name,
+ coverage_array_sum(c->mov_avg_sec, MOV_AVG_SEC_LEN)
+ / (float) MOV_AVG_SEC_LEN,
+ coverage_array_sum(c->mov_avg_min, MOV_AVG_MIN_LEN),
+ coverage_array_sum(c->mov_avg_hr, MOV_AVG_HR_LEN),
+ c->count + c->total));
}
/* Adds coverage counter information to 'lines'. */
@@ -204,8 +225,8 @@ coverage_read(struct svec *lines)
hash = coverage_hash();
n_never_hit = 0;
-svec_add_nocopy(lines, xasprintf("Event coverage (epoch %u/entire run), "
- "hash=%08"PRIx32":", epoch, hash));
+svec_add_nocopy(lines, xasprintf("Event coverage (frequency), "
+ "hash=%08"PRIx32":", hash));
for (i = 0; i < n_coverage_counters; i++) {
struct coverage_counter *c = coverage_counters[i];
if (c->count) {
@@ -225,16 +246,43 @@ coverage_read(struct svec *lines)
svec_add_nocopy(lines, xasprintf("%zu events never hit", n_never_hit));
}
-/* Advances to the next epoch of coverage, resetting all the counters to 0. */
+/* Causes the following call to poll_block() to wake up every second. */
void
-coverage_clear(void)
+coverage_wait(void)
{
-size_t i;
+poll_timer_wait_until(coverage_wakeup_timer);
+}
-epoch++;
-for (i = 0; i < n_coverage_counters; i++) {
-struct coverage_counter *c = coverage_counters[i];
-c->total += c->count;
-c->count = 0;
+/* Runs every second to update the coverage counters' mov_avg_sec array,
+ * mov_avg_min array and mov_avg_hr array. */
+void
+coverage_run(void)
+{
+if (time_msec() >= coverage_wakeup_timer) {
+size_t i;
+
+/* Updates the index variables. */
+mov_avg_sec_idx = idx_count % MOV_AVG_SEC_LEN;
+mov_avg_min_idx = (idx_count / MOV_AVG_SEC_LEN) % MOV_AVG_MIN_LEN;
+mov_avg_hr_idx = idx_count / MOV_AVG_HR_LEN;
+
+for (i = 0; i < n_cove
[ovs-dev] [PATCH V3 3/3] ofproto-dpif.at: Add test for "ovs-appctl coverage/show"
This commit adds an unit test for the "ovs-appctl coverage/show". Signed-off-by: Alex Wang --- tests/ofproto-dpif.at | 26 ++ 1 file changed, 26 insertions(+) diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 9231221..823b07a 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -2549,3 +2549,29 @@ in_port=1,vlan_tci=0x/0x0fff,dl_src=50:54:00:00:00:0b, n_subfacets:1, used:0 ]) OVS_VSWITCHD_STOP AT_CLEANUP + +AT_SETUP([ofproto-dpif coverage/show]) +OVS_VSWITCHD_START + +AT_CHECK([ovs-appctl time/stop]) dnl Want to manipulate the clock +AT_CHECK([ovs-appctl time/warp 1000 && ovs-appctl time/warp 1000], [0], [warped +warped +]) +AT_CHECK([ovs-appctl coverage/show], [0], [stdout], []) + +# Check the unixctl_received and unixctl_replied rates +# There should be in total 4 unixctl_received counts, but the rate should only +# show 3 counts. This is in that the "ovs-appctl coverage/show" is not included +# in the rate calculation. +AT_CHECK([sed -n "s/^\(unixctl_received.*\)$/\1/p" stdout], [0], [dnl +unixctl_received 0.6/sec 3/min 3/hr total: 4 +]) +# There should be in total 3 unixctl_replied counts, and the rate should also +# shouw 3 counts, since at the time of rate calculation, the command is not replied +# yet. +AT_CHECK([sed -n "s/^\(unixctl_replied.*\)$/\1/p" stdout], [0], [dnl +unixctl_replied0.6/sec 3/min 3/hr total: 3 +]) + +OVS_VSWITCHD_STOP +AT_CLEANUP -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH V2 2/3] ofproto-dpif: Compute the subfacet add/del rate using coverage counters
The subfacet rates (e.g. add rate, del rate) were computed by exponential
moving averaging function in ofproto-dpif.c. This patch replaces that
logic with coverage counters. And the rates can be checked by running
"ovs-appctl coverage/show" command.
Signed-off-by: Alex Wang
---
ofproto/ofproto-dpif.c | 90 +++-
ofproto/ofproto-dpif.h | 19 --
tests/ofproto-dpif.at |4 ---
tests/tunnel.at| 26 +++---
4 files changed, 26 insertions(+), 113 deletions(-)
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3bc8dd7..0c20ac6 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -69,6 +69,11 @@ COVERAGE_DEFINE(facet_changed_rule);
COVERAGE_DEFINE(facet_revalidate);
COVERAGE_DEFINE(facet_unexpected);
COVERAGE_DEFINE(facet_suppress);
+COVERAGE_DEFINE(facet_create);
+COVERAGE_DEFINE(facet_remove);
+COVERAGE_DEFINE(handle_flow_miss);
+COVERAGE_DEFINE(subfacet_create);
+COVERAGE_DEFINE(subfacet_destroy);
struct flow_miss;
struct facet;
@@ -319,7 +324,6 @@ static struct shash all_dpif_backers =
SHASH_INITIALIZER(&all_dpif_backers);
static void drop_key_clear(struct dpif_backer *);
static struct ofport_dpif *
odp_port_to_ofport(const struct dpif_backer *, uint32_t odp_port);
-static void update_moving_averages(struct dpif_backer *backer);
/* Defer flow mod completion until "ovs-appctl ofproto/unclog"? (Useful only
* for debugging the asynchronous flow_mod implementation.) */
@@ -906,14 +910,6 @@ open_dpif_backer(const char *type, struct dpif_backer
**backerp)
backer->max_n_subfacet = 0;
backer->created = time_msec();
-backer->last_minute = backer->created;
-memset(&backer->hourly, 0, sizeof backer->hourly);
-memset(&backer->daily, 0, sizeof backer->daily);
-memset(&backer->lifetime, 0, sizeof backer->lifetime);
-backer->subfacet_add_count = 0;
-backer->subfacet_del_count = 0;
-backer->total_subfacet_add_count = 0;
-backer->total_subfacet_del_count = 0;
backer->avg_n_subfacet = 0;
backer->avg_subfacet_life = 0;
@@ -3385,6 +3381,8 @@ handle_flow_miss(struct flow_miss *miss, struct
flow_miss_op *ops,
struct facet *facet;
long long int now;
+COVERAGE_INC(handle_flow_miss);
+
now = time_msec();
memset(stats, 0, sizeof *stats);
stats->used = now;
@@ -4039,8 +4037,6 @@ update_stats(struct dpif_backer *backer)
run_fast_rl();
}
dpif_flow_dump_done(&dump);
-
-update_moving_averages(backer);
}
/* Calculates and returns the number of milliseconds of idle time after which
@@ -4231,6 +4227,8 @@ facet_create(const struct flow_miss *miss, struct
rule_dpif *rule,
struct facet *facet;
struct match match;
+COVERAGE_INC(facet_create);
+
facet = xzalloc(sizeof *facet);
facet->packet_count = facet->prev_packet_count = stats->n_packets;
facet->byte_count = facet->prev_byte_count = stats->n_bytes;
@@ -4298,6 +4296,7 @@ facet_remove(struct facet *facet)
struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
struct subfacet *subfacet, *next_subfacet;
+COVERAGE_INC(facet_remove);
ovs_assert(!list_is_empty(&facet->subfacets));
/* First uninstall all of the subfacets to get final statistics. */
@@ -4812,6 +4811,8 @@ subfacet_create(struct facet *facet, struct flow_miss
*miss,
subfacet = xmalloc(sizeof *subfacet);
}
+COVERAGE_INC(subfacet_create);
+
hmap_insert(&backer->subfacets, &subfacet->hmap_node, key_hash);
list_push_back(&facet->subfacets, &subfacet->list_node);
subfacet->facet = facet;
@@ -4825,7 +4826,6 @@ subfacet_create(struct facet *facet, struct flow_miss
*miss,
subfacet->path = SF_NOT_INSTALLED;
subfacet->backer = backer;
-backer->subfacet_add_count++;
return subfacet;
}
@@ -4835,10 +4835,9 @@ static void
subfacet_destroy__(struct subfacet *subfacet)
{
struct facet *facet = subfacet->facet;
-struct ofproto_dpif *ofproto = ofproto_dpif_cast(facet->rule->up.ofproto);
/* Update ofproto stats before uninstall the subfacet. */
-ofproto->backer->subfacet_del_count++;
+COVERAGE_INC(subfacet_destroy);
subfacet_uninstall(subfacet);
hmap_remove(&subfacet->backer->subfacets, &subfacet->hmap_node);
@@ -6032,21 +6031,12 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn
*conn, int argc OVS_UNUSED,
}
static void
-show_dp_rates(struct ds *ds, const char *heading,
- const struct avg_subfacet_rates *rates)
-{
-ds_put_format(ds, "%s add rate: %5.3f/min, del rate: %5.3f/min\n",
- heading, rates->add_rate, rates->del_rate);
-}
-
-static void
dpif_show_backer(const struct dpif_backer *backer, struct ds *ds)
{
const struct shash_node **ofprotos;
struct ofproto_dpif *ofproto;
struct shash ofproto_shash;
uint64_t n_hit, n_missed;
-long long int minutes;
size_t i;
n_h
Re: [ovs-dev] [megaflow v3 2/2] ovs-dpctl: Add mega flow support
On Wed, Jun 12, 2013 at 11:17 AM, Ben Pfaff wrote: > I don't see anything anywhere that says that a datapath may ignore a > field's wildcards, instead fixing the field's values. That's > important but not at all obvious from the interface, so I would think > that we should document this in the dpif interface and possibly in > datapath/README (check with Jesse on that). I agree that it's good to document this (we should do the datapath/README changes in the kernel patch though). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [megaflow v3 2/2] ovs-dpctl: Add mega flow support
On Wed, Jun 12, 2013 at 11:22:09AM -0700, Jesse Gross wrote: > On Wed, Jun 12, 2013 at 11:17 AM, Ben Pfaff wrote: > > I don't see anything anywhere that says that a datapath may ignore a > > field's wildcards, instead fixing the field's values. That's > > important but not at all obvious from the interface, so I would think > > that we should document this in the dpif interface and possibly in > > datapath/README (check with Jesse on that). > > I agree that it's good to document this (we should do the > datapath/README changes in the kernel patch though). Right, that's what I meant even if I didn't say it ;-) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V3 3/3] ofproto-dpif.at: Add test for "ovs-appctl coverage/show"
Sorry, something wrong with the header, should be V1. On Wed, Jun 12, 2013 at 11:38 AM, Alex Wang wrote: > This commit adds an unit test for the "ovs-appctl coverage/show". > > Signed-off-by: Alex Wang > --- > tests/ofproto-dpif.at | 26 ++ > 1 file changed, 26 insertions(+) > > diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at > index 9231221..823b07a 100644 > --- a/tests/ofproto-dpif.at > +++ b/tests/ofproto-dpif.at > @@ -2549,3 +2549,29 @@ > in_port=1,vlan_tci=0x/0x0fff,dl_src=50:54:00:00:00:0b, n_subfacets:1, > used:0 > ]) > OVS_VSWITCHD_STOP > AT_CLEANUP > + > +AT_SETUP([ofproto-dpif coverage/show]) > +OVS_VSWITCHD_START > + > +AT_CHECK([ovs-appctl time/stop]) dnl Want to manipulate the clock > +AT_CHECK([ovs-appctl time/warp 1000 && ovs-appctl time/warp 1000], [0], > [warped > +warped > +]) > +AT_CHECK([ovs-appctl coverage/show], [0], [stdout], []) > + > +# Check the unixctl_received and unixctl_replied rates > +# There should be in total 4 unixctl_received counts, but the rate should > only > +# show 3 counts. This is in that the "ovs-appctl coverage/show" is not > included > +# in the rate calculation. > +AT_CHECK([sed -n "s/^\(unixctl_received.*\)$/\1/p" stdout], [0], [dnl > +unixctl_received 0.6/sec 3/min 3/hr total: 4 > +]) > +# There should be in total 3 unixctl_replied counts, and the rate should > also > +# shouw 3 counts, since at the time of rate calculation, the command is > not replied > +# yet. > +AT_CHECK([sed -n "s/^\(unixctl_replied.*\)$/\1/p" stdout], [0], [dnl > +unixctl_replied0.6/sec 3/min 3/hr total: 3 > +]) > + > +OVS_VSWITCHD_STOP > +AT_CLEANUP > -- > 1.7.9.5 > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2 2/3] ofproto-dpif: Compute the subfacet add/del rate using coverage counters
Sorry, something wrong with the header, should be V1.
On Wed, Jun 12, 2013 at 11:38 AM, Alex Wang wrote:
> The subfacet rates (e.g. add rate, del rate) were computed by exponential
> moving averaging function in ofproto-dpif.c. This patch replaces that
> logic with coverage counters. And the rates can be checked by running
> "ovs-appctl coverage/show" command.
>
> Signed-off-by: Alex Wang
> ---
> ofproto/ofproto-dpif.c | 90
> +++-
> ofproto/ofproto-dpif.h | 19 --
> tests/ofproto-dpif.at |4 ---
> tests/tunnel.at| 26 +++---
> 4 files changed, 26 insertions(+), 113 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 3bc8dd7..0c20ac6 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -69,6 +69,11 @@ COVERAGE_DEFINE(facet_changed_rule);
> COVERAGE_DEFINE(facet_revalidate);
> COVERAGE_DEFINE(facet_unexpected);
> COVERAGE_DEFINE(facet_suppress);
> +COVERAGE_DEFINE(facet_create);
> +COVERAGE_DEFINE(facet_remove);
> +COVERAGE_DEFINE(handle_flow_miss);
> +COVERAGE_DEFINE(subfacet_create);
> +COVERAGE_DEFINE(subfacet_destroy);
>
> struct flow_miss;
> struct facet;
> @@ -319,7 +324,6 @@ static struct shash all_dpif_backers =
> SHASH_INITIALIZER(&all_dpif_backers);
> static void drop_key_clear(struct dpif_backer *);
> static struct ofport_dpif *
> odp_port_to_ofport(const struct dpif_backer *, uint32_t odp_port);
> -static void update_moving_averages(struct dpif_backer *backer);
>
> /* Defer flow mod completion until "ovs-appctl ofproto/unclog"? (Useful
> only
> * for debugging the asynchronous flow_mod implementation.) */
> @@ -906,14 +910,6 @@ open_dpif_backer(const char *type, struct dpif_backer
> **backerp)
>
> backer->max_n_subfacet = 0;
> backer->created = time_msec();
> -backer->last_minute = backer->created;
> -memset(&backer->hourly, 0, sizeof backer->hourly);
> -memset(&backer->daily, 0, sizeof backer->daily);
> -memset(&backer->lifetime, 0, sizeof backer->lifetime);
> -backer->subfacet_add_count = 0;
> -backer->subfacet_del_count = 0;
> -backer->total_subfacet_add_count = 0;
> -backer->total_subfacet_del_count = 0;
> backer->avg_n_subfacet = 0;
> backer->avg_subfacet_life = 0;
>
> @@ -3385,6 +3381,8 @@ handle_flow_miss(struct flow_miss *miss, struct
> flow_miss_op *ops,
> struct facet *facet;
> long long int now;
>
> +COVERAGE_INC(handle_flow_miss);
> +
> now = time_msec();
> memset(stats, 0, sizeof *stats);
> stats->used = now;
> @@ -4039,8 +4037,6 @@ update_stats(struct dpif_backer *backer)
> run_fast_rl();
> }
> dpif_flow_dump_done(&dump);
> -
> -update_moving_averages(backer);
> }
>
> /* Calculates and returns the number of milliseconds of idle time after
> which
> @@ -4231,6 +4227,8 @@ facet_create(const struct flow_miss *miss, struct
> rule_dpif *rule,
> struct facet *facet;
> struct match match;
>
> +COVERAGE_INC(facet_create);
> +
> facet = xzalloc(sizeof *facet);
> facet->packet_count = facet->prev_packet_count = stats->n_packets;
> facet->byte_count = facet->prev_byte_count = stats->n_bytes;
> @@ -4298,6 +4296,7 @@ facet_remove(struct facet *facet)
> struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
> struct subfacet *subfacet, *next_subfacet;
>
> +COVERAGE_INC(facet_remove);
> ovs_assert(!list_is_empty(&facet->subfacets));
>
> /* First uninstall all of the subfacets to get final statistics. */
> @@ -4812,6 +4811,8 @@ subfacet_create(struct facet *facet, struct
> flow_miss *miss,
> subfacet = xmalloc(sizeof *subfacet);
> }
>
> +COVERAGE_INC(subfacet_create);
> +
> hmap_insert(&backer->subfacets, &subfacet->hmap_node, key_hash);
> list_push_back(&facet->subfacets, &subfacet->list_node);
> subfacet->facet = facet;
> @@ -4825,7 +4826,6 @@ subfacet_create(struct facet *facet, struct
> flow_miss *miss,
> subfacet->path = SF_NOT_INSTALLED;
> subfacet->backer = backer;
>
> -backer->subfacet_add_count++;
> return subfacet;
> }
>
> @@ -4835,10 +4835,9 @@ static void
> subfacet_destroy__(struct subfacet *subfacet)
> {
> struct facet *facet = subfacet->facet;
> -struct ofproto_dpif *ofproto =
> ofproto_dpif_cast(facet->rule->up.ofproto);
>
> /* Update ofproto stats before uninstall the subfacet. */
> -ofproto->backer->subfacet_del_count++;
> +COVERAGE_INC(subfacet_destroy);
>
> subfacet_uninstall(subfacet);
> hmap_remove(&subfacet->backer->subfacets, &subfacet->hmap_node);
> @@ -6032,21 +6031,12 @@ ofproto_unixctl_dpif_dump_dps(struct unixctl_conn
> *conn, int argc OVS_UNUSED,
> }
>
> static void
> -show_dp_rates(struct ds *ds, const char *heading,
> - const struct avg_subfacet_rates *rates)
> -{
> -ds_put_format(ds, "%s add rate: %5.3f/min, del rate: %5.3f/min\n",
>
Re: [ovs-dev] [PATCH V2 2/3] ofproto-dpif: Compute the subfacet add/del rate using coverage counters
On Wed, Jun 12, 2013 at 11:28:22AM -0700, Alex Wang wrote: > Sorry, something wrong with the header, should be V1. For a v1 you can just omit a version number. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2 2/3] ofproto-dpif: Compute the subfacet add/del rate using coverage counters
Thanks Ben, for the tip. This will not happen again. On Wed, Jun 12, 2013 at 11:29 AM, Ben Pfaff wrote: > On Wed, Jun 12, 2013 at 11:28:22AM -0700, Alex Wang wrote: > > Sorry, something wrong with the header, should be V1. > > For a v1 you can just omit a version number. > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [ext-260 v3 2/5] ofp-errors: Make ofperr_decode() static.
It had no users outside of ofp-errors.c.
Signed-off-by: Ben Pfaff
---
lib/ofp-errors.c |2 +-
lib/ofp-errors.h |1 -
2 files changed, 1 insertions(+), 2 deletions(-)
diff --git a/lib/ofp-errors.c b/lib/ofp-errors.c
index 74a3358..a141f3f 100644
--- a/lib/ofp-errors.c
+++ b/lib/ofp-errors.c
@@ -72,7 +72,7 @@ ofperr_is_valid(enum ofperr error)
/* Returns the OFPERR_* value that corresponds to 'type' and 'code' within
* 'version', or 0 if either no such OFPERR_* value exists or 'version' is
* unknown. */
-enum ofperr
+static enum ofperr
ofperr_decode(enum ofp_version version, uint16_t type, uint16_t code)
{
const struct ofperr_domain *domain = ofperr_domain_from_version(version);
diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
index aa3ba50..1f7ea69 100644
--- a/lib/ofp-errors.h
+++ b/lib/ofp-errors.h
@@ -542,7 +542,6 @@ const char *ofperr_domain_get_name(enum ofp_version);
bool ofperr_is_valid(enum ofperr);
-enum ofperr ofperr_decode(enum ofp_version, uint16_t type, uint16_t code);
enum ofperr ofperr_from_name(const char *);
enum ofperr ofperr_decode_msg(const struct ofp_header *,
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [ext-260 v3 4/5] ofp-errors: Implement OpenFlow 1.2+ experimenter error codes.
OpenFlow 1.2 standardized experimenter error codes in a way different from
the Nicira extension. This commit implements the OpenFlow 1.2+ version.
This commit also makes it easy to add error codes for new experimenter IDs
by adding new *_VENDOR_ID definitions to openflow-common.h.
Signed-off-by: Ben Pfaff
---
OPENFLOW-1.1+ |3 -
build-aux/extract-ofp-errors | 326 ++--
include/openflow/nicira-ext.h |1 -
include/openflow/openflow-1.2.h|5 +-
include/openflow/openflow-common.h | 25 +++-
lib/automake.mk|9 +-
lib/ofp-errors.c | 72 ++---
lib/ofp-errors.h | 89 ++
tests/ofp-actions.at |4 +-
tests/ofp-errors.at| 51 +-
utilities/ovs-ofctl.c | 15 +-
11 files changed, 384 insertions(+), 216 deletions(-)
diff --git a/OPENFLOW-1.1+ b/OPENFLOW-1.1+
index c4bcc34..fb39873 100644
--- a/OPENFLOW-1.1+
+++ b/OPENFLOW-1.1+
@@ -92,9 +92,6 @@ OpenFlow 1.2 support requires OpenFlow 1.1 as a prerequisite,
plus the
following additional work. (This is based on the change log at the
end of the OF1.2 spec. I didn't compare the specs carefully yet.)
-* Use new OpenFlow extensible error infrastructure, on OF1.2+
- only, instead of the OVS-specific extension used until now.
-
* OFPT_FLOW_MOD:
* MODIFY and MODIFY_STRICT commands now never insert new flows
diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
index 32d0913..5378dac 100755
--- a/build-aux/extract-ofp-errors
+++ b/build-aux/extract-ofp-errors
@@ -6,6 +6,13 @@ import re
macros = {}
+# Map from OpenFlow version number to version ID used in ofp_header.
+version_map = {"1.0": 0x01,
+ "1.1": 0x02,
+ "1.2": 0x03,
+ "1.3": 0x04}
+version_reverse_map = dict((v, k) for (k, v) in version_map.iteritems())
+
token = None
line = ""
idRe = "[a-zA-Z_][a-zA-Z_0-9]*"
@@ -13,12 +20,24 @@ tokenRe = "#?" + idRe + "|[0-9]+|."
inComment = False
inDirective = False
-def getLine():
+def open_file(fn):
+global fileName
+global inputFile
+global lineNumber
+fileName = fn
+inputFile = open(fileName)
+lineNumber = 0
+
+def tryGetLine():
+global inputFile
global line
global lineNumber
line = inputFile.readline()
lineNumber += 1
-if line == "":
+return line != ""
+
+def getLine():
+if not tryGetLine():
fatal("unexpected end of input")
def getToken():
@@ -133,146 +152,190 @@ def usage():
argv0 = os.path.basename(sys.argv[0])
print ('''\
%(argv0)s, for extracting OpenFlow error codes from header files
-usage: %(argv0)s FILE [FILE...]
+usage: %(argv0)s ERROR_HEADER VENDOR_HEADER
-This program reads the header files specified on the command line and
-outputs a C source file for translating OpenFlow error codes into
-strings, for use as lib/ofp-errors.c in the Open vSwitch source tree.
+This program reads VENDOR_HEADER to obtain OpenFlow vendor (aka
+experimenter IDs), then ERROR_HEADER to obtain OpenFlow error number.
+It outputs a C source file for translating OpenFlow error codes into
+strings.
-This program is specialized for reading lib/ofp-errors.h. It will not
-work on arbitrary header files without extensions.\
+ERROR_HEADER should point to lib/ofp-errors.h.
+VENDOR_HEADER should point to include/openflow/openflow-common.h.
+The output is suitable for use as lib/ofp-errors.inc.\
''' % {"argv0": argv0})
sys.exit(0)
-def extract_ofp_errors(filenames):
+def extract_vendor_ids(fn):
+global vendor_map
+vendor_map = {}
+vendor_loc = {}
+
+open_file(fn)
+while tryGetLine():
+m =
re.match(r'#define\s+([A-Z0-9_]+)_VENDOR_ID\s+(0x[0-9a-fA-F]+|[0-9]+)', line)
+if not m:
+continue
+
+name = m.group(1)
+id_ = int(m.group(2), 0)
+
+if name in vendor_map:
+error("%s: duplicate definition of vendor" % name)
+sys.stderr.write("%s: Here is the location of the previous "
+ "definition.\n" % vendor_loc[name])
+sys.exit(1)
+
+vendor_map[name] = id_
+vendor_loc[name] = "%s:%d" % (fileName, lineNumber)
+
+if not vendor_map:
+fatal("%s: no vendor definitions found" % fn)
+
+inputFile.close()
+
+vendor_reverse_map = {}
+for name, id_ in vendor_map.items():
+if id_ in vendor_reverse_map:
+fatal("%s: duplicate vendor id for vendors %s and %s"
+ % (id_, vendor_reverse_map[id_], name))
+vendor_reverse_map[id_] = name
+
+def extract_ofp_errors(fn):
error_types = {}
comments = []
names = []
domain = {}
reverse = {}
-for domain_name in ("OF1.0", "OF1.1", "OF1.2", "OF1.3",
-"NX1.0", "NX1.1", "NX1.2", "NX1.3"):
+for domai
[ovs-dev] [ext-260 v3 0/5] add support for OF1.2+ experimenter error codes
This series adds support for OF1.2+ experimenter error codes. v1->v2: Make it much easier to add new experimenter IDs and updates the error code used to the one used in the latest version of the EXT-260 proposal, using an ONF experimenter ID. v2->v3: Rebase against current master, repost to get into patchwork. Ben Pfaff (5): ofp-errors: Fix typos in comment. ofp-errors: Make ofperr_decode() static. extract-ofp-errors: Remove support for hexadecimal error types. ofp-errors: Implement OpenFlow 1.2+ experimenter error codes. ofp-errors: New error code ONFBIC_DUP_INSTRUCTION. OPENFLOW-1.1+ |3 - build-aux/extract-ofp-errors | 330 +--- include/openflow/nicira-ext.h |1 - include/openflow/openflow-1.2.h|5 +- include/openflow/openflow-common.h | 25 +++- lib/automake.mk|9 +- lib/ofp-actions.c |4 +- lib/ofp-errors.c | 74 ++--- lib/ofp-errors.h | 97 +++- tests/ofp-actions.at |4 +- tests/ofp-errors.at| 51 +- utilities/ovs-ofctl.c | 15 +- 12 files changed, 391 insertions(+), 227 deletions(-) -- 1.7.2.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [ext-260 v3 1/5] ofp-errors: Fix typos in comment.
Signed-off-by: Ben Pfaff
---
lib/ofp-errors.h |6 +++---
1 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
index 593241d..aa3ba50 100644
--- a/lib/ofp-errors.h
+++ b/lib/ofp-errors.h
@@ -1,5 +1,5 @@
/*
- * Copyright (c) 2008, 2009, 2010, 2011, 2012 Nicira, Inc.
+ * Copyright (c) 2008, 2009, 2010, 2011, 2012, 2013 Nicira, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
@@ -108,7 +108,7 @@ enum ofperr {
/* NX1.0(1,512), OF1.1+(1,9). Specified table-id invalid or does not
exist.
* [ A non-standard error (1,512), formerly OFPERR_NXBRC_BAD_TABLE_ID,
- * is used for OpenFlow 1.0 as there seems to be no appropriste error
+ * is used for OpenFlow 1.0 as there seems to be no appropriate error
* code defined the specification. ] */
OFPERR_OFPBRC_BAD_TABLE_ID,
@@ -118,7 +118,7 @@ enum ofperr {
/* NX1.0(1,514), NX1.1(1,514), OF1.2+(1,11). Invalid port.
* [ A non-standard error (1,514), formerly
* OFPERR_NXBRC_BAD_IN_PORT is used for OpenFlow 1.0 and 1.1 as there
- * seems to be no appropriste error code defined the specifications. ] */
+ * seems to be no appropriate error code defined the specifications. ] */
OFPERR_OFPBRC_BAD_PORT,
/* OF1.2+(1,12). Invalid packet in packet-out. */
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [ext-260 v3 5/5] ofp-errors: New error code ONFBIC_DUP_INSTRUCTION.
This is a prototype of OpenFlow enhancement proposal EXT-260 "Add error
code for duplicate instruction." It uses the error code proposed there.
Signed-off-by: Ben Pfaff
---
lib/ofp-actions.c|4 +---
lib/ofp-errors.h |3 +++
tests/ofp-actions.at |4 ++--
3 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index c9e000f..dd5ebae 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -1002,9 +1002,7 @@ decode_openflow11_instructions(const struct
ofp11_instruction insts[],
}
if (out[type]) {
-return OFPERR_OFPBAC_UNSUPPORTED_ORDER; /* No specific code for
- * a duplicate instruction
- * exist */
+return OFPERR_ONFBIC_DUP_INSTRUCTION;
}
out[type] = inst;
}
diff --git a/lib/ofp-errors.h b/lib/ofp-errors.h
index d144793..17f8bce 100644
--- a/lib/ofp-errors.h
+++ b/lib/ofp-errors.h
@@ -268,6 +268,9 @@ enum ofperr {
/* OF1.2+(3,8). Permissions error. */
OFPERR_OFPBIC_EPERM,
+/* ONF1.1+(2600). Duplicate instruction. */
+OFPERR_ONFBIC_DUP_INSTRUCTION,
+
/* ## --- ## */
/* ## OFPET_BAD_MATCH ## */
/* ## --- ## */
diff --git a/tests/ofp-actions.at b/tests/ofp-actions.at
index 6cd3b42..ebad040 100644
--- a/tests/ofp-actions.at
+++ b/tests/ofp-actions.at
@@ -339,7 +339,7 @@ dnl Check that an empty Apply-Actions instruction gets
dropped.
0004 0008
dnl Duplicate instruction type:
-# bad OF1.1 instructions: NXBIC_DUP_INSTRUCTION
+# bad OF1.1 instructions: ONFBIC_DUP_INSTRUCTION
0004 0008 0004 0008
dnl Instructions not multiple of 8 in length.
@@ -381,7 +381,7 @@ dnl Write-Metadata too long.
0002 0020 fedcba9876543210
dnl Write-Metadata duplicated.
-# bad OF1.1 instructions: NXBIC_DUP_INSTRUCTION
+# bad OF1.1 instructions: ONFBIC_DUP_INSTRUCTION
0002 0018 fedcba9876543210 ff00ff00ff00ff00 0002 0018
fedcba9876543210 ff00ff00ff00ff00
dnl Write-Metadata in wrong position (OpenFlow 1.1+ disregards the order
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [ext-260 v3 3/5] extract-ofp-errors: Remove support for hexadecimal error types.
This feature wasn't used and removing it slightly simplifies the code.
Signed-off-by: Ben Pfaff
---
build-aux/extract-ofp-errors | 10 +++---
1 files changed, 3 insertions(+), 7 deletions(-)
diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
index 965e322..32d0913 100755
--- a/build-aux/extract-ofp-errors
+++ b/build-aux/extract-ofp-errors
@@ -212,17 +212,13 @@ def extract_ofp_errors(filenames):
names.append(enum)
for dst in dsts.split(', '):
-m =
re.match(r'([A-Z0-9.+]+)\((\d+|(0x)[0-9a-fA-F]+)(?:,(\d+))?\)$', dst)
+m = re.match(r'([A-Z0-9.+]+)\((\d+)(?:,(\d+))?\)$', dst)
if not m:
fatal("%s: syntax error in destination" % dst)
targets = m.group(1)
+type_ = int(m.group(2))
if m.group(3):
-base = 16
-else:
-base = 10
-type_ = int(m.group(2), base)
-if m.group(4):
-code = int(m.group(4))
+code = int(m.group(3))
else:
code = None
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2 2/3] ofproto-dpif: Compute the subfacet add/del rate using coverage counters
On Wed, Jun 12, 2013 at 11:32:00AM -0700, Alex Wang wrote: > Thanks Ben, for the tip. > > This will not happen again. Don't worry about it, it's not a big deal. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 1/2] ofp-util: New function ofputil_port_to_string().
This function is more convenient than ofputil_format_port() when a "struct
ds" is not already in use. This commit converts one caller for which this
was already true, and the following commit will add another.
Signed-off-by: Ben Pfaff
---
lib/ofp-util.c | 32 ++--
lib/ofp-util.h |2 ++
2 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/lib/ofp-util.c b/lib/ofp-util.c
index 26da477..3cc4026 100644
--- a/lib/ofp-util.c
+++ b/lib/ofp-util.c
@@ -4144,14 +4144,12 @@ ofputil_port_from_string(const char *s, uint16_t *portp)
*portp = port32;
return true;
} else if (port32 <= OFPP_LAST_RESV) {
-struct ds msg;
+char name[OFP_MAX_PORT_NAME_LEN];
-ds_init(&msg);
-ofputil_format_port(port32, &msg);
+ofputil_port_to_string(port32, name, sizeof name);
VLOG_WARN_ONCE("referring to port %s as %u is deprecated for "
"compatibility with future versions of OpenFlow",
- ds_cstr(&msg), port32);
-ds_destroy(&msg);
+ name, port32);
*portp = port32;
return true;
@@ -4192,18 +4190,32 @@ ofputil_port_from_string(const char *s, uint16_t *portp)
void
ofputil_format_port(uint16_t port, struct ds *s)
{
-const char *name;
+char name[OFP_MAX_PORT_NAME_LEN];
+ofputil_port_to_string(port, name, sizeof name);
+ds_put_cstr(s, name);
+}
+
+/* Puts in the 'bufsize' byte in 'buf' a null-terminated string representation
+ * of OpenFlow port number 'port'. Most ports are represented as just the port
+ * number, but special ports, e.g. OFPP_LOCAL, are represented by name,
+ * e.g. "LOCAL". */
+void
+ofputil_port_to_string(uint16_t port,
+ char namebuf[OFP_MAX_PORT_NAME_LEN], size_t bufsize)
+{
switch (port) {
-#define OFPUTIL_NAMED_PORT(NAME) case OFPP_##NAME: name = #NAME; break;
+#define OFPUTIL_NAMED_PORT(NAME)\
+case OFPP_##NAME: \
+ovs_strlcpy(namebuf, #NAME, bufsize); \
+break;
OFPUTIL_NAMED_PORTS
#undef OFPUTIL_NAMED_PORT
default:
-ds_put_format(s, "%"PRIu16, port);
-return;
+snprintf(namebuf, bufsize, "%"PRIu16, port);
+break;
}
-ds_put_cstr(s, name);
}
/* Given a buffer 'b' that contains an array of OpenFlow ports of type
diff --git a/lib/ofp-util.h b/lib/ofp-util.h
index 1c8d6cd..424bb73 100644
--- a/lib/ofp-util.h
+++ b/lib/ofp-util.h
@@ -38,6 +38,8 @@ ovs_be32 ofputil_port_to_ofp11(uint16_t ofp10_port);
enum ofperr ofputil_check_output_port(uint16_t ofp_port, int max_ports);
bool ofputil_port_from_string(const char *, uint16_t *portp);
void ofputil_format_port(uint16_t port, struct ds *);
+void ofputil_port_to_string(uint16_t port, char namebuf[OFP_MAX_PORT_NAME_LEN],
+size_t bufsize);
/* Converting OFPFW10_NW_SRC_MASK and OFPFW10_NW_DST_MASK wildcard bit counts
* to and from IP bitmasks. */
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 2/2] ofproto-dpif: Make "fdb/show" report OpenFlow port numbers.
Users are more likely to be able to reasonably interpret OpenFlow port
numbers than datapath port numbers.
This issue has existed since at least 2011 but only recently has it been
possible for OpenFlow and datapath port numbers to differ (except for the
"local" port).
Reported-by: Christopher Paggen
Signed-off-by: Ben Pfaff
---
AUTHORS|1 +
ofproto/ofproto-dpif.c |9 ++---
tests/ofproto-dpif.at | 11 ---
3 files changed, 11 insertions(+), 10 deletions(-)
diff --git a/AUTHORS b/AUTHORS
index 91d4dfd..5db58cd 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -123,6 +123,7 @@ Brent Salisbury brent.salisb...@gmail.com
Bryan Fultonbr...@nicira.com
Bryan Osoro bos...@nicira.com
Cedric Hobbsced...@nicira.com
+Christopher Paggen cpag...@cisco.com
Dave Walker davewal...@ubuntu.com
David Palma pa...@onesource.pt
Derek Cormier derek.corm...@lab.ntt.co.jp
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3bc8dd7..843d953 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5623,9 +5623,12 @@ ofproto_unixctl_fdb_show(struct unixctl_conn *conn, int
argc OVS_UNUSED,
ds_put_cstr(&ds, " port VLAN MACAge\n");
LIST_FOR_EACH (e, lru_node, &ofproto->ml->lrus) {
struct ofbundle *bundle = e->port.p;
-ds_put_format(&ds, "%5d %4d "ETH_ADDR_FMT" %3d\n",
- ofbundle_get_a_port(bundle)->odp_port,
- e->vlan, ETH_ADDR_ARGS(e->mac),
+char name[OFP_MAX_PORT_NAME_LEN];
+
+ofputil_port_to_string(ofbundle_get_a_port(bundle)->up.ofp_port,
+ name, sizeof name);
+ds_put_format(&ds, "%5s %4d "ETH_ADDR_FMT" %3d\n",
+ name, e->vlan, ETH_ADDR_ARGS(e->mac),
mac_entry_age(ofproto->ml, e));
}
unixctl_command_reply(conn, ds_cstr(&ds));
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index e8458d0..3bacaca 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -1368,9 +1368,8 @@ AT_CHECK_UNQUOTED([ovs-appctl fdb/show br0 | sed
's/[[0-9]]\{1,\}$/?/'], [0], [d
AT_CHECK(
[ovs-vsctl \
-- add-br br1 \
- -- set bridge br1 datapath-type=dummy \
- -- add-port br1 p4 -- set interface p4 type=dummy \
- -- add-port br1 p5 -- set interface p5 type=dummy])
+ -- set bridge br1 datapath-type=dummy])
+ADD_OF_PORTS([br1], 4, 5)
# Trace some packet arrivals in br1 to create MAC learning entries there too.
OFPROTO_TRACE(
@@ -1408,10 +1407,8 @@ AT_CLEANUP
AT_SETUP([ofproto-dpif - MAC table overflow])
OVS_VSWITCHD_START(
- [set bridge br0 fail-mode=standalone other-config:mac-table-size=10 -- \
- add-port br0 p1 -- set Interface p1 type=dummy -- \
- add-port br0 p2 -- set Interface p2 type=dummy -- \
- add-port br0 p3 -- set Interface p3 type=dummy])
+ [set bridge br0 fail-mode=standalone other-config:mac-table-size=10])
+ADD_OF_PORTS([br0], 1, 2, 3)
arp='eth_type(0x0806),arp(sip=192.168.0.1,tip=192.168.0.2,op=1,sha=50:54:00:00:00:05,tha=00:00:00:00:00:00)'
--
1.7.2.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v2 0/2] fdb/show improvement
v1->v2: Rebase against current master, re-send to get into patchwork. Ben Pfaff (2): ofp-util: New function ofputil_port_to_string(). ofproto-dpif: Make "fdb/show" report OpenFlow port numbers. AUTHORS|1 + lib/ofp-util.c | 32 ++-- lib/ofp-util.h |2 ++ ofproto/ofproto-dpif.c |9 ++--- tests/ofproto-dpif.at | 11 --- 5 files changed, 35 insertions(+), 20 deletions(-) -- 1.7.2.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif-xlate: Hide 'struct xlate_ctx'.
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif-xlate.c | 39 +--
ofproto/ofproto-dpif-xlate.h | 46 +++---
ofproto/ofproto-dpif.c | 18 -
3 files changed, 53 insertions(+), 50 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ca65080..e4b1fd3 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -43,6 +43,41 @@ COVERAGE_DEFINE(ofproto_dpif_xlate);
VLOG_DEFINE_THIS_MODULE(ofproto_dpif_xlate);
+struct xlate_ctx {
+struct xlate_in *xin;
+struct xlate_out *xout;
+
+struct ofproto_dpif *ofproto;
+
+/* Flow at the last commit. */
+struct flow base_flow;
+
+/* Tunnel IP destination address as received. This is stored separately
+ * as the base_flow.tunnel is cleared on init to reflect the datapath
+ * behavior. Used to make sure not to send tunneled output to ourselves,
+ * which might lead to an infinite loop. This could happen easily
+ * if a tunnel is marked as 'ip_remote=flow', and the flow does not
+ * actually set the tun_dst field. */
+ovs_be32 orig_tunnel_ip_dst;
+
+/* Stack for the push and pop actions. Each stack element is of type
+ * "union mf_subvalue". */
+union mf_subvalue init_stack[1024 / sizeof(union mf_subvalue)];
+struct ofpbuf stack;
+
+/* The rule that we are currently translating, or NULL. */
+struct rule_dpif *rule;
+
+int recurse;/* Recursion level, via xlate_table_action. */
+bool max_resubmit_trigger; /* Recursed too deeply during translation. */
+uint32_t orig_skb_priority; /* Priority when packet arrived. */
+uint8_t table_id; /* OpenFlow table ID where flow was found. */
+uint32_t sflow_n_outputs; /* Number of output ports. */
+uint32_t sflow_odp_port;/* Output port for composing sFlow action. */
+uint16_t user_cookie_offset;/* Used for user_action_cookie fixup. */
+bool exit; /* No further actions should be processed. */
+};
+
/* A controller may use OFPP_NONE as the ingress port to indicate that
* it did not arrive on a "real" port. 'ofpp_none_bundle' exists for
* when an input bundle is needed for validation (e.g., mirroring or
@@ -924,7 +959,7 @@ ctx_rule_hooks(struct xlate_ctx *ctx, struct rule_dpif
*rule,
bool may_packet_in)
{
if (ctx->xin->resubmit_hook) {
-ctx->xin->resubmit_hook(ctx, rule);
+ctx->xin->resubmit_hook(ctx->xin, rule, ctx->recurse);
}
if (rule == NULL && may_packet_in) {
/* XXX
@@ -1765,7 +1800,7 @@ static void
xlate_report(struct xlate_ctx *ctx, const char *s)
{
if (ctx->xin->report_hook) {
-ctx->xin->report_hook(ctx, s);
+ctx->xin->report_hook(ctx->xin, s, ctx->recurse);
}
}
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index bffd217..9eecf51 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -88,14 +88,18 @@ struct xlate_in {
* 'rule' is the rule being submitted into. It will be null if the
* resubmit or OFPP_TABLE action didn't find a matching rule.
*
+ * 'recurse' is the resubmit recursion depth at time of invocation.
+ *
* This is normally null so the client has to set it manually after
* calling xlate_in_init(). */
-void (*resubmit_hook)(struct xlate_ctx *, struct rule_dpif *rule);
+void (*resubmit_hook)(struct xlate_in *, struct rule_dpif *rule,
+ int recurse);
/* If nonnull, flow translation calls this function to report some
* significant decision, e.g. to explain why OFPP_NORMAL translation
- * dropped a packet. */
-void (*report_hook)(struct xlate_ctx *, const char *s);
+ * dropped a packet. 'recurse' is the resubmit recursion depth at time of
+ * invocation. */
+void (*report_hook)(struct xlate_in *, const char *s, int recurse);
/* If nonnull, flow translation credits the specified statistics to each
* rule reached through a resubmit or OFPP_TABLE action.
@@ -105,42 +109,6 @@ struct xlate_in {
const struct dpif_flow_stats *resubmit_stats;
};
-/* Context used by xlate_actions() and its callees. */
-struct xlate_ctx {
-struct xlate_in *xin;
-struct xlate_out *xout;
-
-struct ofproto_dpif *ofproto;
-
-/* Flow at the last commit. */
-struct flow base_flow;
-
-/* Tunnel IP destination address as received. This is stored separately
- * as the base_flow.tunnel is cleared on init to reflect the datapath
- * behavior. Used to make sure not to send tunneled output to ourselves,
- * which might lead to an infinite loop. This could happen easily
- * if a tunnel is marked as 'ip_remote=flow', and the flow does not
- * actually set the tun_dst field. */
-ovs_be32 orig_tunnel_ip_dst;
-
-/* Stack for the pus
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Hide 'struct xlate_ctx'.
This patch neglected to delete the struct xlate_ctx; prototype from
ofproto-dpif-xlate.h. Assuming there are no comments, I'll delete
that before merging.
Ethan
On Wed, Jun 12, 2013 at 1:02 PM, Ethan Jackson wrote:
> Signed-off-by: Ethan Jackson
> ---
> ofproto/ofproto-dpif-xlate.c | 39 +--
> ofproto/ofproto-dpif-xlate.h | 46
> +++---
> ofproto/ofproto-dpif.c | 18 -
> 3 files changed, 53 insertions(+), 50 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index ca65080..e4b1fd3 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -43,6 +43,41 @@ COVERAGE_DEFINE(ofproto_dpif_xlate);
>
> VLOG_DEFINE_THIS_MODULE(ofproto_dpif_xlate);
>
> +struct xlate_ctx {
> +struct xlate_in *xin;
> +struct xlate_out *xout;
> +
> +struct ofproto_dpif *ofproto;
> +
> +/* Flow at the last commit. */
> +struct flow base_flow;
> +
> +/* Tunnel IP destination address as received. This is stored separately
> + * as the base_flow.tunnel is cleared on init to reflect the datapath
> + * behavior. Used to make sure not to send tunneled output to ourselves,
> + * which might lead to an infinite loop. This could happen easily
> + * if a tunnel is marked as 'ip_remote=flow', and the flow does not
> + * actually set the tun_dst field. */
> +ovs_be32 orig_tunnel_ip_dst;
> +
> +/* Stack for the push and pop actions. Each stack element is of type
> + * "union mf_subvalue". */
> +union mf_subvalue init_stack[1024 / sizeof(union mf_subvalue)];
> +struct ofpbuf stack;
> +
> +/* The rule that we are currently translating, or NULL. */
> +struct rule_dpif *rule;
> +
> +int recurse;/* Recursion level, via xlate_table_action.
> */
> +bool max_resubmit_trigger; /* Recursed too deeply during translation. */
> +uint32_t orig_skb_priority; /* Priority when packet arrived. */
> +uint8_t table_id; /* OpenFlow table ID where flow was found. */
> +uint32_t sflow_n_outputs; /* Number of output ports. */
> +uint32_t sflow_odp_port;/* Output port for composing sFlow action. */
> +uint16_t user_cookie_offset;/* Used for user_action_cookie fixup. */
> +bool exit; /* No further actions should be processed. */
> +};
> +
> /* A controller may use OFPP_NONE as the ingress port to indicate that
> * it did not arrive on a "real" port. 'ofpp_none_bundle' exists for
> * when an input bundle is needed for validation (e.g., mirroring or
> @@ -924,7 +959,7 @@ ctx_rule_hooks(struct xlate_ctx *ctx, struct rule_dpif
> *rule,
> bool may_packet_in)
> {
> if (ctx->xin->resubmit_hook) {
> -ctx->xin->resubmit_hook(ctx, rule);
> +ctx->xin->resubmit_hook(ctx->xin, rule, ctx->recurse);
> }
> if (rule == NULL && may_packet_in) {
> /* XXX
> @@ -1765,7 +1800,7 @@ static void
> xlate_report(struct xlate_ctx *ctx, const char *s)
> {
> if (ctx->xin->report_hook) {
> -ctx->xin->report_hook(ctx, s);
> +ctx->xin->report_hook(ctx->xin, s, ctx->recurse);
> }
> }
>
> diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
> index bffd217..9eecf51 100644
> --- a/ofproto/ofproto-dpif-xlate.h
> +++ b/ofproto/ofproto-dpif-xlate.h
> @@ -88,14 +88,18 @@ struct xlate_in {
> * 'rule' is the rule being submitted into. It will be null if the
> * resubmit or OFPP_TABLE action didn't find a matching rule.
> *
> + * 'recurse' is the resubmit recursion depth at time of invocation.
> + *
> * This is normally null so the client has to set it manually after
> * calling xlate_in_init(). */
> -void (*resubmit_hook)(struct xlate_ctx *, struct rule_dpif *rule);
> +void (*resubmit_hook)(struct xlate_in *, struct rule_dpif *rule,
> + int recurse);
>
> /* If nonnull, flow translation calls this function to report some
> * significant decision, e.g. to explain why OFPP_NORMAL translation
> - * dropped a packet. */
> -void (*report_hook)(struct xlate_ctx *, const char *s);
> + * dropped a packet. 'recurse' is the resubmit recursion depth at time
> of
> + * invocation. */
> +void (*report_hook)(struct xlate_in *, const char *s, int recurse);
>
> /* If nonnull, flow translation credits the specified statistics to each
> * rule reached through a resubmit or OFPP_TABLE action.
> @@ -105,42 +109,6 @@ struct xlate_in {
> const struct dpif_flow_stats *resubmit_stats;
> };
>
> -/* Context used by xlate_actions() and its callees. */
> -struct xlate_ctx {
> -struct xlate_in *xin;
> -struct xlate_out *xout;
> -
> -struct ofproto_dpif *ofproto;
> -
> -/* Flow at the last commit. */
> -struct flow base_flow;
> -
> -/* Tunnel IP destination address as
[ovs-dev] [PATCH] ofproto-dpif-xlate: Hide 'struct xlate_ctx'.
Signed-off-by: Ethan Jackson
---
Thsi version fixes a bug I found in the original.
---
ofproto/ofproto-dpif-xlate.c | 39 --
ofproto/ofproto-dpif-xlate.h | 48 ++
ofproto/ofproto-dpif.c | 25 +++---
3 files changed, 56 insertions(+), 56 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ca65080..e4b1fd3 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -43,6 +43,41 @@ COVERAGE_DEFINE(ofproto_dpif_xlate);
VLOG_DEFINE_THIS_MODULE(ofproto_dpif_xlate);
+struct xlate_ctx {
+struct xlate_in *xin;
+struct xlate_out *xout;
+
+struct ofproto_dpif *ofproto;
+
+/* Flow at the last commit. */
+struct flow base_flow;
+
+/* Tunnel IP destination address as received. This is stored separately
+ * as the base_flow.tunnel is cleared on init to reflect the datapath
+ * behavior. Used to make sure not to send tunneled output to ourselves,
+ * which might lead to an infinite loop. This could happen easily
+ * if a tunnel is marked as 'ip_remote=flow', and the flow does not
+ * actually set the tun_dst field. */
+ovs_be32 orig_tunnel_ip_dst;
+
+/* Stack for the push and pop actions. Each stack element is of type
+ * "union mf_subvalue". */
+union mf_subvalue init_stack[1024 / sizeof(union mf_subvalue)];
+struct ofpbuf stack;
+
+/* The rule that we are currently translating, or NULL. */
+struct rule_dpif *rule;
+
+int recurse;/* Recursion level, via xlate_table_action. */
+bool max_resubmit_trigger; /* Recursed too deeply during translation. */
+uint32_t orig_skb_priority; /* Priority when packet arrived. */
+uint8_t table_id; /* OpenFlow table ID where flow was found. */
+uint32_t sflow_n_outputs; /* Number of output ports. */
+uint32_t sflow_odp_port;/* Output port for composing sFlow action. */
+uint16_t user_cookie_offset;/* Used for user_action_cookie fixup. */
+bool exit; /* No further actions should be processed. */
+};
+
/* A controller may use OFPP_NONE as the ingress port to indicate that
* it did not arrive on a "real" port. 'ofpp_none_bundle' exists for
* when an input bundle is needed for validation (e.g., mirroring or
@@ -924,7 +959,7 @@ ctx_rule_hooks(struct xlate_ctx *ctx, struct rule_dpif
*rule,
bool may_packet_in)
{
if (ctx->xin->resubmit_hook) {
-ctx->xin->resubmit_hook(ctx, rule);
+ctx->xin->resubmit_hook(ctx->xin, rule, ctx->recurse);
}
if (rule == NULL && may_packet_in) {
/* XXX
@@ -1765,7 +1800,7 @@ static void
xlate_report(struct xlate_ctx *ctx, const char *s)
{
if (ctx->xin->report_hook) {
-ctx->xin->report_hook(ctx, s);
+ctx->xin->report_hook(ctx->xin, s, ctx->recurse);
}
}
diff --git a/ofproto/ofproto-dpif-xlate.h b/ofproto/ofproto-dpif-xlate.h
index bffd217..461a6aa 100644
--- a/ofproto/ofproto-dpif-xlate.h
+++ b/ofproto/ofproto-dpif-xlate.h
@@ -26,8 +26,6 @@
* flow translation. */
#define MAX_RESUBMIT_RECURSION 64
-struct xlate_ctx;
-
struct xlate_out {
/* Wildcards relevant in translation. Any fields that were used to
* calculate the action must be set for caching and kernel
@@ -88,14 +86,18 @@ struct xlate_in {
* 'rule' is the rule being submitted into. It will be null if the
* resubmit or OFPP_TABLE action didn't find a matching rule.
*
+ * 'recurse' is the resubmit recursion depth at time of invocation.
+ *
* This is normally null so the client has to set it manually after
* calling xlate_in_init(). */
-void (*resubmit_hook)(struct xlate_ctx *, struct rule_dpif *rule);
+void (*resubmit_hook)(struct xlate_in *, struct rule_dpif *rule,
+ int recurse);
/* If nonnull, flow translation calls this function to report some
* significant decision, e.g. to explain why OFPP_NORMAL translation
- * dropped a packet. */
-void (*report_hook)(struct xlate_ctx *, const char *s);
+ * dropped a packet. 'recurse' is the resubmit recursion depth at time of
+ * invocation. */
+void (*report_hook)(struct xlate_in *, const char *s, int recurse);
/* If nonnull, flow translation credits the specified statistics to each
* rule reached through a resubmit or OFPP_TABLE action.
@@ -105,42 +107,6 @@ struct xlate_in {
const struct dpif_flow_stats *resubmit_stats;
};
-/* Context used by xlate_actions() and its callees. */
-struct xlate_ctx {
-struct xlate_in *xin;
-struct xlate_out *xout;
-
-struct ofproto_dpif *ofproto;
-
-/* Flow at the last commit. */
-struct flow base_flow;
-
-/* Tunnel IP destination address as received. This is stored separately
- * as the base_flow.tunnel is cleared on init to reflect the datapath
- * be
Re: [ovs-dev] [PATCH] ofctl: Initial attempt for multiple tables in replace-flows
Thanks for the contribution! I have some comments. "git am" reported some trailing whitespace: Applying: ofctl: Initial attempt for multiple tables in replace-flows /home/blp/ovs/.git/rebase-apply/patch:50: trailing whitespace. table_classifiers_find_classifier(struct table_classifiers *tcls, /home/blp/ovs/.git/rebase-apply/patch:55: trailing whitespace. HMAP_FOR_EACH_IN_BUCKET(cls_node, hmap_node, table_id_hash(table_id), /home/blp/ovs/.git/rebase-apply/patch:68: trailing whitespace. hmap_insert(&tcls->classifiers, &cls_node->hmap_node, /home/blp/ovs/.git/rebase-apply/patch:135: trailing whitespace. enum ofputil_protocol protocol, struct list *packets, warning: 4 lines add whitespace errors. Why is it necessary to modify ofputil_flow_dump_protocols? It would be nice to add a test for this functionality in tests/ovs-ofctl.at, so that we can ensure that it works and that it does not regress. GCC reported some issues that made me not look farther for now: cc1: warnings being treated as errors ../utilities/ovs-ofctl.c: In function ‘read_flows_from_source’: ../utilities/ovs-ofctl.c:2190: error: passing argument 2 of ‘read_flows_from_file’ from incompatible pointer type ../utilities/ovs-ofctl.c:1946: note: expected ‘struct table_classifiers *’ but argument is of type ‘struct classifier *’ ../utilities/ovs-ofctl.c:2197: error: passing argument 3 of ‘read_flows_from_switch’ from incompatible pointer type ../utilities/ovs-ofctl.c:2053: note: expected ‘struct table_classifiers *’ but argument is of type ‘struct classifier *’ If you'll send a v2 that fixes these errors, I'll continue reviewing. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [ext-260 v3 3/5] extract-ofp-errors: Remove support for hexadecimal error types.
Looks good to me, tested.
On Wed, Jun 12, 2013 at 11:35 AM, Ben Pfaff wrote:
> This feature wasn't used and removing it slightly simplifies the code.
>
> Signed-off-by: Ben Pfaff
> ---
> build-aux/extract-ofp-errors | 10 +++---
> 1 files changed, 3 insertions(+), 7 deletions(-)
>
> diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
> index 965e322..32d0913 100755
> --- a/build-aux/extract-ofp-errors
> +++ b/build-aux/extract-ofp-errors
> @@ -212,17 +212,13 @@ def extract_ofp_errors(filenames):
> names.append(enum)
>
> for dst in dsts.split(', '):
> -m =
> re.match(r'([A-Z0-9.+]+)\((\d+|(0x)[0-9a-fA-F]+)(?:,(\d+))?\)$', dst)
> +m = re.match(r'([A-Z0-9.+]+)\((\d+)(?:,(\d+))?\)$', dst)
> if not m:
> fatal("%s: syntax error in destination" % dst)
> targets = m.group(1)
> +type_ = int(m.group(2))
> if m.group(3):
> -base = 16
> -else:
> -base = 10
> -type_ = int(m.group(2), base)
> -if m.group(4):
> -code = int(m.group(4))
> +code = int(m.group(3))
> else:
> code = None
>
> --
> 1.7.2.5
>
> ___
> 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] [ext-260 v3 3/5] extract-ofp-errors: Remove support for hexadecimal error types.
Thanks, applied to master.
On Wed, Jun 12, 2013 at 01:21:30PM -0700, Reid Price wrote:
> Looks good to me, tested.
>
>
> On Wed, Jun 12, 2013 at 11:35 AM, Ben Pfaff wrote:
>
> > This feature wasn't used and removing it slightly simplifies the code.
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > build-aux/extract-ofp-errors | 10 +++---
> > 1 files changed, 3 insertions(+), 7 deletions(-)
> >
> > diff --git a/build-aux/extract-ofp-errors b/build-aux/extract-ofp-errors
> > index 965e322..32d0913 100755
> > --- a/build-aux/extract-ofp-errors
> > +++ b/build-aux/extract-ofp-errors
> > @@ -212,17 +212,13 @@ def extract_ofp_errors(filenames):
> > names.append(enum)
> >
> > for dst in dsts.split(', '):
> > -m =
> > re.match(r'([A-Z0-9.+]+)\((\d+|(0x)[0-9a-fA-F]+)(?:,(\d+))?\)$', dst)
> > +m = re.match(r'([A-Z0-9.+]+)\((\d+)(?:,(\d+))?\)$', dst)
> > if not m:
> > fatal("%s: syntax error in destination" % dst)
> > targets = m.group(1)
> > +type_ = int(m.group(2))
> > if m.group(3):
> > -base = 16
> > -else:
> > -base = 10
> > -type_ = int(m.group(2), base)
> > -if m.group(4):
> > -code = int(m.group(4))
> > +code = int(m.group(3))
> > else:
> > code = None
> >
> > --
> > 1.7.2.5
> >
> > ___
> > 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] ofproto-dpif-xlate: Hide 'struct xlate_ctx'.
On Wed, Jun 12, 2013 at 01:12:06PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson > --- > > Thsi version fixes a bug I found in the original. Looks good. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads 13/17] ofp-print: Avoid returning static data.
On Jun 5, 2013, at 1:05 PM, Ben Pfaff wrote:
> +enum { OFP_FLOW_REMOVED_REASON_BUFSIZE = INT_STRLEN(int) + 1 };
The use of an enum to define these surprised me. I don't think it's wrong.
Just a convention I don't remember seeing elsewhere.
> static const char *
> -ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason)
> +ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason,
> + char *reasonbuf, size_t bufsize)
> {
> -static char s[32];
> -
> switch (reason) {
> case OFPRR_IDLE_TIMEOUT:
> return "idle";
> @@ -874,14 +874,15 @@ ofp_flow_removed_reason_to_string(enum
> ofp_flow_removed_reason reason)
> case OFPRR_EVICTION:
> return "eviction";
> default:
> -sprintf(s, "%d", (int) reason);
> -return s;
> +snprintf(reasonbuf, bufsize, "%d", (int) reason);
> +return reasonbuf;
> }
> }
I think the use of this function is sufficiently complicated that it could use
a comment.
> +enum { OFP_PORT_REASON_BUFSIZE = INT_STRLEN(int) + 1 };
> static const char *
> -ofp_port_reason_to_string(enum ofp_port_reason reason)
> +ofp_port_reason_to_string(enum ofp_port_reason reason,
> + char *reasonbuf, size_t bufsize)
> {
> -static char s[32];
> -
> switch (reason) {
> case OFPPR_ADD:
> return "add";
> @@ -1642,8 +1644,8 @@ ofp_port_reason_to_string(enum ofp_port_reason reason)
> return "modify";
>
> default:
> -sprintf(s, "%d", (int) reason);
> -return s;
> +snprintf(reasonbuf, bufsize, "%d", (int) reason);
> +return reasonbuf;
> }
> }
Same here.
--Justin
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 01/11] ofproto-dpif: Check for MPLS depth at the flow.
On Fri, May 31, 2013 at 02:35:11PM +0300, Jarno Rajahalme wrote: > The earlier check on base_flow.mpls_depth seemed wrong, as multiple > MPLS push actions would have resulted in the flow.mpls_depth being > set to 1 each time. > > Signed-off-by: Jarno Rajahalme Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif-xlate: Make code more readable via 'flow' and 'wc' locals.
'ctx->xin->flow' and 'ctx->xout->wc' are both pretty long. Where it gets
in the way of code readability, this patch replaces them by 'xin' and
'xout' using local variables.
Also, replace an explicit comparison against IP and IPv6 Ethertypes by
a call to is_ip_any().
Co-authored-by: Jarno Rajahalme .
Signed-off-by: Ben Pfaff
---
ofproto/ofproto-dpif-xlate.c | 337 +++---
1 files changed, 155 insertions(+), 182 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 9682158..068716a 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -370,6 +370,7 @@ static void
output_normal(struct xlate_ctx *ctx, const struct ofbundle *out_bundle,
uint16_t vlan)
{
+ovs_be16 *flow_tci = &ctx->xin->flow.vlan_tci;
struct ofport_dpif *port;
uint16_t vid;
ovs_be16 tci, old_tci;
@@ -386,18 +387,18 @@ output_normal(struct xlate_ctx *ctx, const struct
ofbundle *out_bundle,
}
}
-old_tci = ctx->xin->flow.vlan_tci;
+old_tci = *flow_tci;
tci = htons(vid);
if (tci || out_bundle->use_priority_tags) {
-tci |= ctx->xin->flow.vlan_tci & htons(VLAN_PCP_MASK);
+tci |= *flow_tci & htons(VLAN_PCP_MASK);
if (tci) {
tci |= htons(VLAN_CFI);
}
}
-ctx->xin->flow.vlan_tci = tci;
+*flow_tci = tci;
compose_output_action(ctx, port->up.ofp_port);
-ctx->xin->flow.vlan_tci = old_tci;
+*flow_tci = old_tci;
}
/* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
@@ -530,6 +531,8 @@ is_admissible(struct xlate_ctx *ctx, struct ofport_dpif
*in_port,
static void
xlate_normal(struct xlate_ctx *ctx)
{
+struct flow_wildcards *wc = &ctx->xout->wc;
+struct flow *flow = &ctx->xin->flow;
struct ofport_dpif *in_port;
struct ofbundle *in_bundle;
struct mac_entry *mac;
@@ -539,17 +542,13 @@ xlate_normal(struct xlate_ctx *ctx)
ctx->xout->has_normal = true;
/* Check the dl_type, since we may check for gratuituous ARP. */
-memset(&ctx->xout->wc.masks.dl_type, 0xff,
- sizeof ctx->xout->wc.masks.dl_type);
+memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
-memset(&ctx->xout->wc.masks.dl_src, 0xff,
- sizeof ctx->xout->wc.masks.dl_src);
-memset(&ctx->xout->wc.masks.dl_dst, 0xff,
- sizeof ctx->xout->wc.masks.dl_dst);
-memset(&ctx->xout->wc.masks.vlan_tci, 0xff,
- sizeof ctx->xout->wc.masks.vlan_tci);
+memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
+memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
+memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
-in_bundle = lookup_input_bundle(ctx->ofproto, ctx->xin->flow.in_port,
+in_bundle = lookup_input_bundle(ctx->ofproto, flow->in_port,
ctx->xin->packet != NULL, &in_port);
if (!in_bundle) {
xlate_report(ctx, "no input bundle, dropping");
@@ -557,8 +556,8 @@ xlate_normal(struct xlate_ctx *ctx)
}
/* Drop malformed frames. */
-if (ctx->xin->flow.dl_type == htons(ETH_TYPE_VLAN) &&
-!(ctx->xin->flow.vlan_tci & htons(VLAN_CFI))) {
+if (flow->dl_type == htons(ETH_TYPE_VLAN) &&
+!(flow->vlan_tci & htons(VLAN_CFI))) {
if (ctx->xin->packet != NULL) {
static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
VLOG_WARN_RL(&rl, "bridge %s: dropping packet with partial "
@@ -582,7 +581,7 @@ xlate_normal(struct xlate_ctx *ctx)
}
/* Check VLAN. */
-vid = vlan_tci_to_vid(ctx->xin->flow.vlan_tci);
+vid = vlan_tci_to_vid(flow->vlan_tci);
if (!input_vid_is_valid(vid, in_bundle, ctx->xin->packet != NULL)) {
xlate_report(ctx, "disallowed VLAN VID for this input port, dropping");
return;
@@ -596,12 +595,11 @@ xlate_normal(struct xlate_ctx *ctx)
/* Learn source MAC. */
if (ctx->xin->may_learn) {
-update_learning_table(ctx->ofproto, &ctx->xin->flow, &ctx->xout->wc,
- vlan, in_bundle);
+update_learning_table(ctx->ofproto, flow, wc, vlan, in_bundle);
}
/* Determine output bundle. */
-mac = mac_learning_lookup(ctx->ofproto->ml, ctx->xin->flow.dl_dst, vlan,
+mac = mac_learning_lookup(ctx->ofproto->ml, flow->dl_dst, vlan,
&ctx->xout->tags);
if (mac) {
if (mac->port.p != in_bundle) {
@@ -794,6 +792,7 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
bool check_stp)
{
const struct ofport_dpif *ofport = get_ofp_port(ctx->ofproto, ofp_port);
+struct flow *flow = &ctx->xin->flow;
ovs_be16 flow_vlan_tci;
uint32_t flow_skb_mark;
uint8_t flow_nw_tos;
@@ -834,25 +833,25 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
}
ctx->ofproto = ofproto_dpif_
[ovs-dev] [PATCH 3/3] ofproto-dpif: Hide struct dpif_backer.
This patch removes the last reference to dpif_backer from
ofproto-dpif-xlate, and moves it's definition into ofproto-dpif.c
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif-xlate.c |7 +++---
ofproto/ofproto-dpif.c | 54 ++
ofproto/ofproto-dpif.h | 49 ++
3 files changed, 59 insertions(+), 51 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 500712b..d41a4b6 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1279,8 +1279,7 @@ xlate_enqueue_action(struct xlate_ctx *ctx,
int error;
/* Translate queue to priority. */
-error = dpif_queue_to_priority(ctx->ofproto->backer->dpif,
- queue_id, &priority);
+error = ofproto_dpif_queue_to_priority(ctx->ofproto, queue_id, &priority);
if (error) {
/* Fall back to ordinary output action. */
xlate_output_action(ctx, enqueue->port, 0, false);
@@ -1313,8 +1312,8 @@ xlate_set_queue_action(struct xlate_ctx *ctx, uint32_t
queue_id)
{
uint32_t skb_priority;
-if (!dpif_queue_to_priority(ctx->ofproto->backer->dpif,
-queue_id, &skb_priority)) {
+if (!ofproto_dpif_queue_to_priority(ctx->ofproto, queue_id,
+&skb_priority)) {
ctx->xin->flow.skb_priority = skb_priority;
} else {
/* Couldn't translate queue to a priority. Nothing to do. A warning
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index a51b60d..46fec41 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -313,6 +313,53 @@ struct drop_key {
size_t key_len;
};
+/* All datapaths of a given type share a single dpif backer instance. */
+struct dpif_backer {
+char *type;
+int refcount;
+struct dpif *dpif;
+struct timer next_expiration;
+struct hmap odp_to_ofport_map; /* ODP port to ofport mapping. */
+
+struct simap tnl_backers; /* Set of dpif ports backing tunnels. */
+
+/* Facet revalidation flags applying to facets which use this backer. */
+enum revalidate_reason need_revalidate; /* Revalidate every facet. */
+struct tag_set revalidate_set; /* Revalidate only matching facets. */
+
+struct hmap drop_keys; /* Set of dropped odp keys. */
+bool recv_set_enable; /* Enables or disables receiving packets. */
+
+struct hmap subfacets;
+struct governor *governor;
+
+/* Subfacet statistics.
+ *
+ * These keep track of the total number of subfacets added and deleted and
+ * flow life span. They are useful for computing the flow rates stats
+ * exposed via "ovs-appctl dpif/show". The goal is to learn about
+ * traffic patterns in ways that we can use later to improve Open vSwitch
+ * performance in new situations. */
+long long int created; /* Time when it is created. */
+unsigned max_n_subfacet; /* Maximum number of flows */
+unsigned avg_n_subfacet; /* Average number of flows. */
+long long int avg_subfacet_life; /* Average life span of subfacets. */
+
+/* The average number of subfacets... */
+struct avg_subfacet_rates hourly; /* ...over the last hour. */
+struct avg_subfacet_rates daily;/* ...over the last day. */
+struct avg_subfacet_rates lifetime; /* ...over the switch lifetime. */
+long long int last_minute; /* Last time 'hourly' was updated. */
+
+/* Number of subfacets added or deleted since 'last_minute'. */
+unsigned subfacet_add_count;
+unsigned subfacet_del_count;
+
+/* Number of subfacets added or deleted from 'created' to 'last_minute.' */
+unsigned long long int total_subfacet_add_count;
+unsigned long long int total_subfacet_del_count;
+};
+
/* All existing ofproto_backer instances, indexed by ofproto->up.type. */
static struct shash all_dpif_backers = SHASH_INITIALIZER(&all_dpif_backers);
@@ -1881,6 +1928,13 @@ stp_process_packet(const struct ofport_dpif *ofport,
}
}
+int
+ofproto_dpif_queue_to_priority(const struct ofproto_dpif *ofproto,
+ uint32_t queue_id, uint32_t *priority)
+{
+return dpif_queue_to_priority(ofproto->backer->dpif, queue_id, priority);
+}
+
struct priority_to_dscp *
get_priority(const struct ofport_dpif *ofport, uint32_t priority)
{
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 0c3252c..38a4ec2 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -80,53 +80,6 @@ struct avg_subfacet_rates {
double del_rate; /* Moving average of flows deleted per minute. */
};
-/* All datapaths of a given type share a single dpif backer instance. */
-struct dpif_backer {
-char *type;
-int refcount;
-struct dpif *dpif;
-struct timer next_expiration;
-struct hmap odp_to_ofport_map; /* ODP port to ofport mapping. */
-
-
[ovs-dev] [PATCH 1/3] mac-learning: Simplify mac_learning_changed().
With this patch, the mac_learning module takes responsibility for
remembering tags which need revalidation after a
mac_learning_changed() call. This removes one of
ofproto-dpif-xlate's dpif_backer uses.
Signed-off-by: Ethan Jackson
---
lib/mac-learning.c | 15 +--
lib/mac-learning.h |3 ++-
ofproto/ofproto-dpif-xlate.c |3 +--
3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/lib/mac-learning.c b/lib/mac-learning.c
index 052ac48..d66f331 100644
--- a/lib/mac-learning.c
+++ b/lib/mac-learning.c
@@ -123,6 +123,7 @@ mac_learning_create(unsigned int idle_time)
ml->flood_vlans = NULL;
ml->idle_time = normalize_idle_time(idle_time);
ml->max_entries = MAC_DEFAULT_MAX;
+tag_set_init(&ml->tags);
return ml;
}
@@ -245,22 +246,20 @@ mac_learning_insert(struct mac_learning *ml,
return e;
}
-/* Changes 'e''s tag to a new, randomly selected one, and returns the tag that
- * would have been previously used for this entry's MAC and VLAN (either before
- * 'e' was inserted, if it is new, or otherwise before its port was updated.)
+/* Changes 'e''s tag to a new, randomly selected one.
*
* The client should call this function after obtaining a MAC learning entry
* from mac_learning_insert(), if the entry is either new or if its learned
* port has changed. */
-tag_type
+void
mac_learning_changed(struct mac_learning *ml, struct mac_entry *e)
{
-tag_type old_tag = e->tag;
+tag_type tag = e->tag ? e->tag : make_unknown_mac_tag(ml, e->mac, e->vlan);
COVERAGE_INC(mac_learning_learned);
e->tag = tag_create_random();
-return old_tag ? old_tag : make_unknown_mac_tag(ml, e->mac, e->vlan);
+tag_set_add(&ml->tags, tag);
}
/* Looks up MAC 'dst' for VLAN 'vlan' in 'ml' and returns the associated MAC
@@ -322,6 +321,10 @@ void
mac_learning_run(struct mac_learning *ml, struct tag_set *set)
{
struct mac_entry *e;
+
+tag_set_union(set, &ml->tags);
+tag_set_init(&ml->tags);
+
while (get_lru(ml, &e)
&& (hmap_count(&ml->table) > ml->max_entries
|| time_now() >= e->expires)) {
diff --git a/lib/mac-learning.h b/lib/mac-learning.h
index 666b00f..1cbacfe 100644
--- a/lib/mac-learning.h
+++ b/lib/mac-learning.h
@@ -85,6 +85,7 @@ struct mac_learning {
unsigned long *flood_vlans; /* Bitmap of learning disabled VLANs. */
unsigned int idle_time; /* Max age before deleting an entry. */
size_t max_entries; /* Max number of learned MACs. */
+struct tag_set tags;/* Tags which have changed. */
};
/* Basics. */
@@ -107,7 +108,7 @@ bool mac_learning_may_learn(const struct mac_learning *,
struct mac_entry *mac_learning_insert(struct mac_learning *,
const uint8_t src[ETH_ADDR_LEN],
uint16_t vlan);
-tag_type mac_learning_changed(struct mac_learning *, struct mac_entry *);
+void mac_learning_changed(struct mac_learning *, struct mac_entry *);
/* Lookup. */
struct mac_entry *mac_learning_lookup(const struct mac_learning *,
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index e4b1fd3..e53eaad 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -465,8 +465,7 @@ update_learning_table(struct ofproto_dpif *ofproto,
in_bundle->name, vlan);
mac->port.p = in_bundle;
-tag_set_add(&ofproto->backer->revalidate_set,
-mac_learning_changed(ofproto->ml, mac));
+mac_learning_changed(ofproto->ml, mac);
}
}
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 02/11] ofproto-dpif: Use flow pointer in action execution.
On Fri, May 31, 2013 at 02:35:12PM +0300, Jarno Rajahalme wrote: > With the recent change in xlate_ctx some of the code became repetitive > in accessing the ctx->xin->flow. This replaces the *ctx argument with > *flow argument for action execution functions that only need to access > ctx->xin->flow, making the code more readable. > Summary:Summary: > > Signed-off-by: Jarno Rajahalme This has pretty severely bit-rotted, but it's a really nice improvement. I redid it all and sent out my own version: http://patchwork.openvswitch.org/patch/202/ ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/3] ofproto-dpif: Verify backer in ofport_get_peer().
This marginally simplifies the code, and removes a reference to
dpif_backer from ofproto-dpif-xlate.
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif-xlate.c |7 ---
ofproto/ofproto-dpif.c |8 +++-
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index e53eaad..500712b 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -817,7 +817,6 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
if (netdev_vport_is_patch(ofport->up.netdev)) {
struct ofport_dpif *peer = ofport_get_peer(ofport);
struct flow old_flow = ctx->xin->flow;
-const struct ofproto_dpif *peer_ofproto;
enum slow_path_reason special;
struct ofport_dpif *in_port;
@@ -826,12 +825,6 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
return;
}
-peer_ofproto = ofproto_dpif_cast(peer->up.ofproto);
-if (peer_ofproto->backer != ctx->ofproto->backer) {
-xlate_report(ctx, "Patch port peer on a different datapath");
-return;
-}
-
ctx->ofproto = ofproto_dpif_cast(peer->up.ofproto);
ctx->xin->flow.in_port = peer->up.ofp_port;
ctx->xin->flow.metadata = htonll(0);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3033aac..a51b60d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2737,6 +2737,7 @@ struct ofport_dpif *
ofport_get_peer(const struct ofport_dpif *ofport_dpif)
{
const struct ofproto_dpif *ofproto;
+const struct dpif_backer *backer;
const char *peer;
peer = netdev_vport_patch_peer(ofport_dpif->up.netdev);
@@ -2744,11 +2745,16 @@ ofport_get_peer(const struct ofport_dpif *ofport_dpif)
return NULL;
}
+backer = ofproto_dpif_cast(ofport_dpif->up.ofproto)->backer;
HMAP_FOR_EACH (ofproto, all_ofproto_dpifs_node, &all_ofproto_dpifs) {
struct ofport *ofport;
+if (ofproto->backer != backer) {
+continue;
+}
+
ofport = shash_find_data(&ofproto->up.port_by_name, peer);
-if (ofport && ofport->ofproto->ofproto_class == &ofproto_dpif_class) {
+if (ofport) {
return ofport_dpif_cast(ofport);
}
}
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Make code more readable via 'flow' and 'wc' locals.
Acked-by: Ethan Jackson
On Wed, Jun 12, 2013 at 2:23 PM, Ben Pfaff wrote:
> 'ctx->xin->flow' and 'ctx->xout->wc' are both pretty long. Where it gets
> in the way of code readability, this patch replaces them by 'xin' and
> 'xout' using local variables.
>
> Also, replace an explicit comparison against IP and IPv6 Ethertypes by
> a call to is_ip_any().
>
> Co-authored-by: Jarno Rajahalme .
> Signed-off-by: Ben Pfaff
> ---
> ofproto/ofproto-dpif-xlate.c | 337
> +++---
> 1 files changed, 155 insertions(+), 182 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index 9682158..068716a 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -370,6 +370,7 @@ static void
> output_normal(struct xlate_ctx *ctx, const struct ofbundle *out_bundle,
>uint16_t vlan)
> {
> +ovs_be16 *flow_tci = &ctx->xin->flow.vlan_tci;
> struct ofport_dpif *port;
> uint16_t vid;
> ovs_be16 tci, old_tci;
> @@ -386,18 +387,18 @@ output_normal(struct xlate_ctx *ctx, const struct
> ofbundle *out_bundle,
> }
> }
>
> -old_tci = ctx->xin->flow.vlan_tci;
> +old_tci = *flow_tci;
> tci = htons(vid);
> if (tci || out_bundle->use_priority_tags) {
> -tci |= ctx->xin->flow.vlan_tci & htons(VLAN_PCP_MASK);
> +tci |= *flow_tci & htons(VLAN_PCP_MASK);
> if (tci) {
> tci |= htons(VLAN_CFI);
> }
> }
> -ctx->xin->flow.vlan_tci = tci;
> +*flow_tci = tci;
>
> compose_output_action(ctx, port->up.ofp_port);
> -ctx->xin->flow.vlan_tci = old_tci;
> +*flow_tci = old_tci;
> }
>
> /* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
> @@ -530,6 +531,8 @@ is_admissible(struct xlate_ctx *ctx, struct ofport_dpif
> *in_port,
> static void
> xlate_normal(struct xlate_ctx *ctx)
> {
> +struct flow_wildcards *wc = &ctx->xout->wc;
> +struct flow *flow = &ctx->xin->flow;
> struct ofport_dpif *in_port;
> struct ofbundle *in_bundle;
> struct mac_entry *mac;
> @@ -539,17 +542,13 @@ xlate_normal(struct xlate_ctx *ctx)
> ctx->xout->has_normal = true;
>
> /* Check the dl_type, since we may check for gratuituous ARP. */
> -memset(&ctx->xout->wc.masks.dl_type, 0xff,
> - sizeof ctx->xout->wc.masks.dl_type);
> +memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
>
> -memset(&ctx->xout->wc.masks.dl_src, 0xff,
> - sizeof ctx->xout->wc.masks.dl_src);
> -memset(&ctx->xout->wc.masks.dl_dst, 0xff,
> - sizeof ctx->xout->wc.masks.dl_dst);
> -memset(&ctx->xout->wc.masks.vlan_tci, 0xff,
> - sizeof ctx->xout->wc.masks.vlan_tci);
> +memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
> +memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
> +memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
>
> -in_bundle = lookup_input_bundle(ctx->ofproto, ctx->xin->flow.in_port,
> +in_bundle = lookup_input_bundle(ctx->ofproto, flow->in_port,
> ctx->xin->packet != NULL, &in_port);
> if (!in_bundle) {
> xlate_report(ctx, "no input bundle, dropping");
> @@ -557,8 +556,8 @@ xlate_normal(struct xlate_ctx *ctx)
> }
>
> /* Drop malformed frames. */
> -if (ctx->xin->flow.dl_type == htons(ETH_TYPE_VLAN) &&
> -!(ctx->xin->flow.vlan_tci & htons(VLAN_CFI))) {
> +if (flow->dl_type == htons(ETH_TYPE_VLAN) &&
> +!(flow->vlan_tci & htons(VLAN_CFI))) {
> if (ctx->xin->packet != NULL) {
> static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> VLOG_WARN_RL(&rl, "bridge %s: dropping packet with partial "
> @@ -582,7 +581,7 @@ xlate_normal(struct xlate_ctx *ctx)
> }
>
> /* Check VLAN. */
> -vid = vlan_tci_to_vid(ctx->xin->flow.vlan_tci);
> +vid = vlan_tci_to_vid(flow->vlan_tci);
> if (!input_vid_is_valid(vid, in_bundle, ctx->xin->packet != NULL)) {
> xlate_report(ctx, "disallowed VLAN VID for this input port,
> dropping");
> return;
> @@ -596,12 +595,11 @@ xlate_normal(struct xlate_ctx *ctx)
>
> /* Learn source MAC. */
> if (ctx->xin->may_learn) {
> -update_learning_table(ctx->ofproto, &ctx->xin->flow, &ctx->xout->wc,
> - vlan, in_bundle);
> +update_learning_table(ctx->ofproto, flow, wc, vlan, in_bundle);
> }
>
> /* Determine output bundle. */
> -mac = mac_learning_lookup(ctx->ofproto->ml, ctx->xin->flow.dl_dst, vlan,
> +mac = mac_learning_lookup(ctx->ofproto->ml, flow->dl_dst, vlan,
>&ctx->xout->tags);
> if (mac) {
> if (mac->port.p != in_bundle) {
> @@ -794,6 +792,7 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
> ofp_port,
> bool check_stp)
> {
> const struct ofport_dpif *ofport =
Re: [ovs-dev] [PATCH 03/11] ofproto-dpif: Harmonize naming of internal functions.
On Fri, May 31, 2013 at 02:35:13PM +0300, Jarno Rajahalme wrote: > It would be good to be able to harminize the use of "xlate", "execute", > "compose", etc. "xlate" clearly relates to the use of the various translation > context structures, but the distinction between "execute" and "compose" is > not that clear, so these names could be going either way. Choose to go with > "compose", keeping with the older tradition. > > Signed-off-by: Jarno Rajahalme Applied to master, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 04/11] ofproto-dpif: Use flow pointers inside functions.
On Fri, May 31, 2013 at 02:35:14PM +0300, Jarno Rajahalme wrote: > This will make later patches easier to follow. > > Signed-off-by: Jarno Rajahalme I caught many of these in my rewrite of patch 2 as reposted at: http://openvswitch.org/pipermail/dev/2013-June/028383.html I see that I missed some, so feel free to repost if you think those are worthwhile. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Make code more readable via 'flow' and 'wc' locals.
Thanks, I'll apply this in a minute.
On Wed, Jun 12, 2013 at 02:34:14PM -0700, Ethan Jackson wrote:
> Acked-by: Ethan Jackson
>
> On Wed, Jun 12, 2013 at 2:23 PM, Ben Pfaff wrote:
> > 'ctx->xin->flow' and 'ctx->xout->wc' are both pretty long. Where it gets
> > in the way of code readability, this patch replaces them by 'xin' and
> > 'xout' using local variables.
> >
> > Also, replace an explicit comparison against IP and IPv6 Ethertypes by
> > a call to is_ip_any().
> >
> > Co-authored-by: Jarno Rajahalme .
> > Signed-off-by: Ben Pfaff
> > ---
> > ofproto/ofproto-dpif-xlate.c | 337
> > +++---
> > 1 files changed, 155 insertions(+), 182 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> > index 9682158..068716a 100644
> > --- a/ofproto/ofproto-dpif-xlate.c
> > +++ b/ofproto/ofproto-dpif-xlate.c
> > @@ -370,6 +370,7 @@ static void
> > output_normal(struct xlate_ctx *ctx, const struct ofbundle *out_bundle,
> >uint16_t vlan)
> > {
> > +ovs_be16 *flow_tci = &ctx->xin->flow.vlan_tci;
> > struct ofport_dpif *port;
> > uint16_t vid;
> > ovs_be16 tci, old_tci;
> > @@ -386,18 +387,18 @@ output_normal(struct xlate_ctx *ctx, const struct
> > ofbundle *out_bundle,
> > }
> > }
> >
> > -old_tci = ctx->xin->flow.vlan_tci;
> > +old_tci = *flow_tci;
> > tci = htons(vid);
> > if (tci || out_bundle->use_priority_tags) {
> > -tci |= ctx->xin->flow.vlan_tci & htons(VLAN_PCP_MASK);
> > +tci |= *flow_tci & htons(VLAN_PCP_MASK);
> > if (tci) {
> > tci |= htons(VLAN_CFI);
> > }
> > }
> > -ctx->xin->flow.vlan_tci = tci;
> > +*flow_tci = tci;
> >
> > compose_output_action(ctx, port->up.ofp_port);
> > -ctx->xin->flow.vlan_tci = old_tci;
> > +*flow_tci = old_tci;
> > }
> >
> > /* A VM broadcasts a gratuitous ARP to indicate that it has resumed after
> > @@ -530,6 +531,8 @@ is_admissible(struct xlate_ctx *ctx, struct ofport_dpif
> > *in_port,
> > static void
> > xlate_normal(struct xlate_ctx *ctx)
> > {
> > +struct flow_wildcards *wc = &ctx->xout->wc;
> > +struct flow *flow = &ctx->xin->flow;
> > struct ofport_dpif *in_port;
> > struct ofbundle *in_bundle;
> > struct mac_entry *mac;
> > @@ -539,17 +542,13 @@ xlate_normal(struct xlate_ctx *ctx)
> > ctx->xout->has_normal = true;
> >
> > /* Check the dl_type, since we may check for gratuituous ARP. */
> > -memset(&ctx->xout->wc.masks.dl_type, 0xff,
> > - sizeof ctx->xout->wc.masks.dl_type);
> > +memset(&wc->masks.dl_type, 0xff, sizeof wc->masks.dl_type);
> >
> > -memset(&ctx->xout->wc.masks.dl_src, 0xff,
> > - sizeof ctx->xout->wc.masks.dl_src);
> > -memset(&ctx->xout->wc.masks.dl_dst, 0xff,
> > - sizeof ctx->xout->wc.masks.dl_dst);
> > -memset(&ctx->xout->wc.masks.vlan_tci, 0xff,
> > - sizeof ctx->xout->wc.masks.vlan_tci);
> > +memset(&wc->masks.dl_src, 0xff, sizeof wc->masks.dl_src);
> > +memset(&wc->masks.dl_dst, 0xff, sizeof wc->masks.dl_dst);
> > +memset(&wc->masks.vlan_tci, 0xff, sizeof wc->masks.vlan_tci);
> >
> > -in_bundle = lookup_input_bundle(ctx->ofproto, ctx->xin->flow.in_port,
> > +in_bundle = lookup_input_bundle(ctx->ofproto, flow->in_port,
> > ctx->xin->packet != NULL, &in_port);
> > if (!in_bundle) {
> > xlate_report(ctx, "no input bundle, dropping");
> > @@ -557,8 +556,8 @@ xlate_normal(struct xlate_ctx *ctx)
> > }
> >
> > /* Drop malformed frames. */
> > -if (ctx->xin->flow.dl_type == htons(ETH_TYPE_VLAN) &&
> > -!(ctx->xin->flow.vlan_tci & htons(VLAN_CFI))) {
> > +if (flow->dl_type == htons(ETH_TYPE_VLAN) &&
> > +!(flow->vlan_tci & htons(VLAN_CFI))) {
> > if (ctx->xin->packet != NULL) {
> > static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
> > VLOG_WARN_RL(&rl, "bridge %s: dropping packet with partial "
> > @@ -582,7 +581,7 @@ xlate_normal(struct xlate_ctx *ctx)
> > }
> >
> > /* Check VLAN. */
> > -vid = vlan_tci_to_vid(ctx->xin->flow.vlan_tci);
> > +vid = vlan_tci_to_vid(flow->vlan_tci);
> > if (!input_vid_is_valid(vid, in_bundle, ctx->xin->packet != NULL)) {
> > xlate_report(ctx, "disallowed VLAN VID for this input port,
> > dropping");
> > return;
> > @@ -596,12 +595,11 @@ xlate_normal(struct xlate_ctx *ctx)
> >
> > /* Learn source MAC. */
> > if (ctx->xin->may_learn) {
> > -update_learning_table(ctx->ofproto, &ctx->xin->flow,
> > &ctx->xout->wc,
> > - vlan, in_bundle);
> > +update_learning_table(ctx->ofproto, flow, wc, vlan, in_bundle);
> > }
> >
> > /* Determine output bundle. */
> > -mac = mac_learning_lookup(ctx->ofproto->ml, ctx->xin->flow.dl_dst,
> > vlan,
> > +mac = ma
[ovs-dev] [PATCH 3/3] ofproto-dpif: Hide struct dpif_backer.
This patch removes the last reference to dpif_backer from
ofproto-dpif-xlate, and moves it's definition into ofproto-dpif.c.
Signed-off-by: Ethan Jackson
---
In this version, I moved revalidate_reason out of ofproto-dpif.h as well.
---
ofproto/ofproto-dpif-xlate.c |7 ++---
ofproto/ofproto-dpif.c | 69 ++
ofproto/ofproto-dpif.h | 65 ++-
3 files changed, 74 insertions(+), 67 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 500712b..d41a4b6 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1279,8 +1279,7 @@ xlate_enqueue_action(struct xlate_ctx *ctx,
int error;
/* Translate queue to priority. */
-error = dpif_queue_to_priority(ctx->ofproto->backer->dpif,
- queue_id, &priority);
+error = ofproto_dpif_queue_to_priority(ctx->ofproto, queue_id, &priority);
if (error) {
/* Fall back to ordinary output action. */
xlate_output_action(ctx, enqueue->port, 0, false);
@@ -1313,8 +1312,8 @@ xlate_set_queue_action(struct xlate_ctx *ctx, uint32_t
queue_id)
{
uint32_t skb_priority;
-if (!dpif_queue_to_priority(ctx->ofproto->backer->dpif,
-queue_id, &skb_priority)) {
+if (!ofproto_dpif_queue_to_priority(ctx->ofproto, queue_id,
+&skb_priority)) {
ctx->xin->flow.skb_priority = skb_priority;
} else {
/* Couldn't translate queue to a priority. Nothing to do. A warning
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index a51b60d..eddbd67 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -298,6 +298,21 @@ struct dpif_completion {
struct ofoperation *op;
};
+/* Reasons that we might need to revalidate every facet, and corresponding
+ * coverage counters.
+ *
+ * A value of 0 means that there is no need to revalidate.
+ *
+ * It would be nice to have some cleaner way to integrate with coverage
+ * counters, but with only a few reasons I guess this is good enough for
+ * now. */
+enum revalidate_reason {
+REV_RECONFIGURE = 1, /* Switch configuration changed. */
+REV_STP, /* Spanning tree protocol port status change. */
+REV_PORT_TOGGLED, /* Port enabled or disabled by CFM, LACP, ...*/
+REV_FLOW_TABLE,/* Flow table changed. */
+REV_INCONSISTENCY /* Facet self-check failed. */
+};
COVERAGE_DEFINE(rev_reconfigure);
COVERAGE_DEFINE(rev_stp);
COVERAGE_DEFINE(rev_port_toggled);
@@ -313,6 +328,53 @@ struct drop_key {
size_t key_len;
};
+/* All datapaths of a given type share a single dpif backer instance. */
+struct dpif_backer {
+char *type;
+int refcount;
+struct dpif *dpif;
+struct timer next_expiration;
+struct hmap odp_to_ofport_map; /* ODP port to ofport mapping. */
+
+struct simap tnl_backers; /* Set of dpif ports backing tunnels. */
+
+/* Facet revalidation flags applying to facets which use this backer. */
+enum revalidate_reason need_revalidate; /* Revalidate every facet. */
+struct tag_set revalidate_set; /* Revalidate only matching facets. */
+
+struct hmap drop_keys; /* Set of dropped odp keys. */
+bool recv_set_enable; /* Enables or disables receiving packets. */
+
+struct hmap subfacets;
+struct governor *governor;
+
+/* Subfacet statistics.
+ *
+ * These keep track of the total number of subfacets added and deleted and
+ * flow life span. They are useful for computing the flow rates stats
+ * exposed via "ovs-appctl dpif/show". The goal is to learn about
+ * traffic patterns in ways that we can use later to improve Open vSwitch
+ * performance in new situations. */
+long long int created; /* Time when it is created. */
+unsigned max_n_subfacet; /* Maximum number of flows */
+unsigned avg_n_subfacet; /* Average number of flows. */
+long long int avg_subfacet_life; /* Average life span of subfacets. */
+
+/* The average number of subfacets... */
+struct avg_subfacet_rates hourly; /* ...over the last hour. */
+struct avg_subfacet_rates daily;/* ...over the last day. */
+struct avg_subfacet_rates lifetime; /* ...over the switch lifetime. */
+long long int last_minute; /* Last time 'hourly' was updated. */
+
+/* Number of subfacets added or deleted since 'last_minute'. */
+unsigned subfacet_add_count;
+unsigned subfacet_del_count;
+
+/* Number of subfacets added or deleted from 'created' to 'last_minute.' */
+unsigned long long int total_subfacet_add_count;
+unsigned long long int total_subfacet_del_count;
+};
+
/* All existing ofproto_backer instances, indexed by ofproto->up.type. */
static struct shash all_dpif_backers = SHASH_INITIALIZER(&all_dpif_backers
Re: [ovs-dev] [threads 13/17] ofp-print: Avoid returning static data.
On Wed, Jun 12, 2013 at 02:18:43PM -0700, Justin Pettit wrote:
>
> On Jun 5, 2013, at 1:05 PM, Ben Pfaff wrote:
>
> > +enum { OFP_FLOW_REMOVED_REASON_BUFSIZE = INT_STRLEN(int) + 1 };
>
> The use of an enum to define these surprised me. I don't think it's
> wrong. Just a convention I don't remember seeing elsewhere.
I changed it to a #define.
> > static const char *
> > -ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason)
> > +ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason,
> > + char *reasonbuf, size_t bufsize)
> > {
> > -static char s[32];
> > -
> > switch (reason) {
> > case OFPRR_IDLE_TIMEOUT:
> > return "idle";
> > @@ -874,14 +874,15 @@ ofp_flow_removed_reason_to_string(enum
> > ofp_flow_removed_reason reason)
> > case OFPRR_EVICTION:
> > return "eviction";
> > default:
> > -sprintf(s, "%d", (int) reason);
> > -return s;
> > +snprintf(reasonbuf, bufsize, "%d", (int) reason);
> > +return reasonbuf;
> > }
> > }
>
> I think the use of this function is sufficiently complicated that it could
> use a comment.
I added:
/* Returns a string form of 'reason'. The return value is either a statically
* allocated constant string or the 'bufsize'-byte buffer 'reasonbuf'.
* 'bufsize' should be at least OFP_FLOW_REMOVED_REASON_BUFSIZE. */
How's that?
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [backer v2 2/3] ofproto-dpif: Verify backer in ofport_get_peer().
This marginally simplifies the code, and removes a reference to
dpif_backer from ofproto-dpif-xlate.
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif-xlate.c |7 ---
ofproto/ofproto-dpif.c |8 +++-
2 files changed, 7 insertions(+), 8 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index eb41993..7d3a7c4 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -816,7 +816,6 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
if (netdev_vport_is_patch(ofport->up.netdev)) {
struct ofport_dpif *peer = ofport_get_peer(ofport);
struct flow old_flow = ctx->xin->flow;
-const struct ofproto_dpif *peer_ofproto;
enum slow_path_reason special;
struct ofport_dpif *in_port;
@@ -825,12 +824,6 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
return;
}
-peer_ofproto = ofproto_dpif_cast(peer->up.ofproto);
-if (peer_ofproto->backer != ctx->ofproto->backer) {
-xlate_report(ctx, "Patch port peer on a different datapath");
-return;
-}
-
ctx->ofproto = ofproto_dpif_cast(peer->up.ofproto);
flow->in_port = peer->up.ofp_port;
flow->metadata = htonll(0);
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 3033aac..a51b60d 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -2737,6 +2737,7 @@ struct ofport_dpif *
ofport_get_peer(const struct ofport_dpif *ofport_dpif)
{
const struct ofproto_dpif *ofproto;
+const struct dpif_backer *backer;
const char *peer;
peer = netdev_vport_patch_peer(ofport_dpif->up.netdev);
@@ -2744,11 +2745,16 @@ ofport_get_peer(const struct ofport_dpif *ofport_dpif)
return NULL;
}
+backer = ofproto_dpif_cast(ofport_dpif->up.ofproto)->backer;
HMAP_FOR_EACH (ofproto, all_ofproto_dpifs_node, &all_ofproto_dpifs) {
struct ofport *ofport;
+if (ofproto->backer != backer) {
+continue;
+}
+
ofport = shash_find_data(&ofproto->up.port_by_name, peer);
-if (ofport && ofport->ofproto->ofproto_class == &ofproto_dpif_class) {
+if (ofport) {
return ofport_dpif_cast(ofport);
}
}
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [threads v2] ofp-print: Avoid returning static data.
Returning a static data buffer makes code more brittle and definitely
not thread-safe, so this commit switches to using a caller-provided
buffer instead.
Signed-off-by: Ben Pfaff
---
lib/ofp-print.c | 47 ---
1 files changed, 32 insertions(+), 15 deletions(-)
diff --git a/lib/ofp-print.c b/lib/ofp-print.c
index 9471385..092a4bc 100644
--- a/lib/ofp-print.c
+++ b/lib/ofp-print.c
@@ -859,11 +859,14 @@ ofp_print_duration(struct ds *string, unsigned int sec,
unsigned int nsec)
ds_put_char(string, 's');
}
+/* Returns a string form of 'reason'. The return value is either a statically
+ * allocated constant string or the 'bufsize'-byte buffer 'reasonbuf'.
+ * 'bufsize' should be at least OFP_FLOW_REMOVED_REASON_BUFSIZE. */
+#define OFP_FLOW_REMOVED_REASON_BUFSIZE (INT_STRLEN(int) + 1)
static const char *
-ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason)
+ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason,
+ char *reasonbuf, size_t bufsize)
{
-static char s[32];
-
switch (reason) {
case OFPRR_IDLE_TIMEOUT:
return "idle";
@@ -876,14 +879,15 @@ ofp_flow_removed_reason_to_string(enum
ofp_flow_removed_reason reason)
case OFPRR_EVICTION:
return "eviction";
default:
-sprintf(s, "%d", (int) reason);
-return s;
+snprintf(reasonbuf, bufsize, "%d", (int) reason);
+return reasonbuf;
}
}
static void
ofp_print_flow_removed(struct ds *string, const struct ofp_header *oh)
{
+char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE];
struct ofputil_flow_removed fr;
enum ofperr error;
@@ -897,7 +901,8 @@ ofp_print_flow_removed(struct ds *string, const struct
ofp_header *oh)
match_format(&fr.match, string, fr.priority);
ds_put_format(string, " reason=%s",
- ofp_flow_removed_reason_to_string(fr.reason));
+ ofp_flow_removed_reason_to_string(fr.reason, reasonbuf,
+sizeof reasonbuf));
if (fr.table_id != 255) {
ds_put_format(string, " table_id=%"PRIu8, fr.table_id);
@@ -1628,11 +1633,11 @@ ofp_print_nxt_set_packet_in_format(struct ds *string,
}
}
+enum { OFP_PORT_REASON_BUFSIZE = INT_STRLEN(int) + 1 };
static const char *
-ofp_port_reason_to_string(enum ofp_port_reason reason)
+ofp_port_reason_to_string(enum ofp_port_reason reason,
+ char *reasonbuf, size_t bufsize)
{
-static char s[32];
-
switch (reason) {
case OFPPR_ADD:
return "add";
@@ -1644,8 +1649,8 @@ ofp_port_reason_to_string(enum ofp_port_reason reason)
return "modify";
default:
-sprintf(s, "%d", (int) reason);
-return s;
+snprintf(reasonbuf, bufsize, "%d", (int) reason);
+return reasonbuf;
}
}
@@ -1679,7 +1684,12 @@ ofp_print_nxt_set_async_config(struct ds *string,
ds_put_cstr(string, " PORT_STATUS:");
for (j = 0; j < 32; j++) {
if (nac->port_status_mask[i] & htonl(1u << j)) {
-ds_put_format(string, " %s", ofp_port_reason_to_string(j));
+char reasonbuf[OFP_PORT_REASON_BUFSIZE];
+const char *reason;
+
+reason = ofp_port_reason_to_string(j, reasonbuf,
+ sizeof reasonbuf);
+ds_put_format(string, " %s", reason);
}
}
if (!nac->port_status_mask[i]) {
@@ -1690,8 +1700,12 @@ ofp_print_nxt_set_async_config(struct ds *string,
ds_put_cstr(string, "FLOW_REMOVED:");
for (j = 0; j < 32; j++) {
if (nac->flow_removed_mask[i] & htonl(1u << j)) {
-ds_put_format(string, " %s",
- ofp_flow_removed_reason_to_string(j));
+char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE];
+const char *reason;
+
+reason = ofp_flow_removed_reason_to_string(j, reasonbuf,
+ sizeof reasonbuf);
+ds_put_format(string, " %s", reason);
}
}
if (!nac->flow_removed_mask[i]) {
@@ -1782,6 +1796,7 @@ ofp_print_nxst_flow_monitor_reply(struct ds *string,
ofpbuf_use_const(&b, oh, ntohs(oh->length));
ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
for (;;) {
+char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE];
struct ofputil_flow_update update;
struct match match;
int retval;
@@ -1804,7 +1819,9 @@ ofp_print_nxst_flow_monitor_reply(struct ds *string,
case NXFME_DELETED:
ds_put_format(string, "DELETED reason=%s",
- ofp_flow_removed_reason_to_string(update.reason));
+ ofp_flow_removed_reason_to_string(update.reason,
+
[ovs-dev] [backer v2 1/3] mac-learning: Simplify mac_learning_changed().
With this patch, the mac_learning module takes responsibility for
remembering tags which need revalidation after a
mac_learning_changed() call. This removes one of
ofproto-dpif-xlate's dpif_backer uses.
Signed-off-by: Ethan Jackson
---
lib/mac-learning.c | 15 +--
lib/mac-learning.h |3 ++-
ofproto/ofproto-dpif-xlate.c |3 +--
3 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/lib/mac-learning.c b/lib/mac-learning.c
index 052ac48..d66f331 100644
--- a/lib/mac-learning.c
+++ b/lib/mac-learning.c
@@ -123,6 +123,7 @@ mac_learning_create(unsigned int idle_time)
ml->flood_vlans = NULL;
ml->idle_time = normalize_idle_time(idle_time);
ml->max_entries = MAC_DEFAULT_MAX;
+tag_set_init(&ml->tags);
return ml;
}
@@ -245,22 +246,20 @@ mac_learning_insert(struct mac_learning *ml,
return e;
}
-/* Changes 'e''s tag to a new, randomly selected one, and returns the tag that
- * would have been previously used for this entry's MAC and VLAN (either before
- * 'e' was inserted, if it is new, or otherwise before its port was updated.)
+/* Changes 'e''s tag to a new, randomly selected one.
*
* The client should call this function after obtaining a MAC learning entry
* from mac_learning_insert(), if the entry is either new or if its learned
* port has changed. */
-tag_type
+void
mac_learning_changed(struct mac_learning *ml, struct mac_entry *e)
{
-tag_type old_tag = e->tag;
+tag_type tag = e->tag ? e->tag : make_unknown_mac_tag(ml, e->mac, e->vlan);
COVERAGE_INC(mac_learning_learned);
e->tag = tag_create_random();
-return old_tag ? old_tag : make_unknown_mac_tag(ml, e->mac, e->vlan);
+tag_set_add(&ml->tags, tag);
}
/* Looks up MAC 'dst' for VLAN 'vlan' in 'ml' and returns the associated MAC
@@ -322,6 +321,10 @@ void
mac_learning_run(struct mac_learning *ml, struct tag_set *set)
{
struct mac_entry *e;
+
+tag_set_union(set, &ml->tags);
+tag_set_init(&ml->tags);
+
while (get_lru(ml, &e)
&& (hmap_count(&ml->table) > ml->max_entries
|| time_now() >= e->expires)) {
diff --git a/lib/mac-learning.h b/lib/mac-learning.h
index 666b00f..1cbacfe 100644
--- a/lib/mac-learning.h
+++ b/lib/mac-learning.h
@@ -85,6 +85,7 @@ struct mac_learning {
unsigned long *flood_vlans; /* Bitmap of learning disabled VLANs. */
unsigned int idle_time; /* Max age before deleting an entry. */
size_t max_entries; /* Max number of learned MACs. */
+struct tag_set tags;/* Tags which have changed. */
};
/* Basics. */
@@ -107,7 +108,7 @@ bool mac_learning_may_learn(const struct mac_learning *,
struct mac_entry *mac_learning_insert(struct mac_learning *,
const uint8_t src[ETH_ADDR_LEN],
uint16_t vlan);
-tag_type mac_learning_changed(struct mac_learning *, struct mac_entry *);
+void mac_learning_changed(struct mac_learning *, struct mac_entry *);
/* Lookup. */
struct mac_entry *mac_learning_lookup(const struct mac_learning *,
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index e46a854..eb41993 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -466,8 +466,7 @@ update_learning_table(struct ofproto_dpif *ofproto,
in_bundle->name, vlan);
mac->port.p = in_bundle;
-tag_set_add(&ofproto->backer->revalidate_set,
-mac_learning_changed(ofproto->ml, mac));
+mac_learning_changed(ofproto->ml, mac);
}
}
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [backer v2 3/3] ofproto-dpif: Hide struct dpif_backer.
This patch removes the last reference to dpif_backer from
ofproto-dpif-xlate, and moves it's definition into ofproto-dpif.c.
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif-xlate.c |7 ++--
ofproto/ofproto-dpif.c | 74 ++
ofproto/ofproto-dpif.h | 70 ++-
3 files changed, 79 insertions(+), 72 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 7d3a7c4..86c5348 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -1279,8 +1279,7 @@ xlate_enqueue_action(struct xlate_ctx *ctx,
int error;
/* Translate queue to priority. */
-error = dpif_queue_to_priority(ctx->ofproto->backer->dpif,
- queue_id, &priority);
+error = ofproto_dpif_queue_to_priority(ctx->ofproto, queue_id, &priority);
if (error) {
/* Fall back to ordinary output action. */
xlate_output_action(ctx, enqueue->port, 0, false);
@@ -1313,8 +1312,8 @@ xlate_set_queue_action(struct xlate_ctx *ctx, uint32_t
queue_id)
{
uint32_t skb_priority;
-if (!dpif_queue_to_priority(ctx->ofproto->backer->dpif,
-queue_id, &skb_priority)) {
+if (!ofproto_dpif_queue_to_priority(ctx->ofproto, queue_id,
+&skb_priority)) {
ctx->xin->flow.skb_priority = skb_priority;
} else {
/* Couldn't translate queue to a priority. Nothing to do. A warning
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index a51b60d..a221734 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -298,6 +298,21 @@ struct dpif_completion {
struct ofoperation *op;
};
+/* Reasons that we might need to revalidate every facet, and corresponding
+ * coverage counters.
+ *
+ * A value of 0 means that there is no need to revalidate.
+ *
+ * It would be nice to have some cleaner way to integrate with coverage
+ * counters, but with only a few reasons I guess this is good enough for
+ * now. */
+enum revalidate_reason {
+REV_RECONFIGURE = 1, /* Switch configuration changed. */
+REV_STP, /* Spanning tree protocol port status change. */
+REV_PORT_TOGGLED, /* Port enabled or disabled by CFM, LACP, ...*/
+REV_FLOW_TABLE,/* Flow table changed. */
+REV_INCONSISTENCY /* Facet self-check failed. */
+};
COVERAGE_DEFINE(rev_reconfigure);
COVERAGE_DEFINE(rev_stp);
COVERAGE_DEFINE(rev_port_toggled);
@@ -313,6 +328,58 @@ struct drop_key {
size_t key_len;
};
+struct avg_subfacet_rates {
+double add_rate; /* Moving average of new flows created per minute. */
+double del_rate; /* Moving average of flows deleted per minute. */
+};
+
+/* All datapaths of a given type share a single dpif backer instance. */
+struct dpif_backer {
+char *type;
+int refcount;
+struct dpif *dpif;
+struct timer next_expiration;
+struct hmap odp_to_ofport_map; /* ODP port to ofport mapping. */
+
+struct simap tnl_backers; /* Set of dpif ports backing tunnels. */
+
+/* Facet revalidation flags applying to facets which use this backer. */
+enum revalidate_reason need_revalidate; /* Revalidate every facet. */
+struct tag_set revalidate_set; /* Revalidate only matching facets. */
+
+struct hmap drop_keys; /* Set of dropped odp keys. */
+bool recv_set_enable; /* Enables or disables receiving packets. */
+
+struct hmap subfacets;
+struct governor *governor;
+
+/* Subfacet statistics.
+ *
+ * These keep track of the total number of subfacets added and deleted and
+ * flow life span. They are useful for computing the flow rates stats
+ * exposed via "ovs-appctl dpif/show". The goal is to learn about
+ * traffic patterns in ways that we can use later to improve Open vSwitch
+ * performance in new situations. */
+long long int created; /* Time when it is created. */
+unsigned max_n_subfacet; /* Maximum number of flows */
+unsigned avg_n_subfacet; /* Average number of flows. */
+long long int avg_subfacet_life; /* Average life span of subfacets. */
+
+/* The average number of subfacets... */
+struct avg_subfacet_rates hourly; /* ...over the last hour. */
+struct avg_subfacet_rates daily;/* ...over the last day. */
+struct avg_subfacet_rates lifetime; /* ...over the switch lifetime. */
+long long int last_minute; /* Last time 'hourly' was updated. */
+
+/* Number of subfacets added or deleted since 'last_minute'. */
+unsigned subfacet_add_count;
+unsigned subfacet_del_count;
+
+/* Number of subfacets added or deleted from 'created' to 'last_minute.' */
+unsigned long long int total_subfacet_add_count;
+unsigned long long int total_subfacet_del_count;
+};
+
/* All existing ofproto_backer instances, i
Re: [ovs-dev] [threads 13/17] ofp-print: Avoid returning static data.
On Jun 12, 2013, at 2:44 PM, Ben Pfaff wrote: > I added: > /* Returns a string form of 'reason'. The return value is either a statically > * allocated constant string or the 'bufsize'-byte buffer 'reasonbuf'. > * 'bufsize' should be at least OFP_FLOW_REMOVED_REASON_BUFSIZE. */ > > How's that? Looks good. I assume you'll add a similar comment to the port function? --Justin ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads v2] ofp-print: Avoid returning static data.
Looks good, but I think similar changes should be made to
ofp_port_reason_to_string() and OFP_PORT_REASON_BUFSIZE.
--Justin
On Jun 12, 2013, at 2:45 PM, Ben Pfaff wrote:
> Returning a static data buffer makes code more brittle and definitely
> not thread-safe, so this commit switches to using a caller-provided
> buffer instead.
>
> Signed-off-by: Ben Pfaff
> ---
> lib/ofp-print.c | 47 ---
> 1 files changed, 32 insertions(+), 15 deletions(-)
>
> diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> index 9471385..092a4bc 100644
> --- a/lib/ofp-print.c
> +++ b/lib/ofp-print.c
> @@ -859,11 +859,14 @@ ofp_print_duration(struct ds *string, unsigned int sec,
> unsigned int nsec)
> ds_put_char(string, 's');
> }
>
> +/* Returns a string form of 'reason'. The return value is either a
> statically
> + * allocated constant string or the 'bufsize'-byte buffer 'reasonbuf'.
> + * 'bufsize' should be at least OFP_FLOW_REMOVED_REASON_BUFSIZE. */
> +#define OFP_FLOW_REMOVED_REASON_BUFSIZE (INT_STRLEN(int) + 1)
> static const char *
> -ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason)
> +ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason,
> + char *reasonbuf, size_t bufsize)
> {
> -static char s[32];
> -
> switch (reason) {
> case OFPRR_IDLE_TIMEOUT:
> return "idle";
> @@ -876,14 +879,15 @@ ofp_flow_removed_reason_to_string(enum
> ofp_flow_removed_reason reason)
> case OFPRR_EVICTION:
> return "eviction";
> default:
> -sprintf(s, "%d", (int) reason);
> -return s;
> +snprintf(reasonbuf, bufsize, "%d", (int) reason);
> +return reasonbuf;
> }
> }
>
> static void
> ofp_print_flow_removed(struct ds *string, const struct ofp_header *oh)
> {
> +char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE];
> struct ofputil_flow_removed fr;
> enum ofperr error;
>
> @@ -897,7 +901,8 @@ ofp_print_flow_removed(struct ds *string, const struct
> ofp_header *oh)
> match_format(&fr.match, string, fr.priority);
>
> ds_put_format(string, " reason=%s",
> - ofp_flow_removed_reason_to_string(fr.reason));
> + ofp_flow_removed_reason_to_string(fr.reason, reasonbuf,
> +sizeof reasonbuf));
>
> if (fr.table_id != 255) {
> ds_put_format(string, " table_id=%"PRIu8, fr.table_id);
> @@ -1628,11 +1633,11 @@ ofp_print_nxt_set_packet_in_format(struct ds *string,
> }
> }
>
> +enum { OFP_PORT_REASON_BUFSIZE = INT_STRLEN(int) + 1 };
> static const char *
> -ofp_port_reason_to_string(enum ofp_port_reason reason)
> +ofp_port_reason_to_string(enum ofp_port_reason reason,
> + char *reasonbuf, size_t bufsize)
> {
> -static char s[32];
> -
> switch (reason) {
> case OFPPR_ADD:
> return "add";
> @@ -1644,8 +1649,8 @@ ofp_port_reason_to_string(enum ofp_port_reason reason)
> return "modify";
>
> default:
> -sprintf(s, "%d", (int) reason);
> -return s;
> +snprintf(reasonbuf, bufsize, "%d", (int) reason);
> +return reasonbuf;
> }
> }
>
> @@ -1679,7 +1684,12 @@ ofp_print_nxt_set_async_config(struct ds *string,
> ds_put_cstr(string, " PORT_STATUS:");
> for (j = 0; j < 32; j++) {
> if (nac->port_status_mask[i] & htonl(1u << j)) {
> -ds_put_format(string, " %s", ofp_port_reason_to_string(j));
> +char reasonbuf[OFP_PORT_REASON_BUFSIZE];
> +const char *reason;
> +
> +reason = ofp_port_reason_to_string(j, reasonbuf,
> + sizeof reasonbuf);
> +ds_put_format(string, " %s", reason);
> }
> }
> if (!nac->port_status_mask[i]) {
> @@ -1690,8 +1700,12 @@ ofp_print_nxt_set_async_config(struct ds *string,
> ds_put_cstr(string, "FLOW_REMOVED:");
> for (j = 0; j < 32; j++) {
> if (nac->flow_removed_mask[i] & htonl(1u << j)) {
> -ds_put_format(string, " %s",
> - ofp_flow_removed_reason_to_string(j));
> +char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE];
> +const char *reason;
> +
> +reason = ofp_flow_removed_reason_to_string(j, reasonbuf,
> + sizeof reasonbuf);
> +ds_put_format(string, " %s", reason);
> }
> }
> if (!nac->flow_removed_mask[i]) {
> @@ -1782,6 +1796,7 @@ ofp_print_nxst_flow_monitor_reply(struct ds *string,
> ofpbuf_use_const(&b, oh, ntohs(oh->length));
> ofpbuf_use_stub(&ofpacts, ofpacts_stub, sizeof ofpacts_stub);
> for (;;) {
> +char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE];
> struct ofputil_flow_update update;
> struct matc
Re: [ovs-dev] [threads 13/17] ofp-print: Avoid returning static data.
On Wed, Jun 12, 2013 at 02:46:57PM -0700, Justin Pettit wrote: > On Jun 12, 2013, at 2:44 PM, Ben Pfaff wrote: > > > I added: > > /* Returns a string form of 'reason'. The return value is either a > > statically > > * allocated constant string or the 'bufsize'-byte buffer 'reasonbuf'. > > * 'bufsize' should be at least OFP_FLOW_REMOVED_REASON_BUFSIZE. */ > > > > How's that? > > Looks good. I assume you'll add a similar comment to the port function? Oops, thanks for pointing that out. I'll fix that one the same way and push. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [threads v2] ofp-print: Avoid returning static data.
Done, and pushed.
On Wed, Jun 12, 2013 at 02:48:41PM -0700, Justin Pettit wrote:
> Looks good, but I think similar changes should be made to
> ofp_port_reason_to_string() and OFP_PORT_REASON_BUFSIZE.
>
> --Justin
>
>
> On Jun 12, 2013, at 2:45 PM, Ben Pfaff wrote:
>
> > Returning a static data buffer makes code more brittle and definitely
> > not thread-safe, so this commit switches to using a caller-provided
> > buffer instead.
> >
> > Signed-off-by: Ben Pfaff
> > ---
> > lib/ofp-print.c | 47 ---
> > 1 files changed, 32 insertions(+), 15 deletions(-)
> >
> > diff --git a/lib/ofp-print.c b/lib/ofp-print.c
> > index 9471385..092a4bc 100644
> > --- a/lib/ofp-print.c
> > +++ b/lib/ofp-print.c
> > @@ -859,11 +859,14 @@ ofp_print_duration(struct ds *string, unsigned int
> > sec, unsigned int nsec)
> > ds_put_char(string, 's');
> > }
> >
> > +/* Returns a string form of 'reason'. The return value is either a
> > statically
> > + * allocated constant string or the 'bufsize'-byte buffer 'reasonbuf'.
> > + * 'bufsize' should be at least OFP_FLOW_REMOVED_REASON_BUFSIZE. */
> > +#define OFP_FLOW_REMOVED_REASON_BUFSIZE (INT_STRLEN(int) + 1)
> > static const char *
> > -ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason)
> > +ofp_flow_removed_reason_to_string(enum ofp_flow_removed_reason reason,
> > + char *reasonbuf, size_t bufsize)
> > {
> > -static char s[32];
> > -
> > switch (reason) {
> > case OFPRR_IDLE_TIMEOUT:
> > return "idle";
> > @@ -876,14 +879,15 @@ ofp_flow_removed_reason_to_string(enum
> > ofp_flow_removed_reason reason)
> > case OFPRR_EVICTION:
> > return "eviction";
> > default:
> > -sprintf(s, "%d", (int) reason);
> > -return s;
> > +snprintf(reasonbuf, bufsize, "%d", (int) reason);
> > +return reasonbuf;
> > }
> > }
> >
> > static void
> > ofp_print_flow_removed(struct ds *string, const struct ofp_header *oh)
> > {
> > +char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE];
> > struct ofputil_flow_removed fr;
> > enum ofperr error;
> >
> > @@ -897,7 +901,8 @@ ofp_print_flow_removed(struct ds *string, const struct
> > ofp_header *oh)
> > match_format(&fr.match, string, fr.priority);
> >
> > ds_put_format(string, " reason=%s",
> > - ofp_flow_removed_reason_to_string(fr.reason));
> > + ofp_flow_removed_reason_to_string(fr.reason, reasonbuf,
> > +sizeof reasonbuf));
> >
> > if (fr.table_id != 255) {
> > ds_put_format(string, " table_id=%"PRIu8, fr.table_id);
> > @@ -1628,11 +1633,11 @@ ofp_print_nxt_set_packet_in_format(struct ds
> > *string,
> > }
> > }
> >
> > +enum { OFP_PORT_REASON_BUFSIZE = INT_STRLEN(int) + 1 };
> > static const char *
> > -ofp_port_reason_to_string(enum ofp_port_reason reason)
> > +ofp_port_reason_to_string(enum ofp_port_reason reason,
> > + char *reasonbuf, size_t bufsize)
> > {
> > -static char s[32];
> > -
> > switch (reason) {
> > case OFPPR_ADD:
> > return "add";
> > @@ -1644,8 +1649,8 @@ ofp_port_reason_to_string(enum ofp_port_reason reason)
> > return "modify";
> >
> > default:
> > -sprintf(s, "%d", (int) reason);
> > -return s;
> > +snprintf(reasonbuf, bufsize, "%d", (int) reason);
> > +return reasonbuf;
> > }
> > }
> >
> > @@ -1679,7 +1684,12 @@ ofp_print_nxt_set_async_config(struct ds *string,
> > ds_put_cstr(string, " PORT_STATUS:");
> > for (j = 0; j < 32; j++) {
> > if (nac->port_status_mask[i] & htonl(1u << j)) {
> > -ds_put_format(string, " %s", ofp_port_reason_to_string(j));
> > +char reasonbuf[OFP_PORT_REASON_BUFSIZE];
> > +const char *reason;
> > +
> > +reason = ofp_port_reason_to_string(j, reasonbuf,
> > + sizeof reasonbuf);
> > +ds_put_format(string, " %s", reason);
> > }
> > }
> > if (!nac->port_status_mask[i]) {
> > @@ -1690,8 +1700,12 @@ ofp_print_nxt_set_async_config(struct ds *string,
> > ds_put_cstr(string, "FLOW_REMOVED:");
> > for (j = 0; j < 32; j++) {
> > if (nac->flow_removed_mask[i] & htonl(1u << j)) {
> > -ds_put_format(string, " %s",
> > - ofp_flow_removed_reason_to_string(j));
> > +char reasonbuf[OFP_FLOW_REMOVED_REASON_BUFSIZE];
> > +const char *reason;
> > +
> > +reason = ofp_flow_removed_reason_to_string(j, reasonbuf,
> > + sizeof
> > reasonbuf);
> > +ds_put_format(string, " %s", reason);
> > }
> > }
> > if (!nac->flow_removed_mask[i]
Re: [ovs-dev] [backer v2 1/3] mac-learning: Simplify mac_learning_changed().
On Wed, Jun 12, 2013 at 02:45:47PM -0700, Ethan Jackson wrote: > With this patch, the mac_learning module takes responsibility for > remembering tags which need revalidation after a > mac_learning_changed() call. This removes one of > ofproto-dpif-xlate's dpif_backer uses. > > Signed-off-by: Ethan Jackson Before, mac_learning_run() was happy with a NULL 'set' argument, and lswitch_run() in lib/learning-switch.c takes advantage of that. Now, it potentially segfaults. I would add "if (set)" around the tag_set_union() call (but not the tag_set_init() call). If mac_learning_run() is called at the wrong time, then the mac_learning object could effectively be hoarding up tags until something else wakes up poll_block(). So I would argue that it is safer if we add something like "if (!tag_set_is_empty()) poll_immediate_wake();" to mac_learning_wait() to stave off future potential for trouble. It would be nice for mac_learning_changed() to mention that it affects the next mac_learning_run(). ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [backer v2 2/3] ofproto-dpif: Verify backer in ofport_get_peer().
On Wed, Jun 12, 2013 at 02:45:48PM -0700, Ethan Jackson wrote: > This marginally simplifies the code, and removes a reference to > dpif_backer from ofproto-dpif-xlate. > > Signed-off-by: Ethan Jackson Looks good, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [backer v2 3/3] ofproto-dpif: Hide struct dpif_backer.
On Wed, Jun 12, 2013 at 02:45:49PM -0700, Ethan Jackson wrote: > This patch removes the last reference to dpif_backer from > ofproto-dpif-xlate, and moves it's definition into ofproto-dpif.c. "its" > Signed-off-by: Ethan Jackson Looks good, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 00/11] ofproto-dpif: Cleanup, reduce copying struct flow.
On Fri, May 31, 2013 at 02:35:10PM +0300, Jarno Rajahalme wrote: > The code in ofproto-dpif.c became harder on the eye after the reorg of > struct xlate. This series tries cleaning that up a bit, and while at it, > identifying and reducing unnecessary copying of struct flow. The main > enabler for this is to 1) pass struct flow around via a pointer, and 2) > initializing a new one only if necessary. I apologize for taking so long to review this series. It looks worthwhile. Are you willing to rebase the unapplied patches against current master? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv3 1/5] lib: Add CRC32C Implementation
On Wed, Jun 12, 2013 at 02:35:54PM +0900, Joe Stringer wrote: > This implementation was derived from FreeBSD: > http://code.google.com/p/freebsd-head/source/browse/sys/libkern/crc32.c > > Reviewed-by: Simon Horman > Signed-off-by: Joe Stringer Looks good, but is there any reason not to fold patch 5 into this one? ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv3 2/5] ofproto-dpif: Add SCTP support
On Wed, Jun 12, 2013 at 02:35:55PM +0900, Joe Stringer wrote: > Reviewed-by: Simon Horman > Signed-off-by: Joe Stringer I'm happy with the userspace portions of this. It changes kernel headers so it needs approval from Jesse. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv3 4/5] ofp-util: Add SCTP support
On Wed, Jun 12, 2013 at 02:35:57PM +0900, Joe Stringer wrote: > Reviewed-by: Simon Horman > Signed-off-by: Joe Stringer This looks good, once Jesse is happy with patches 2 and 3. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [backer v2 1/3] mac-learning: Simplify mac_learning_changed().
Thanks for the review. Here's an incremental.
---
lib/mac-learning.c | 12 +---
1 file changed, 9 insertions(+), 3 deletions(-)
diff --git a/lib/mac-learning.c b/lib/mac-learning.c
index d66f331..f9b3171 100644
--- a/lib/mac-learning.c
+++ b/lib/mac-learning.c
@@ -246,7 +246,10 @@ mac_learning_insert(struct mac_learning *ml,
return e;
}
-/* Changes 'e''s tag to a new, randomly selected one.
+/* Changes 'e''s tag to a new, randomly selected one. Causes
+ * mac_learning_run() to flag for revalidation the tag that would have been
+ * previously used for this entry's MAC and VLAN (either before 'e' was
+ * inserted, if it is new, or otherwise before its port was updated.)
*
* The client should call this function after obtaining a MAC learning entry
* from mac_learning_insert(), if the entry is either new or if its learned
@@ -322,7 +325,9 @@ mac_learning_run(struct mac_learning *ml, struct tag_set
*set)
{
struct mac_entry *e;
-tag_set_union(set, &ml->tags);
+if (set) {
+tag_set_union(set, &ml->tags);
+}
tag_set_init(&ml->tags);
while (get_lru(ml, &e)
@@ -339,7 +344,8 @@ mac_learning_run(struct mac_learning *ml, struct tag_set
*set)
void
mac_learning_wait(struct mac_learning *ml)
{
-if (hmap_count(&ml->table) > ml->max_entries) {
+if (hmap_count(&ml->table) > ml->max_entries
+|| !tag_set_is_empty(&ml->tags)) {
poll_immediate_wake();
} else if (!list_is_empty(&ml->lrus)) {
struct mac_entry *e = mac_entry_from_lru_node(ml->lrus.next);
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [backer v2 1/3] mac-learning: Simplify mac_learning_changed().
On Wed, Jun 12, 2013 at 03:27:13PM -0700, Ethan Jackson wrote: > Thanks for the review. Here's an incremental. That's what I had in mind, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [backer v2 1/3] mac-learning: Simplify mac_learning_changed().
Thanks, I'll merge this series shortly. Ethan On Wed, Jun 12, 2013 at 3:27 PM, Ben Pfaff wrote: > On Wed, Jun 12, 2013 at 03:27:13PM -0700, Ethan Jackson wrote: >> Thanks for the review. Here's an incremental. > > That's what I had in mind, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: Hide struct priority_to_dscp.
Signed-off-by: Ethan Jackson
---
ofproto/ofproto-dpif-xlate.c |7 +++
ofproto/ofproto-dpif.c | 21 -
ofproto/ofproto-dpif.h | 14 ++
3 files changed, 25 insertions(+), 17 deletions(-)
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 86c5348..a796f98 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -795,8 +795,8 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
ovs_be16 flow_vlan_tci;
uint32_t flow_skb_mark;
uint8_t flow_nw_tos;
-struct priority_to_dscp *pdscp;
uint32_t out_port, odp_port;
+uint8_t dscp;
/* If 'struct flow' gets additional metadata, we'll need to zero it out
* before traversing a patch port. */
@@ -864,10 +864,9 @@ compose_output_action__(struct xlate_ctx *ctx, uint16_t
ofp_port,
flow_skb_mark = flow->skb_mark;
flow_nw_tos = flow->nw_tos;
-pdscp = get_priority(ofport, flow->skb_priority);
-if (pdscp) {
+if (ofproto_dpif_dscp_from_priority(ofport, flow->skb_priority, &dscp)) {
flow->nw_tos &= ~IP_DSCP_MASK;
-flow->nw_tos |= pdscp->dscp;
+flow->nw_tos |= dscp;
}
if (ofport->tnl_port) {
diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index a221734..b917dc7 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -258,6 +258,16 @@ static void push_all_stats(void);
static bool facet_is_controller_flow(struct facet *);
+/* Node in 'ofport_dpif''s 'priorities' map. Used to maintain a map from
+ * 'priority' (the datapath's term for QoS queue) to the dscp bits which all
+ * traffic egressing the 'ofport' with that priority should be marked with. */
+struct priority_to_dscp {
+struct hmap_node hmap_node; /* Node in 'ofport_dpif''s 'priorities' map. */
+uint32_t priority; /* Priority of this queue (see struct flow). */
+
+uint8_t dscp; /* DSCP bits to mark outgoing traffic with. */
+};
+
/* Linux VLAN device support (e.g. "eth0.10" for VLAN 10.)
*
* This is deprecated. It is only for compatibility with broken device drivers
@@ -1955,7 +1965,7 @@ ofproto_dpif_queue_to_priority(const struct ofproto_dpif
*ofproto,
return dpif_queue_to_priority(ofproto->backer->dpif, queue_id, priority);
}
-struct priority_to_dscp *
+static struct priority_to_dscp *
get_priority(const struct ofport_dpif *ofport, uint32_t priority)
{
struct priority_to_dscp *pdscp;
@@ -1970,6 +1980,15 @@ get_priority(const struct ofport_dpif *ofport, uint32_t
priority)
return NULL;
}
+bool
+ofproto_dpif_dscp_from_priority(const struct ofport_dpif *ofport,
+uint32_t priority, uint8_t *dscp)
+{
+struct priority_to_dscp *pdscp = get_priority(ofport, priority);
+*dscp = pdscp ? pdscp->dscp : 0;
+return pdscp != NULL;
+}
+
static void
ofport_clear_priorities(struct ofport_dpif *ofport)
{
diff --git a/ofproto/ofproto-dpif.h b/ofproto/ofproto-dpif.h
index 313141b..52e9639 100644
--- a/ofproto/ofproto-dpif.h
+++ b/ofproto/ofproto-dpif.h
@@ -200,16 +200,6 @@ struct ofmirror {
int64_t byte_count; /* Number of bytes sent. */
};
-/* Node in 'ofport_dpif''s 'priorities' map. Used to maintain a map from
- * 'priority' (the datapath's term for QoS queue) to the dscp bits which all
- * traffic egressing the 'ofport' with that priority should be marked with. */
-struct priority_to_dscp {
-struct hmap_node hmap_node; /* Node in 'ofport_dpif''s 'priorities' map. */
-uint32_t priority; /* Priority of this queue (see struct flow). */
-
-uint8_t dscp; /* DSCP bits to mark outgoing traffic with. */
-};
-
static inline struct rule_dpif *rule_dpif_cast(const struct rule *rule)
{
return rule ? CONTAINER_OF(rule, struct rule_dpif, up) : NULL;
@@ -276,8 +266,8 @@ uint16_t vsp_realdev_to_vlandev(const struct ofproto_dpif *,
uint16_t realdev_ofp_port,
ovs_be16 vlan_tci);
-struct priority_to_dscp *get_priority(const struct ofport_dpif *,
- uint32_t priority);
+bool ofproto_dpif_dscp_from_priority(const struct ofport_dpif *,
+ uint32_t priority, uint8_t *dscp);
int ofproto_dpif_queue_to_priority(const struct ofproto_dpif *,
uint32_t queue_id, uint32_t *priority);
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: Hide struct priority_to_dscp.
On Wed, Jun 12, 2013 at 03:33:45PM -0700, Ethan Jackson wrote: > Signed-off-by: Ethan Jackson Looks good, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH net-next 0/4] openvswitch tunneling preparation
Following patches are required but not directly related to ovs tunneling. So I am sending them as separate series. These cleanups are already there in out of tree ovs module. Pravin B Shelar (4): openvswitch: Unify vport error stats handling. openvswitch: Simplify interface ovs_flow_metadata_from_nlattrs() openvswitch: Fix struct comment. openvsiwtch: make skb->csum consistent with rest of networking stack. include/uapi/linux/openvswitch.h |1 - net/openvswitch/actions.c|4 net/openvswitch/datapath.c |6 ++ net/openvswitch/flow.c | 26 ++ net/openvswitch/flow.h |4 ++-- net/openvswitch/vport-internal_dev.c |1 + net/openvswitch/vport-netdev.c |3 ++- net/openvswitch/vport.c |9 +++-- net/openvswitch/vport.h | 10 +- 9 files changed, 41 insertions(+), 23 deletions(-) ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH net-next 1/4] openvswitch: Unify vport error stats handling.
Following patch changes vport->send return type so that vport
layer can do error accounting.
Signed-off-by: Pravin B Shelar
---
net/openvswitch/vport-netdev.c |1 -
net/openvswitch/vport.c|9 +++--
net/openvswitch/vport.h|3 ++-
3 files changed, 9 insertions(+), 4 deletions(-)
diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
index 4f01c6d..c3ee7de 100644
--- a/net/openvswitch/vport-netdev.c
+++ b/net/openvswitch/vport-netdev.c
@@ -181,7 +181,6 @@ static int netdev_send(struct vport *vport, struct sk_buff
*skb)
error:
kfree_skb(skb);
- ovs_vport_record_error(vport, VPORT_E_TX_DROPPED);
return 0;
}
diff --git a/net/openvswitch/vport.c b/net/openvswitch/vport.c
index 7206231..7f20f6d 100644
--- a/net/openvswitch/vport.c
+++ b/net/openvswitch/vport.c
@@ -351,7 +351,7 @@ int ovs_vport_send(struct vport *vport, struct sk_buff *skb)
{
int sent = vport->ops->send(vport, skb);
- if (likely(sent)) {
+ if (likely(sent > 0)) {
struct pcpu_tstats *stats;
stats = this_cpu_ptr(vport->percpu_stats);
@@ -360,7 +360,12 @@ int ovs_vport_send(struct vport *vport, struct sk_buff
*skb)
stats->tx_packets++;
stats->tx_bytes += sent;
u64_stats_update_end(&stats->syncp);
- }
+ } else if (sent < 0) {
+ ovs_vport_record_error(vport, VPORT_E_TX_ERROR);
+ kfree_skb(skb);
+ } else
+ ovs_vport_record_error(vport, VPORT_E_TX_DROPPED);
+
return sent;
}
diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
index 68a377b..75d3ccb 100644
--- a/net/openvswitch/vport.h
+++ b/net/openvswitch/vport.h
@@ -125,7 +125,8 @@ struct vport_parms {
* @get_name: Get the device's name.
* @get_config: Get the device's configuration.
* May be null if the device does not have an ifindex.
- * @send: Send a packet on the device. Returns the length of the packet sent.
+ * @send: Send a packet on the device. Returns the length of the packet sent,
+ * zero for dropped packets or negative for error.
*/
struct vport_ops {
enum ovs_vport_type type;
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH net-next 2/4] openvswitch: Simplify interface ovs_flow_metadata_from_nlattrs()
This is not functional change, this is just code cleanup.
Signed-off-by: Pravin B Shelar
---
net/openvswitch/datapath.c |5 +
net/openvswitch/flow.c | 23 +++
net/openvswitch/flow.h |4 ++--
3 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index d12d6b8..39cd821 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -739,10 +739,7 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb,
struct genl_info *info)
if (err)
goto err_flow_free;
- err = ovs_flow_metadata_from_nlattrs(&flow->key.phy.priority,
-&flow->key.phy.skb_mark,
-&flow->key.phy.in_port,
-a[OVS_PACKET_ATTR_KEY]);
+ err = ovs_flow_metadata_from_nlattrs(flow, key_len,
a[OVS_PACKET_ATTR_KEY]);
if (err)
goto err_flow_free;
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index b15321a..150f181 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -1122,10 +1122,9 @@ int ovs_flow_from_nlattrs(struct sw_flow_key *swkey, int
*key_lenp,
/**
* ovs_flow_metadata_from_nlattrs - parses Netlink attributes into a flow key.
- * @priority: receives the skb priority
- * @mark: receives the skb mark
- * @in_port: receives the extracted input port.
- * @key: Netlink attribute holding nested %OVS_KEY_ATTR_* Netlink attribute
+ * @flow: Receives extracted in_port, priority, tun_key and skb_mark.
+ * @key_len: Length of key in @flow. Used for calculating flow hash.
+ * @attr: Netlink attribute holding nested %OVS_KEY_ATTR_* Netlink attribute
* sequence.
*
* This parses a series of Netlink attributes that form a flow key, which must
@@ -1133,15 +1132,15 @@ int ovs_flow_from_nlattrs(struct sw_flow_key *swkey,
int *key_lenp,
* get the metadata, that is, the parts of the flow key that cannot be
* extracted from the packet itself.
*/
-int ovs_flow_metadata_from_nlattrs(u32 *priority, u32 *mark, u16 *in_port,
- const struct nlattr *attr)
+int ovs_flow_metadata_from_nlattrs(struct sw_flow *flow, int key_len,
+ const struct nlattr *attr)
{
const struct nlattr *nla;
int rem;
- *in_port = DP_MAX_PORTS;
- *priority = 0;
- *mark = 0;
+ flow->key.phy.in_port = DP_MAX_PORTS;
+ flow->key.phy.priority = 0;
+ flow->key.phy.skb_mark = 0;
nla_for_each_nested(nla, attr, rem) {
int type = nla_type(nla);
@@ -1152,17 +1151,17 @@ int ovs_flow_metadata_from_nlattrs(u32 *priority, u32
*mark, u16 *in_port,
switch (type) {
case OVS_KEY_ATTR_PRIORITY:
- *priority = nla_get_u32(nla);
+ flow->key.phy.priority = nla_get_u32(nla);
break;
case OVS_KEY_ATTR_IN_PORT:
if (nla_get_u32(nla) >= DP_MAX_PORTS)
return -EINVAL;
- *in_port = nla_get_u32(nla);
+ flow->key.phy.in_port = nla_get_u32(nla);
break;
case OVS_KEY_ATTR_SKB_MARK:
- *mark = nla_get_u32(nla);
+ flow->key.phy.skb_mark = nla_get_u32(nla);
break;
}
}
diff --git a/net/openvswitch/flow.h b/net/openvswitch/flow.h
index 0875fde..1feff5d 100644
--- a/net/openvswitch/flow.h
+++ b/net/openvswitch/flow.h
@@ -141,8 +141,8 @@ u64 ovs_flow_used_time(unsigned long flow_jiffies);
int ovs_flow_to_nlattrs(const struct sw_flow_key *, struct sk_buff *);
int ovs_flow_from_nlattrs(struct sw_flow_key *swkey, int *key_lenp,
const struct nlattr *);
-int ovs_flow_metadata_from_nlattrs(u32 *priority, u32 *mark, u16 *in_port,
- const struct nlattr *);
+int ovs_flow_metadata_from_nlattrs(struct sw_flow *flow, int key_len,
+ const struct nlattr *attr);
#define MAX_ACTIONS_BUFSIZE(16 * 1024)
#define TBL_MIN_BUCKETS1024
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH net-next 4/4] openvswitch: make skb->csum consistent with rest of networking stack.
Following patch keeps skb->csum correct across ovs.
Signed-off-by: Pravin B Shelar
---
net/openvswitch/actions.c|4
net/openvswitch/datapath.c |1 +
net/openvswitch/flow.c |3 +++
net/openvswitch/vport-internal_dev.c |1 +
net/openvswitch/vport-netdev.c |2 ++
net/openvswitch/vport.h |7 +++
6 files changed, 18 insertions(+), 0 deletions(-)
diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
index 894b6cb..596d637 100644
--- a/net/openvswitch/actions.c
+++ b/net/openvswitch/actions.c
@@ -130,9 +130,13 @@ static int set_eth_addr(struct sk_buff *skb,
if (unlikely(err))
return err;
+ skb_postpull_rcsum(skb, eth_hdr(skb), ETH_ALEN * 2);
+
memcpy(eth_hdr(skb)->h_source, eth_key->eth_src, ETH_ALEN);
memcpy(eth_hdr(skb)->h_dest, eth_key->eth_dst, ETH_ALEN);
+ ovs_skb_postpush_rcsum(skb, eth_hdr(skb), ETH_ALEN * 2);
+
return 0;
}
diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
index 39cd821..e6ffb82 100644
--- a/net/openvswitch/datapath.c
+++ b/net/openvswitch/datapath.c
@@ -406,6 +406,7 @@ static int queue_userspace_packet(struct net *net, int
dp_ifindex,
nskb = __vlan_put_tag(nskb, nskb->vlan_proto,
vlan_tx_tag_get(nskb));
if (!nskb)
return -ENOMEM;
+ ovs_skb_postpush_rcsum(skb, skb->data + (2 * ETH_ALEN),
VLAN_HLEN);
nskb->vlan_tci = 0;
skb = nskb;
diff --git a/net/openvswitch/flow.c b/net/openvswitch/flow.c
index 150f181..cc39085 100644
--- a/net/openvswitch/flow.c
+++ b/net/openvswitch/flow.c
@@ -618,6 +618,9 @@ int ovs_flow_extract(struct sk_buff *skb, u16 in_port,
struct sw_flow_key *key,
memcpy(key->eth.dst, eth->h_dest, ETH_ALEN);
__skb_pull(skb, 2 * ETH_ALEN);
+ /* We are going to push all headers that we pull, so no need to
+* update skb->csum here.
+*/
if (vlan_tx_tag_present(skb))
key->eth.tci = htons(skb->vlan_tci);
diff --git a/net/openvswitch/vport-internal_dev.c
b/net/openvswitch/vport-internal_dev.c
index 84e0a03..e284c7e 100644
--- a/net/openvswitch/vport-internal_dev.c
+++ b/net/openvswitch/vport-internal_dev.c
@@ -221,6 +221,7 @@ static int internal_dev_recv(struct vport *vport, struct
sk_buff *skb)
skb->dev = netdev;
skb->pkt_type = PACKET_HOST;
skb->protocol = eth_type_trans(skb, netdev);
+ skb_postpull_rcsum(skb, eth_hdr(skb), ETH_HLEN);
netif_rx(skb);
diff --git a/net/openvswitch/vport-netdev.c b/net/openvswitch/vport-netdev.c
index c3ee7de..018aadf 100644
--- a/net/openvswitch/vport-netdev.c
+++ b/net/openvswitch/vport-netdev.c
@@ -49,6 +49,8 @@ static void netdev_port_receive(struct vport *vport, struct
sk_buff *skb)
return;
skb_push(skb, ETH_HLEN);
+ ovs_skb_postpush_rcsum(skb, skb->data, ETH_HLEN);
+
ovs_vport_receive(vport, skb);
return;
diff --git a/net/openvswitch/vport.h b/net/openvswitch/vport.h
index 75d3ccb..98f3341 100644
--- a/net/openvswitch/vport.h
+++ b/net/openvswitch/vport.h
@@ -195,4 +195,11 @@ void ovs_vport_record_error(struct vport *, enum
vport_err_type err_type);
extern const struct vport_ops ovs_netdev_vport_ops;
extern const struct vport_ops ovs_internal_vport_ops;
+static inline void ovs_skb_postpush_rcsum(struct sk_buff *skb,
+ const void *start, unsigned int len)
+{
+ if (skb->ip_summed == CHECKSUM_COMPLETE)
+ skb->csum = csum_add(skb->csum, csum_partial(start, len, 0));
+}
+
#endif /* vport.h */
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH net-next 3/4] openvswitch: Fix struct comment.
Signed-off-by: Pravin B Shelar
---
include/uapi/linux/openvswitch.h |1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index 405918d..424672d 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -192,7 +192,6 @@ enum ovs_vport_type {
* optional; if not specified a free port number is automatically selected.
* Whether %OVS_VPORT_ATTR_OPTIONS is required or optional depends on the type
* of vport.
- * and other attributes are ignored.
*
* For other requests, if %OVS_VPORT_ATTR_NAME is specified then it is used to
* look up the vport to operate on; otherwise dp_idx from the &struct
--
1.7.1
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V1 1/3] coverage: Reimplement the "ovs-appclt coverage/show" command
On Wed, Jun 12, 2013 at 11:38:41AM -0700, Alex Wang wrote: > This commit changes the "ovs-appclt coverage/show" command to show the > per-second, per-minute and per-hour rates of function invocation. More > importantly, this makes using coverage counter an easy way to monitor > the execution of specific functions. > > Signed-off-by: Alex Wang > --- > Possible Issue: > > The "coverage_run" runs every second. And the number of addition operations is > not very large. If it is still considered to be time-consuming, I'll adjust > further. > I have two high-level design questions. First is the cost, not of the periodic summations (I doubt that cost matters) but of forcing a wakeup every second. This probably doesn't matter in ovs-vswitchd, but other daemons that don't wake up so frequently also use the poll_loop library, and it would be a shame to drive up system load unnecessarily. So I would prefer, if possible, for the coverage code not to use a timer but instead to just do work in coverage_run() whenever the process happens to wake up. Second is accuracy. I believe that the current code considers any interval greater than or equal to a second as exactly a second. Because wakeups aren't precise, that tends to stretch out the definition of a second to somewhat longer. If you take my suggestion about cost, above, that makes the problem worse. So I think that it may be worthwhile to measure the length of the actual interval for a "second" and divide by its length. For example, if there were 100 events of some type over 1.5 seconds, we would count that as 100 / 1.5 == 66.67 events per second, and use that in the average. That might require using "double"s for the average counters, or we could use fixed-point arithmetic; I think either solution could be OK. I didn't do a detailed review yet because I'd like to hear your thoughts on these points first. I did notice that coverage_array_sum() is only used in coverage.c, so I would define it there (and not "inline"), not in coverage.h. Thanks, Ben. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V2 2/3] ofproto-dpif: Compute the subfacet add/del rate using coverage counters
On Wed, Jun 12, 2013 at 11:38:42AM -0700, Alex Wang wrote: > The subfacet rates (e.g. add rate, del rate) were computed by exponential > moving averaging function in ofproto-dpif.c. This patch replaces that > logic with coverage counters. And the rates can be checked by running > "ovs-appctl coverage/show" command. > > Signed-off-by: Alex Wang Once we get patch 1 sorted out, this makes lots of sense. Thank you. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH V3 3/3] ofproto-dpif.at: Add test for "ovs-appctl coverage/show"
On Wed, Jun 12, 2013 at 11:38:43AM -0700, Alex Wang wrote: > This commit adds an unit test for the "ovs-appctl coverage/show". > > Signed-off-by: Alex Wang Seems reasonable, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] lib: A separate file for rxbuf.
On Mon, Jun 10, 2013 at 01:10:28AM -0700, Gurucharan Shetty wrote: > Till now, rxbuf was only used by the worker. An upcoming > commit will have another client. So carve out rxbuf from > the worker files. > > Signed-off-by: Gurucharan Shetty Thanks for doing this. We need a fix. I don't see a problem coding-wise with these patches, but they are more complex than I had hoped. Also, I had planned to get rid of 'worker' and its components, so I don't like to see a new use for them. I think I have a simpler design that will also work. I'm going to try it out, and I'll report back on my results. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] lib: A separate file for rxbuf.
On Wed, Jun 12, 2013 at 4:38 PM, Ben Pfaff wrote: > On Mon, Jun 10, 2013 at 01:10:28AM -0700, Gurucharan Shetty wrote: > > Till now, rxbuf was only used by the worker. An upcoming > > commit will have another client. So carve out rxbuf from > > the worker files. > > > > Signed-off-by: Gurucharan Shetty > > Thanks for doing this. We need a fix. > > I don't see a problem coding-wise with these patches, but they are > more complex than I had hoped. Also, I had planned to get rid of > 'worker' and its components, so I don't like to see a new use for > them. > > I think I have a simpler design that will also work. I'm going to try > it out, and I'll report back on my results. > Okay. I am working on the ability to add and remove ovsdb databases during run time and will need the same syncing ability. So, please keep that in mind when you implement your solution. Thanks, Guru ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/3] lib: A separate file for rxbuf.
On Wed, Jun 12, 2013 at 04:43:18PM -0700, Gurucharan Shetty wrote: > On Wed, Jun 12, 2013 at 4:38 PM, Ben Pfaff wrote: > > > On Mon, Jun 10, 2013 at 01:10:28AM -0700, Gurucharan Shetty wrote: > > > Till now, rxbuf was only used by the worker. An upcoming > > > commit will have another client. So carve out rxbuf from > > > the worker files. > > > > > > Signed-off-by: Gurucharan Shetty > > > > Thanks for doing this. We need a fix. > > > > I don't see a problem coding-wise with these patches, but they are > > more complex than I had hoped. Also, I had planned to get rid of > > 'worker' and its components, so I don't like to see a new use for > > them. > > > > I think I have a simpler design that will also work. I'm going to try > > it out, and I'll report back on my results. > > > Okay. I am working on the ability to add and remove ovsdb databases during > run time and will need the same syncing ability. > So, please keep that in mind when you implement your solution. OK, thanks for pointing that out. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv3 1/5] lib: Add CRC32C Implementation
No reason. I wasn't sure whether there were Debian-specific policies on this or something. On Thu, Jun 13, 2013 at 7:22 AM, Ben Pfaff wrote: > On Wed, Jun 12, 2013 at 02:35:54PM +0900, Joe Stringer wrote: > > This implementation was derived from FreeBSD: > > http://code.google.com/p/freebsd-head/source/browse/sys/libkern/crc32.c > > > > Reviewed-by: Simon Horman > > Signed-off-by: Joe Stringer > > Looks good, but is there any reason not to fold patch 5 into this one? > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCHv3 1/5] lib: Add CRC32C Implementation
Nope. On Thu, Jun 13, 2013 at 08:58:31AM +0900, Joe Stringer wrote: > No reason. I wasn't sure whether there were Debian-specific policies on > this or something. > > > On Thu, Jun 13, 2013 at 7:22 AM, Ben Pfaff wrote: > > > On Wed, Jun 12, 2013 at 02:35:54PM +0900, Joe Stringer wrote: > > > This implementation was derived from FreeBSD: > > > http://code.google.com/p/freebsd-head/source/browse/sys/libkern/crc32.c > > > > > > Reviewed-by: Simon Horman > > > Signed-off-by: Joe Stringer > > > > Looks good, but is there any reason not to fold patch 5 into this one? > > ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH RFC] ovsdb-server: Preserve remotes across crash and restart.
---
This is unpolished but it shows the approach I have in mind.
Sending out for comments on the approach.
diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 20e1964..d87360a 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -81,8 +81,14 @@ struct add_remote_aux {
struct sset *remotes;
struct db *dbs;
size_t n_dbs;
+FILE *config_tmpfile;
};
static unixctl_cb_func ovsdb_server_add_remote;
+
+struct remove_remote_aux {
+struct sset *remotes;
+FILE *config_tmpfile;
+};
static unixctl_cb_func ovsdb_server_remove_remote;
static unixctl_cb_func ovsdb_server_list_remotes;
@@ -99,6 +105,9 @@ static void update_remote_status(const struct
ovsdb_jsonrpc_server *jsonrpc,
const struct sset *remotes,
struct db dbs[], size_t n_dbs);
+static void save_config(FILE *config_file, const struct sset *);
+static void load_config(FILE *config_file, struct sset *);
+
int
main(int argc, char *argv[])
{
@@ -112,6 +121,8 @@ main(int argc, char *argv[])
int retval;
long long int status_timer = LLONG_MIN;
struct add_remote_aux add_remote_aux;
+struct remove_remote_aux remove_remote_aux;
+FILE *config_tmpfile;
struct db *dbs;
int n_dbs;
@@ -125,7 +136,14 @@ main(int argc, char *argv[])
parse_options(&argc, &argv, &remotes, &unixctl_path, &run_command);
+config_tmpfile = tmpfile();
+if (!config_tmpfile) {
+ovs_fatal(errno, "failed to create temporary file");
+}
+
+save_config(config_tmpfile, &remotes);
daemonize_start();
+load_config(config_tmpfile, &remotes);
n_dbs = MAX(1, argc);
dbs = xcalloc(n_dbs + 1, sizeof *dbs);
@@ -195,10 +213,13 @@ main(int argc, char *argv[])
add_remote_aux.remotes = &remotes;
add_remote_aux.dbs = dbs;
add_remote_aux.n_dbs = n_dbs;
+add_remote_aux.config_tmpfile = config_tmpfile;
unixctl_command_register("ovsdb-server/add-remote", "REMOTE", 1, 1,
ovsdb_server_add_remote, &add_remote_aux);
+remove_remote_aux.remotes = &remotes;
+remove_remote_aux.config_tmpfile = config_tmpfile;
unixctl_command_register("ovsdb-server/remove-remote", "REMOTE", 1, 1,
- ovsdb_server_remove_remote, &remotes);
+ ovsdb_server_remove_remote, &remove_remote_aux);
unixctl_command_register("ovsdb-server/list-remotes", "", 0, 0,
ovsdb_server_list_remotes, &remotes);
@@ -922,7 +943,9 @@ ovsdb_server_add_remote(struct unixctl_conn *conn, int argc
OVS_UNUSED,
: parse_db_column(aux->dbs, aux->n_dbs, remote,
&db, &table, &column));
if (!retval) {
-sset_add(aux->remotes, remote);
+if (sset_add(aux->remotes, remote)) {
+save_config(aux->config_tmpfile, aux->remotes);
+}
unixctl_command_reply(conn, NULL);
} else {
unixctl_command_reply_error(conn, retval);
@@ -934,14 +957,15 @@ ovsdb_server_add_remote(struct unixctl_conn *conn, int
argc OVS_UNUSED,
* that ovsdb-server services. */
static void
ovsdb_server_remove_remote(struct unixctl_conn *conn, int argc OVS_UNUSED,
- const char *argv[], void *remotes_)
+ const char *argv[], void *aux_)
{
-struct sset *remotes = remotes_;
+struct remove_remote_aux *aux = aux_;
struct sset_node *node;
-node = sset_find(remotes, argv[1]);
+node = sset_find(aux->remotes, argv[1]);
if (node) {
-sset_delete(remotes, node);
+sset_delete(aux->remotes, node);
+save_config(aux->config_tmpfile, aux->remotes);
unixctl_command_reply(conn, NULL);
} else {
unixctl_command_reply_error(conn, "no such remote");
@@ -1092,3 +1116,48 @@ usage(void)
leak_checker_usage();
exit(EXIT_SUCCESS);
}
+
+static void
+save_config(FILE *config_file, const struct sset *remotes)
+{
+const char *remote;
+struct json *json;
+char *s;
+
+if (ftruncate(fileno(config_file), 0) == -1) {
+ovs_fatal(errno, "truncate failed");
+}
+
+json = json_array_create_empty();
+SSET_FOR_EACH (remote, remotes) {
+json_array_add(json, json_string_create(remote));
+}
+s = json_to_string(json, 0);
+json_destroy(json);
+
+if (fputs(s, config_file) == EOF || fflush(config_file) == EOF) {
+ovs_fatal(errno, "write failed");
+}
+free(s);
+}
+
+static void
+load_config(FILE *config_file, struct sset *remotes)
+{
+struct json *json;
+size_t i;
+
+sset_clear(remotes);
+
+rewind(config_file);
+json = json_from_stream(config_file);
+if (json->type == JSON_STRING) {
+ovs_fatal(0, "reading json failed (%s)", json_string(json));
+}
+ovs_assert(json->type == JSON_ARRAY);
+for (i = 0; i < json->u.array.n; i++) {
+const struct jso
Re: [ovs-dev] [PATCH 1/3] lib: A separate file for rxbuf.
On Wed, Jun 12, 2013 at 04:48:28PM -0700, Ben Pfaff wrote: > On Wed, Jun 12, 2013 at 04:43:18PM -0700, Gurucharan Shetty wrote: > > On Wed, Jun 12, 2013 at 4:38 PM, Ben Pfaff wrote: > > > > > On Mon, Jun 10, 2013 at 01:10:28AM -0700, Gurucharan Shetty wrote: > > > > Till now, rxbuf was only used by the worker. An upcoming > > > > commit will have another client. So carve out rxbuf from > > > > the worker files. > > > > > > > > Signed-off-by: Gurucharan Shetty > > > > > > Thanks for doing this. We need a fix. > > > > > > I don't see a problem coding-wise with these patches, but they are > > > more complex than I had hoped. Also, I had planned to get rid of > > > 'worker' and its components, so I don't like to see a new use for > > > them. > > > > > > I think I have a simpler design that will also work. I'm going to try > > > it out, and I'll report back on my results. > > > > > Okay. I am working on the ability to add and remove ovsdb databases during > > run time and will need the same syncing ability. > > So, please keep that in mind when you implement your solution. > > OK, thanks for pointing that out. I sent out a patch that shows the approach I have in mind. Does it make sense? What do you think? http://openvswitch.org/pipermail/dev/2013-June/028428.html ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] Running openvswitch in Android
ovs-vsctl hang for bridge related operations usually indicates that ovs-vswitchd daemon is not running. Can you check it? Also, ovs-vsctl show cmd works since it only interacts with ovsdb & ovsdb-server. Best, Jing On Wed, Jun 12, 2013 at 4:26 PM, muddin wrote: > Hello, > > > We have compiled and installed the openvswitch in the android platform. > Following is the version details of the Android and the OpenvSwitch > > > >Android version: 4.2.2 (jelly bean) > > Android source git: > https://android.googlesource.com/device/lge/mako-kernel > > Android Kernel source git: > https://android.googlesource.com/kernel/msm.git > > Kernel version: 3.4 > > Android Build: full_mako-usedebug 4.2.2 JDQ39 > > Openvswitch Kernel Module version: Open vSwitch switching > datapath 1.11.90 > > Openvswitch version: (Open vSwitch) 1.11.90 > > OpenFlow versions 0x1:0x4 (from $ ./ovs-ofctl -V) > > OpenvSwitch source git: git clone git:// > openvswitch.org/openvswitch > > OpenvSwitch git commit: c985ec94a2fe051f083a55d67311e643a432a7ae > > > > > > However we have been facing some problems to run some of the user > commands in Android, for example, > > > #this command runs fine > > $ ./ovs-vsctl --verbose add-br br0 > > > > #this command actually hang. Then we apply ctl+z to stop > > $ ./ovs-vsctl --verbose add-port br0 wlan0 > > > #this command also hang. Then we apply ctl+z to stop > > $ ovs-vsctl add-port br0 vif1.0 > > > > #this command runs fine > > $ ./ovs-vsctl show > > >aff90ca3-aabd-4bf9-84e5-a8b81e6e8a8e > > Bridge "br0" > > Port "vif1.0" > > Interface "vif1.0" > > Port "wlan0" > > Interface "wlan0" > > Port "br0" > > Interface "br0" > > type: internal > > > #this command shows error > > $ ./ovs-ofctl --verbose dump-flows br0 > > ovs-ofctl: /data/local/tmp/run/openvswitch/br0.mgmt: failed to open socket > (Connection refused) > > > > Can you please help on explaining the possible reason of such error? > > > Here is more details about the files in > "/data/local/tmp/run/openvswitch/" > > > shell@android:/data/local/tmp # ls -l /data/local/tmp/run/openvswitch/ > > srwx-- root root 2013-06-12 20:17 br0.mgmt > > srwx-- root root 2013-06-12 20:17 br0.snoop > > srwx-- root root 2013-06-12 20:16 db.sock > > srwx-- root root 2013-06-12 20:17 > ovs-vswitchd.1703.ctl > > -rw-rw-rw- root root5 2013-06-12 20:17 ovs-vswitchd.pid > > srwx-- root root 2013-06-12 20:16 > ovsdb-server.1691.ctl > > -rw-rw-rw- root root5 2013-06-12 20:16 ovsdb-server.pid > > > > > The attached file "verbosecmd.txt" shows the output of the above > commands with verbose enable. > > > The details of building and installing openvswitch for android can be > found in the file "Build-Openvswitch.txt". > > In order build the openvswitch a patch file "ovs_android.patch" and a > script file "envsetup.sh" is required. More details about using these two > files can be found in "Build-Openvswitch.txt". > > > Regards > > Mostafa Uddin > > http://cs.odu.edu/~muddin > > ___ > 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] Possible Regression in 'netdev: Add new "struct netdev_rx" for capturing packets from a netdev.'
On Wed, Jun 12, 2013 at 08:08:50AM -0700, Ben Pfaff wrote:
> On Wed, Jun 12, 2013 at 07:11:50AM -0700, Murphy McCauley wrote:
> >
> > On Jun 12, 2013, at 6:28 AM, Ed Maste wrote:
> >
> > > On 12 June 2013 07:04, Murphy McCauley wrote:
> > >> (Sorry this isn't an actual reply and is missing context -- I wasn't on
> > >> the list when it was originally posted.)
> > >>
> > >> Simon and I have been in touch about this, and I thought I'd share my
> > >> findings for what they're worth.
> > >>
> > >> The problem is from the commit Simon mentioned
> > >> (796223f5bc3a4896e6398733c798390158479400). Specifically, it's in
> > >> netdev-linux.c in netdev_linux_send().
> > >>
> > >> The new version always sends using the "sender" socket made by
> > >> af_packet_sock() unless the interface is a tap, in which case it sends
> > >> it using the tap fd. This differs from the old version which sent using
> > >> whatever was in the fd field of the netdev if it was available. For tap
> > >> interfaces, this was the tap fd, so the result was the same as it is
> > >> now. But for other interfaces, this held the socket opened for
> > >> receiving if the interface was listening (which was maybe never "right"
> > >> in some sense and isn't convenient anymore since this socket descriptor
> > >> is no longer stored in the non-rx netdev).
> > >>
> > >> The comments indicate that the exception is made for tap interfaces
> > >> since writing to a tap interface with an AF_PACKET socket results in
> > >> receiving the packet you just wrote. However, I don't think this
> > >> behavior is limited to taps. Since the old version of the code sent and
> > >> received with the same socket descriptor, I think the loop was fixed by
> > >> the check in dev_queue_xmit_nit() in net/core/dev.c. Since they're two
> > >> different socket descriptors now, this no longer works and you get the
> > >> loop.
> > >
> > > Ahh, it turns out Ben explained this to me when I ran into a related
> > > issue with the FreeBSD userspace implementation. Ben's message in the
> > > thread is at http://openvswitch.org/pipermail/dev/2012-July/018806.html
> > > .
> > >
> > >> I fixed it (I think) by adding a BPF packet filter on the rx socket so
> > >> that it only receives incoming packets. There's probably a better fix,
> > >> but you're welcome to the patch if you want it.
> > >
> > > I think it's worth taking a look.
> >
> >
> > I think this is more or less against master.
> >
> > diff --git a/lib/netdev-linux.c b/lib/netdev-linux.c
> > index d73115b..cc47a6b 100644
> > --- a/lib/netdev-linux.c
> > +++ b/lib/netdev-linux.c
> > @@ -744,6 +744,14 @@ netdev_linux_rx_open(struct netdev *netdev_, struct
> > netdev_rx **rxp)
> > } else {
> > struct sockaddr_ll sll;
> > int ifindex;
> > +/* Result of tcpdump -dd inbound */
> > +static struct sock_filter filt[] = {
> > +{ 0x28, 0, 0, 0xf004 },
> > +{ 0x15, 0, 1, 0x0004 },
> > +{ 0x6, 0, 0, 0x },
> > +{ 0x6, 0, 0, 0x }
> > +};
> > +static struct sock_fprog fprog = {ARRAY_SIZE(filt), filt};
> >
> > /* Create file descriptor. */
> > fd = socket(PF_PACKET, SOCK_RAW, 0);
> > @@ -776,6 +784,16 @@ netdev_linux_rx_open(struct netdev *netdev_, struct
> > netdev_rx **rxp)
> > netdev_get_name(netdev_), strerror(error));
> > goto error;
> > }
> > +
> > +/* Filter for only incoming packets. */
> > +error = setsockopt(fd, SOL_SOCKET, SO_ATTACH_FILTER, &fprog,
> > + sizeof fprog);
> > +if (error) {
> > +error = errno;
> > +VLOG_ERR("%s: failed attach filter (%s)",
> > + netdev_get_name(netdev_), strerror(error));
> > +goto error;
> > +}
> > }
> >
> > rx = xmalloc(sizeof *rx);
>
> Thanks, Murphy.
>
> Simon, does this solve the problem you see?
Hi Ben, Hi all,
I have lightly tested this using the scenarios that I mentioned
in my initial post - multicast IPv6 and IPv6, broadcast ARP - and
yes, this seems to resolve the problem that I observed.
Tested-by: Simon Horman
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH v4] datapath: make skb->csum consistent with rest of networking stack.
Thanks, I pushed it to master. On Wed, Jun 12, 2013 at 9:46 AM, Jesse Gross wrote: > On Tue, Jun 11, 2013 at 6:59 PM, Pravin B Shelar wrote: >> As suggested by Jesse in the comment for patch "gre: Restructure >> tunneling", following patch keeps skb->csum correct across ovs. >> >> Signed-off-by: Pravin B Shelar > > Acked-by: Jesse Gross ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH v5] gre: Restructure tunneling.
Following patch restructures ovs tunneling and gre vport
implementation to make ovs tunneling more in sync with
upstream kernel tunneling. Doing this tunneling code is
simplified as most of protocol processing on send and
recv is pushed to kernel tunneling. For external ovs
module the code is moved to kernel compatibility code.
Signed-off-by: Pravin B Shelar
---
v4-v5:
- Added context to erro msg.
- remove cb restore from gso.
- use standard skb_needs_linearize().
- clearer dst refcnt.
- fixed gso ip-id
- removed IS_ERR_VALUE
v4-v3:
- Fix coding style.
- Fix __build_header() parameters.
- Using looser check of skb->ip_summed at __skb_gso_segment.
v2-v3:
- Move rcv packet datapath processing to ip_tunnel code.
- Use TUNNEL_* flags.
- better check gre64 vport.
- return skb if there is no gre port created.
v1-v2:
- Fix Modules path for ip_tunnels_core.c
---
datapath/compat.h |6 +
datapath/flow.c | 12 +-
datapath/flow.h |9 +-
datapath/linux/Modules.mk |6 +
datapath/linux/compat/gre.c | 352 +++
datapath/linux/compat/gso.c | 157 ++
datapath/linux/compat/gso.h | 72 +
datapath/linux/compat/include/linux/if_ether.h |4 +
datapath/linux/compat/include/linux/netdevice.h | 13 +
datapath/linux/compat/include/net/gre.h | 102 +++
datapath/linux/compat/include/net/ip_tunnels.h | 54
datapath/linux/compat/ip_tunnels_core.c | 112 +++
datapath/tunnel.c | 14 +-
datapath/tunnel.h | 11 +-
datapath/vport-gre.c| 349 +--
datapath/vport-lisp.c |2 +-
datapath/vport-vxlan.c |2 +-
17 files changed, 1036 insertions(+), 241 deletions(-)
create mode 100644 datapath/linux/compat/gre.c
create mode 100644 datapath/linux/compat/gso.c
create mode 100644 datapath/linux/compat/gso.h
create mode 100644 datapath/linux/compat/include/net/gre.h
create mode 100644 datapath/linux/compat/include/net/ip_tunnels.h
create mode 100644 datapath/linux/compat/ip_tunnels_core.c
diff --git a/datapath/compat.h b/datapath/compat.h
index c7fd225..a6a01d5 100644
--- a/datapath/compat.h
+++ b/datapath/compat.h
@@ -94,4 +94,10 @@ static inline void skb_set_mark(struct sk_buff *skb, u32
mark)
}
#endif /* after 2.6.20 */
+#if LINUX_VERSION_CODE >= KERNEL_VERSION(2,6,36)
+#define rt_dst(rt) (rt->dst)
+#else
+#define rt_dst(rt) (rt->u.dst)
+#endif
+
#endif /* compat.h */
diff --git a/datapath/flow.c b/datapath/flow.c
index 7604405..1f5a8e5 100644
--- a/datapath/flow.c
+++ b/datapath/flow.c
@@ -1015,7 +1015,7 @@ int ipv4_tun_from_nlattr(const struct nlattr *attr,
switch (type) {
case OVS_TUNNEL_KEY_ATTR_ID:
tun_key->tun_id = nla_get_be64(a);
- tun_key->tun_flags |= OVS_TNL_F_KEY;
+ tun_key->tun_flags |= TUNNEL_KEY;
break;
case OVS_TUNNEL_KEY_ATTR_IPV4_SRC:
tun_key->ipv4_src = nla_get_be32(a);
@@ -1031,10 +1031,10 @@ int ipv4_tun_from_nlattr(const struct nlattr *attr,
ttl = true;
break;
case OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT:
- tun_key->tun_flags |= OVS_TNL_F_DONT_FRAGMENT;
+ tun_key->tun_flags |= TUNNEL_DONT_FRAGMENT;
break;
case OVS_TUNNEL_KEY_ATTR_CSUM:
- tun_key->tun_flags |= OVS_TNL_F_CSUM;
+ tun_key->tun_flags |= TUNNEL_CSUM;
break;
default:
return -EINVAL;
@@ -1062,7 +1062,7 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb,
if (!nla)
return -EMSGSIZE;
- if (tun_key->tun_flags & OVS_TNL_F_KEY &&
+ if (tun_key->tun_flags & TUNNEL_KEY &&
nla_put_be64(skb, OVS_TUNNEL_KEY_ATTR_ID, tun_key->tun_id))
return -EMSGSIZE;
if (tun_key->ipv4_src &&
@@ -1075,10 +1075,10 @@ int ipv4_tun_to_nlattr(struct sk_buff *skb,
return -EMSGSIZE;
if (nla_put_u8(skb, OVS_TUNNEL_KEY_ATTR_TTL, tun_key->ipv4_ttl))
return -EMSGSIZE;
- if ((tun_key->tun_flags & OVS_TNL_F_DONT_FRAGMENT) &&
+ if ((tun_key->tun_flags & TUNNEL_DONT_FRAGMENT) &&
nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_DONT_FRAGMENT))
return -EMSGSIZE;
- if ((tun_key->tun_flags & OVS_TNL_F_CSUM) &&
+ if ((tun_key->tun_flags & TUNNEL_CSUM) &&
nla_put_flag(skb, OVS_TUNNEL_KEY_ATTR_CSUM))
return -EMSGSIZE;
diff --git a/datapath/flow.h b/datapath/flow.h
ind
