[ovs-dev] Nouvelle version d'123envoi pour envoyer de gros fichier !

[Oph�lie PETIT]

Title: Nouvelles licences 123envoi



	
		 
			

	
		
			


	
	
		
			

Nouvelle version ! 

Vous avez déjà été en contact avec 123envoi ou vous êtes déjà clients de nos services et nous vous en remercions.
123envoi a été conçu pour offrir une solution simple, facile et efficace au problème d'envoi de gros fichiers sur Internet.

Les besoins en matière d'envoi de fichiers ayant encore récemment évolués, nous avons, suite à la demande grandissante de nos utilisateurs, finalisé la version 3.0 de notre logiciel 123envoi.
Celle-ci pourra, par le biais des nouvelles licences commercialisées,  vous permettre d'envoyer jusqu'à 4Go de données par envoi !


Nouvelles licences :
Voici un récapitulatif de nos licences (sans abonnement, licence à vie !), de leurs fonctionnalités et de leur prix unitaire :


	 Version HOME
	1 seul fichier par envoi, jusqu'à 1Go et 1 an de support par téléphone29.00€ seulement !
	
	 Version PRO
	Plusieurs fichiers par envoi, jusqu'à 1Go et 1 an de support par téléphone39.00€ seulement !
	
	 Version PREMIUM
	Plusieurs fichiers par envoi, jusqu'à 2Go et 1 an de support par téléphone59.00€ seulement !
	
	 Version PLATINUM
	Plusieurs fichiers par envoi, jusqu'à 4Go et 1 an de support par téléphone79.00€ seulement !
	

Retrouvez aussi ces licences dans des packs (par 2 ou par 5) au tarif très avantageux !

Pour en savoir + :


http://www.123envoi.com/envoyer_gros_fichiers/register.php



Mise à niveau de votre licence :
Vous disposez deja d'une licence 123envoi ? vous allez peut-etre pouvoir beneficier d'un tarif exceptionnel !
Découvrez vite votre offre personnalisee en cliquant sur le lien ci-dessous :

http://www.123envoi.com/envoyer_gros_fichiers/discount.php


Nouveautés/Fonctionnalités:

	Pause/Reprise pendant l'envoi
	Reprise lors d'une coupure Internet
	Contrôle de la vitesse de l'envoi
	Multi destinataire
	Carnet d'adresses
	Fonctionne avec tout type de fichiers
	Protection par mot de passe
	Accusé de réception
	Création de messages types, signatures, ...
	Choix de la langue de l'email
	Estimation d'envoi précise
	Statistiques
	Signal sonore à la fin de l'envoi
	Confirmation d'annulation de l'envoi
	Messages d'information aux utilisateurs
	Sans abonnement ni engagement
	Uniquement nécessaire pour l'expéditeur
	Facile à installer & à utiliser
	Aucune connaissance n'est nécessaire
	Français / Anglais
	Ne bloque pas la boite aux lettres de votre destinataire



	Si cet email vous a dérangé, n’hésitez pas à nous en faire part au 0899 490 198 en spécifiant ce numéro 008430366
	 ou bien en cliquant sur ce lien
	 

			
		
	
	
	

			
		
	


		
	

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v3 2/4] datapath: Move mega-flow list out of rehashing struct.

[Pravin B Shelar]

ovs-flow rehash does not touch mega flow list. Following patch
moves it dp struct datapath.  Avoid one extra indirection for
accessing mega-flow list head on every packet receive.

Signed-off-by: Pravin B Shelar 
---
v3:
No change.
v2:
No change.
---
 datapath/datapath.c   |   77 +---
 datapath/datapath.h   |6 +-
 datapath/flow_table.c |  157 +
 datapath/flow_table.h |   31 --
 4 files changed, 137 insertions(+), 134 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 7178513..1e7806c 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -61,8 +61,6 @@
 #include "vport-internal_dev.h"
 #include "vport-netdev.h"
 
