Re: [ovs-dev] [PATCH 2/2] datapath: Add basic MPLS support to kernel
On Wed, Mar 20, 2013 at 05:46:38PM -0700, Jesse Gross wrote:
> On Wed, Mar 20, 2013 at 5:33 PM, Simon Horman wrote:
> > On Wed, Mar 20, 2013 at 03:43:38PM -0700, Jesse Gross wrote:
> >> On Wed, Mar 20, 2013 at 6:18 AM, Simon Horman wrote:
> >> > Allow datapath to recognize and extract MPLS labels into flow keys
> >> > and execute actions which push, pop, and set labels on packets.
> >> >
> >> > Based heavily on work by Leo Alterman and Ravi K.
> >> >
> >> > Cc: Ravi K
> >> > Cc: Leo Alterman
> >> > Reviewed-by: Isaku Yamahata
> >> > Signed-off-by: Simon Horman
> >> >
> >> > ---
> >> >
> >> > TODO:
> >> > * Enhance core kernel code to handle GSO for MPLS or
> >> > otherwise deal with accelerations. (Linux network core)
> >> > * Add ETH_TYPE_MIN or similar to Linux network core
> >> >
> >> > v2.22
> >> > * As suggested by Jesse Gross:
> >> > - Fix sparse warning in validate_and_copy_actions()
> >> > I have no idea why sparse doesn't show this up this on my system.
> >> > - Remove call to skb_cow_head() from push_mpls() as it
> >> > is already covered by a call to make_writable()
> >> > - Check (key_type > OVS_KEY_ATTR_MAX) in ovs_flow_verify_key_len()
> >> > - Disallow set actions on l2.5+ data and MPLS push and pop actions
> >> > after an MPLS pop action as there is no verification that the packet
> >> > is actually of the new ethernet type. This may later be supported
> >> > using recirculation or by other means.
> >> > - Do not add spurious debuging message to ovs_flow_cmd_new_or_set()
> >>
> >> There were a couple other things from the previous time that don't
> >> seem to have made it here. Looking back I see:
> >> * MPLS label userspace/kernel change (just the type definition) and
> >> only allowing a single label in the stack at this time.
> >
> > This one I was planning to fix but it slipped my mind.
> >
> >> * Validation of actions using both paths of the sample action.
> >
> > This one I had not realised was related to MPLS.
> > Could you explain it in a little more detail?
>
> Currently, no action can affect the validity of any other action, so
> we just evaluate sample actions unconditionally to make sure that we
> hit all of them. However, with MPLS that's no longer the case - a
> push_mpls action can make a set(mpls) action valid or a set(ip) action
> invalid.
>
> Right now, we're not carrying the current_eth_type through the sample
> action into/out of it's subactions. I think we need to do that and
> also evaluate both paths to make sure that they are each valid.
Thanks.
The approach that I have come up with is to track a stack of ethernet
types, one for each (sample) depth of validation. When validation is
entered the initial ethernet type for the current depth is pushed onto the
stack. It may be modified by actions during validation. When a modification
occurs the ethernet types for higher stack-depths are popped off the stack.
All entries on the stack are checked when validating the ethernet type
required by an action.
I am, however, unsure how to construct sample actions and exercise
this code other than for depth=0.
The incremental diff I have so far is as follows:
diff --git a/datapath/datapath.c b/datapath/datapath.c
index 9c30168..af60b2c 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -491,13 +491,26 @@ static inline void add_nested_action_end(struct
sw_flow_actions *sfa, int st_off
a->nla_len = sfa->actions_len - st_offset;
}
-static int validate_and_copy_actions(const struct nlattr *attr,
+struct eth_types {
+ size_t depth;
+ __be16 types[SAMPLE_ACTION_DEPTH];
+};
+
+static void eth_types_set(struct eth_types *types, size_t depth, __be16 type)
+{
+ types->depth = depth;
+ types->types[depth] = type;
+}
+
+static int validate_and_copy_actions__(const struct nlattr *attr,
const struct sw_flow_key *key, int depth,
- struct sw_flow_actions **sfa);
+ struct sw_flow_actions **sfa,
+ struct eth_types *eth_types);
static int validate_and_copy_sample(const struct nlattr *attr,
const struct sw_flow_key *key, int depth,
- struct sw_flow_actions **sfa)
+ struct sw_flow_actions **sfa,
+ struct eth_types *eth_types)
{
const struct nlattr *attrs[OVS_SAMPLE_ATTR_MAX + 1];
const struct nlattr *probability, *actions;
@@ -533,7 +546,8 @@ static int validate_and_copy_sample(const struct nlattr
*attr,
if (st_acts < 0)
return st_acts;
- err = validate_and_copy_actions(actions, key, depth + 1, sfa);
+ err = validate_and_copy_actions__(actions, key, depth + 1, sfa,
+ eth_types);
if (err)
return err;
@@ -543,13 +557,12 @@ static int validate_and_copy_sample(const s
[ovs-dev] [PATCH v2.23] datapath: Add basic MPLS support to kernel
Allow datapath to recognize and extract MPLS labels into flow keys and execute actions which push, pop, and set labels on packets. Based heavily on work by Leo Alterman and Ravi K. Cc: Ravi K Cc: Leo Alterman Reviewed-by: Isaku Yamahata Signed-off-by: Simon Horman --- This is the remaining patch of the series "MPLS actions and matches". It is available in git at: git://github.com/horms/openvswitch.git devel/mpls-v2.23 TODO: * Enhance core kernel code to handle GSO for MPLS or otherwise deal with accelerations. (Linux network core) * Add ETH_TYPE_MIN or similar to Linux network core * Exercise sample actions v2.23 * As suggested by Jesse Gross: - Verify the current ethernet type when validating sample actions both for the taken and not-taken path if the sample action. - Document that the OVS_KEY_ATTR_MPLS attribute accepts a list of struct ovs_key_mpls but that an implementation may restrict the length it accepts. - Restrict the array length of the OVS_KEY_ATTR_MPLS to one. + Don't add ovs_flow_verify_key_len as it was added to handle attributes whose values are arrays but there are no attributes with values that are arrays (of length greater than one). v2.22 * As suggested by Jesse Gross: - Fix sparse warning in validate_and_copy_actions() I have no idea why sparse doesn't show this up this on my system. - Remove call to skb_cow_head() from push_mpls() as it is already covered by a call to make_writable() - Check (key_type > OVS_KEY_ATTR_MAX) in ovs_flow_verify_key_len() - Disallow set actions on l2.5+ data and MPLS push and pop actions after an MPLS pop action as there is no verification that the packet is actually of the new ethernet type. This may later be supported using recirculation or by other means. - Do not add spurious debuging message to ovs_flow_cmd_new_or_set() v2.21 * As suggested by Jesse Gross: - Verify that l3 and l4 actions always always occur prior to a push_mpls action and use the network header pointer of an skb to track the top of the MPLS stack. This avoids adding an l2_size element to the skb callback. v2.20 * As suggested by Jesse Gross: - Do not add ovs_dp_ioctl_hook + This appears to be garbage from a rebase - Do not add skb_cb_set_l2_size. Instead set OVS_CB(skb)->l2_size in ovs_flow_extract(). - Do not free skb on error in push_mpls(), it is freed in the caller - Call skb_reset_mac_len() in pop_mpls() and push_mpls() - Update checksums in pop_mpls(), push_mpls() and set_mpls(). - Rename skb_cb_mpls_bos() as skb_cb_mpls_stack(). It returns the top not the bottom of the stack. - Track the current eth_type in validate_and_copy_actions which is initially the eth_type of the flow and may be modified by push_mpls and pop_mpls actions. Use this to correctly validate mpls_set actions. This is to allow mpls_set actions to be applied to a non-MPLS frame after an mpls_push action (although ovs-vswitchd doesn't currently do that). Also: + Remove the check of the eth_type in set_mpls() as the new validation scheme should ensure it cannot be incorrect. + Use the current eth_type to validate mpls_pop actions and remove the eth_type check from pop_mpls(). - Move OVS_KEY_ATTR_MPLS to non-upstream group in ovs_key_lens - Remove unnecessary memset of mpls_key in ovs_flow_to_nlattrs() - Make a union of the mpls and ip elements of struct sw_flow_key. Currently the code stops parsing after an MPLS header so it is not possible for the ip and mpls elements to be used simultaneously and some space can be saved by using a union. - Allow an array of MPLS key attributes + Currently all but the first element is ignored + User-space needs to be updated to accept more than one element, currently it will treat their presence as an error - Do not update network header in ovs_flow_extract() for after parsing the MPLS stack as it is never used because no l3+ processing occurs on MPLS frames. - Allow multiple MPLS entries in a match by allowing the OVS_KEY_ATTR_MPLS to be an array of struct ovs_key_mpls with at least one entry. Currently only one entry is used which is byte-for-byte compatible with the previous scheme of having OVS_KEY_ATTR_MPLS as a struct ovs_key_mpls. * Make skb writable in pop_mpls(), push_mpls() and set_mpls(). v2.18 - v2.19 * No change v2.17 * As suggested by Ben Pfaff - Use consistent terminology for MPLS. + Consistently refer to the MPLS component of a packet as the MPLS label stack and entries in the stack as MPLS label stack entries (LSE). An MPLS label is a component of an MPLS label stack entry. The other components are the traffic class (TC), time to live (TTL) and bottom of stack (BoS) bit. - Rename compose_.*mpls_ functions as execute_.*mpls_ v2.16 * No change v2.15 * As suggested by Ben Pfaff - Use OVS_ACTION_SET
[ovs-dev] [PATCH] net: add ETH_P_802_3_MIN
Add a new constant ETH_P_802_3_MIN, the minimum ethernet type for
an 802.3 frame. Frames with a lower value in the ethernet type field
are Ethernet II.
Also update all the users of this value that I could find to use the
new constant.
I anticipate adding some more users of this constant when
adding MPLS support to Open vSwtich.
As suggested by Jesse Gross.
Compile tested only.
Cc: Jesse Gross
Cc: Stefan Richter
Cc: Karsten Keil
Cc: Mauro Carvalho Chehab
Cc: linux1394-de...@lists.sourceforge.net
Cc: linux-me...@vger.kernel.org
Cc: dev@openvswitch.org
Signed-off-by: Simon Horman
---
drivers/firewire/net.c |2 +-
drivers/isdn/i4l/isdn_net.c |2 +-
drivers/media/dvb-core/dvb_net.c |6 +++---
drivers/net/ethernet/sun/niu.c |2 +-
drivers/net/plip/plip.c |2 +-
include/uapi/linux/if_ether.h|3 +++
net/ethernet/eth.c |2 +-
net/openvswitch/datapath.c |2 +-
8 files changed, 12 insertions(+), 9 deletions(-)
diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
index 2b27bff..bd34ca1 100644
--- a/drivers/firewire/net.c
+++ b/drivers/firewire/net.c
@@ -630,7 +630,7 @@ static int fwnet_finish_incoming_packet(struct net_device
*net,
if (memcmp(eth->h_dest, net->dev_addr, net->addr_len))
skb->pkt_type = PACKET_OTHERHOST;
}
- if (ntohs(eth->h_proto) >= 1536) {
+ if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN) {
protocol = eth->h_proto;
} else {
rawp = (u16 *)skb->data;
diff --git a/drivers/isdn/i4l/isdn_net.c b/drivers/isdn/i4l/isdn_net.c
index babc621..88d657d 100644
--- a/drivers/isdn/i4l/isdn_net.c
+++ b/drivers/isdn/i4l/isdn_net.c
@@ -1385,7 +1385,7 @@ isdn_net_type_trans(struct sk_buff *skb, struct
net_device *dev)
if (memcmp(eth->h_dest, dev->dev_addr, ETH_ALEN))
skb->pkt_type = PACKET_OTHERHOST;
}
- if (ntohs(eth->h_proto) >= 1536)
+ if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
return eth->h_proto;
rawp = skb->data;
diff --git a/drivers/media/dvb-core/dvb_net.c b/drivers/media/dvb-core/dvb_net.c
index 44225b1..9fc82a1 100644
--- a/drivers/media/dvb-core/dvb_net.c
+++ b/drivers/media/dvb-core/dvb_net.c
@@ -185,7 +185,7 @@ static __be16 dvb_net_eth_type_trans(struct sk_buff *skb,
skb->pkt_type=PACKET_MULTICAST;
}
- if (ntohs(eth->h_proto) >= 1536)
+ if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
return eth->h_proto;
rawp = skb->data;
@@ -228,9 +228,9 @@ static int ule_test_sndu( struct dvb_net_priv *p )
static int ule_bridged_sndu( struct dvb_net_priv *p )
{
struct ethhdr *hdr = (struct ethhdr*) p->ule_next_hdr;
- if(ntohs(hdr->h_proto) < 1536) {
+ if(ntohs(hdr->h_proto) < ETH_P_802_3_MIN) {
int framelen = p->ule_sndu_len -
((p->ule_next_hdr+sizeof(struct ethhdr)) - p->ule_skb->data);
- /* A frame Type < 1536 for a bridged frame, introduces a LLC
Length field. */
+ /* A frame Type < ETH_P_802_3_MIN for a bridged frame,
introduces a LLC Length field. */
if(framelen != ntohs(hdr->h_proto)) {
return -1;
}
diff --git a/drivers/net/ethernet/sun/niu.c b/drivers/net/ethernet/sun/niu.c
index e4c1c88..95cff98 100644
--- a/drivers/net/ethernet/sun/niu.c
+++ b/drivers/net/ethernet/sun/niu.c
@@ -6618,7 +6618,7 @@ static u64 niu_compute_tx_flags(struct sk_buff *skb,
struct ethhdr *ehdr,
(len << TXHDR_LEN_SHIFT) |
((l3off / 2) << TXHDR_L3START_SHIFT) |
(ihl << TXHDR_IHL_SHIFT) |
- ((eth_proto_inner < 1536) ? TXHDR_LLC : 0) |
+ ((eth_proto_inner < ETH_P_802_3_MIN) ? TXHDR_LLC : 0) |
((eth_proto == ETH_P_8021Q) ? TXHDR_VLAN : 0) |
(ipv6 ? TXHDR_IP_VER : 0) |
csum_bits);
diff --git a/drivers/net/plip/plip.c b/drivers/net/plip/plip.c
index bed62d9..1f7bef9 100644
--- a/drivers/net/plip/plip.c
+++ b/drivers/net/plip/plip.c
@@ -560,7 +560,7 @@ static __be16 plip_type_trans(struct sk_buff *skb, struct
net_device *dev)
* so don't forget to remove it.
*/
- if (ntohs(eth->h_proto) >= 1536)
+ if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN)
return eth->h_proto;
rawp = skb->data;
diff --git a/include/uapi/linux/if_ether.h b/include/uapi/linux/if_ether.h
index 798032d..ade07f1 100644
--- a/include/uapi/linux/if_ether.h
+++ b/include/uapi/linux/if_ether.h
@@ -94,6 +94,9 @@
#define ETH_P_EDSA 0xDADA /* Ethertype DSA [ NOT AN OFFICIALLY
REGISTERED ID ] */
#define ETH_P_AF_IUCV 0xFBFB /* IBM af_iucv [ NOT AN OFFICIALLY
REGISTERED ID ] */
+#define ETH_P_802_3_MIN0x0600 /* If the value in the ether
Re: [ovs-dev] [PATCH] net: add ETH_P_802_3_MIN
On Mar 21 Simon Horman wrote:
> Add a new constant ETH_P_802_3_MIN, the minimum ethernet type for
> an 802.3 frame. Frames with a lower value in the ethernet type field
> are Ethernet II.
>
> Also update all the users of this value that I could find to use the
> new constant.
>
> I anticipate adding some more users of this constant when
> adding MPLS support to Open vSwtich.
>
> As suggested by Jesse Gross.
>
> Compile tested only.
>
> Cc: Jesse Gross
> Cc: Stefan Richter
> Cc: Karsten Keil
> Cc: Mauro Carvalho Chehab
> Cc: linux1394-de...@lists.sourceforge.net
> Cc: linux-me...@vger.kernel.org
> Cc: dev@openvswitch.org
> Signed-off-by: Simon Horman
> ---
> drivers/firewire/net.c |2 +-
Acked-by: Stefan Richter
> drivers/isdn/i4l/isdn_net.c |2 +-
> drivers/media/dvb-core/dvb_net.c |6 +++---
> drivers/net/ethernet/sun/niu.c |2 +-
> drivers/net/plip/plip.c |2 +-
> include/uapi/linux/if_ether.h|3 +++
> net/ethernet/eth.c |2 +-
> net/openvswitch/datapath.c |2 +-
> 8 files changed, 12 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/firewire/net.c b/drivers/firewire/net.c
> index 2b27bff..bd34ca1 100644
> --- a/drivers/firewire/net.c
> +++ b/drivers/firewire/net.c
> @@ -630,7 +630,7 @@ static int fwnet_finish_incoming_packet(struct net_device
> *net,
> if (memcmp(eth->h_dest, net->dev_addr, net->addr_len))
> skb->pkt_type = PACKET_OTHERHOST;
> }
> - if (ntohs(eth->h_proto) >= 1536) {
> + if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN) {
> protocol = eth->h_proto;
> } else {
> rawp = (u16 *)skb->data;
[...]
--
Stefan Richter
-=-===-= --== =-=-=
http://arcgraph.de/sr/
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] net: add ETH_P_802_3_MIN
Em Thu, 21 Mar 2013 17:29:28 +0900 Simon Horman escreveu: > Add a new constant ETH_P_802_3_MIN, the minimum ethernet type for > an 802.3 frame. Frames with a lower value in the ethernet type field > are Ethernet II. > > Also update all the users of this value that I could find to use the > new constant. > > I anticipate adding some more users of this constant when > adding MPLS support to Open vSwtich. > > As suggested by Jesse Gross. > > Compile tested only. > > Cc: Jesse Gross > Cc: Stefan Richter > Cc: Karsten Keil > Cc: Mauro Carvalho Chehab > Cc: linux1394-de...@lists.sourceforge.net > Cc: linux-me...@vger.kernel.org > Cc: dev@openvswitch.org > Signed-off-by: Simon Horman ... > diff --git a/drivers/media/dvb-core/dvb_net.c > b/drivers/media/dvb-core/dvb_net.c > index 44225b1..9fc82a1 100644 > --- a/drivers/media/dvb-core/dvb_net.c > +++ b/drivers/media/dvb-core/dvb_net.c > @@ -185,7 +185,7 @@ static __be16 dvb_net_eth_type_trans(struct sk_buff *skb, > skb->pkt_type=PACKET_MULTICAST; > } > > - if (ntohs(eth->h_proto) >= 1536) > + if (ntohs(eth->h_proto) >= ETH_P_802_3_MIN) > return eth->h_proto; ... Acked-by: Mauro Carvalho Chehab Cheers, Mauro ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Preallocate reply skb in ovs_vport_cmd_set().
On Mar 20, 2013, at 6:44 PM, Jesse Gross wrote: > Allocation of the Netlink notification skb can potentially fail > after changing vport configuration. In general, we try to avoid > this by undoing any change we made but that is difficult for existing > objects. This avoids the problem by preallocating the buffer (which > is fixed size). > > Signed-off-by: Jesse Gross Looks good to me Jesse. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] net: add ETH_P_802_3_MIN
From: Simon Horman
Date: Thu, 21 Mar 2013 17:29:28 +0900
> Add a new constant ETH_P_802_3_MIN, the minimum ethernet type for
> an 802.3 frame. Frames with a lower value in the ethernet type field
> are Ethernet II.
>
> Also update all the users of this value that I could find to use the
> new constant.
>
> I anticipate adding some more users of this constant when
> adding MPLS support to Open vSwtich.
>
> As suggested by Jesse Gross.
>
> Compile tested only.
>
> Signed-off-by: Simon Horman
You missed a few cases:
drivers/media/dvb-core/dvb_net.c: } while (p->ule_sndu_type < 1536);
drivers/media/dvb-core/dvb_net.c: if
(priv->ule_sndu_type < 1536) {
net/atm/lec.h: *is less than 1536(0x0600) MUST be encoded by placing that
length
drivers/net/wireless/ray_cs.c: if (ntohs(proto) >= 1536) { /* DIX II ethernet
frame */
net/bridge/netfilter/ebtables.c:if (FWINV2(ntohs(ethproto) >=
1536, EBT_IPROTO))
include/linux/if_vlan.h:if (ntohs(proto) >= 1536) {
net/bluetooth/bnep/netdev.c:if (proto >= 1536)
net/openvswitch/flow.c: if (ntohs(proto) >= 1536)
net/mac80211/tx.c: } else if (ethertype >= 0x600) {
net/wireless/util.c:} else if (ethertype > 0x600) {
In fact, the last line looks like a bug, it should be >= not >.
Could you take care of these bits and respin your patch?
Thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: xlate actions once with subfacets.
Before this patch, when ofproto-dpif decided that a particular flow miss needed a facet, it would do action translation multiple times. Once in subfacet_make_actions(), and once per packet in subfacet_update_stats(). In the common case (once per miss), this would double the amount of work required in xlate_actions(). The call to facet_push_stats() in subfacet_update_stats() is unnecessary. If the packets are simply accounted to the facet, they will eventually be pushed to the relevant rules in update_stats() or when the facet is removed. Removing the unnecessary step gives us a 20% improvement of the netperf TCP_CRR benchmark with the complex flow tables installed by our controller. Signed-off-by: Ethan Jackson --- ofproto/ofproto-dpif.c |1 - 1 file changed, 1 deletion(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index d1b9f34..d0fd179 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5269,7 +5269,6 @@ subfacet_update_stats(struct subfacet *subfacet, facet->packet_count += stats->n_packets; facet->byte_count += stats->n_bytes; facet->tcp_flags |= stats->tcp_flags; -facet_push_stats(facet); netflow_flow_update_flags(&facet->nf_flow, stats->tcp_flags); } } -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] nicira-ext: Fix comment on NXAST_STACK_PUSH and NXAST_STACK_POP.
CC: Pankaj Thakkar
Signed-off-by: Andy Zhou
---
include/openflow/nicira-ext.h |2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/include/openflow/nicira-ext.h b/include/openflow/nicira-ext.h
index e7e7264..54fc4f9 100644
--- a/include/openflow/nicira-ext.h
+++ b/include/openflow/nicira-ext.h
@@ -575,7 +575,7 @@ struct nx_action_stack {
ovs_be16 type; /* OFPAT_VENDOR. */
ovs_be16 len; /* Length is 16. */
ovs_be32 vendor;/* NX_VENDOR_ID. */
-ovs_be16 subtype; /* NXAST_REG_PUSH or NXAST_REG_POP. */
+ovs_be16 subtype; /* NXAST_STACK_PUSH or NXAST_STACK_POP. */
ovs_be16 offset;/* Bit offset into the field. */
ovs_be32 field; /* The field used for push or pop. */
ovs_be16 n_bits;/* (n_bits + 1) bits of the field. */
--
1.7.9.5
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: xlate actions once with subfacets.
On Thu, Mar 21, 2013 at 11:35:11AM -0700, Ethan Jackson wrote: > Before this patch, when ofproto-dpif decided that a particular flow > miss needed a facet, it would do action translation multiple times. > Once in subfacet_make_actions(), and once per packet in > subfacet_update_stats(). In the common case (once per miss), this > would double the amount of work required in xlate_actions(). > > The call to facet_push_stats() in subfacet_update_stats() is > unnecessary. If the packets are simply accounted to the facet, > they will eventually be pushed to the relevant rules in > update_stats() or when the facet is removed. Removing the > unnecessary step gives us a 20% improvement of the netperf TCP_CRR > benchmark with the complex flow tables installed by our controller. > > Signed-off-by: Ethan Jackson Good catch! Please apply. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] nicira-ext: Fix comment on NXAST_STACK_PUSH and NXAST_STACK_POP.
On Thu, Mar 21, 2013 at 11:37:32AM -0700, Andy Zhou wrote: > CC: Pankaj Thakkar > Signed-off-by: Andy Zhou Looks good, thanks! ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] ofproto-dpif: xlate actions once with subfacets.
Ooops, failed to run the unit tests. Here's a version that passes. --- Before this patch, when ofproto-dpif decided that a particular flow miss needed a facet, it would do action translation multiple times. Once in subfacet_make_actions(), and once per packet in subfacet_update_stats(). In the common case (once per miss), this would double the amount of work required in xlate_actions(). The call to facet_push_stats() in subfacet_update_stats() is unnecessary. If the packets are simply accounted to the facet, they will eventually be pushed to the relevant rules in update_stats() or when the facet is removed. Removing the unnecessary step gives us a 20% improvement of the netperf TCP_CRR benchmark with the complex flow tables installed by our controller. Signed-off-by: Ethan Jackson --- ofproto/ofproto-dpif.c |1 - tests/ofproto-dpif.at |1 + 2 files changed, 1 insertion(+), 1 deletion(-) diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c index d1b9f34..d0fd179 100644 --- a/ofproto/ofproto-dpif.c +++ b/ofproto/ofproto-dpif.c @@ -5269,7 +5269,6 @@ subfacet_update_stats(struct subfacet *subfacet, facet->packet_count += stats->n_packets; facet->byte_count += stats->n_bytes; facet->tcp_flags |= stats->tcp_flags; -facet_push_stats(facet); netflow_flow_update_flags(&facet->nf_flow, stats->tcp_flags); } } diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at index 354fdc9..f390f29 100644 --- a/tests/ofproto-dpif.at +++ b/tests/ofproto-dpif.at @@ -608,6 +608,7 @@ NXT_PACKET_IN (xid=0x0): table_id=7 cookie=0x9 total_len=64 in_port=1 tun_id=0x6 udp,metadata=0,in_port=0,dl_vlan=80,dl_vlan_pcp=0,dl_src=80:81:81:81:81:81,dl_dst=82:82:82:82:82:82,nw_src=83.83.83.83,nw_dst=84.84.84.84,nw_tos=0,nw_ecn=0,nw_ttl=0,tp_src=85,tp_dst=86 udp_csum:43a1 ]) +AT_CHECK([ovs-appctl time/warp 5000], [0], [ignore]) AT_CHECK([ovs-ofctl dump-flows br0 | ofctl_strip | sort], [0], [dnl cookie=0x1, n_packets=2, n_bytes=120, dl_src=20:22:22:22:22:22 actions=CONTROLLER:65535,resubmit(80,1) cookie=0x2, n_packets=3, n_bytes=180, dl_src=30:33:33:33:33:33 actions=mod_vlan_vid:15,CONTROLLER:65535 -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] ofproto-dpif: xlate actions once with subfacets.
On Thu, Mar 21, 2013 at 12:10:08PM -0700, Ethan Jackson wrote: > Ooops, failed to run the unit tests. Here's a version that passes. This is fine with me too. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] automake: ofp-errors.inc is generated inside srcdir
On Thu, Mar 21, 2013 at 10:52:37AM +0100, Damien Millescamps wrote:
> On 03/20/2013 10:20 PM, Ben Pfaff wrote:
> > Some more background: we don't require Python to build because very
> > old versions of the XenServer DDK (under which we have to build OVS to
> > produce XenServer binaries) did not include a Python interpreter. I
> > was playing with a newer XenServer DDK the other day, though, and
> > noticed that it does include Python. So it might be possible to just
> > add Python as a build-time requirement these days, in which case we
> > could simplify the makefiles and make them much more straightforward.
>
> I agree with it. Actually to be able to do a "make dist", the ./boot.sh
> is not sufficient, I have to launch a configure first. But since I am
> cross compiling, it doesn't make that much sense to do "./boot.sh &&
> ./configure && make dist", then "${OVS_DIST}/configure --host=${TARGET}
> && make".
> Something more straight forward should be feasible.
The straightforward solution is to use a writable source directory.
Since you are not using the straightforward solution, you have to use
some other solution.
> From my understanding, my patch could be part of the solution. There
> could be an additional check on the presence of python during the
> configure step, and lib/ofp-errors.inc could be removed from the make dist.
> Is this what you mean ?
No. That would make everything more complicated. It would also make
the contents of the distribution tarball unpredictable: its contents
would depend on what software was installed on the computer that runs
"make dist", whereas it does not currently depend on that.
I am suggesting that we require the machine on which Open vSwitch is
built to have Python installed. Then we can always generate
ofp-errors.inc at build time, so there is no need to distribute it.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [bfd] bfd: Implement Bidirectional Forwarding Detection.
On Tue, Mar 12, 2013 at 07:44:10PM -0700, Ethan Jackson wrote:
> Traditionally, Open vSwitch has used a variant of 802.1ag "CFM" for
> interface liveness detection. This has served us well until now,
> but has several serious drawbacks which have steadily become more
> inconvenient. First, the 802.1ag standard does not implement
> several useful features forcing us to (optionally) break
> compatibility. Second, 802.1.ag is not particularly popular
> outside of carrier grade networking equipment. Third, 802.1ag is
> simply quite awkward.
>
> In an effort to solve the aforementioned problems, this patch
> implements BFD which is ubiquitous, well designed, straight
> forward, and implements required features in a standard way. The
> initial cut of the protocol focuses on getting the basics of the
> specification correct, leaving performance optimizations, and
> advanced features as future work. The protocol should be
> considered experimental pending future testing.
>
> Signed-off-by: Ethan Jackson
bfd.c
-
> +#include
> +#include
We usually use double quotes ("bfd.h") for our own headers.
> +/* XXX Finish Experimnetal BFD support.
What part is experimental? (The protocol or the implementation?)
> + * - CFM random VLAN option equivalent. Instead of vlan, randomly modulate
> the
> + * UDP source port to get the required entropy.
Ugh. Do we really need random anything? I've never liked that
"feature".
> +#define BFD_VERSION 1
I think the following is microseconds, could you add a comment /*
microseconds */, put _USEC in the name, etc.?
> +#define OVS_BFD_MIN_RX 20
I'd be half-inclined to write the states as 0 << 6, 1 << 6, 2 << 6, 3
<< 6. But it doesn't really matter:
> +enum state {
> +STATE_ADMIN_DOWN = 0,
> +STATE_DOWN = 1,
> +STATE_INIT = 2,
> +STATE_UP = 3
> +};
...
> +/* RFC 5880 Section 4.1
I think you left out the line that gives the first digits of the
two-digit numbers here. It's harder to read without them:
> + * 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + * |Vers | Diag |Sta|P|F|C|A|D|M| Detect Mult |Length |
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + * | My Discriminator|
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + * | Your Discriminator |
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + * |Desired Min TX Interval|
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + * | Required Min RX Interval|
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
> + * | Required Min Echo RX Interval |
> + * +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+ */
The BFD timers are 32-bit counts of microseconds. The timers in
struct bfd are 64-bit and appear to be milliseconds, but I'm not
certain that all of them have the same unit. Comments on units would
be welcome. Do they need to be 64-bit?
...
> +/* Returns a 'smap' of key value pairs representing the status of 'bfd'
> + * intended for the OVS database. */
> +void
> +bfd_get_status(const struct bfd *bfd, struct smap *smap)
> +{
> +smap_add(smap, "forwarding", bfd_forwarding(bfd) ? "true" : "false");
> +smap_add(smap, "state", bfd_state_str(bfd->state));
> +smap_add(smap, "diagnostic", bfd_diag_str(bfd->diag));
> +
> +if (bfd->state != STATE_DOWN) {
Spaces are OK in the OVSDB protocol but I don't think we use them
much. Usually we use hyphen or underscores (probably should
standardize that in fact):
> +smap_add(smap, "remote state", bfd_state_str(bfd->rmt_state));
> +smap_add(smap, "remote diagnostic", bfd_diag_str(bfd->rmt_diag));
> +}
> +}
The interface for bfd_configure() is unusual. It does seem to be
appropriate.
bfd_configure() checks for a key "enable" in the configuration that is
passed in, but I don't see any documentation for that key.
This might be a personal problem, but I find the following:
> +udp_src++;
> +udp_src = udp_src < 49152 ? 49152 : udp_src;
> +bfd->udp_src = udp_src;
a little harder to verify as correct than:
bfd->udp_src = (udp_src++ & 16383) + 49152;
The following, and similar, bugs me a bit because it can call
smap_get_int() twice due to the macro expansion:
> +min_tx = MAX(smap_get_int(cfg, "min_tx", 100), 100);
> +void
> +bfd_run(struct bfd *bfd)
> +{
> +if (bfd->state > STATE_DOWN && time_msec() > bfd->detect_time) {
The test above should probably be >= bfd->detect_time or we should
wait until detect_time + 1 in bfd_wait(), otherwise we could wake up 1
ms too early.
> +bfd_set_state(bfd, STATE_DOWN, DIAG_EXPIRED);
> +}
[ovs-dev] [PATCH 1/2] ovs-bugtool: Remove calls of ovs-ofctl on ovs-system.
With single datapath, making ovs-ofctl calls on ovs-system does not give the necessary o/p. This patch removes those calls. The next patch adds the correct commands to bugtool plugin scripts. Signed-off-by: Gurucharan Shetty --- utilities/bugtool/ovs-bugtool.in |2 -- 1 file changed, 2 deletions(-) diff --git a/utilities/bugtool/ovs-bugtool.in b/utilities/bugtool/ovs-bugtool.in index ea72349..19b3378 100755 --- a/utilities/bugtool/ovs-bugtool.in +++ b/utilities/bugtool/ovs-bugtool.in @@ -575,8 +575,6 @@ exclude those logs from the archive. if os.path.exists(OPENVSWITCH_VSWITCHD_PID): cmd_output(CAP_NETWORK_STATUS, [OVS_DPCTL, 'show', '-s']) for d in dp_list(): -cmd_output(CAP_NETWORK_STATUS, [OVS_OFCTL, 'show', d]) -cmd_output(CAP_NETWORK_STATUS, [OVS_OFCTL, 'dump-flows', d]) cmd_output(CAP_NETWORK_STATUS, [OVS_DPCTL, 'dump-flows', d]) try: vspidfile = open(OPENVSWITCH_VSWITCHD_PID) -- 1.7.9.5 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH 2/2] ovs-bugtool: Add ovs-ofctl commands to bugtool plugin scripts.
This patch adds two new scripts that run "ovs-ofctl show" and
"ovs-ofctl dump-flows" on each bridge.
Signed-off-by: Gurucharan Shetty
---
utilities/bugtool/automake.mk |2 ++
utilities/bugtool/ovs-bugtool-ovs-ofctl-dump-flows | 24
utilities/bugtool/ovs-bugtool-ovs-ofctl-show | 24
.../bugtool/plugins/network-status/openvswitch.xml |2 ++
xenserver/README | 10
5 files changed, 62 insertions(+)
create mode 100755 utilities/bugtool/ovs-bugtool-ovs-ofctl-dump-flows
create mode 100755 utilities/bugtool/ovs-bugtool-ovs-ofctl-show
diff --git a/utilities/bugtool/automake.mk b/utilities/bugtool/automake.mk
index 88f905a..022ed71 100644
--- a/utilities/bugtool/automake.mk
+++ b/utilities/bugtool/automake.mk
@@ -22,6 +22,8 @@ bugtool_scripts = \
utilities/bugtool/ovs-bugtool-vsctl-show \
utilities/bugtool/ovs-bugtool-ovsdb-dump \
utilities/bugtool/ovs-bugtool-daemons-ver \
+ utilities/bugtool/ovs-bugtool-ovs-ofctl-show \
+ utilities/bugtool/ovs-bugtool-ovs-ofctl-dump-flows \
utilities/bugtool/ovs-bugtool-bond-show
scripts_SCRIPTS += $(bugtool_scripts)
diff --git a/utilities/bugtool/ovs-bugtool-ovs-ofctl-dump-flows
b/utilities/bugtool/ovs-bugtool-ovs-ofctl-dump-flows
new file mode 100755
index 000..e033da9
--- /dev/null
+++ b/utilities/bugtool/ovs-bugtool-ovs-ofctl-dump-flows
@@ -0,0 +1,24 @@
+#! /bin/sh
+
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of version 2.1 of the GNU Lesser General
+# Public License as published by the Free Software Foundation.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
+# USA
+#
+# Copyright (C) 2013 Nicira, Inc.
+
+for bridge in `ovs-vsctl list-br`
+do
+echo "ovs-ofctl dump-flows "${bridge}""
+ovs-ofctl dump-flows "$bridge"
+echo ""
+done
diff --git a/utilities/bugtool/ovs-bugtool-ovs-ofctl-show
b/utilities/bugtool/ovs-bugtool-ovs-ofctl-show
new file mode 100755
index 000..a24a71c
--- /dev/null
+++ b/utilities/bugtool/ovs-bugtool-ovs-ofctl-show
@@ -0,0 +1,24 @@
+#! /bin/sh
+
+# This library is free software; you can redistribute it and/or
+# modify it under the terms of version 2.1 of the GNU Lesser General
+# Public License as published by the Free Software Foundation.
+#
+# This library is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+# Lesser General Public License for more details.
+#
+# You should have received a copy of the GNU Lesser General Public
+# License along with this library; if not, write to the Free Software
+# Foundation, Inc., 59 Temple Place, Suite 330, Boston, MA 02111-1307
+# USA
+#
+# Copyright (C) 2013 Nicira, Inc.
+
+for bridge in `ovs-vsctl list-br`
+do
+echo "ovs-ofctl show "${bridge}""
+ovs-ofctl show "$bridge"
+echo ""
+done
diff --git a/utilities/bugtool/plugins/network-status/openvswitch.xml
b/utilities/bugtool/plugins/network-status/openvswitch.xml
index 9539f7c..ebd0260 100644
--- a/utilities/bugtool/plugins/network-status/openvswitch.xml
+++ b/utilities/bugtool/plugins/network-status/openvswitch.xml
@@ -25,4 +25,6 @@
/usr/share/openvswitch/scripts/ovs-bugtool-coverage-show
/usr/share/openvswitch/scripts/ovs-bugtool-bond-show
/usr/share/openvswitch/scripts/ovs-bugtool-memory-show
+ /usr/share/openvswitch/scripts/ovs-bugtool-ovs-ofctl-show
+ /usr/share/openvswitch/scripts/ovs-bugtool-ovs-ofctl-dump-flows
diff --git a/xenserver/README b/xenserver/README
index e72655f..c278c16 100644
--- a/xenserver/README
+++ b/xenserver/README
@@ -86,6 +86,8 @@ debugging. The sources for the extensions are in
* ovs-bugtool-ovsdb-dump
* ovs-bugtool-tc-class-show
* ovs-bugtool-bond-show
+* ovs-bugtool-ovs-ofctl-show
+* ovs-bugtool-ovs-ofctl-dump-flows
system-configuration/openvswitch.xml
@@ -122,6 +124,14 @@ scripts are located in ../utilities/bugtool:
Script to dump tc class configuration for all network interfaces.
+ovs-bugtool-ovs-ofctl-show
+
+Script to dump information about flow tables and ports of each bridge.
+
+ovs-bugtool-ovs-ofctl-dump-flows
+
+Script to dump openflow flows of each bridge.
+
ovs-bugtool-daemons-ver
Script to dump version information for all Open vSwitch daemons.
--
1.7.9.5
__
Re: [ovs-dev] [PATCH] nicira-ext: Fix comment on NXAST_STACK_PUSH and NXAST_STACK_POP.
On Thu, Mar 21, 2013 at 11:37:32AM -0700, Andy Zhou wrote: > CC: Pankaj Thakkar > Signed-off-by: Andy Zhou Applied, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] ovs-bugtool: Remove calls of ovs-ofctl on ovs-system.
On Thu, Mar 21, 2013 at 02:07:38PM -0700, Gurucharan Shetty wrote: > With single datapath, making ovs-ofctl calls on ovs-system > does not give the necessary o/p. This patch removes those calls. > > The next patch adds the correct commands to bugtool plugin scripts. > > Signed-off-by: Gurucharan Shetty Looks good, thanks. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-bugtool: Add ovs-ofctl commands to bugtool plugin scripts.
On Thu, Mar 21, 2013 at 02:07:39PM -0700, Gurucharan Shetty wrote:
> This patch adds two new scripts that run "ovs-ofctl show" and
> "ovs-ofctl dump-flows" on each bridge.
>
> Signed-off-by: Gurucharan Shetty
> +for bridge in `ovs-vsctl list-br`
> +do
The quoting here is really curious:
> +echo "ovs-ofctl dump-flows "${bridge}""
> +ovs-ofctl dump-flows "$bridge"
> +echo ""
> +done
There is something similar in the other script.
Otherwise this looks good, thank you.
I guess that you will backport these patches to 1.10?
Thanks,
Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-bugtool: Add ovs-ofctl commands to bugtool plugin scripts.
On Thu, Mar 21, 2013 at 2:31 PM, Ben Pfaff wrote:
> On Thu, Mar 21, 2013 at 02:07:39PM -0700, Gurucharan Shetty wrote:
>> This patch adds two new scripts that run "ovs-ofctl show" and
>> "ovs-ofctl dump-flows" on each bridge.
>>
>> Signed-off-by: Gurucharan Shetty
>
>> +for bridge in `ovs-vsctl list-br`
>> +do
>
> The quoting here is really curious:
>
>> +echo "ovs-ofctl dump-flows "${bridge}""
I will get rid of the extra quotes and push this to master and 1.10
Do you think we need a new script that does "ovs-appctl
dpif/dump-flows" on each bridge?
The current "ovs-dpctl dump-flows system@ovs-system" may give a lot of o/p.
>> +ovs-ofctl dump-flows "$bridge"
>> +echo ""
>> +done
>
> There is something similar in the other script.
>
> Otherwise this looks good, thank you.
>
> I guess that you will backport these patches to 1.10?
>
> Thanks,
>
> Ben.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 2/2] ovs-bugtool: Add ovs-ofctl commands to bugtool plugin scripts.
On Thu, Mar 21, 2013 at 02:38:44PM -0700, Gurucharan Shetty wrote: > Do you think we need a new script that does "ovs-appctl > dpif/dump-flows" on each bridge? I don't feel strongly either way. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] FAQ: Describe use of Linux bond devices with Open vSwitch.
CC: Jesse Gross Signed-off-by: Ben Pfaff --- FAQ | 29 + 1 file changed, 29 insertions(+) diff --git a/FAQ b/FAQ index 1a2c4f8..90d0003 100644 --- a/FAQ +++ b/FAQ @@ -765,6 +765,35 @@ A: Do you have a controller configured on br0 (as the commands above can refer to the answer there for more information. +Bonds +- + +Q: Can I use Linux bond devices on Open vSwitch bridges? + +A: Yes. If you are already accustomed to configuring and using Linux + bond devices, then you can continue to use them with Open vSwitch. + You can add a Linux bond device to an Open vSwitch bridge with + "ovs-vsctl add-port", the same way as with other network devices. + + You should not configure an IP address on a Linux bond device that + you add to an Open vSwitch bridge. See the "Configuration + Problems" section for more information. + + Linux bonds may have reduced performance compared to Open vSwitch + bonds, because Open vSwitch lacks the direct visibility into the + attached interfaces that would allow it take advantage of NIC + offload features. + + Linux bonds and Open vSwitch bonds have different features. Linux + bonds don't, for example, offer the SLB balancing mode, and cannot + because the bond device does not have access to the Open vSwitch + MAC table. + +Q: It looks like each of the interfaces in my bonded port shows up + as an individual OpenFlow port. Is that right? + +A: See the "Controllers" section below. + Controllers --- -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
[ovs-dev] [PATCH] FAQ: Explain why VMs on a VLAN can't access the Internet.
Signed-off-by: Ben Pfaff --- FAQ | 14 ++ 1 file changed, 14 insertions(+) diff --git a/FAQ b/FAQ index 90d0003..d9224fa 100644 --- a/FAQ +++ b/FAQ @@ -714,6 +714,20 @@ A: It is to be expected that the VMs can't access each other. VLANs the machines you are trying to access are not on VLAN 9 (or 10) and that the Internet is not available on VLAN 9 (or 10). +Q: I added a pair of VMs on the same VLAN, like this: + + ovs-vsctl add-br br0 + ovs-vsctl add-port br0 eth0 + ovs-vsctl add-port br0 tap0 tag=9 + ovs-vsctl add-port br0 tap1 tag=9 + +The VMs can access each other, but not the external network or the +Internet. + +A: It seems likely that the machines you are trying to access in the + external network are not on VLAN 9 and that the Internet is not + available on VLAN 9. + Q: Can I configure an IP address on a VLAN? A: Yes. Use an "internal port" configured as an access port. For -- 1.7.10.4 ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] FAQ: Describe use of Linux bond devices with Open vSwitch.
One nit, there is only one line between the 'Bonds' section and 'Controllers' section. Everywhere else in the file you use 2 lines, with the (accidental) exception of between 'General' and 'Releases', which has 3. -Reid On Thu, Mar 21, 2013 at 3:22 PM, Ben Pfaff wrote: > CC: Jesse Gross > Signed-off-by: Ben Pfaff > --- > FAQ | 29 + > 1 file changed, 29 insertions(+) > > diff --git a/FAQ b/FAQ > index 1a2c4f8..90d0003 100644 > --- a/FAQ > +++ b/FAQ > @@ -765,6 +765,35 @@ A: Do you have a controller configured on br0 (as the > commands above > can refer to the answer there for more information. > > > +Bonds > +- > + > +Q: Can I use Linux bond devices on Open vSwitch bridges? > + > +A: Yes. If you are already accustomed to configuring and using Linux > + bond devices, then you can continue to use them with Open vSwitch. > + You can add a Linux bond device to an Open vSwitch bridge with > + "ovs-vsctl add-port", the same way as with other network devices. > + > + You should not configure an IP address on a Linux bond device that > + you add to an Open vSwitch bridge. See the "Configuration > + Problems" section for more information. > + > + Linux bonds may have reduced performance compared to Open vSwitch > + bonds, because Open vSwitch lacks the direct visibility into the > + attached interfaces that would allow it take advantage of NIC > + offload features. > + > + Linux bonds and Open vSwitch bonds have different features. Linux > + bonds don't, for example, offer the SLB balancing mode, and cannot > + because the bond device does not have access to the Open vSwitch > + MAC table. > + > +Q: It looks like each of the interfaces in my bonded port shows up > + as an individual OpenFlow port. Is that right? > + > +A: See the "Controllers" section below. > + > Controllers > --- > > -- > 1.7.10.4 > > ___ > dev mailing list > dev@openvswitch.org > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] FAQ: Describe use of Linux bond devices with Open vSwitch.
Fixed, thanks. On Thu, Mar 21, 2013 at 03:42:23PM -0700, Reid Price wrote: > One nit, there is only one line between the 'Bonds' section and 'Controllers' > section. Everywhere else in the file you use 2 lines, with the (accidental) > exception of between 'General' and 'Releases', which has 3. > > -Reid > > On Thu, Mar 21, 2013 at 3:22 PM, Ben Pfaff wrote: > > CC: Jesse Gross > > Signed-off-by: Ben Pfaff > > --- > > FAQ | 29 + > > 1 file changed, 29 insertions(+) > > > > diff --git a/FAQ b/FAQ > > index 1a2c4f8..90d0003 100644 > > --- a/FAQ > > +++ b/FAQ > > @@ -765,6 +765,35 @@ A: Do you have a controller configured on br0 (as the > > commands above > > can refer to the answer there for more information. > > > > > > +Bonds > > +- > > + > > +Q: Can I use Linux bond devices on Open vSwitch bridges? > > + > > +A: Yes. If you are already accustomed to configuring and using Linux > > + bond devices, then you can continue to use them with Open vSwitch. > > + You can add a Linux bond device to an Open vSwitch bridge with > > + "ovs-vsctl add-port", the same way as with other network devices. > > + > > + You should not configure an IP address on a Linux bond device that > > + you add to an Open vSwitch bridge. See the "Configuration > > + Problems" section for more information. > > + > > + Linux bonds may have reduced performance compared to Open vSwitch > > + bonds, because Open vSwitch lacks the direct visibility into the > > + attached interfaces that would allow it take advantage of NIC > > + offload features. > > + > > + Linux bonds and Open vSwitch bonds have different features. Linux > > + bonds don't, for example, offer the SLB balancing mode, and cannot > > + because the bond device does not have access to the Open vSwitch > > + MAC table. > > + > > +Q: It looks like each of the interfaces in my bonded port shows up > > + as an individual OpenFlow port. Is that right? > > + > > +A: See the "Controllers" section below. > > + > > Controllers > > --- > > > > -- > > 1.7.10.4 > > > > ___ > > dev mailing list > > dev@openvswitch.org > > http://openvswitch.org/mailman/listinfo/dev ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] mpls: Allow l3 and l4 actions to prior to a push_mpls action
On Wed, Mar 20, 2013 at 03:31:51PM -0700, Jesse Gross wrote: > On Wed, Mar 20, 2013 at 6:18 AM, Simon Horman wrote: > > * Update the order in which actions are committed and thus > > appear in the datapath such that MPLS actions appear after > > l3 and l4 (nw and port) actions. > > > > In the case where an mpls_push action is present it should ensure > > that l3 and l4 actions occur first, which seems logical as > > once a mpls_push has occur the frame will be MPLS rather > > than IPv4 or IPv6. > > > > In the case where there is an mpls_pop action is present this should > > not make any difference as the frame will have been MPLS to start with > > and thus not satisfy the pre-requisites for l3 or l4 actions. > > > > * Update commit_set_nw_action() to use the base ethertype when considering > > eligibility to commit l3 (nw) actions. This allows l3 actions to be > > applied so long as the frame was originally IPv4 or IPv6, even if > > an mpls_push action will be applied and thus flow indicates the > > frame will be MPLS. > > > > * Make actions that may modify port or network information conditional on > > the flow's ethernet type being an IP ethernet type. This is to ensure > > that actions that modify network and port information do not occur > > on non IP packets, for example if an mpls_push action has changed a > > packet from IP to MPLS. > > > > Note that modification of network data is already prevented by > > virtue of commit_set_nw_action() only having cases for when the > > ethernet type of the flow is IPV4 or IPV6. The conditionality > > of network actions on the ethernet type has been added to > > do_xlate_actions() to make it explicit. > > > > Signed-off-by: Simon Horman > > Applied, thanks. Thanks! > I realized that there is one more possible case that could cause > problems here: if something like reg_move is used to load a value into > the transport port field without using one of the field specific > actions. Completely fixing this seemed like more trouble than it is > worth for the time being, so I essentially combined the two strategies > - the one you have here to give nice behavior in the common case and > blocking it at the commit_set_port_action() level to provide final > protection. The idea that you had earlier of just checking that the > base flow is IP seemed cleanest, so I just did that. Thanks, that sounds reasonable to me. I think that the way that actions are marshalled by execute_* functions and then turned into ODP actions using commit_* has worked well for now and serves to provide a reasonably minimal set of ODP actions. However it is difficult to make that approach work well if the ethernet type of the packet is allowed to change, as is the case with MPLS. So it doesn't surprise me that we are deploying checks multiple locations in order to ensure the ODP actions are valid. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH 1/2] mpls: Allow l3 and l4 actions to prior to a push_mpls action
On Thu, Mar 21, 2013 at 5:41 PM, Simon Horman wrote: > I think that the way that actions are marshalled by execute_* functions > and then turned into ODP actions using commit_* has worked well for now > and serves to provide a reasonably minimal set of ODP actions. However > it is difficult to make that approach work well if the ethernet type > of the packet is allowed to change, as is the case with MPLS. So it doesn't > surprise me that we are deploying checks multiple locations in order to > ensure the ODP actions are valid. I agree. I like the fact that it allows us to combine an arbitrarily complex set of userspace actions into a minimal set of kernel actions and would really like to keep the property. However, the current implementation does seem a little at odds with MPLS. ___ dev mailing list dev@openvswitch.org http://openvswitch.org/mailman/listinfo/dev
Re: [ovs-dev] [PATCH] datapath: Preallocate reply skb in ovs_vport_cmd_set().
On Wed, Mar 20, 2013 at 4:44 PM, Jesse Gross wrote:
> Allocation of the Netlink notification skb can potentially fail
> after changing vport configuration. In general, we try to avoid
> this by undoing any change we made but that is difficult for existing
> objects. This avoids the problem by preallocating the buffer (which
> is fixed size).
>
> Signed-off-by: Jesse Gross
> ---
> datapath/datapath.c | 23 +++
> 1 file changed, 15 insertions(+), 8 deletions(-)
>
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index b5eb232..88f5371 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -2005,10 +2005,16 @@ static int ovs_vport_cmd_set(struct sk_buff *skb,
> struct genl_info *info)
> nla_get_u32(a[OVS_VPORT_ATTR_TYPE]) != vport->ops->type)
> err = -EINVAL;
>
> + reply = nlmsg_new(NLMSG_DEFAULT_SIZE, GFP_KERNEL);
> + if (!reply) {
> + err = -ENOMEM;
> + goto exit_unlock;
> + }
> +
> if (!err && a[OVS_VPORT_ATTR_OPTIONS])
> err = ovs_vport_set_options(vport, a[OVS_VPORT_ATTR_OPTIONS]);
> if (err)
> - goto exit_unlock;
> + goto exit_free;
>
> if (a[OVS_VPORT_ATTR_STATS])
> ovs_vport_set_stats(vport, nla_data(a[OVS_VPORT_ATTR_STATS]));
> @@ -2016,17 +2022,18 @@ static int ovs_vport_cmd_set(struct sk_buff *skb,
> struct genl_info *info)
> if (a[OVS_VPORT_ATTR_UPCALL_PID])
> vport->upcall_portid =
> nla_get_u32(a[OVS_VPORT_ATTR_UPCALL_PID]);
>
> - reply = ovs_vport_cmd_build_info(vport, info->snd_portid,
> -info->snd_seq, OVS_VPORT_CMD_NEW);
> - if (IS_ERR(reply)) {
> - netlink_set_err(GENL_SOCK(sock_net(skb->sk)), 0,
> - ovs_dp_vport_multicast_group.id,
> PTR_ERR(reply));
> - goto exit_unlock;
> - }
> + err = ovs_vport_cmd_fill_info(vport, reply, info->snd_portid,
> + info->snd_seq, 0, OVS_VPORT_CMD_NEW);
> + BUG_ON(err < 0);
>
I am not sure about BUG_ON() ?
ovs_vport_cmd_build_info() does not have this assert.
> genl_notify(reply, genl_info_net(info), info->snd_portid,
> ovs_dp_vport_multicast_group.id, info->nlhdr, GFP_KERNEL);
>
> + rtnl_unlock();
> + return 0;
> +
> +exit_free:
> + kfree_skb(reply);
> exit_unlock:
> rtnl_unlock();
> exit:
> --
> 1.7.10.4
>
> ___
> dev mailing list
> dev@openvswitch.org
> http://openvswitch.org/mailman/listinfo/dev
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