-#define REHASH_FLOW_INTERVAL (10 * 60 * HZ)
-
 int ovs_net_id __read_mostly;
 
 static void ovs_notify(struct sk_buff *skb, struct genl_info *info,
@@ -165,7 +163,7 @@ static void destroy_dp_rcu(struct rcu_head *rcu)
 {
struct datapath *dp = container_of(rcu, struct datapath, rcu);
 
-   ovs_flow_tbl_destroy((__force struct flow_table *)dp->table, false);
+   ovs_flow_tbl_destroy(&dp->table, false);
free_percpu(dp->stats_percpu);
release_net(ovs_dp_get_net(dp));
kfree(dp->ports);
@@ -237,7 +235,7 @@ void ovs_dp_process_received_packet(struct vport *p, struct 
sk_buff *skb)
}
 
/* Look up flow. */
-   flow = ovs_flow_tbl_lookup(rcu_dereference(dp->table), &key);
+   flow = ovs_flow_tbl_lookup(&dp->table, &key);
if (unlikely(!flow)) {
struct dp_upcall_info upcall;
 
@@ -456,23 +454,6 @@ out:
return err;
 }
 
-/* Called with ovs_mutex. */
-static int flush_flows(struct datapath *dp)
-{
-   struct flow_table *old_table;
-   struct flow_table *new_table;
-
-   old_table = ovsl_dereference(dp->table);
-   new_table = ovs_flow_tbl_alloc(TBL_MIN_BUCKETS);
-   if (!new_table)
-   return -ENOMEM;
-
-   rcu_assign_pointer(dp->table, new_table);
-
-   ovs_flow_tbl_destroy(old_table, true);
-   return 0;
-}
-
 static void clear_stats(struct sw_flow *flow)
 {
flow->used = 0;
@@ -587,11 +568,9 @@ static struct genl_ops dp_packet_genl_ops[] = {
 
 static void get_dp_stats(struct datapath *dp, struct ovs_dp_stats *stats)
 {
-   struct flow_table *table;
int i;
 
-   table = rcu_dereference_check(dp->table, lockdep_ovsl_is_held());
-   stats->n_flows = ovs_flow_tbl_count(table);
+   stats->n_flows = ovs_flow_tbl_count(&dp->table);
 
stats->n_hit = stats->n_missed = stats->n_lost = 0;
for_each_possible_cpu(i) {
@@ -777,7 +756,6 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
struct sw_flow_mask mask;
struct sk_buff *reply;
struct datapath *dp;
-   struct flow_table *table;
struct sw_flow_actions *acts = NULL;
struct sw_flow_match match;
int error;
@@ -818,12 +796,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
if (!dp)
goto err_unlock_ovs;
 
-   table = ovsl_dereference(dp->table);
-
/* Check if this is a duplicate flow */
-   flow = ovs_flow_tbl_lookup(table, &key);
+   flow = ovs_flow_tbl_lookup(&dp->table, &key);
if (!flow) {
-   struct flow_table *new_table = NULL;
struct sw_flow_mask *mask_p;
 
/* Bail out if we're not allowed to create a new flow. */
@@ -831,19 +806,6 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
if (info->genlhdr->cmd == OVS_FLOW_CMD_SET)
goto err_unlock_ovs;
 
-   /* Expand table, if necessary, to make room. */
-   if (ovs_flow_tbl_need_to_expand(table))
-   new_table = ovs_flow_tbl_expand(table);
-   else if (time_after(jiffies, dp->last_rehash + 
REHASH_FLOW_INTERVAL))
-   new_table = ovs_flow_tbl_rehash(table);
-
-   if (new_table && !IS_ERR(new_table)) {
-   rcu_assign_pointer(dp->table, new_table);
-   ovs_flow_tbl_destroy(table, true);
-   table = ovsl_dereference(dp->table);
-   dp->last_rehash = jiffies;
-   }
-
/* Allocate flow. */
flow = ovs_flow_alloc();
if (IS_ERR(flow)) {
@@ -856,7 +818,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
flow->unmasked_key = key;
 
/* Make sure mask is unique in the system */
-   mask_p = ovs_sw_flow_mask_find(table, &mask);
+   mask_p = ovs_sw_flow_mask_find(&dp->table, &mask);
if (!mask_p) {
/* Allocate a new mask if none exsits. */
mask_p = ovs_sw_flow_mask_alloc();
@@ -864,7 +82

[ovs-dev] [PATCH v3 3/4] datapath: Simplify mega-flow APIs.

[Pravin B Shelar]

Hides mega-flow implementation in flow_table.c rather than
datapath.c.  This also helps next patch.

Signed-off-by: Pravin B Shelar 
v3:
No change.
v2:
No change.
---
 datapath/datapath.c   |   27 +++---
 datapath/flow_table.c |  136 -
 datapath/flow_table.h |   12 +---
 3 files changed, 88 insertions(+), 87 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 1e7806c..78f18bc 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -163,7 +163,7 @@ static void destroy_dp_rcu(struct rcu_head *rcu)
 {
struct datapath *dp = container_of(rcu, struct datapath, rcu);
 
-   ovs_flow_tbl_destroy(&dp->table, false);
+   ovs_flow_tbl_destroy(&dp->table);
free_percpu(dp->stats_percpu);
release_net(ovs_dp_get_net(dp));
kfree(dp->ports);
@@ -799,8 +799,6 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
/* Check if this is a duplicate flow */
flow = ovs_flow_tbl_lookup(&dp->table, &key);
if (!flow) {
-   struct sw_flow_mask *mask_p;
-
/* Bail out if we're not allowed to create a new flow. */
error = -ENOENT;
if (info->genlhdr->cmd == OVS_FLOW_CMD_SET)
@@ -816,25 +814,14 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
 
flow->key = masked_key;
flow->unmasked_key = key;
-
-   /* Make sure mask is unique in the system */
-   mask_p = ovs_sw_flow_mask_find(&dp->table, &mask);
-   if (!mask_p) {
-   /* Allocate a new mask if none exsits. */
-   mask_p = ovs_sw_flow_mask_alloc();
-   if (!mask_p)
-   goto err_flow_free;
-   mask_p->key = mask.key;
-   mask_p->range = mask.range;
-   ovs_sw_flow_mask_insert(&dp->table, mask_p);
-   }
-
-   ovs_sw_flow_mask_add_ref(mask_p);
-   flow->mask = mask_p;
rcu_assign_pointer(flow->sf_acts, acts);
 
/* Put flow in bucket. */
-   ovs_flow_tbl_insert(&dp->table, flow);
+   error = ovs_flow_tbl_insert(&dp->table, flow, &mask);
+   if (error) {
+   acts = NULL;
+   goto err_flow_free;
+   }
 
reply = ovs_flow_cmd_build_info(flow, dp, info->snd_portid,
info->snd_seq, 
OVS_FLOW_CMD_NEW);
@@ -1250,7 +1237,7 @@ err_destroy_ports_array:
 err_destroy_percpu:
free_percpu(dp->stats_percpu);
 err_destroy_table:
-   ovs_flow_tbl_destroy(&dp->table, false);
+   ovs_flow_tbl_destroy(&dp->table);
 err_free_dp:
release_net(ovs_dp_get_net(dp));
kfree(dp);
diff --git a/datapath/flow_table.c b/datapath/flow_table.c
index 0836ec2..b983669 100644
--- a/datapath/flow_table.c
+++ b/datapath/flow_table.c
@@ -147,12 +147,36 @@ static void rcu_free_flow_callback(struct rcu_head *rcu)
__flow_free(flow);
 }
 
+static void rcu_free_sw_flow_mask_cb(struct rcu_head *rcu)
+{
+   struct sw_flow_mask *mask = container_of(rcu, struct sw_flow_mask, rcu);
+
+   kfree(mask);
+}
+
+static void flow_mask_del_ref(struct sw_flow_mask *mask, bool deferred)
+{
+   if (!mask)
+   return;
+
+   BUG_ON(!mask->ref_count);
+   mask->ref_count--;
+
+   if (!mask->ref_count) {
+   list_del_rcu(&mask->list);
+   if (deferred)
+   call_rcu(&mask->rcu, rcu_free_sw_flow_mask_cb);
+   else
+   kfree(mask);
+   }
+}
+
 void ovs_flow_free(struct sw_flow *flow, bool deferred)
 {
if (!flow)
return;
 
-   ovs_sw_flow_mask_del_ref(flow->mask, deferred);
+   flow_mask_del_ref(flow->mask, deferred);
 
if (deferred)
call_rcu(&flow->rcu, rcu_free_flow_callback);
@@ -244,11 +268,11 @@ static void __ovs_flow_tbl_destroy(struct hash_table 
*table, bool deferred)
__flow_tbl_destroy(table);
 }
 
-void ovs_flow_tbl_destroy(struct flow_table *table, bool deferred)
+void ovs_flow_tbl_destroy(struct flow_table *table)
 {
struct hash_table *htable = ovsl_dereference(table->htable);
 
-   __ovs_flow_tbl_destroy(htable, deferred);
+   __ovs_flow_tbl_destroy(htable, false);
 }
 
 struct sw_flow *ovs_flow_tbl_dump_next(struct hash_table *table,
@@ -325,7 +349,7 @@ static struct hash_table *__flow_tbl_rehash(struct 
hash_table *table,
 
new_htable = __flow_tbl_alloc(n_buckets);
if (!new_htable)
-   return ERR_PTR(-ENOMEM);
+   return NULL;
 
flow_table_copy_flows(table, new_htable);
 
@@ -465,31 +489,6 @@ static struct hash_table *flow_tbl_expand(struct 
hash_table *table)
ret

[ovs-dev] [PATCH v3 4/4] datapath: Scalable locking.

[Pravin B Shelar]

Following patch breaks down ovs_mutex into multiple locks.  This
patch specifically targets flow-install.  By breaking down
ovs-locking parallel flow-installs are possible.

Signed-off-by: Pravin B Shelar 
---
v3:
Handle dup flow in flow insert.
v2:
get rid of mask_list lock.
---
 datapath/datapath.c   |   90 +---
 datapath/datapath.h   |7 +-
 datapath/dp_notify.c  |4 +-
 datapath/flow.h   |3 +-
 datapath/flow_table.c |  285 +++--
 datapath/flow_table.h |6 +-
 6 files changed, 244 insertions(+), 151 deletions(-)

diff --git a/datapath/datapath.c b/datapath/datapath.c
index 78f18bc..4d7c816 100644
--- a/datapath/datapath.c
+++ b/datapath/datapath.c
@@ -87,23 +87,33 @@ static void ovs_notify(struct sk_buff *skb, struct 
genl_info *info,
  * The RTNL lock nests inside ovs_mutex.
  */
 
-static DEFINE_MUTEX(ovs_mutex);
+static DECLARE_RWSEM(ovs_sem);
 
-void ovs_lock(void)
+void ovs_wr_lock(void)
 {
-   mutex_lock(&ovs_mutex);
+   down_write(&ovs_sem);
 }
 
-void ovs_unlock(void)
+void ovs_wr_unlock(void)
 {
-   mutex_unlock(&ovs_mutex);
+   up_write(&ovs_sem);
+}
+
+void ovs_rd_lock(void)
+{
+   down_read(&ovs_sem);
+}
+
+void ovs_rd_unlock(void)
+{
+   up_read(&ovs_sem);
 }
 
 #ifdef CONFIG_LOCKDEP
 int lockdep_ovsl_is_held(void)
 {
if (debug_locks)
-   return lockdep_is_held(&ovs_mutex);
+   return lockdep_is_held(&ovs_sem);
else
return 1;
 }
@@ -539,13 +549,13 @@ static int ovs_packet_cmd_execute(struct sk_buff *skb, 
struct genl_info *info)
local_bh_enable();
rcu_read_unlock();
 
-   ovs_flow_free(flow, false);
+   ovs_flow_free(&dp->table, flow, false);
return err;
 
 err_unlock:
rcu_read_unlock();
 err_flow_free:
-   ovs_flow_free(flow, false);
+   ovs_flow_free(&dp->table, flow, false);
 err_kfree_skb:
kfree_skb(packet);
 err:
@@ -790,7 +800,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
goto error;
}
 
-   ovs_lock();
+   ovs_rd_lock();
dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
error = -ENODEV;
if (!dp)
@@ -862,7 +872,7 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
spin_unlock_bh(&flow->lock);
}
}
-   ovs_unlock();
+   ovs_rd_unlock();
 
if (!IS_ERR(reply))
ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
@@ -872,9 +882,9 @@ static int ovs_flow_cmd_new_or_set(struct sk_buff *skb, 
struct genl_info *info)
return 0;
 
 err_flow_free:
-   ovs_flow_free(flow, false);
+   ovs_flow_free(&dp->table, flow, false);
 err_unlock_ovs:
-   ovs_unlock();
+   ovs_rd_unlock();
 err_kfree:
kfree(acts);
 error:
@@ -902,7 +912,7 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct 
genl_info *info)
if (err)
return err;
 
-   ovs_lock();
+   ovs_rd_lock();
dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
if (!dp) {
err = -ENODEV;
@@ -927,10 +937,10 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, struct 
genl_info *info)
goto unlock;
}
 
-   ovs_unlock();
+   ovs_rd_unlock();
return genlmsg_reply(reply, info);
 unlock:
-   ovs_unlock();
+   ovs_rd_unlock();
return err;
 }
 
@@ -945,7 +955,7 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct 
genl_info *info)
struct sw_flow_match match;
int err;
 
-   ovs_lock();
+   ovs_rd_lock();
dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
if (!dp) {
err = -ENODEV;
@@ -985,13 +995,13 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct 
genl_info *info)
 info->snd_seq, 0, OVS_FLOW_CMD_DEL);
BUG_ON(err < 0);
 
-   ovs_flow_free(flow, true);
-   ovs_unlock();
+   ovs_flow_free(&dp->table, flow, true);
+   ovs_rd_unlock();
 
ovs_notify(reply, info, &ovs_dp_flow_multicast_group);
return 0;
 unlock:
-   ovs_unlock();
+   ovs_rd_unlock();
return err;
 }
 
@@ -1169,7 +1179,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct 
genl_info *info)
if (!a[OVS_DP_ATTR_NAME] || !a[OVS_DP_ATTR_UPCALL_PID])
goto err;
 
-   ovs_lock();
+   ovs_wr_lock();
 
err = -ENOMEM;
dp = kzalloc(sizeof(*dp), GFP_KERNEL);
@@ -1225,7 +1235,7 @@ static int ovs_dp_cmd_new(struct sk_buff *skb, struct 
genl_info *info)
ovs_net = net_generic(ovs_dp_get_net(dp), ovs_net_id);
list_add_tail_rcu(&dp->list_node, &ovs_net->dps);
 
-   ovs_unlock();
+   ovs_wr_unlock();
 
ovs_notify(reply, info, &ovs_dp_datapath_multicast_group);
return 0;
@@ -1242,7 +1252,7 @@ err_

Re: [ovs-dev] [PATCH v2 2/4] datapath: Move mega-flow list out of rehashing struct.

[Jesse Gross]

This on the previous version of the patch since I had already started
writing it before you sent out the new version:

On Thu, Sep 26, 2013 at 9:01 AM, Pravin B Shelar  wrote:
> ovs-flow rehash does not touch mega flow list. Following patch
> moves it dp struct datapath.  Avoid one extra indirection for
> accessing mega-flow list head on every packet receive.

Does this actually reduce the number of cache misses? When processing
packets we don't touch the mask list without also touching the
buckets, which still require a dereference.

> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 13002b0..ff572ac 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1010,10 +969,9 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, struct 
> genl_info *info)
> if (err)
> goto unlock;
>
> -   table = ovsl_dereference(dp->table);
> -   flow = ovs_flow_lookup_unmasked_key(table, &match);
> -   if (!flow) {
> -   err = -ENOENT;
> +   flow = ovs_flow_lookup_unmasked_key(&dp->table, &match);
> +   if (IS_ERR(flow)) {
> +   err = PTR_ERR(flow);

This looks suspicious to me since I think
ovs_flow_lookup_unmasked_key() returns either a flow or NULL.

>  static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback 
> *cb)
>  {
> struct ovs_header *ovs_header = genlmsg_data(nlmsg_data(cb->nlh));
> +   struct hash_table *htable;
> struct datapath *dp;
> -   struct flow_table *table;
>
> rcu_read_lock();
> dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
> @@ -1051,15 +1009,15 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, 
> struct netlink_callback *cb)
> rcu_read_unlock();
> return -ENODEV;
> }
> +   htable = rcu_dereference(dp->table.htable);
>
> -   table = rcu_dereference(dp->table);
> for (;;) {
> struct sw_flow *flow;
> u32 bucket, obj;
>
> bucket = cb->args[0];
> obj = cb->args[1];
> -   flow = ovs_flow_dump_next(table, &bucket, &obj);
> +   flow = ovs_flow_dump_next(htable, &bucket, &obj);

Do we need to pull out htable here? It seems like it would be cleaner
if ovs_flow_dump_next() was able to get it directly without needing to
expose the internals like other places.

> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
> index cfabc44..f7655fa 100644
> --- a/datapath/flow_table.c
> +++ b/datapath/flow_table.c
> -static void __flow_tbl_destroy(struct flow_table *table)
> +static void __flow_tbl_destroy(struct hash_table *table)

We might want to rename these functions now that they are
allocating/freeing hash_tables, rather than flow tables.

> diff --git a/datapath/flow_table.h b/datapath/flow_table.h
> index 619ead6..6c3355e 100644
> --- a/datapath/flow_table.h
> +++ b/datapath/flow_table.h
> -struct flow_table {
> +struct hash_table {

hash_table is a pretty generic name - is there something more
descriptive that we can use?

> struct flex_array *buckets;
> unsigned int count, n_buckets;
> struct rcu_head rcu;
> -   struct list_head *mask_list;
> int node_ver;
> u32 hash_seed;
> bool keep_flows;
>  };

Are there any other fields that we can move into struct flow_table,
such as count or keep_flows?

> @@ -97,4 +90,5 @@ struct sw_flow_mask *ovs_sw_flow_mask_find(const struct 
> flow_table *,
>  void ovs_flow_key_mask(struct sw_flow_key *dst, const struct sw_flow_key 
> *src,
>const struct sw_flow_mask *mask);
>
> +int ovs_flush_flows(struct flow_table *flow_table);

Can we group this with some related functions - maybe the insert and
delete ones?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH V2 1/3] ofproto-dpif: Move send_packet() to ofproto-dpif-xlate module.

[Ethan Jackson]

I think this is on the right track, just some minor comments.

You aren't planning to have anyone call xlate_actions_unsafe() in
future patches right?  If that's true I'd rather keep the public API
the same and do the safe/unsafe split internally.  I.E:

Rename xlate_actions_safe() to xlate_actions().

Remove xlate_actions_unsafe() entirely, and simply call
xlate_actions__() directly when necessary.  Alternatively you could
rename xlate_actions__() xlate_actions_unsafe() instead, that's a
matter of personal preference.

I doubt it matters much in practice, but I'd prefer we release the
xlate_rwlock before we call dpif_execute() in case that takes a long
time.  You don't need the xlate_rwlock to call xlate_out_uninit(), so
it should be a simple matter of moving the call before the
dpif_execute() call.

Along those same lines, there's a lot of working setting up the xin
which could be pulled out of the critical section.  We could do the
flow_extract() beforehand for example.  Also initializing the the
output ofpact.

Are you planning to shove a lock around ofproto->stats?  Updating it
in ofproto_dpif_send_packet() isn't thread safe.

Thanks,
Ethan


On Fri, Sep 27, 2013 at 3:39 PM, Alex Wang  wrote:
> This commit moves the main logic of send_packet() function into
> the ofproto-dpif-xlate module.  Also, the xlate_actions() function
> in ofproto-dpif-xlate module is adjusted to guarantee thread safety.
>
> Signed-off-by: Alex Wang 
> ---
>
> v1 -> v2:
> - change xlate_actions to a thread safe version and thread unsafe version
>
> ---
>  ofproto/ofproto-dpif-upcall.c |4 +-
>  ofproto/ofproto-dpif-xlate.c  |   85 
> +
>  ofproto/ofproto-dpif-xlate.h  |5 ++-
>  ofproto/ofproto-dpif.c|   71 --
>  ofproto/ofproto-dpif.h|1 +
>  5 files changed, 101 insertions(+), 65 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
> index 180b87e..33c75b7 100644
> --- a/ofproto/ofproto-dpif-upcall.c
> +++ b/ofproto/ofproto-dpif-upcall.c
> @@ -230,7 +230,7 @@ udpif_wait(struct udpif *udpif)
>  }
>
>  /* Notifies 'udpif' that something changed which may render previous
> - * xlate_actions() results invalid. */
> + * xlate_actions_safe() results invalid. */
>  void
>  udpif_revalidate(struct udpif *udpif)
>  {
> @@ -731,7 +731,7 @@ handle_miss_upcalls(struct udpif *udpif, struct list 
> *upcalls)
>miss->stats.tcp_flags, NULL);
>  xin.may_learn = true;
>  xin.resubmit_stats = &miss->stats;
> -xlate_actions(&xin, &miss->xout);
> +xlate_actions_safe(&xin, &miss->xout);
>  flow_wildcards_or(&miss->xout.wc, &miss->xout.wc, &wc);
>  rule_dpif_unref(rule);
>  }
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index a5b6814..020588f 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -192,6 +192,8 @@ static struct hmap xports = HMAP_INITIALIZER(&xports);
>  static bool may_receive(const struct xport *, struct xlate_ctx *);
>  static void do_xlate_actions(const struct ofpact *, size_t ofpacts_len,
>   struct xlate_ctx *);
> +static void xlate_actions__(struct xlate_in *, struct xlate_out *)
> +OVS_REQ_RDLOCK(xlate_rwlock);
>  static void xlate_normal(struct xlate_ctx *);
>  static void xlate_report(struct xlate_ctx *, const char *);
>  static void xlate_table_action(struct xlate_ctx *, ofp_port_t in_port,
> @@ -2434,7 +2436,7 @@ xlate_actions_for_side_effects(struct xlate_in *xin)
>  {
>  struct xlate_out xout;
>
> -xlate_actions(xin, &xout);
> +xlate_actions_safe(xin, &xout);
>  xlate_out_uninit(&xout);
>  }
>
> @@ -2515,13 +2517,31 @@ actions_output_to_local_port(const struct xlate_ctx 
> *ctx)
>  return false;
>  }
>
> +/* Thread safe call to xlate_actions__(). */
> +void
> +xlate_actions_safe(struct xlate_in *xin, struct xlate_out *xout)
> +{
> +ovs_rwlock_rdlock(&xlate_rwlock);
> +xlate_actions__(xin, xout);
> +ovs_rwlock_unlock(&xlate_rwlock);
> +}
> +
> +/* Thread unsafe call to xlate_actions__().
> + * Caller must acquire rdlock first. */
> +void
> +xlate_actions_unsafe(struct xlate_in *xin, struct xlate_out *xout)
> +{
> +xlate_actions__(xin, xout);
> +}
> +
>  /* Translates the 'ofpacts_len' bytes of "struct ofpacts" starting at 
> 'ofpacts'
>   * into datapath actions in 'odp_actions', using 'ctx'.
>   *
>   * The caller must take responsibility for eventually freeing 'xout', with
>   * xlate_out_uninit(). */
> -void
> -xlate_actions(struct xlate_in *xin, struct xlate_out *xout)
> +static void
> +xlate_actions__(struct xlate_in *xin, struct xlate_out *xout)
> +OVS_REQ_RDLOCK(xlate_rwlock)
>  {
>  struct flow_wildcards *wc = &xout->wc;
>  struct flow *flow = &xin->flow;
> @@ -2537,8 +2557,6 @@ xlate_actions(struct xlate_in *xin, struct xlate_out 
> *xout)
>
>  COVERAGE_INC(xl

[ovs-dev] [PATCH 1/2] bridge: Let ofprotos run once before reporting configuration complete.

[Ben Pfaff]

Occasionally in the unit tests the following race can happen:

   1. ovs-vsctl updates database
   2. ovs-vswitchd reconfigures, notifies ovs-vsctl that it is complete
   3. ovs-appctl ofproto/trace fails to see newly added port
   4. ovs-vswitchd main loop calls ofproto's ->type_run(), making the
  new port visible to translation.

This race may be seen in the failures of tests 5 and 624 here:
https://launchpadlibrarian.net/151884888/buildlog_ubuntu-precise-amd64.openvswitch_2.0~201309300804-1ppa1~precise_FAILEDTOBUILD.txt.gz

Reported-by: Vasiliy Tolstov 
Signed-off-by: Ben Pfaff 
---
 AUTHORS   |1 +
 vswitchd/bridge.c |   46 --
 2 files changed, 33 insertions(+), 14 deletions(-)

diff --git a/AUTHORS b/AUTHORS
index af34bfe..29f44e2 100644
--- a/AUTHORS
+++ b/AUTHORS
@@ -214,6 +214,7 @@ Takayuki HAMA   t-h...@cb.jp.nec.com
 Teemu Koponen   kopo...@nicira.com
 Timothy Chentc...@nicira.com
 Valentin Budvalen...@hackaserver.com
+Vasiliy Tolstov v.tols...@selfip.ru
 Vishal Swarankarvishal.swarn...@gmail.com
 Vjekoslav Brajkovic bal...@cs.washington.edu
 Voravit T.  vora...@kth.se
diff --git a/vswitchd/bridge.c b/vswitchd/bridge.c
index b3ca42b..11a9408 100644
--- a/vswitchd/bridge.c
+++ b/vswitchd/bridge.c
@@ -186,6 +186,7 @@ static long long int iface_stats_timer = LLONG_MIN;
 static bool reconfiguring = false;
 
 static void add_del_bridges(const struct ovsrec_open_vswitch *);
+static void bridge_run__(void);
 static void bridge_update_ofprotos(void);
 static void bridge_create(const struct ovsrec_bridge *);
 static void bridge_destroy(struct bridge *);
@@ -645,6 +646,15 @@ bridge_reconfigure_continue(const struct 
ovsrec_open_vswitch *ovs_cfg)
 }
 free(managers);
 
+/* The ofproto-dpif provider does some final reconfiguration in its
+ * ->type_run() function.  We have to call it before notifying the database
+ * client that reconfiguration is complete, otherwise there is a very
+ * narrow race window in which e.g. ofproto/trace will not recognize the
+ * new configuration (sometimes this causes unit test failures). */
+if (done) {
+bridge_run__();
+}
+
 return done;
 }
 
@@ -2315,14 +2325,33 @@ bridge_run_fast(void)
 }
 }
 
+static void
+bridge_run__(void)
+{
+struct bridge *br;
+struct sset types;
+const char *type;
+
+/* Let each datapath type do the work that it needs to do. */
+sset_init(&types);
+ofproto_enumerate_types(&types);
+SSET_FOR_EACH (type, &types) {
+ofproto_type_run(type);
+}
+sset_destroy(&types);
+
+/* Let each bridge do the work that it needs to do. */
+HMAP_FOR_EACH (br, node, &all_bridges) {
+ofproto_run(br->ofproto);
+}
+}
+
 void
 bridge_run(void)
 {
 static struct ovsrec_open_vswitch null_cfg;
 const struct ovsrec_open_vswitch *cfg;
 struct ovsdb_idl_txn *reconf_txn = NULL;
-struct sset types;
-const char *type;
 
 bool vlan_splinters_changed;
 struct bridge *br;
@@ -2368,18 +2397,7 @@ bridge_run(void)
 "flow-restore-wait", false));
 }
 
-/* Let each datapath type do the work that it needs to do. */
-sset_init(&types);
-ofproto_enumerate_types(&types);
-SSET_FOR_EACH (type, &types) {
-ofproto_type_run(type);
-}
-sset_destroy(&types);
-
-/* Let each bridge do the work that it needs to do. */
-HMAP_FOR_EACH (br, node, &all_bridges) {
-ofproto_run(br->ofproto);
-}
+bridge_run__();
 
 /* Re-configure SSL.  We do this on every trip through the main loop,
  * instead of just when the database changes, because the contents of the
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] latest git from branch-2.0 failed to build

[Ben Pfaff]

On Mon, Sep 30, 2013 at 08:50:50AM +0400, Vasiliy Tolstov wrote:
> Hi, all. I'm try to build deb package for openvswitch 2.0.
> Now build fails:
> https://launchpadlibrarian.net/151884888/buildlog_ubuntu-precise-amd64.openvswitch_2.0~201309300804-1ppa1~precise_FAILEDTOBUILD.txt.gz

Thank you for the report.  I sent out fixes, starting here:
http://openvswitch.org/pipermail/dev/2013-September/032320.html
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/2] lacp: Give LACP a moment to initialize before testing its state, in tests.

[Ben Pfaff]

These tests configured LACP and then immediately dumped out its state.
Most of the time, this worked, but there was a brief race window in which
the "negotiated" flag could be missing because this took one pass through
the main loop.  This fixes the problem.

This race may be seen in the failures of tests 11 and 12 here:
https://launchpadlibrarian.net/151884888/buildlog_ubuntu-precise-amd64.openvswitch_2.0~201309300804-1ppa1~precise_FAILEDTOBUILD.txt.gz

Reported-by: Vasiliy Tolstov 
Signed-off-by: Ben Pfaff 
---
 tests/lacp.at |   10 ++
 1 file changed, 10 insertions(+)

diff --git a/tests/lacp.at b/tests/lacp.at
index ffc8996..234cf4d 100644
--- a/tests/lacp.at
+++ b/tests/lacp.at
@@ -6,6 +6,11 @@ OVS_VSWITCHD_START([\
 set Port p1 lacp=active --\
 set Interface p1 type=dummy ])
 
+ovs-appctl time/stop
+ovs-appctl time/warp 100
+ovs-appctl time/warp 100
+ovs-appctl time/warp 100
+
 AT_CHECK([ovs-appctl lacp/show], [0], [dnl
  p1 
status: active negotiated
@@ -53,6 +58,11 @@ OVS_VSWITCHD_START([dnl
 other_config:lacp-port-priority=222 \
 other_config:lacp-aggregation-key= ])
 
+ovs-appctl time/stop
+ovs-appctl time/warp 100
+ovs-appctl time/warp 100
+ovs-appctl time/warp 100
+
 AT_CHECK([ovs-appctl lacp/show], [0], [stdout])
 AT_CHECK([sed -e 's/aggregation key:.*/aggregation key: /' < stdout], 
[0], [dnl
  bond 
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v2 2/4] datapath: Move mega-flow list out of rehashing struct.

[Pravin Shelar]

On Mon, Sep 30, 2013 at 1:16 PM, Jesse Gross  wrote:
> This on the previous version of the patch since I had already started
> writing it before you sent out the new version:
>
> On Thu, Sep 26, 2013 at 9:01 AM, Pravin B Shelar  wrote:
>> ovs-flow rehash does not touch mega flow list. Following patch
>> moves it dp struct datapath.  Avoid one extra indirection for
>> accessing mega-flow list head on every packet receive.
>
> Does this actually reduce the number of cache misses? When processing
> packets we don't touch the mask list without also touching the
> buckets, which still require a dereference.
>
It might reduce cache miss, depends on actual memory allocation. But
it does avoid indirection for accessing list_head.

>> diff --git a/datapath/datapath.c b/datapath/datapath.c
>> index 13002b0..ff572ac 100644
>> --- a/datapath/datapath.c
>> +++ b/datapath/datapath.c
>> @@ -1010,10 +969,9 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, 
>> struct genl_info *info)
>> if (err)
>> goto unlock;
>>
>> -   table = ovsl_dereference(dp->table);
>> -   flow = ovs_flow_lookup_unmasked_key(table, &match);
>> -   if (!flow) {
>> -   err = -ENOENT;
>> +   flow = ovs_flow_lookup_unmasked_key(&dp->table, &match);
>> +   if (IS_ERR(flow)) {
>> +   err = PTR_ERR(flow);
>
> This looks suspicious to me since I think
> ovs_flow_lookup_unmasked_key() returns either a flow or NULL.
>
right.

>>  static int ovs_flow_cmd_dump(struct sk_buff *skb, struct netlink_callback 
>> *cb)
>>  {
>> struct ovs_header *ovs_header = genlmsg_data(nlmsg_data(cb->nlh));
>> +   struct hash_table *htable;
>> struct datapath *dp;
>> -   struct flow_table *table;
>>
>> rcu_read_lock();
>> dp = get_dp(sock_net(skb->sk), ovs_header->dp_ifindex);
>> @@ -1051,15 +1009,15 @@ static int ovs_flow_cmd_dump(struct sk_buff *skb, 
>> struct netlink_callback *cb)
>> rcu_read_unlock();
>> return -ENODEV;
>> }
>> +   htable = rcu_dereference(dp->table.htable);
>>
>> -   table = rcu_dereference(dp->table);
>> for (;;) {
>> struct sw_flow *flow;
>> u32 bucket, obj;
>>
>> bucket = cb->args[0];
>> obj = cb->args[1];
>> -   flow = ovs_flow_dump_next(table, &bucket, &obj);
>> +   flow = ovs_flow_dump_next(htable, &bucket, &obj);
>
> Do we need to pull out htable here? It seems like it would be cleaner
> if ovs_flow_dump_next() was able to get it directly without needing to
> expose the internals like other places.
>
I thought abt that but this might give different result if
rehashing/expansion is going on in parallel.
so I decided to keep current behavior.

>> diff --git a/datapath/flow_table.c b/datapath/flow_table.c
>> index cfabc44..f7655fa 100644
>> --- a/datapath/flow_table.c
>> +++ b/datapath/flow_table.c
>> -static void __flow_tbl_destroy(struct flow_table *table)
>> +static void __flow_tbl_destroy(struct hash_table *table)
>
> We might want to rename these functions now that they are
> allocating/freeing hash_tables, rather than flow tables.
>
ok.

>> diff --git a/datapath/flow_table.h b/datapath/flow_table.h
>> index 619ead6..6c3355e 100644
>> --- a/datapath/flow_table.h
>> +++ b/datapath/flow_table.h
>> -struct flow_table {
>> +struct hash_table {
>
> hash_table is a pretty generic name - is there something more
> descriptive that we can use?
>
>> struct flex_array *buckets;
>> unsigned int count, n_buckets;
>> struct rcu_head rcu;
>> -   struct list_head *mask_list;
>> int node_ver;
>> u32 hash_seed;
>> bool keep_flows;
>>  };
>
> Are there any other fields that we can move into struct flow_table,
> such as count or keep_flows?
>
I can move count to flow_table. keep_flows is related to hash table so
I will keep it as is.

>> @@ -97,4 +90,5 @@ struct sw_flow_mask *ovs_sw_flow_mask_find(const struct 
>> flow_table *,
>>  void ovs_flow_key_mask(struct sw_flow_key *dst, const struct sw_flow_key 
>> *src,
>>const struct sw_flow_mask *mask);
>>
>> +int ovs_flush_flows(struct flow_table *flow_table);
>
> Can we group this with some related functions - maybe the insert and
> delete ones?
yes, it is done in v3.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH V2 1/3] ofproto-dpif: Move send_packet() to ofproto-dpif-xlate module.

[Alex Wang]

On Mon, Sep 30, 2013 at 1:21 PM, Ethan Jackson  wrote:

> I think this is on the right track, just some minor comments.
>
> You aren't planning to have anyone call xlate_actions_unsafe() in
> future patches right?  If that's true I'd rather keep the public API
> the same and do the safe/unsafe split internally.  I.E:
>
> Rename xlate_actions_safe() to xlate_actions().
>
> Remove xlate_actions_unsafe() entirely, and simply call
> xlate_actions__() directly when necessary.  Alternatively you could
> rename xlate_actions__() xlate_actions_unsafe() instead, that's a
> matter of personal preference.
>


This makes sense, I'll adjust accordingly,



> I doubt it matters much in practice, but I'd prefer we release the
> xlate_rwlock before we call dpif_execute() in case that takes a long
> time.  You don't need the xlate_rwlock to call xlate_out_uninit(), so
> it should be a simple matter of moving the call before the
> dpif_execute() call.
>
> Along those same lines, there's a lot of working setting up the xin
> which could be pulled out of the critical section.  We could do the
> flow_extract() beforehand for example.  Also initializing the the
> output ofpact.
>


I'll shorten the critical section.


Are you planning to shove a lock around ofproto->stats?  Updating it
> in ofproto_dpif_send_packet() isn't thread safe.
>


Yes, the lock is added in patch 2/3
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3 3/4] datapath: Simplify mega-flow APIs.

[Jesse Gross]

On Mon, Sep 30, 2013 at 1:01 PM, Pravin B Shelar  wrote:
> Hides mega-flow implementation in flow_table.c rather than
> datapath.c.  This also helps next patch.
>
> 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] [PATCH V2 2/3] ofproto-dpif-monitor: Add ofproto-dpif-monitor module.

[Ethan Jackson]

On the right track, mostly nits at this point.

I'd prefer we simply copied the hw_addr around instead of maintaining
a pointer to the same data in multiple threads.  It's only 6 bytes, so
it shouldn't be too expensive.

Let's rename ofproto_dpif_monitor_mport_update() =>
ofproto_dpif_monitor_port_update().  The rest of the code shouldn't
need to know anything about mports.

In xlate_ofport_set() could you just get the hw_addr from the netdev
instead of passing it as another argument?

We could ditch the update_monitor bool pretty easily by doing
something like the following:

if (xport->bfd != bfd || xport->cfm != cfm) {
bfd_unref()
cfm_unref()
xport->bfd = bfd
xport->cfm = cfm
port_update()
}

Less variables means less wrong variables.

I think it'd be cleaner if the stats mutex change was pulled into it's
own separate patch before this one and the previous.

I don't totally get the unit test changes.  Could you explain them?
Is there some simpler way to achieve what you're going for?  Why are
they needed?

Thanks,
Ethan


On Fri, Sep 27, 2013 at 3:40 PM, Alex Wang  wrote:
> This commit adds a new module ofproto-dpif-monitor in ofproto
> directory.  This module is in charge of executing the periodic
> functions of monitoring code (e.g. bfd and cfm).
>
> Signed-off-by: Alex Wang 
> ---
>
> v1 -> v2:
> - ditch the monitor struct in the monitor module.
> - add monitor_rwlock in the monitor module.
> - use pointer to ofport_dpif struct as hash.
> - add the bfd/cfm reference to mport.
> - make ofproto.stats update thread safe in ofproto-dpif.c.
>
> ---
>  ofproto/automake.mk|2 +
>  ofproto/ofproto-dpif-monitor.c |  201 
> 
>  ofproto/ofproto-dpif-monitor.h |   33 +++
>  ofproto/ofproto-dpif-xlate.c   |   20 +++-
>  ofproto/ofproto-dpif-xlate.h   |5 +-
>  ofproto/ofproto-dpif.c |   66 +++--
>  tests/bfd.at   |   23 +++--
>  7 files changed, 286 insertions(+), 64 deletions(-)
>  create mode 100644 ofproto/ofproto-dpif-monitor.c
>  create mode 100644 ofproto/ofproto-dpif-monitor.h
>
> diff --git a/ofproto/automake.mk b/ofproto/automake.mk
> index 47ca1b8..432f083 100644
> --- a/ofproto/automake.mk
> +++ b/ofproto/automake.mk
> @@ -28,6 +28,8 @@ ofproto_libofproto_a_SOURCES = \
> ofproto/ofproto-dpif-ipfix.h \
> ofproto/ofproto-dpif-mirror.c \
> ofproto/ofproto-dpif-mirror.h \
> +   ofproto/ofproto-dpif-monitor.c \
> +   ofproto/ofproto-dpif-monitor.h \
> ofproto/ofproto-dpif-sflow.c \
> ofproto/ofproto-dpif-sflow.h \
> ofproto/ofproto-dpif-upcall.c \
> diff --git a/ofproto/ofproto-dpif-monitor.c b/ofproto/ofproto-dpif-monitor.c
> new file mode 100644
> index 000..97c6e40
> --- /dev/null
> +++ b/ofproto/ofproto-dpif-monitor.c
> @@ -0,0 +1,201 @@
> +/*
> + * Copyright (c) 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.
> + * You may obtain a copy of the License at:
> + *
> + * http://www.apache.org/licenses/LICENSE-2.0
> + *
> + * Unless required by applicable law or agreed to in writing, software
> + * distributed under the License is distributed on an "AS IS" BASIS,
> + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
> + * See the License for the specific language governing permissions and
> + * limitations under the License.
> + */
> +
> +#include 
> +#include "ofproto-dpif-monitor.h"
> +
> +#include "bfd.h"
> +#include "cfm.h"
> +#include "hash.h"
> +#include "hmap.h"
> +#include "ofpbuf.h"
> +#include "ofproto-dpif.h"
> +#include "util.h"
> +#include "vlog.h"
> +
> +/* Monitored port.  It contains references to ofport, bfd, cfm structs. */
> +struct mport {
> +struct hmap_node hmap_node;   /* In monitor's hmap. */
> +const struct ofport_dpif *ofport; /* The corresponding ofport. */
> +
> +struct cfm *cfm;  /* Reference to cfm. */
> +struct bfd *bfd;  /* Reference to bfd. */
> +uint8_t *hw_addr; /* Hardware address. */
> +};
> +
> +/* hmap that contains all port monitors. */
> +static struct hmap monitor_hmap = HMAP_INITIALIZER(&monitor_hmap);
> +
> +static struct ovs_rwlock monitor_rwlock = OVS_RWLOCK_INITIALIZER;
> +
> +static void mport_register(const struct ofport_dpif *, struct bfd *,
> +   struct cfm *, uint8_t *)
> +OVS_REQ_WRLOCK(monitor_rwlock);
> +static void mport_unregister(const struct ofport_dpif *)
> +OVS_REQ_WRLOCK(monitor_rwlock);
> +static void mport_update(struct mport *, struct bfd *, struct cfm *, uint8_t 
> *)
> +OVS_REQ_WRLOCK(monitor_rwlock);
> +static struct mport *mport_find(const struct ofport_dpif *)
> +OVS_REQ_WRLOCK(monitor_rwlock);
> +
> +/* Tries finding and returning the 'mport' from the monitor's hash map.
> + * If there is n

Re: [ovs-dev] [PATCH] ovs-dpctl, ofproto/trace: Show and handle the in_port name in flows.

[Ben Pfaff]

On Wed, Sep 25, 2013 at 12:55:43AM -0700, Gurucharan Shetty wrote:
> With this commit, whenever the verbosity is enabled with '-m'
> option, the ovs-dpctl dump-flows command will display the flows with
> in_port field showing the name instead of a port number.
> 
> Conversely, one can also use a name in the in_port field with del-flow,
> add-flow and mod-flow commands of ovs-dpctl. One should also be able
> to use the port name when supplying the datapath flow as an input
> to ofproto/trace command.
> 
> Signed-off-by: Gurucharan Shetty 

Does ofproto_unixctl_trace() leak its port_names simap?

Would you mind refining this comment to something like "A map from odp
port number to name." so that it's clear what the main direction of the
mapping is?
> +/* A map between odp port number and its name. */
> +struct odp_portno_names {

Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3 4/4] datapath: Scalable locking.

[Jesse Gross]

Some very quick high level things while I look at this:

On Mon, Sep 30, 2013 at 1:01 PM, Pravin B Shelar  wrote:
> Following patch breaks down ovs_mutex into multiple locks.  This
> patch specifically targets flow-install.  By breaking down
> ovs-locking parallel flow-installs are possible.
>
> Signed-off-by: Pravin B Shelar 

I think we should have some additional documentation on the new
locking scheme, both in the commit message and in the source code.
There's an existing comment in datapath.c that I believe is now
inaccurate.

I got a sparse warning with this patch:
  CHECK   /home/jgross/openvswitch/datapath/linux/flow_table.c
/home/jgross/openvswitch/datapath/linux/flow_table.c:364:12: warning:
context imbalance in 'tbl_insert' - wrong count at exit

It looks like a real issue to me if we hit the EEXIST error path.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 1/2] flow: Fill in ->l7 in flow_compose().

[Ben Pfaff]

flow_extract() fills in ->l7 but flow_compose() wasn't doing it, which
confused bfd_process_packet() when invoked via the ofproto/trace appctl
command.

Signed-off-by: Ben Pfaff 
---
 lib/flow.c |1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/flow.c b/lib/flow.c
index 0678c6f..7372e3b 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1032,6 +1032,7 @@ flow_compose(struct ofpbuf *b, const struct flow *flow)
 icmp->icmp_code = ntohs(flow->tp_dst);
 icmp->icmp_csum = csum(icmp, ICMP_HEADER_LEN);
 }
+b->l7 = ofpbuf_tail(b);
 }
 
 ip = b->l3;
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH 2/2] bfd: Improve log message.

[Ben Pfaff]

Signed-off-by: Ben Pfaff 
---
 lib/bfd.c |6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/lib/bfd.c b/lib/bfd.c
index 6c9e920..dbb0e13 100644
--- a/lib/bfd.c
+++ b/lib/bfd.c
@@ -625,8 +625,10 @@ bfd_process_packet(struct bfd *bfd, const struct flow 
*flow,
 
 msg = ofpbuf_at(p, (uint8_t *)p->l7 - (uint8_t *)p->data, BFD_PACKET_LEN);
 if (!msg) {
-VLOG_INFO_RL(&rl, "%s: Received unparseable BFD control message.",
- bfd->name);
+VLOG_INFO_RL(&rl, "%s: Received too-short BFD control message (only "
+ "%td bytes long, at least %d required).",
+ bfd->name, (uint8_t *) ofpbuf_tail(p) - (uint8_t *) p->l7,
+ BFD_PACKET_LEN);
 goto out;
 }
 
-- 
1.7.10.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] ovs-dpctl, ofproto/trace: Show and handle the in_port name in flows.

[Gurucharan Shetty]

On Mon, Sep 30, 2013 at 2:20 PM, Ben Pfaff  wrote:
> On Wed, Sep 25, 2013 at 12:55:43AM -0700, Gurucharan Shetty wrote:
>> With this commit, whenever the verbosity is enabled with '-m'
>> option, the ovs-dpctl dump-flows command will display the flows with
>> in_port field showing the name instead of a port number.
>>
>> Conversely, one can also use a name in the in_port field with del-flow,
>> add-flow and mod-flow commands of ovs-dpctl. One should also be able
>> to use the port name when supplying the datapath flow as an input
>> to ofproto/trace command.
>>
>> Signed-off-by: Gurucharan Shetty 
>
> Does ofproto_unixctl_trace() leak its port_names simap?
Yes, it does.

I plan to add the following incremental.
index 2e61515..43db1b8 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -5354,6 +5354,7 @@ ofproto_unixctl_trace(struct unixctl_conn *conn,
int argc, const char *argv[],
 ds_init(&result);
 ofpbuf_init(&odp_key, 0);
 ofpbuf_init(&odp_mask, 0);
+simap_init(&port_names);

 /* Handle "-generate" or a hex string as the last argument. */
 if (!strcmp(argv[argc - 1], "-generate")) {
@@ -5389,7 +5390,6 @@ ofproto_unixctl_trace(struct unixctl_conn *conn,
int argc, const char *argv[],
 backer = node->data;
 }
 }
-simap_init(&port_names);
 if (backer && backer->dpif) {
 struct dpif_port dpif_port;
 struct dpif_port_dump port_dump;
@@ -5459,6 +5459,7 @@ exit:
 ofpbuf_delete(packet);
 ofpbuf_uninit(&odp_key);
 ofpbuf_uninit(&odp_mask);
+simap_destroy(&port_names);
 }

>
> Would you mind refining this comment to something like "A map from odp
> port number to name." so that it's clear what the main direction of the
> mapping is?
Done.

>> +/* A map between odp port number and its name. */
>> +struct odp_portno_names {
>
> Acked-by: Ben Pfaff 
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH V2 3/3] ofproto-dpif-monitor: Move ofproto-dpif-monitor to a single thread.

[Ethan Jackson]

Why do we need the bfd->last_tx change?  Should it be in a different
patch with a commit message explaining it?

As discussed off list, the monitor_seq thing is a layering violation.
Instead we should have a separate patch which causes time warp to wake
up threads.

I don't really like the function name monitor_handler().  It's not
really handling anything in the same sense that the miss_handler is.
I think we should rename the function interface_monitor(), and
set_subprogram_name("interface_monitor") as well.

It's probably cleaner to take the monitor_rwlock once and hold it over
monitor_run() and monitor_wait().

I don't really like how we're handling the thread creation and
deletion here.   What if removed monitor_check and folded it's code
into mport_update() after we've made the register/unregister calls?
That way we could get rid of the rather add requirement to heap
allocate the tid.  Also it would make the API more straight forward.
One couldn't forget to start/stop the threads.

Theres a redundant "o" at the end of one of changed unit test lines.

Ethan

On Fri, Sep 27, 2013 at 3:41 PM, Alex Wang  wrote:
> This commit moves the ofproto-dpif-monitor module into a
> dedicated thread.
>
> Signed-off-by: Alex Wang 
> ---
>
> v1 -> v2:
> - re-adjust the code base on changes made to previous patches.
>
> ---
>  lib/bfd.c  |4 +-
>  lib/timeval.c  |   20 ++
>  lib/timeval.h  |4 ++
>  ofproto/ofproto-dpif-monitor.c |  149 
> +++-
>  ofproto/ofproto-dpif-monitor.h |7 +-
>  ofproto/ofproto-dpif.c |7 +-
>  tests/bfd.at   |   33 +
>  tests/ofproto-dpif.at  |   58 
>  8 files changed, 225 insertions(+), 57 deletions(-)
>
> diff --git a/lib/bfd.c b/lib/bfd.c
> index 6c9e920..c106983 100644
> --- a/lib/bfd.c
> +++ b/lib/bfd.c
> @@ -721,8 +721,10 @@ bfd_process_packet(struct bfd *bfd, const struct flow 
> *flow,
>  rmt_min_rx = MAX(ntohl(msg->min_rx) / 1000, 1);
>  if (bfd->rmt_min_rx != rmt_min_rx) {
>  bfd->rmt_min_rx = rmt_min_rx;
> -bfd_set_next_tx(bfd);
>  log_msg(VLL_INFO, msg, "New remote min_rx", bfd);
> +if (bfd->last_tx) {
> +bfd_set_next_tx(bfd);
> +}
>  }
>
>  bfd->rmt_min_tx = MAX(ntohl(msg->min_tx) / 1000, 1);
> diff --git a/lib/timeval.c b/lib/timeval.c
> index 223ed30..befba1c 100644
> --- a/lib/timeval.c
> +++ b/lib/timeval.c
> @@ -33,6 +33,7 @@
>  #include "hmap.h"
>  #include "ovs-thread.h"
>  #include "signals.h"
> +#include "seq.h"
>  #include "unixctl.h"
>  #include "util.h"
>  #include "vlog.h"
> @@ -57,6 +58,9 @@ static struct clock wall_clock;  /* CLOCK_REALTIME. */
>  /* The monotonic time at which the time module was initialized. */
>  static long long int boot_time;
>
> +/* Reference to the seq struct of monitor thread. */
> +static struct seq *monitor_seq;
> +
>  /* Monotonic time in milliseconds at which to die with SIGALRM (if not
>   * LLONG_MAX). */
>  static long long int deadline = LLONG_MAX;
> @@ -294,6 +298,18 @@ time_boot_msec(void)
>  return boot_time;
>  }
>
> +/* Sets monitor_seq to 'seq'. */
> +void
> +time_set_monitor_seq(struct seq *seq) {
> +monitor_seq = seq;
> +}
> +
> +/* Clears monitor_seq. */
> +void
> +time_clear_monitor_seq(void) {
> +monitor_seq = NULL;
> +}
> +
>  void
>  xgettimeofday(struct timeval *tv)
>  {
> @@ -509,6 +525,10 @@ timeval_warp_cb(struct unixctl_conn *conn,
>  ovs_mutex_lock(&monotonic_clock.mutex);
>  atomic_store(&monotonic_clock.slow_path, true);
>  timespec_add(&monotonic_clock.warp, &monotonic_clock.warp, &ts);
> +/* Changes 'monitor_seq' to wakeup monitor thread. */
> +if (monitor_seq) {
> +seq_change(monitor_seq);
> +}
>  ovs_mutex_unlock(&monotonic_clock.mutex);
>
>  unixctl_command_reply(conn, "warped");
> diff --git a/lib/timeval.h b/lib/timeval.h
> index 99b3af0..1273dd8 100644
> --- a/lib/timeval.h
> +++ b/lib/timeval.h
> @@ -27,6 +27,7 @@ extern "C" {
>
>  struct ds;
>  struct pollfd;
> +struct seq;
>  struct timespec;
>  struct timeval;
>
> @@ -69,6 +70,9 @@ int get_cpu_usage(void);
>
>  long long int time_boot_msec(void);
>
> +void time_set_monitor_seq(struct seq *);
> +void time_clear_monitor_seq(void);
> +
>  #ifdef  __cplusplus
>  }
>  #endif
> diff --git a/ofproto/ofproto-dpif-monitor.c b/ofproto/ofproto-dpif-monitor.c
> index 97c6e40..5847b08 100644
> --- a/ofproto/ofproto-dpif-monitor.c
> +++ b/ofproto/ofproto-dpif-monitor.c
> @@ -21,11 +21,18 @@
>  #include "cfm.h"
>  #include "hash.h"
>  #include "hmap.h"
> +#include "latch.h"
>  #include "ofpbuf.h"
>  #include "ofproto-dpif.h"
> +#include "ovs-thread.h"
> +#include "poll-loop.h"
> +#include "seq.h"
> +#include "timeval.h"
>  #include "util.h"
>  #include "vlog.h"
>
> +VLOG_DEFINE_THIS_MODULE(ofproto_dpif_monitor);
> +
>  /* Monitored port.  It contains references to ofport, bfd, cfm stru

Re: [ovs-dev] [PATCH] ovs-dpctl, ofproto/trace: Show and handle the in_port name in flows.

[Ben Pfaff]

On Mon, Sep 30, 2013 at 03:11:17PM -0700, Gurucharan Shetty wrote:
> On Mon, Sep 30, 2013 at 2:20 PM, Ben Pfaff  wrote:
> > On Wed, Sep 25, 2013 at 12:55:43AM -0700, Gurucharan Shetty wrote:
> >> With this commit, whenever the verbosity is enabled with '-m'
> >> option, the ovs-dpctl dump-flows command will display the flows with
> >> in_port field showing the name instead of a port number.
> >>
> >> Conversely, one can also use a name in the in_port field with del-flow,
> >> add-flow and mod-flow commands of ovs-dpctl. One should also be able
> >> to use the port name when supplying the datapath flow as an input
> >> to ofproto/trace command.
> >>
> >> Signed-off-by: Gurucharan Shetty 
> >
> > Does ofproto_unixctl_trace() leak its port_names simap?
> Yes, it does.
> 
> I plan to add the following incremental.

Thanks, that looks fine.

> > Would you mind refining this comment to something like "A map from odp
> > port number to name." so that it's clear what the main direction of the
> > mapping is?
> Done.

Thanks!
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3 1/4] datapath: Restructure datapath.c and flow.c

[Jesse Gross]

On Mon, Sep 30, 2013 at 1:01 PM, Pravin B Shelar  wrote:
> Over the time datapath.c and flow.c has became pretty large files.
> Following patch restructures functionality of component into three
> different components:
>
> flow.c: contains flow extract.
> flow_netlink.c: netlink flow api.
> flow_table.c: flow table api.
>
> Diffstat is showing wrong count. This patch mostly restructures code
> without changing logic.
>
> Signed-off-by: Pravin B Shelar 

I see some errors/warnings on this due to one of the renamed functions:
WARNING: "ovs_flow_tbl_dump_next"
[/home/jgross/openvswitch/datapath/linux/openvswitch.ko] undefined!

  CHECK   /home/jgross/openvswitch/datapath/linux/flow_table.c
/home/jgross/openvswitch/datapath/linux/flow_table.c:243:16: warning:
symbol 'ovs_flow_dump_next' was not declared. Should it be static?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3] ofproto-dpif: Move special upcall handling into ofproto-dpif-upcall.

[Ethan Jackson]

Thanks for the review, sorry it took me so long to merge it.

Ethan
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3] ofproto-dpif: Move special upcall handling into ofproto-dpif-upcall.

[Ben Pfaff]

On Mon, Sep 30, 2013 at 05:39:09PM -0700, Ethan Jackson wrote:
> Thanks for the review, sorry it took me so long to merge it.

No problem, I wouldn't care except that I'd told Yamamoto Takashi that
I'd apply his patch from last week after yours went in.  I'll take a
look at that next.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3 1/4] datapath: Restructure datapath.c and flow.c

[Jesse Gross]

On Mon, Sep 30, 2013 at 1:01 PM, Pravin B Shelar  wrote:
> diff --git a/datapath/datapath.c b/datapath/datapath.c
> index 4defcdb..7178513 100644
> --- a/datapath/datapath.c
> +++ b/datapath/datapath.c
> @@ -1428,12 +962,17 @@ static int ovs_flow_cmd_get(struct sk_buff *skb, 
> struct genl_info *info)
> }
>
> table = ovsl_dereference(dp->table);
> -   flow = ovs_flow_lookup_unmasked_key(table, &match);
> +   flow = ovs_flow_tbl_lookup(table, &key);
> if (!flow) {
> err = -ENOENT;
> goto unlock;
> }
>
> +   if (flow && (!ovs_flow_cmp_unmasked_key(flow, &match))) {
> +   err = -ENOENT;
> +   goto unlock;
> +   }

I think we probably could just make this into a single condition with:
if (!flow || (!ovs_flow_cmp_unmasked_key(flow, &match))) {
[...]

> @@ -1473,24 +1012,29 @@ static int ovs_flow_cmd_del(struct sk_buff *skb, 
> struct genl_info *info)
> }
>
> ovs_match_init(&match, &key, NULL);
> -   err = ovs_match_from_nlattrs(&match, a[OVS_FLOW_ATTR_KEY], NULL);
> +   err = ovs_nla_get_match(&match, a[OVS_FLOW_ATTR_KEY], NULL);
> if (err)
> goto unlock;
>
> table = ovsl_dereference(dp->table);
> -   flow = ovs_flow_lookup_unmasked_key(table, &match);
> +   flow = ovs_flow_tbl_lookup(table, &key);
> if (!flow) {
> err = -ENOENT;
> goto unlock;
> }
>
> +   if (flow && (!ovs_flow_cmp_unmasked_key(flow, &match))) {
> +   err = -ENOENT;
> +   goto unlock;
> +   }

And here as well.

> diff --git a/datapath/flow.h b/datapath/flow.h
> index 03eae03..c50c76f 100644
> --- a/datapath/flow.h
> +++ b/datapath/flow.h
>
> +#define MAX_ACTIONS_BUFSIZE(32 * 1024)

We might just want to put this in flow_netlink.c since I think that's
the only place where it is used.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 1/8] nsh: datapath support for network service headers

[Jesse Gross]

On Fri, Sep 20, 2013 at 1:04 AM, pritesh  wrote:
> This patch adds support for Network Service Headers (nsh) over VXLAN
> as mentioned in [1]. Here changes are made to datapath to add nsh
> headers whenever a vxlan port with destination port as 9030 is created.
> IANA port allocation for nsh over vxlan is yet to be done.

I'm pretty concerned about using a magic UDP port, especially since
one hasn't been allocated yet (we went through the same situation with
VXLAN itself). Is there a way that we can just insert/remove this
header on a flow basis?

> Signed-off-by: pritesh 

One other thing - would you mind using your full name in the signed-off-by line?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] ofproto-dpif: Compute the subfacet add/del rate using coverage counters.

[Ethan Jackson]

Both of these look good to me.  Would you please rebase them and send
them out.  Then I'll merge.

Ethan

On Tue, Sep 24, 2013 at 5:54 PM, Alex Wang  wrote:
> So far, the subfacet rates (e.g. add rate, del rate) are computed by
> exponential moving averaging function in ofproto-dpif.c.  This commit
> 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 |  101 
> 
>  tests/ofproto-dpif.at  |4 --
>  tests/tunnel.at|   26 ++---
>  3 files changed, 21 insertions(+), 110 deletions(-)
>
> diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> index 80874b8..5ddd71d 100644
> --- a/ofproto/ofproto-dpif.c
> +++ b/ofproto/ofproto-dpif.c
> @@ -72,6 +72,10 @@ 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(subfacet_create);
> +COVERAGE_DEFINE(subfacet_destroy);
>  COVERAGE_DEFINE(subfacet_install_fail);
>  COVERAGE_DEFINE(packet_in_overflow);
>  COVERAGE_DEFINE(flow_mod_overflow);
> @@ -437,20 +441,6 @@ struct dpif_backer {
>  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;
> -
>  /* Number of upcall handling threads. */
>  unsigned int n_handler_threads;
>  };
> @@ -459,7 +449,6 @@ struct dpif_backer {
>  static struct shash all_dpif_backers = SHASH_INITIALIZER(&all_dpif_backers);
>
>  static void drop_key_clear(struct dpif_backer *);
> -static void update_moving_averages(struct dpif_backer *backer);
>
>  struct ofproto_dpif {
>  struct hmap_node all_ofproto_dpifs_node; /* In 'all_ofproto_dpifs'. */
> @@ -1217,14 +1206,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;
>
> @@ -3739,8 +3720,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
> @@ -3923,6 +3902,7 @@ facet_create(const struct flow_miss *miss)
>  struct facet *facet;
>  struct match match;
>
> +COVERAGE_INC(facet_create);
>  facet = xzalloc(sizeof *facet);
>  facet->ofproto = miss->ofproto;
>  facet->used = miss->stats.used;
> @@ -3986,6 +3966,7 @@ facet_remove(struct facet *facet)
>  {
>  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. */
> @@ -4523,6 +4504,7 @@ 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;
> @@ -4535,7 +4517,6 @@ subfacet_create(struct facet *facet, struct flow_miss 
> *miss)
>  subfacet->path = SF_NOT_INSTALLED;
>  subfacet->backer = backer;
>
> -backer->subfacet_add_count++;
>  return subfacet;
>  }
>
> @@ -4545,11 +4526,8 @@ static void
>  subfacet_destroy__(struct subfacet *subfacet)
>  {
>  struct facet *facet = subfacet->facet;
> -struct ofproto_dpif *ofproto = facet->ofproto;
> -
> -/* Update ofproto stats before uninstall the subfacet. */
> -ofproto->backer->subfacet_del_count++;
>
> +COVERAGE_INC(subfacet

Re: [ovs-dev] [PATCH V2 2/3] ofproto-dpif-monitor: Add ofproto-dpif-monitor module.

[Alex Wang]

On Mon, Sep 30, 2013 at 2:09 PM, Ethan Jackson  wrote:

> On the right track, mostly nits at this point.
>
> I'd prefer we simply copied the hw_addr around instead of maintaining
> a pointer to the same data in multiple threads.  It's only 6 bytes, so
> it shouldn't be too expensive.
>


I see, I'll memcpy it,



> Let's rename ofproto_dpif_monitor_mport_update() =>
> ofproto_dpif_monitor_port_update().  The rest of the code shouldn't
> need to know anything about mports.
>


That's good point, thanks.



> In xlate_ofport_set() could you just get the hw_addr from the netdev
> instead of passing it as another argument?
>


Yes, I'll modify accordingly,



> We could ditch the update_monitor bool pretty easily by doing
> something like the following:
>
> if (xport->bfd != bfd || xport->cfm != cfm) {
> bfd_unref()
> cfm_unref()
> xport->bfd = bfd
> xport->cfm = cfm
> port_update()
> }
>
> Less variables means less wrong variables.
>


I still think that will make the code ckear.  So, I'm keeping it in my
following V3 patch and wait for your feedback.


I think it'd be cleaner if the stats mutex change was pulled into it's
> own separate patch before this one and the previous.
>


I'll use a separate patch,


I don't totally get the unit test changes.  Could you explain them?
> Is there some simpler way to achieve what you're going for?  Why are
> they needed?
>

As we discussed off list, I'll add explicit explanation, to this.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH 2/2] ofproto-dpif: Compute the subfacet add/del rate using coverage counters.

[Alex Wang]

Sure, I'll do that quickly,



On Mon, Sep 30, 2013 at 6:05 PM, Ethan Jackson  wrote:

> Both of these look good to me.  Would you please rebase them and send
> them out.  Then I'll merge.
>
> Ethan
>
> On Tue, Sep 24, 2013 at 5:54 PM, Alex Wang  wrote:
> > So far, the subfacet rates (e.g. add rate, del rate) are computed by
> > exponential moving averaging function in ofproto-dpif.c.  This commit
> > 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 |  101
> 
> >  tests/ofproto-dpif.at  |4 --
> >  tests/tunnel.at|   26 ++---
> >  3 files changed, 21 insertions(+), 110 deletions(-)
> >
> > diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
> > index 80874b8..5ddd71d 100644
> > --- a/ofproto/ofproto-dpif.c
> > +++ b/ofproto/ofproto-dpif.c
> > @@ -72,6 +72,10 @@ 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(subfacet_create);
> > +COVERAGE_DEFINE(subfacet_destroy);
> >  COVERAGE_DEFINE(subfacet_install_fail);
> >  COVERAGE_DEFINE(packet_in_overflow);
> >  COVERAGE_DEFINE(flow_mod_overflow);
> > @@ -437,20 +441,6 @@ struct dpif_backer {
> >  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;
> > -
> >  /* Number of upcall handling threads. */
> >  unsigned int n_handler_threads;
> >  };
> > @@ -459,7 +449,6 @@ struct dpif_backer {
> >  static struct shash all_dpif_backers =
> SHASH_INITIALIZER(&all_dpif_backers);
> >
> >  static void drop_key_clear(struct dpif_backer *);
> > -static void update_moving_averages(struct dpif_backer *backer);
> >
> >  struct ofproto_dpif {
> >  struct hmap_node all_ofproto_dpifs_node; /* In 'all_ofproto_dpifs'.
> */
> > @@ -1217,14 +1206,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;
> >
> > @@ -3739,8 +3720,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
> > @@ -3923,6 +3902,7 @@ facet_create(const struct flow_miss *miss)
> >  struct facet *facet;
> >  struct match match;
> >
> > +COVERAGE_INC(facet_create);
> >  facet = xzalloc(sizeof *facet);
> >  facet->ofproto = miss->ofproto;
> >  facet->used = miss->stats.used;
> > @@ -3986,6 +3966,7 @@ facet_remove(struct facet *facet)
> >  {
> >  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. */
> > @@ -4523,6 +4504,7 @@ 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;
> > @@ -4535,7 +4517,6 @@ subfacet_create(struct facet *facet, struct
> flow_miss *miss)
> >  subfacet->path = SF_NOT_INSTALLED;
> >  subfacet->backer = backer;
> >
> > -backer->subfacet_add_count++;
> >  return subfacet;
> >  }
> >
> > @@ -

[ovs-dev] [PATCH V2 1/2] coverage: Reimplement the "ovs-appctl coverage/show" command.

[Alex Wang]

This commit changes the "ovs-appctl coverage/show" command to show the
the averaged per-second rates for the last few seconds, the last minute
and the last hour, and the total counts of all of the coverage counters.

Signed-off-by: Alex Wang 

---

v1 -> v2:
- rebase to master.

---
 lib/coverage-unixctl.man |4 +-
 lib/coverage.c   |  113 --
 lib/coverage.h   |   17 ++-
 lib/timeval.c|1 +
 tests/ofproto-dpif.at|   34 ++
 5 files changed, 162 insertions(+), 7 deletions(-)

diff --git a/lib/coverage-unixctl.man b/lib/coverage-unixctl.man
index 9718894..8e5df81 100644
--- a/lib/coverage-unixctl.man
+++ b/lib/coverage-unixctl.man
@@ -8,4 +8,6 @@ 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 averaged per-second rates for the last few seconds, the
+last minute and the last hour, and the total counts of all of the
+coverage counters.
diff --git a/lib/coverage.c b/lib/coverage.c
index 23e2997..4364734 100644
--- a/lib/coverage.c
+++ b/lib/coverage.c
@@ -63,7 +63,14 @@ struct coverage_counter *coverage_counters[] = {
 
 static struct ovs_mutex coverage_mutex = OVS_MUTEX_INITIALIZER;
 
+static long long int coverage_run_time = LLONG_MIN;
+
+/* Index counter used to compute the moving average array's index. */
+static unsigned int idx_count = 0;
+
 static void coverage_read(struct svec *);
+static unsigned int coverage_array_sum(const unsigned int *arr,
+   const unsigned int len);
 
 static void
 coverage_unixctl_show(struct unixctl_conn *conn, int argc OVS_UNUSED,
@@ -206,6 +213,7 @@ coverage_log(void)
 static void
 coverage_read(struct svec *lines)
 {
+struct coverage_counter **c = coverage_counters;
 unsigned long long int *totals;
 size_t n_never_hit;
 uint32_t hash;
@@ -215,24 +223,37 @@ coverage_read(struct svec *lines)
 
 n_never_hit = 0;
 svec_add_nocopy(lines,
-xasprintf("Event coverage, hash=%08"PRIx32":", hash));
+xasprintf("Event coverage, avg rate over last: %d "
+  "seconds, last minute, last hour,  "
+  "hash=%08"PRIx32":",
+  COVERAGE_RUN_INTERVAL/1000, hash));
 
 totals = xmalloc(n_coverage_counters * sizeof *totals);
 ovs_mutex_lock(&coverage_mutex);
 for (i = 0; i < n_coverage_counters; i++) {
-totals[i] = coverage_counters[i]->total;
+totals[i] = c[i]->total;
 }
 ovs_mutex_unlock(&coverage_mutex);
 
 for (i = 0; i < n_coverage_counters; i++) {
 if (totals[i]) {
-svec_add_nocopy(lines, xasprintf("%-24s %9llu",
- coverage_counters[i]->name,
- totals[i]));
+/* Shows the averaged per-second rates for the last
+ * COVERAGE_RUN_INTERVAL interval, the last minute and
+ * the last hour. */
+svec_add_nocopy(lines,
+xasprintf("%-24s %5.1f/sec %9.3f/sec "
+  "%13.4f/sec   total: %llu",
+  c[i]->name,
+  (c[i]->min[(idx_count - 1) % MIN_AVG_LEN]
+   * 1000.0 / COVERAGE_RUN_INTERVAL),
+  coverage_array_sum(c[i]->min, MIN_AVG_LEN) / 60.0,
+  coverage_array_sum(c[i]->hr,  HR_AVG_LEN) / 3600.0,
+  totals[i]));
 } else {
 n_never_hit++;
 }
 }
+
 svec_add_nocopy(lines, xasprintf("%zu events never hit", n_never_hit));
 free(totals);
 }
@@ -249,3 +270,85 @@ coverage_clear(void)
 }
 ovs_mutex_unlock(&coverage_mutex);
 }
+
+/* Runs approximately every COVERAGE_RUN_INTERVAL amount of time to update the
+ * coverage counters' 'min' and 'hr' array.  'min' array is for cumulating
+ * per second counts into per minute count.  'hr' array is for cumulating per
+ * minute counts into per hour count.  Every thread may call this function. */
+void
+coverage_run(void)
+{
+/* Defines the moving average array index variables. */
+static unsigned int min_idx, hr_idx;
+struct coverage_counter **c = coverage_counters;
+long long int now;
+
+ovs_mutex_lock(&coverage_mutex);
+now = time_msec();
+/* Initialize the coverage_run_time. */
+if (coverage_run_time == LLONG_MIN) {
+coverage_run_time = now + COVERAGE_RUN_INTERVAL;
+}
+
+if (now >= coverage_run_time) {
+size_t i, j;
+/* Computes the number of COVERAGE_RUN_INTERVAL slots, since
+ * it is possible that the actual run interval is multiple of
+ * COVERAGE_RUN_INTERVAL. */
+int slots = (now - coverage_run_time) / COVERAGE_RUN_INTERVAL +

[ovs-dev] [PATCH V2 2/2] ofproto-dpif: Compute the subfacet add/del rate using coverage counters.

[Alex Wang]

So far, the subfacet rates (e.g. add rate, del rate) are computed by
exponential moving averaging function in ofproto-dpif.c.  This commit
replaces that logic with coverage counters.  And the rates can be
checked by running "ovs-appctl coverage/show" command.

Signed-off-by: Alex Wang 

---

v1 -> v2:
- rebase to master.

---
 ofproto/ofproto-dpif.c |  101 
 tests/ofproto-dpif.at  |4 --
 tests/tunnel.at|   26 ++---
 3 files changed, 21 insertions(+), 110 deletions(-)

diff --git a/ofproto/ofproto-dpif.c b/ofproto/ofproto-dpif.c
index 7a15e64..037ee4b 100644
--- a/ofproto/ofproto-dpif.c
+++ b/ofproto/ofproto-dpif.c
@@ -70,6 +70,10 @@ VLOG_DEFINE_THIS_MODULE(ofproto_dpif);
 COVERAGE_DEFINE(ofproto_dpif_expired);
 COVERAGE_DEFINE(facet_revalidate);
 COVERAGE_DEFINE(facet_unexpected);
+COVERAGE_DEFINE(facet_create);
+COVERAGE_DEFINE(facet_remove);
+COVERAGE_DEFINE(subfacet_create);
+COVERAGE_DEFINE(subfacet_destroy);
 COVERAGE_DEFINE(subfacet_install_fail);
 COVERAGE_DEFINE(packet_in_overflow);
 
@@ -434,20 +438,6 @@ struct dpif_backer {
 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;
-
 /* Number of upcall handling threads. */
 unsigned int n_handler_threads;
 };
@@ -456,7 +446,6 @@ struct dpif_backer {
 static struct shash all_dpif_backers = SHASH_INITIALIZER(&all_dpif_backers);
 
 static void drop_key_clear(struct dpif_backer *);
-static void update_moving_averages(struct dpif_backer *backer);
 
 struct ofproto_dpif {
 struct hmap_node all_ofproto_dpifs_node; /* In 'all_ofproto_dpifs'. */
@@ -1214,14 +1203,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;
 
@@ -3641,8 +3622,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
@@ -3825,6 +3804,7 @@ facet_create(const struct flow_miss *miss)
 struct facet *facet;
 struct match match;
 
+COVERAGE_INC(facet_create);
 facet = xzalloc(sizeof *facet);
 facet->ofproto = miss->ofproto;
 facet->used = miss->stats.used;
@@ -3888,6 +3868,7 @@ facet_remove(struct facet *facet)
 {
 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. */
@@ -4425,6 +4406,7 @@ 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;
@@ -4437,7 +4419,6 @@ subfacet_create(struct facet *facet, struct flow_miss 
*miss)
 subfacet->path = SF_NOT_INSTALLED;
 subfacet->backer = backer;
 
-backer->subfacet_add_count++;
 return subfacet;
 }
 
@@ -4447,11 +4428,8 @@ static void
 subfacet_destroy__(struct subfacet *subfacet)
 {
 struct facet *facet = subfacet->facet;
-struct ofproto_dpif *ofproto = facet->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);
 list_remove(&subfacet->list_node);
@@ -5539,21 +5517,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 

[ovs-dev] [PATCH] rhel: update openvswitch init script to include functionality in debians init script.

[Duffie Cooley]

From: Duffie Cooley 

This pulls some of the changes to the debian init script into the rhel init
script. Specifically this is a fix for a request to make sure that the
openvswitch status command  in rhel based distros gives a useful exit
status. That was fixed in

commit 5e0c05bc058c78a11be6747f62e6ad88e5d06b70
debian: Fix exit status of openvswitch-switch init script "status"
command

This patch also pulls in the load_kmod logic in

commit 19cbf2b8a49d18eb8a8047c3b03953e6e9f0116f
debian: force-reload-kmod while package upgrading.

to enable users of rhel based distros to have the same
functionality.


Signed-off-by: Duffie Cooley 
---
 rhel/etc_init.d_openvswitch | 56
++---
 1 file changed, 53 insertions(+), 3 deletions(-)

diff --git a/rhel/etc_init.d_openvswitch b/rhel/etc_init.d_openvswitch
index 7e64132..e02174d 100755
--- a/rhel/etc_init.d_openvswitch
+++ b/rhel/etc_init.d_openvswitch
@@ -1,6 +1,6 @@
 #!/bin/sh
 #
-# openvswitch
+# Copyright (C) 2013 Nicira, Inc.
 #
 # chkconfig: 2345 09 91
 # description: Manage Open vSwitch kernel modules and user-space daemons
@@ -30,7 +30,29 @@
 . /usr/share/openvswitch/scripts/ovs-lib || exit 1
 test -e /etc/sysconfig/openvswitch && . /etc/sysconfig/openvswitch
 
+load_kmod () {
+ovs_ctl load-kmod || exit $?
+}
+
 start () {
+if ovs_ctl load-kmod; then
+:
+else
+echo "Module has probably not been built for this kernel."
+fi
 set ovs_ctl ${1-start}
 set "$@" --system-id=random
 if test X"$FORCE_COREFILES" != X; then
@@ -46,7 +68,7 @@ start () {
 set "$@" --mlockall="$VSWITCHD_MLOCKALL"
 fi
 set "$@" $OVS_CTL_OPTS
-"$@"
+"$@" || exit $?
 
 touch /var/lock/subsys/openvswitch
 }
@@ -57,8 +79,35 @@ stop () {
 }
 
 restart () {
-if [ "$1" = "--save-flows=yes" ]; then
+# OVS_FORCE_RELOAD_KMOD can be set by package postinst script.
+if [ "$1" = "--save-flows=yes" ] || \
+[ "${OVS_FORCE_RELOAD_KMOD}" = "no" ]; then
 start restart
+elif [ "${OVS_FORCE_RELOAD_KMOD}" = "yes" ]; then
+depmod -a
+
+if [ -e /sys/module/openvswitch ]; then
+LOADED_SRCVERSION=`cat /sys/module/openvswitch/srcversion`
+LOADED_VERSION=`cat /sys/module/openvswitch/version`
+elif [ -e /sys/module/openvswitch_mod ]; then
+LOADED_SRCVERSION=`cat /sys/module/openvswitch_mod/srcversion`
+LOADED_VERSION=`cat /sys/module/openvswitch_mod/version`
+fi
+SRCVERSION=`modinfo -F srcversion openvswitch 2>/dev/null`
+VERSION=`modinfo -F version openvswitch 2>/dev/null`
+
+ovs_ctl_log "Package upgrading:\n"\
+"Loaded version: ${LOADED_VERSION}
${LOADED_SRCVERSION}.\n"\
+"Version on disk: ${VERSION} ${SRCVERSION}."
+
+# If the kernel module was previously loaded and it is
different than
+# the kernel module on disk, then do a 'force-reload-kmod'.
+if [ -n "${LOADED_SRCVERSION}" ] && [ -n "${SRCVERSION}" ] && \
+[ "${SRCVERSION}" != "${LOADED_SRCVERSION}" ]; then
+start force-reload-kmod
+else
+start restart
+fi
 else
 stop
 start
@@ -81,6 +130,7 @@ case $1 in
 ;;
 status)
 ovs_ctl status
+exit $?
 ;;
 version)
 ovs_ctl version
-- 
1.8.1.2

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] rhel: update openvswitch init script to include functionality in debians init script.

[Ben Pfaff]

On Mon, Sep 30, 2013 at 06:47:43PM -0700, Duffie Cooley wrote:
> From: Duffie Cooley 
> 
> This pulls some of the changes to the debian init script into the rhel init
> script. Specifically this is a fix for a request to make sure that the
> openvswitch status command  in rhel based distros gives a useful exit
> status. That was fixed in
> 
> commit 5e0c05bc058c78a11be6747f62e6ad88e5d06b70
> debian: Fix exit status of openvswitch-switch init script "status"
> command
> 
> This patch also pulls in the load_kmod logic in
> 
> commit 19cbf2b8a49d18eb8a8047c3b03953e6e9f0116f
> debian: force-reload-kmod while package upgrading.
> 
> to enable users of rhel based distros to have the same
> functionality.

If two of the init scripts need this, do all of them?  Should we pull
this functionality into ovs-ctl.in?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH] rhel: update openvswitch init script to include functionality in debians init script.

[Duffie Cooley]

On 09/30/2013 07:12 PM, Ben Pfaff wrote:
> On Mon, Sep 30, 2013 at 06:47:43PM -0700, Duffie Cooley wrote:
>> From: Duffie Cooley 
>>
>> This pulls some of the changes to the debian init script into the rhel init
>> script. Specifically this is a fix for a request to make sure that the
>> openvswitch status command  in rhel based distros gives a useful exit
>> status. That was fixed in
>>
>> commit 5e0c05bc058c78a11be6747f62e6ad88e5d06b70
>> debian: Fix exit status of openvswitch-switch init script "status"
>> command
>>
>> This patch also pulls in the load_kmod logic in
>>
>> commit 19cbf2b8a49d18eb8a8047c3b03953e6e9f0116f
>> debian: force-reload-kmod while package upgrading.
>>
>> to enable users of rhel based distros to have the same
>> functionality.
> If two of the init scripts need this, do all of them?  Should we pull
> this functionality into ovs-ctl.in?
In my opinion yes this should be pulled into ovs-ctl.in. I will chat
with Guru about this and if he agrees. I will submit a patch for both
the rhel init script and ovs-ctl.

Thanks!

Duffie
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] rhel: fix the exit status of the openvswitch init script.

[Duffie Cooley]

From: Duffie Cooley 
This is a fix for a request to make sure that the openvswitch status command
in rhel based distros gives a useful exit status. That was fixed in

commit 5e0c05bc058c78a11be6747f62e6ad88e5d06b70
debian: Fix exit status of openvswitch-switch init script "status" command



Signed-off-by: Duffie Cooley 
---
 rhel/etc_init.d_openvswitch | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/rhel/etc_init.d_openvswitch b/rhel/etc_init.d_openvswitch
index 7e64132..6a53cef 100755
--- a/rhel/etc_init.d_openvswitch
+++ b/rhel/etc_init.d_openvswitch
@@ -5,7 +5,7 @@
 # chkconfig: 2345 09 91
 # description: Manage Open vSwitch kernel modules and user-space daemons
 
-# Copyright (C) 2009, 2010, 2011 Nicira, Inc.
+# Copyright (C) 2009, 2010, 2011, 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.
@@ -81,6 +81,7 @@ case $1 in
 ;;
 status)
 ovs_ctl status
+exit $?
 ;;
 version)
 ovs_ctl version
-- 
1.8.1.2

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3] ofproto-dpif: Move special upcall handling into ofproto-dpif-upcall.

[Simon Horman]

On Wed, Sep 25, 2013 at 08:54:31AM -0700, Ben Pfaff wrote:
> On Tue, Sep 24, 2013 at 04:45:14PM -0700, Ethan Jackson wrote:
> > Both the IPFIX and SFLOW modules are thread safe, so there's no
> > particular reason to pass them up to the main thread.  Eliminating
> > this step significantly simplifies the code.
> > 
> > Signed-off-by: Ethan Jackson 
> 
> The comments on enum upcall_type are wrong, since none of the upcalls
> still require main thread involvement.
> 
> The comments on udpif_dispatcher() and udpif_upcall_handler() are
> inaccurate now.
> 
> Some comments on pre-existing code:
> 
> * I spotted another misspelling of "receiving" (as "receving").
> 
> * I think that the VLOG_WARN in recv_upcalls should be rate-limited.
>   If it happens once it'll happen a lot.
> 
> * Part of this code:
> 
> if (type == OVS_KEY_ATTR_IN_PORT
> || type == OVS_KEY_ATTR_TCP
> || type == OVS_KEY_ATTR_UDP) {
> if (nl_attr_get_size(nla) == 4) {
> ovs_be32 attr = nl_attr_get_be32(nla);
> hash = mhash_add(hash, (OVS_FORCE uint32_t) attr);
> 
>   could be simplified:
> 
> hash = mhash_add(hash, nl_attr_get_u32(nla));

Sparse seems unhappy about that:

# MAKE=make make C=2 CF="-D__CHECK_ENDIAN__ -Wsparse-all" all
ofproto/ofproto-dpif-upcall.c:518:60: warning: incorrect type in argument 2 
(different base types)
ofproto/ofproto-dpif-upcall.c:518:60:expected unsigned int [unsigned] 
[usertype] data
ofproto/ofproto-dpif-upcall.c:518:60:got restricted __be32

The sparse version I have is the following checkout from
git://git.kernel.org/pub/scm/devel/sparse/sparse.git.
I believe that you guys were using this version at some point.

30cb3a104d4fb8b70409e8f1e8a0a08363236f58
("Get rid of gcc warning about enum values")

> 
>   It's "incorrect" for TCP and UDP but the pre-existing code is
>   "incorrect" in the same way for in_port.
> 
> In ofproto/ofproto-dpif-xlate.h, I would put a blank line before the
> #endif ending the file.  Also the space after " *" below is not our
> usual style:
> struct dpif_sflow * xlate_get_sflow(const struct ofproto_dpif *)
> 
> Acked-by: Ben Pfaff 
> ___
> 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 v3] ofproto-dpif: Move special upcall handling into ofproto-dpif-upcall.

[Ben Pfaff]

On Tue, Oct 01, 2013 at 01:23:20PM +0900, Simon Horman wrote:
> On Wed, Sep 25, 2013 at 08:54:31AM -0700, Ben Pfaff wrote:
> > On Tue, Sep 24, 2013 at 04:45:14PM -0700, Ethan Jackson wrote:
> > > Both the IPFIX and SFLOW modules are thread safe, so there's no
> > > particular reason to pass them up to the main thread.  Eliminating
> > > this step significantly simplifies the code.
> > > 
> > > Signed-off-by: Ethan Jackson 
> > 
> > The comments on enum upcall_type are wrong, since none of the upcalls
> > still require main thread involvement.
> > 
> > The comments on udpif_dispatcher() and udpif_upcall_handler() are
> > inaccurate now.
> > 
> > Some comments on pre-existing code:
> > 
> > * I spotted another misspelling of "receiving" (as "receving").
> > 
> > * I think that the VLOG_WARN in recv_upcalls should be rate-limited.
> >   If it happens once it'll happen a lot.
> > 
> > * Part of this code:
> > 
> > if (type == OVS_KEY_ATTR_IN_PORT
> > || type == OVS_KEY_ATTR_TCP
> > || type == OVS_KEY_ATTR_UDP) {
> > if (nl_attr_get_size(nla) == 4) {
> > ovs_be32 attr = nl_attr_get_be32(nla);
> > hash = mhash_add(hash, (OVS_FORCE uint32_t) attr);
> > 
> >   could be simplified:
> > 
> > hash = mhash_add(hash, nl_attr_get_u32(nla));
> 
> Sparse seems unhappy about that:
> 
> # MAKE=make make C=2 CF="-D__CHECK_ENDIAN__ -Wsparse-all" all
> ofproto/ofproto-dpif-upcall.c:518:60: warning: incorrect type in argument 2 
> (different base types)
> ofproto/ofproto-dpif-upcall.c:518:60:expected unsigned int [unsigned] 
> [usertype] data
> ofproto/ofproto-dpif-upcall.c:518:60:got restricted __be32

I don't understand that: no __be32 is involved in the code I suggested.
Maybe you accidentally tried nl_attr_get_be32(nla) instead of
nl_attr_get_u32(nla)?
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3] ofproto-dpif: Move special upcall handling into ofproto-dpif-upcall.

[Simon Horman]

On Mon, Sep 30, 2013 at 09:45:25PM -0700, Ben Pfaff wrote:
> On Tue, Oct 01, 2013 at 01:23:20PM +0900, Simon Horman wrote:
> > On Wed, Sep 25, 2013 at 08:54:31AM -0700, Ben Pfaff wrote:
> > > On Tue, Sep 24, 2013 at 04:45:14PM -0700, Ethan Jackson wrote:
> > > > Both the IPFIX and SFLOW modules are thread safe, so there's no
> > > > particular reason to pass them up to the main thread.  Eliminating
> > > > this step significantly simplifies the code.
> > > > 
> > > > Signed-off-by: Ethan Jackson 
> > > 
> > > The comments on enum upcall_type are wrong, since none of the upcalls
> > > still require main thread involvement.
> > > 
> > > The comments on udpif_dispatcher() and udpif_upcall_handler() are
> > > inaccurate now.
> > > 
> > > Some comments on pre-existing code:
> > > 
> > > * I spotted another misspelling of "receiving" (as "receving").
> > > 
> > > * I think that the VLOG_WARN in recv_upcalls should be rate-limited.
> > >   If it happens once it'll happen a lot.
> > > 
> > > * Part of this code:
> > > 
> > > if (type == OVS_KEY_ATTR_IN_PORT
> > > || type == OVS_KEY_ATTR_TCP
> > > || type == OVS_KEY_ATTR_UDP) {
> > > if (nl_attr_get_size(nla) == 4) {
> > > ovs_be32 attr = nl_attr_get_be32(nla);
> > > hash = mhash_add(hash, (OVS_FORCE uint32_t) attr);
> > > 
> > >   could be simplified:
> > > 
> > > hash = mhash_add(hash, nl_attr_get_u32(nla));
> > 
> > Sparse seems unhappy about that:
> > 
> > # MAKE=make make C=2 CF="-D__CHECK_ENDIAN__ -Wsparse-all" all
> > ofproto/ofproto-dpif-upcall.c:518:60: warning: incorrect type in argument 2 
> > (different base types)
> > ofproto/ofproto-dpif-upcall.c:518:60:expected unsigned int [unsigned] 
> > [usertype] data
> > ofproto/ofproto-dpif-upcall.c:518:60:got restricted __be32
> 
> I don't understand that: no __be32 is involved in the code I suggested.
> Maybe you accidentally tried nl_attr_get_be32(nla) instead of
> nl_attr_get_u32(nla)?

Indeed. But its not me. The problem is in the master branch.

The following seems to resolve the problem.
Should I send a formal patch?


diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 4c92db3..571d312 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -515,7 +515,7 @@ recv_upcalls(struct udpif *udpif)
 || type == OVS_KEY_ATTR_TCP
 || type == OVS_KEY_ATTR_UDP) {
 if (nl_attr_get_size(nla) == 4) {
-hash = mhash_add(hash, nl_attr_get_be32(nla));
+hash = mhash_add(hash, nl_attr_get_u32(nla));
 n_bytes += 4;
 } else {
 VLOG_WARN_RL(&rl,

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

Re: [ovs-dev] [PATCH v3] ofproto-dpif: Move special upcall handling into ofproto-dpif-upcall.

[Ben Pfaff]

On Tue, Oct 01, 2013 at 02:15:49PM +0900, Simon Horman wrote:
> On Mon, Sep 30, 2013 at 09:45:25PM -0700, Ben Pfaff wrote:
> > On Tue, Oct 01, 2013 at 01:23:20PM +0900, Simon Horman wrote:
> > > On Wed, Sep 25, 2013 at 08:54:31AM -0700, Ben Pfaff wrote:
> > > > On Tue, Sep 24, 2013 at 04:45:14PM -0700, Ethan Jackson wrote:
> > > > > Both the IPFIX and SFLOW modules are thread safe, so there's no
> > > > > particular reason to pass them up to the main thread.  Eliminating
> > > > > this step significantly simplifies the code.
> > > > > 
> > > > > Signed-off-by: Ethan Jackson 
> > > > 
> > > > The comments on enum upcall_type are wrong, since none of the upcalls
> > > > still require main thread involvement.
> > > > 
> > > > The comments on udpif_dispatcher() and udpif_upcall_handler() are
> > > > inaccurate now.
> > > > 
> > > > Some comments on pre-existing code:
> > > > 
> > > > * I spotted another misspelling of "receiving" (as "receving").
> > > > 
> > > > * I think that the VLOG_WARN in recv_upcalls should be rate-limited.
> > > >   If it happens once it'll happen a lot.
> > > > 
> > > > * Part of this code:
> > > > 
> > > > if (type == OVS_KEY_ATTR_IN_PORT
> > > > || type == OVS_KEY_ATTR_TCP
> > > > || type == OVS_KEY_ATTR_UDP) {
> > > > if (nl_attr_get_size(nla) == 4) {
> > > > ovs_be32 attr = nl_attr_get_be32(nla);
> > > > hash = mhash_add(hash, (OVS_FORCE uint32_t) attr);
> > > > 
> > > >   could be simplified:
> > > > 
> > > > hash = mhash_add(hash, nl_attr_get_u32(nla));
> > > 
> > > Sparse seems unhappy about that:
> > > 
> > > # MAKE=make make C=2 CF="-D__CHECK_ENDIAN__ -Wsparse-all" all
> > > ofproto/ofproto-dpif-upcall.c:518:60: warning: incorrect type in argument 
> > > 2 (different base types)
> > > ofproto/ofproto-dpif-upcall.c:518:60:expected unsigned int [unsigned] 
> > > [usertype] data
> > > ofproto/ofproto-dpif-upcall.c:518:60:got restricted __be32
> > 
> > I don't understand that: no __be32 is involved in the code I suggested.
> > Maybe you accidentally tried nl_attr_get_be32(nla) instead of
> > nl_attr_get_u32(nla)?
> 
> Indeed. But its not me. The problem is in the master branch.

Ah.  I didn't understand what you meant (I haven't pulled for a few
hours).

> The following seems to resolve the problem.
> Should I send a formal patch?

Yes, please.  Thank you.
___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH] ofproto-dpif: Correct endian problem in recv_upcalls()

[Simon Horman]

Use nl_attr_get_u32() instead of nl_attr_get_be32() to parse nla
so that the decoded value which is passed to mhash_add()
is host byte order as mhash_add() expects.

This resolves a minor regression introduced by
10e576406c7444ef ("ofproto-dpif: Move special upcall handling into
ofproto-dpif-upcall.").

I do not expect this change has any runtime implications.

Detected using sparse.

Cc: Ethan Jackson 
Cc: Ben Pfaff 
Signed-off-by: Simon Horman 
---
 ofproto/ofproto-dpif-upcall.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-upcall.c b/ofproto/ofproto-dpif-upcall.c
index 4c92db3..571d312 100644
--- a/ofproto/ofproto-dpif-upcall.c
+++ b/ofproto/ofproto-dpif-upcall.c
@@ -515,7 +515,7 @@ recv_upcalls(struct udpif *udpif)
 || type == OVS_KEY_ATTR_TCP
 || type == OVS_KEY_ATTR_UDP) {
 if (nl_attr_get_size(nla) == 4) {
-hash = mhash_add(hash, nl_attr_get_be32(nla));
+hash = mhash_add(hash, nl_attr_get_u32(nla));
 n_bytes += 4;
 } else {
 VLOG_WARN_RL(&rl,
-- 
1.8.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2.41 2/5] ofp-actions: Add separate OpenFlow 1.3 action parser

[Simon Horman]

From: Joe Stringer 

This patch adds new ofpact_from_openflow13() and
ofpacts_from_openflow13() functions parallel to the existing ofpact
handling code. In the OpenFlow 1.3 version, push_mpls is handled
differently, but all other actions are handled by the existing code.

In the case of push_mpls for OpenFlow 1.3 the new mpls_before_vlan field of
struct ofpact_push_mpls is set to true.  This will be used by a subsequent
patch to allow allow the correct VLAN+MPLS datapath behaviour to be
determined at odp translation time.

Signed-off-by: Joe Stringer 
Signed-off-by: Simon Horman 

---

v2.41 [Simon Horman]
* As suggested by Ben Pfaff
  + Expand struct ofpact_reg_load to include a mpls_before_vlan field
rather than using the compat field of the ofpact field of
struct ofpact_reg_load.
  + Pass version to  ofpacts_pull_openflow11_actions and
ofpacts_pull_openflow11_instructions.  This removes the invalid
assumption that that these functions are passed a full message and are
thus able to deduce the OpenFlow version.

v2.36 - v2.40
* No change

v2.35 [Joe Stringer]
* First post
---
 lib/ofp-actions.c | 68 ---
 lib/ofp-actions.h |  9 +++
 lib/ofp-print.c   |  2 +-
 lib/ofp-util.c| 24 ++
 lib/ofp-util.h|  2 +-
 utilities/ovs-ofctl.c |  8 +++---
 6 files changed, 93 insertions(+), 20 deletions(-)

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 65430f3..434d020 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -881,6 +881,40 @@ ofpacts_from_openflow11(const union ofp_action *in, size_t 
n_in,
 return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow11);
 }
 
+static enum ofperr
+ofpact_from_openflow13(const union ofp_action *a, struct ofpbuf *out)
+{
+enum ofputil_action_code code;
+enum ofperr error;
+
+error = decode_openflow11_action(a, &code);
+if (error) {
+return error;
+}
+
+if (code == OFPUTIL_OFPAT11_PUSH_MPLS) {
+struct ofpact_push_mpls *oam;
+struct ofp11_action_push *oap = (struct ofp11_action_push *)a;
+if (!eth_type_mpls(oap->ethertype)) {
+return OFPERR_OFPBAC_BAD_ARGUMENT;
+}
+oam = ofpact_put_PUSH_MPLS(out);
+oam->ethertype = oap->ethertype;
+oam->mpls_before_vlan = true;
+} else {
+return ofpact_from_openflow11(a, out);
+}
+
+return error;
+}
+
+static enum ofperr
+ofpacts_from_openflow13(const union ofp_action *in, size_t n_in,
+struct ofpbuf *out)
+{
+return ofpacts_from_openflow(in, n_in, out, ofpact_from_openflow13);
+}
+
 /* OpenFlow 1.1 instructions. */
 
 #define DEFINE_INST(ENUM, STRUCT, EXTENSIBLE, NAME) \
@@ -1085,11 +1119,14 @@ get_actions_from_instruction(const struct 
ofp11_instruction *inst,
 *n_actions = (ntohs(inst->len) - sizeof *inst) / OFP11_INSTRUCTION_ALIGN;
 }
 
-/* Attempts to convert 'actions_len' bytes of OpenFlow 1.1 actions from the
+/* Attempts to convert 'actions_len' bytes of OpenFlow actions from the
  * front of 'openflow' into ofpacts.  On success, replaces any existing content
  * in 'ofpacts' by the converted ofpacts; on failure, clears 'ofpacts'.
  * Returns 0 if successful, otherwise an OpenFlow error.
  *
+ * Actions are processed according to their OpenFlow version which
+ * is provided in the 'version' parameter.
+ *
  * In most places in OpenFlow 1.1 and 1.2, actions appear encapsulated in
  * instructions, so you should call ofpacts_pull_openflow11_instructions()
  * instead of this function.
@@ -1101,15 +1138,27 @@ get_actions_from_instruction(const struct 
ofp11_instruction *inst,
  * valid in a specific context. */
 enum ofperr
 ofpacts_pull_openflow11_actions(struct ofpbuf *openflow,
+enum ofp_version version,
 unsigned int actions_len,
 struct ofpbuf *ofpacts)
 {
-return ofpacts_pull_actions(openflow, actions_len, ofpacts,
-ofpacts_from_openflow11);
+switch (version) {
+case OFP10_VERSION:
+case OFP11_VERSION:
+case OFP12_VERSION:
+return ofpacts_pull_actions(openflow, actions_len, ofpacts,
+ofpacts_from_openflow11);
+case OFP13_VERSION:
+return ofpacts_pull_actions(openflow, actions_len, ofpacts,
+ofpacts_from_openflow13);
+default:
+NOT_REACHED();
+}
 }
 
 enum ofperr
 ofpacts_pull_openflow11_instructions(struct ofpbuf *openflow,
+ enum ofp_version version,
  unsigned int instructions_len,
  struct ofpbuf *ofpacts)
 {
@@ -1160,7 +1209,18 @@ ofpacts_pull_openflow11_instructions(struct ofpbuf 
*openflow,
 
 get_actions_from_instruction(insts[OVSINST_OFPIT11_APPLY_ACTIONS],
   

[ovs-dev] [PATCH v2.41 3/5] lib: Support pushing of MPLS LSE before or after VLAN tag

[Simon Horman]

From: Joe Stringer 

This patch modifies the push_mpls behaviour to allow
pushing of an MPLS LSE either before any VLAN tag that may be present.

Pushing the MPLS LSE before any VLAN tag that is present is the
behaviour specified in OpenFlow 1.3.

Pushing the MPLS LSE after the any VLAN tag that is present is the
behaviour specified in OpenFlow 1.1 and 1.2. This is the only behaviour
that was supported prior to this patch.

When an push_mpls action has been inserted using OpenFlow 1.2 or earlier
the behaviour of pushing the MPLS LSE before any VLAN tag that may be
present is implemented by by inserting VLAN actions around the MPLS push
action during odp translation; Pop VLAN tags before committing MPLS
actions, and push the expected VLAN tag afterwards.

The trigger condition for the two different behaviours is the value of the
mpls_before_vlan field of struct ofpact_push_mpls.  This field is set when
parsing OpenFlow actions.

Signed-off-by: Joe Stringer 
Signed-off-by: Simon Horman 

---

v2.41 [Simon Horman]
* Use mpls_before_vlan field of struct ofpact_reg_load.
* Reword changelog to describe the difference in behaviour between
  different Open Flow versions.

v2.40 [Simon Horman]
* Trivial rebase for removal of set_ethertype()

v2.36 - v2.39
* No change

v2.35 [Joe Stringer]
* First post
---
 lib/flow.c   |   2 +-
 lib/packets.c|  10 +-
 lib/packets.h|   2 +-
 ofproto/ofproto-dpif-xlate.c |  27 ++---
 tests/ofproto-dpif.at| 237 +++
 5 files changed, 259 insertions(+), 19 deletions(-)

diff --git a/lib/flow.c b/lib/flow.c
index 0678c6f..6242ef9 100644
--- a/lib/flow.c
+++ b/lib/flow.c
@@ -1061,7 +1061,7 @@ flow_compose(struct ofpbuf *b, const struct flow *flow)
 }
 
 if (eth_type_mpls(flow->dl_type)) {
-b->l2_5 = b->l3;
+b->l2_5 = (char*)b->l2 + ETH_HEADER_LEN;
 push_mpls(b, flow->dl_type, flow->mpls_lse);
 }
 }
diff --git a/lib/packets.c b/lib/packets.c
index 922c5db..f8a58b6 100644
--- a/lib/packets.c
+++ b/lib/packets.c
@@ -220,11 +220,11 @@ eth_pop_vlan(struct ofpbuf *packet)
 
 /* Set ethertype of the packet. */
 void
-set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type)
+set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type, bool inner)
 {
 struct eth_header *eh = packet->data;
 
-if (eh->eth_type == htons(ETH_TYPE_VLAN)) {
+if (inner && eh->eth_type == htons(ETH_TYPE_VLAN)) {
 ovs_be16 *p;
 p = ALIGNED_CAST(ovs_be16 *,
 (char *)(packet->l2_5 ? packet->l2_5 : packet->l3) - 2);
@@ -332,8 +332,8 @@ push_mpls(struct ofpbuf *packet, ovs_be16 ethtype, ovs_be32 
lse)
 
 if (!is_mpls(packet)) {
 /* Set ethtype and MPLS label stack entry. */
-set_ethertype(packet, ethtype);
-packet->l2_5 = packet->l3;
+set_ethertype(packet, ethtype, false);
+packet->l2_5 = (char*)packet->l2 + ETH_HEADER_LEN;
 }
 
 /* Push new MPLS shim header onto packet. */
@@ -354,7 +354,7 @@ pop_mpls(struct ofpbuf *packet, ovs_be16 ethtype)
 size_t len;
 mh = packet->l2_5;
 len = (char*)packet->l2_5 - (char*)packet->l2;
-set_ethertype(packet, ethtype);
+set_ethertype(packet, ethtype, true);
 if (mh->mpls_lse & htonl(MPLS_BOS_MASK)) {
 packet->l2_5 = NULL;
 } else {
diff --git a/lib/packets.h b/lib/packets.h
index 7388152..38fec70 100644
--- a/lib/packets.h
+++ b/lib/packets.h
@@ -143,7 +143,7 @@ void compose_rarp(struct ofpbuf *, const uint8_t 
eth_src[ETH_ADDR_LEN]);
 void eth_push_vlan(struct ofpbuf *, ovs_be16 tci);
 void eth_pop_vlan(struct ofpbuf *);
 
-void set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type);
+void set_ethertype(struct ofpbuf *packet, ovs_be16 eth_type, bool inner);
 
 const char *eth_from_hex(const char *hex, struct ofpbuf **packetp);
 void eth_format_masked(const uint8_t eth[ETH_ADDR_LEN],
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 6757933..2afd760 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -2419,24 +2419,27 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 break;
 }
 
-case OFPACT_PUSH_MPLS:
-if (compose_mpls_push_action(ctx,
- ofpact_get_PUSH_MPLS(a)->ethertype)) {
+case OFPACT_PUSH_MPLS: {
+struct ofpact_push_mpls *oam = ofpact_get_PUSH_MPLS(a);
+
+if (compose_mpls_push_action(ctx, oam->ethertype)) {
 return;
 }
 
-/* Save and pop any existing VLAN tags. They will be pushed
- * back onto the packet after pushing the MPLS LSE. The overall
- * effect is to push the MPLS LSE after any VLAN tags that may
- * be present. This is the behaviour described for OpenFlow 1.1
- * and 1.2. This code needs to be enhanced to make this

[ovs-dev] [PATCH v2.41 4/5] datapath: Break out deacceleration portion of vlan_push

[Simon Horman]

Break out deacceleration portion of vlan_push into vlan_put
so that it may be re-used by mpls_push.

For both vlan_push and mpls_push if there is an accelerated VLAN tag
present then it should be deaccelerated, adding it to the data of
the skb, before the new tag is added.

Signed-off-by: Simon Horman 

---
v2.40
* As suggested by Jesse Gross
  + Simplify vlan_push by returning an error code
rather than an error code encoded as a struct xkb_buff *

v2.39
* First post
---
 datapath/actions.c | 29 +++--
 1 file changed, 19 insertions(+), 10 deletions(-)

diff --git a/datapath/actions.c b/datapath/actions.c
index 30ea1d2..d961e5d 100644
--- a/datapath/actions.c
+++ b/datapath/actions.c
@@ -105,22 +105,31 @@ static int pop_vlan(struct sk_buff *skb)
return 0;
 }
 
-static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan 
*vlan)
+/* push down current VLAN tag */
+static int put_vlan(struct sk_buff *skb)
 {
-   if (unlikely(vlan_tx_tag_present(skb))) {
-   u16 current_tag;
+   u16 current_tag = vlan_tx_tag_get(skb);
 
-   /* push down current VLAN tag */
-   current_tag = vlan_tx_tag_get(skb);
+   if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
+   return -ENOMEM;
 
-   if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
-   return -ENOMEM;
+   if (skb->ip_summed == CHECKSUM_COMPLETE)
+   skb->csum = csum_add(skb->csum, csum_partial(skb->data
+   + (2 * ETH_ALEN), VLAN_HLEN, 0));
 
-   if (skb->ip_summed == CHECKSUM_COMPLETE)
-   skb->csum = csum_add(skb->csum, csum_partial(skb->data
-   + (2 * ETH_ALEN), VLAN_HLEN, 0));
+   return 0;
+}
 
+static int push_vlan(struct sk_buff *skb, const struct ovs_action_push_vlan 
*vlan)
+{
+   if (unlikely(vlan_tx_tag_present(skb))) {
+   int err;
+
+   err = put_vlan(skb);
+   if (unlikely(err))
+   return err;
}
+
__vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & 
~VLAN_TAG_PRESENT);
return 0;
 }
-- 
1.8.4

___
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev

[ovs-dev] [PATCH v2.41 0/5] MPLS actions and matches

[Simon Horman]

Hi,

This series implements MPLS actions and matches based on work by
Ravi K, Leo Alterman, Yamahata-san and Joe Stringer.

This series provides two changes

* Patches 1 - 3

  Provide user-space support for the VLAN/MPLS tag insertion order
  up to and including OpenFlow 1.2, and the different ordering
  specified from OpenFlow 1.3. In a nutshell the datapath always
  uses the OpenFlow 1.3 ordering, which is to always insert tags
  immediately after the L2 header, regardless of the presence of other
  tags. And ovs-vswtichd provides compatibility for the behaviour up
  to OpenFlow 1.2, which is that MPLS tags should follow VLAN tags
  if present.

  These patches have been updated since v2.40.

  Ben, these are for you to review.

* Patches 4 and 5

  Adding basic MPLS action and match support to the kernel datapath

  These patches have not been updated since v2.40.

  Jesse, these are for you to review.


Differences between v2.41 and v2.40:

* As suggested by Ben Pfaff
  + Expand struct ofpact_reg_load to include a mpls_before_vlan field
rather than using the compat field of the ofpact field of
struct ofpact_reg_load.
  + Pass version to  ofpacts_pull_openflow11_actions and
ofpacts_pull_openflow11_instructions.  This removes the invalid
assumption that that these functions are passed a full message and are
thus able to deduce the OpenFlow version.


Differences between v2.40 and v2.39:

* Rebase for:
  + New dev_queue_xmit compat code
  + Updated put_vlan()
  + Removal of mpls_depth field from struct flow
* As suggested by Jesse Gross
  + Remove bogus mac_len update from push_mpls()
  + Slightly simplify push_mpls() by using eth_hdr()
  + Remove dubious condition !eth_p_mpls(inner_protocol) on
an skb being considered to be MPLS in netdev_send()
  + Only use compatibility code for MPLS GSO segmentation on kernels
older than 3.11
  + Revamp setting of inner_protocol
1. Do not unconditionally set inner_protocol to the value of
   skb->protocol in ovs_execute_actions().
2. Initialise inner_protocol it to zero only if compatibility code is in
   use. In the case where compatibility code is not in use it will either
   be zero due since the allocation of the skb or some other value set
   by some other user.
3. Conditionally set the inner_protocol in push_mpls() to the value of
   skb->protocol when entering push_mpls(). The condition is that
   inner_protocol is zero and the value of skb->protocol is not an MPLS
   ethernet type.
- This new scheme:
  + Pushes logic to set inner_protocol closer to the case where it is
needed.
  + Avoids over-writing values set by other users.
* As suggested by Pravin Shelar
  + Only set and restore skb->protocol in rpl___skb_gso_segment() in the
case of MPLS
  + Add inner_protocol field to struct ovs_gso_cb instead of ovs_skb_cb.
This moves compatibility code closer to where it is used
and creates fewer differences with mainline.
* Update comment on mac_len updates in datapath/actions.c
* Remove HAVE_INNER_PROCOTOL and instead just check
  against kernel version 3.11 directly.
  HAVE_INNER_PROCOTOL is a hang-over from work done prior
  to the merge of inner_protocol into the kernel.
* Remove dubious condition !eth_p_mpls(inner_protocol) on
  using inner_protocol as the type in rpl_skb_network_protocol()
* Do not update type of features in rpl_dev_queue_xmit.
  Though arguably correct this is not an inherent part of
  the changes made by this patch.
* Use skb_cow_head() in push_mpls()
  + Call skb_cow_head(skb, MPLS_HLEN) instead of
make_writable(skb, skb->mac_len) to ensure that there is enough head
room to push an MPLS LSE regardless of whether the skb is cloned or not.
  + This is consistent with the behaviour of rpl__vlan_put_tag().
  + This is a fix for crashes reported when performing mpls_push
with headroom less than 4. This problem was introduced in v3.36.
* Skip popping in mpls_pop if the skb is too short to contain an MPLS LSE


Differences between v2.39 and v2.38:

* Rebase for removal of vlan, checksum and skb->mark compat code
  - This includes adding adding a new patch,
"[PATCH v2.39 6/7] datapath: Break out deacceleration portion of
vlan_push" to allow re-use of some existing code.


Differences between v2.38 and v2.37:

* Rebase for SCTP support
* Refactor validate_tp_port() to iterate over eth_types rather
  than open-coding the loop. With the addition of SCTP this logic
  is now used three times.


Differences between v2.37 and v2.36:

* Rebase


Differences between v2.36 and v2.35:

* Rebase

* Do not add set_ethertype() to datapath/actions.c.
  As this patch has evolved this function had devolved into
  to sets of functionality wrapped into a single function with
  only one line of common code. Refactor things to simply
  open-code setting the ether type in the two locations where
  set_ethertype() was previously used. The aim here is to 

[ovs-dev] [PATCH v2.41 1/5] odp: Allow VLAN actions after MPLS actions

[Simon Horman]

From: Joe Stringer 

OpenFlow 1.1 and 1.2, and 1.3 differ in their handling of MPLS actions in the
presence of VLAN tags. To allow correct behaviour to be committed in
each situation, this patch adds a second round of VLAN tag action
handling to commit_odp_actions(), which occurs after MPLS actions. This
is implemented with a new field in 'struct xlate_in' called 'vlan_tci'.

When an push_mpls action is composed, the flow's current VLAN state is
stored into xin->vlan_tci, and flow->vlan_tci is set to 0 (pop_vlan). If
a VLAN tag is present, it is stripped; if not, then there is no change.
Any later modifications to the VLAN state is written to xin->vlan_tci.
When committing the actions, flow->vlan_tci is used before MPLS actions,
and xin->vlan_tci is used afterwards. This retains the current datapath
behaviour, but allows VLAN actions to be applied in a more flexible
manner.

Both before and after this patch MPLS LSEs are pushed onto a packet after
any VLAN tags that may be present. This is the behaviour described in
OpenFlow 1.1 and 1.2. OpenFlow 1.3 specifies that MPLS LSEs should be
pushed onto a packet before any VLAN tags that are present. Support
for this will be added by a subsequent patch that makes use of
the infrastructure added by this patch.

Signed-off-by: Joe Stringer 
Signed-off-by: Simon Horman 

---

v2.41
* Rework comments to make things a little clearer

v2.40
* Rebase for removal of mpls_depth from struct flow

v2.38 - v2.39
* No change

v2.37
* Rebase

v2.36
* No change

v2.5
* First post
---
 lib/odp-util.c   |   9 +-
 lib/odp-util.h   |   2 +-
 ofproto/ofproto-dpif-xlate.c | 101 -
 ofproto/ofproto-dpif-xlate.h |   5 ++
 tests/ofproto-dpif.at| 209 +++
 5 files changed, 303 insertions(+), 23 deletions(-)

diff --git a/lib/odp-util.c b/lib/odp-util.c
index 0785c6a..fcfa91b 100644
--- a/lib/odp-util.c
+++ b/lib/odp-util.c
@@ -3549,11 +3549,15 @@ commit_set_pkt_mark_action(const struct flow *flow, 
struct flow *base,
  * key from 'base' into 'flow', and then changes 'base' the same way.  Does not
  * commit set_tunnel actions.  Users should call commit_odp_tunnel_action()
  * in addition to this function if needed.  Sets fields in 'wc' that are
- * used as part of the action. */
+ * used as part of the action.
+ *
+ * VLAN actions may be committed twice; If vlan_tci in 'flow' differs from the
+ * one in 'base', then it is committed before MPLS actions. If 'final_vlan_tci'
+ * differs from 'flow->vlan_tci', it is committed afterwards. */
 void
 commit_odp_actions(const struct flow *flow, struct flow *base,
struct ofpbuf *odp_actions, struct flow_wildcards *wc,
-   int *mpls_depth_delta)
+   int *mpls_depth_delta, ovs_be16 final_vlan_tci)
 {
 commit_set_ether_addr_action(flow, base, odp_actions, wc);
 commit_vlan_action(flow->vlan_tci, base, odp_actions, wc);
@@ -3564,6 +3568,7 @@ commit_odp_actions(const struct flow *flow, struct flow 
*base,
  * that it is no longer IP and thus nw and port actions are no longer 
valid.
  */
 commit_mpls_action(flow, base, odp_actions, wc, mpls_depth_delta);
+commit_vlan_action(final_vlan_tci, base, odp_actions, wc);
 commit_set_priority_action(flow, base, odp_actions, wc);
 commit_set_pkt_mark_action(flow, base, odp_actions, wc);
 }
diff --git a/lib/odp-util.h b/lib/odp-util.h
index 4abf543..c7fc1eb 100644
--- a/lib/odp-util.h
+++ b/lib/odp-util.h
@@ -131,7 +131,7 @@ void commit_odp_tunnel_action(const struct flow *, struct 
flow *base,
   struct ofpbuf *odp_actions);
 void commit_odp_actions(const struct flow *, struct flow *base,
 struct ofpbuf *odp_actions, struct flow_wildcards *wc,
-int *mpls_depth_delta);
+int *mpls_depth_delta, ovs_be16 final_vlan_tci);
 
 /* ofproto-dpif interface.
  *
diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index cced7cc..6757933 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -982,10 +982,11 @@ static void
 output_normal(struct xlate_ctx *ctx, const struct xbundle *out_xbundle,
   uint16_t vlan)
 {
-ovs_be16 *flow_tci = &ctx->xin->flow.vlan_tci;
+ovs_be16 *flow_tci = &ctx->xin->vlan_tci;
 uint16_t vid;
 ovs_be16 tci, old_tci;
 struct xport *xport;
+bool flow_tci_equal_to_xin = (*flow_tci == ctx->xin->flow.vlan_tci);
 
 vid = output_vlan_to_vid(out_xbundle, vlan);
 if (list_is_empty(&out_xbundle->xports)) {
@@ -1016,9 +1017,15 @@ output_normal(struct xlate_ctx *ctx, const struct 
xbundle *out_xbundle,
 }
 }
 *flow_tci = tci;
+if (flow_tci_equal_to_xin) {
+ctx->xin->flow.vlan_tci = tci;
+}
 
 compose_output_action(ctx, xport->ofp_port);
 *flow_tci = old_tci;
+if (flow_tci_equal_to_xin) {
+ctx->xin->flo

[ovs-dev] [PATCH v2.41 5/5] datapath: Add basic MPLS support to kernel

[Simon Horman]

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, Ravi K, Isaku Yamahata and Joe Stringer.

Cc: Ravi K 
Cc: Leo Alterman 
Cc: Isaku Yamahata 
Cc: Joe Stringer 
Signed-off-by: Simon Horman 

---

v2.40
* Rebase for:
  + New dev_queue_xmit compat code
  + Updated put_vlan()
* As suggested by Jesse Gross
  + Remove bogus mac_len update from push_mpls()
  + Slightly simplify push_mpls() by using eth_hdr()
  + Remove dubious condition !eth_p_mpls(inner_protocol) on
an skb being considered to be MPLS in netdev_send()
  + Only use compatibility code for MPLS GSO segmentation on kernels
older than 3.11
  + Revamp setting of inner_protocol
1. Do not unconditionally set inner_protocol to the value of
   skb->protocol in ovs_execute_actions().
2. Initialise inner_protocol it to zero only if compatibility code is in
   use. In the case where compatibility code is not in use it will either
   be zero due since the allocation of the skb or some other value set
   by some other user.
3. Conditionally set the inner_protocol in push_mpls() to the value of
   skb->protocol when entering push_mpls(). The condition is that
   inner_protocol is zero and the value of skb->protocol is not an MPLS
   ethernet type.
- This new scheme:
  + Pushes logic to set inner_protocol closer to the case where it is
needed.
  + Avoids over-writing values set by other users.
* As suggested by Pravin Shelar
  + Only set and restore skb->protocol in rpl___skb_gso_segment() in the
case of MPLS
  + Add inner_protocol field to struct ovs_gso_cb instead of ovs_skb_cb.
This moves compatibility code closer to where it is used
and creates fewer differences with mainline.
* Update comment on mac_len updates in datapath/actions.c
* Remove HAVE_INNER_PROCOTOL and instead just check
  against kernel version 3.11 directly.
  HAVE_INNER_PROCOTOL is a hang-over from work done prior
  to the merge of inner_protocol into the kernel.
* Remove dubious condition !eth_p_mpls(inner_protocol) on
  using inner_protocol as the type in rpl_skb_network_protocol()
* Do not update type of features in rpl_dev_queue_xmit.
  Though arguably correct this is not an inherent part of
  the changes made by this patch.
* Use skb_cow_head() in push_mpls()
  + Call skb_cow_head(skb, MPLS_HLEN) instead of
make_writable(skb, skb->mac_len) to ensure that there is enough head
room to push an MPLS LSE regardless of whether the skb is cloned or not.
  + This is consistent with the behaviour of rpl__vlan_put_tag().
  + This is a fix for crashes reported when performing mpls_push
with headroom less than 4. This problem was introduced in v3.36.
* Skip popping in mpls_pop if the skb is too short to contain an MPLS LSE

v2.39
* Rebase for removal of vlan, checksum and skb->mark compat code

v2.38
* Rebase for SCTP support
* Refactor validate_tp_port() to iterate over eth_types rather
  than open-coding the loop. With the addition of SCTP this logic
  is now used three times.

v2.36 - v2.37
* Rebase

* Do not add set_ethertype() to datapath/actions.c.
  As this patch has evolved this function had devolved into
  to sets of functionality wrapped into a single function with
  only one line of common code. Refactor things to simply
  open-code setting the ether type in the two locations where
  set_ethertype() was previously used. The aim here is to improve
  readability.

* Update setting skb->ethertype after mpls push and pop.
  - In the case of push_mpls it should be set unconditionally
as in v2.35 the behaviour of this function to always push
an MPLS LSE before any VLAN tags.
  - In the case of mpls_pop eth_p_mpls(skb->protocol) is a better
test than skb->protocol != htons(ETH_P_8021Q) as it will give the
correct behaviour in the presence of other VLAN ethernet types,
for example 0x88a8 which is used by 802.1ad. Moreover, it seems
correct to update the ethernet type if it was previously set
according to the top-most MPLS LSE.

* Deaccelerate VLANs when pushing MPLS tags the
  - Since v2.35 MPLS push will insert an MPLS LSE before any VLAN tags.
This means that if an accelerated tag is present it should be
deaccelerated to ensure it ends up in the correct position.

* Update skb->mac_len in push_mpls() so that it will be correct
  when used by a subsequent call to pop_mpls().

  As things stand I do not believe this is strictly necessary as
  ovs-vswitchd will not send a pop MPLS action after a push MPLS action.
  However, I have added this in order to code more defensively as I believe
  that if such a sequence did occur it would be rather unobvious why
  it didn't work.

* Do not add skb_cow_head() call in push_mpls().
  It is unnecessary as there is a make_writable() call.
  This change was also made in v2.30 but some how the
  code regressed betwee