date:20171009

Re: [ovs-dev] [PATCH net-next] openvswitch: add ct_clear action

2017-10-09 Thread Pravin Shelar

On Fri, Oct 6, 2017 at 9:44 AM, Eric Garver  wrote:
> This adds a ct_clear action for clearing conntrack state. ct_clear is
> currently implemented in OVS userspace, but is not backed by an action
> in the kernel datapath. This is useful for flows that may modify a
> packet tuple after a ct lookup has already occurred.
>
> Signed-off-by: Eric Garver 
Patch mostly looks good. I have following comments.

> ---
>  include/uapi/linux/openvswitch.h |  2 ++
>  net/openvswitch/actions.c|  5 +
>  net/openvswitch/conntrack.c  | 12 
>  net/openvswitch/conntrack.h  |  7 +++
>  net/openvswitch/flow_netlink.c   |  5 +
>  5 files changed, 31 insertions(+)
>
> diff --git a/include/uapi/linux/openvswitch.h 
> b/include/uapi/linux/openvswitch.h
> index 156ee4cab82e..1b6e510e2cc6 100644
> --- a/include/uapi/linux/openvswitch.h
> +++ b/include/uapi/linux/openvswitch.h
> @@ -806,6 +806,7 @@ struct ovs_action_push_eth {
>   * packet.
>   * @OVS_ACTION_ATTR_POP_ETH: Pop the outermost Ethernet header off the
>   * packet.
> + * @OVS_ACTION_ATTR_CT_CLEAR: Clear conntrack state from the packet.
>   *
>   * Only a single header can be set with a single %OVS_ACTION_ATTR_SET.  Not 
> all
>   * fields within a header are modifiable, e.g. the IPv4 protocol and fragment
> @@ -835,6 +836,7 @@ enum ovs_action_attr {
> OVS_ACTION_ATTR_TRUNC,/* u32 struct ovs_action_trunc. */
> OVS_ACTION_ATTR_PUSH_ETH, /* struct ovs_action_push_eth. */
> OVS_ACTION_ATTR_POP_ETH,  /* No argument. */
> +   OVS_ACTION_ATTR_CT_CLEAR, /* No argument. */
>
> __OVS_ACTION_ATTR_MAX,/* Nothing past this will be accepted
>* from userspace. */
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index a54a556fcdb5..db9c7f2e662b 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -1203,6 +1203,10 @@ static int do_execute_actions(struct datapath *dp, 
> struct sk_buff *skb,
> return err == -EINPROGRESS ? 0 : err;
> break;
>
> +   case OVS_ACTION_ATTR_CT_CLEAR:
> +   err = ovs_ct_clear(skb, key);
> +   break;
> +
> case OVS_ACTION_ATTR_PUSH_ETH:
> err = push_eth(skb, key, nla_data(a));
> break;
> @@ -1210,6 +1214,7 @@ static int do_execute_actions(struct datapath *dp, 
> struct sk_buff *skb,
> case OVS_ACTION_ATTR_POP_ETH:
> err = pop_eth(skb, key);
> break;
> +
> }
Unrelated change.

>
> if (unlikely(err)) {
> diff --git a/net/openvswitch/conntrack.c b/net/openvswitch/conntrack.c
> index d558e882ca0c..f9b73c726ad7 100644
> --- a/net/openvswitch/conntrack.c
> +++ b/net/openvswitch/conntrack.c
> @@ -1129,6 +1129,18 @@ int ovs_ct_execute(struct net *net, struct sk_buff 
> *skb,
> return err;
>  }
>
> +int ovs_ct_clear(struct sk_buff *skb, struct sw_flow_key *key)
> +{
> +   if (skb_nfct(skb)) {
> +   nf_conntrack_put(skb_nfct(skb));
> +   nf_ct_set(skb, NULL, 0);
Can the new conntract state be appropriate? may be IP_CT_UNTRACKED?

> +   }
> +
> +   ovs_ct_fill_key(skb, key);
> +
I do not see need to refill the key if there is no skb-nf-ct.

> +   return 0;
> +}
> +
>  static int ovs_ct_add_helper(struct ovs_conntrack_info *info, const char 
> *name,
>  const struct sw_flow_key *key, bool log)
>  {
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH net-next] openvswitch: add ct_clear action

2017-10-09 Thread David Miller

From: Eric Garver 
Date: Fri,  6 Oct 2017 12:44:26 -0400

> This adds a ct_clear action for clearing conntrack state. ct_clear is
> currently implemented in OVS userspace, but is not backed by an action
> in the kernel datapath. This is useful for flows that may modify a
> packet tuple after a ct lookup has already occurred.
> 
> Signed-off-by: Eric Garver 

Pravin et al., this needs your review.

Thanks.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Remove assertion for truncated

2017-10-09 Thread Iwase Yusuke


Hi Ben and Andy,

Thank you very much!
I'm looking forward to review comments.


On 2017年10月10日 12:08, Ben Pfaff wrote:

You submitted the patch correctly.  It is my fault because I've been
slow about reviews lately.

I think that Andy Zhou should take a look at this patch since he added
the assertions.  I sent him a message asking about it.

On Tue, Oct 10, 2017 at 11:39:42AM +0900, Iwase Yusuke wrote:

Hi,

I'm very sorry for disturbing you.

Could someone review this patch? Or are there some more procedures for 
submitting patch?

Thanks,
Iwase


On 2017年10月04日 22:54, IWASE Yusuke wrote:

Because OpenFlow Spec does not clearly stipulate that "max_len" in
OUTPUT action must be zero when "port" is other than OFPP_CONTROLLER,
it is too strict assertion that confirm "max_len" is not zero, and
"max_len" should be ignored when not used.
Also this assertion causes the lack of the interoperability with some
controller implementations.

This patch removes these redundant assertions of if truncated or not.

Signed-off-by: IWASE Yusuke 
---
  ofproto/ofproto-dpif-xlate.c | 5 -
  1 file changed, 5 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d320d57..c5ed6a0 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3700,7 +3700,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t 
ofp_port,
  }
  if (xport->peer) {
-   ovs_assert(!truncate)
 patch_port_output(ctx, xport, xport->peer);
 return;
  }
@@ -4839,21 +4838,17 @@ xlate_output_action(struct xlate_ctx *ctx,
is_last_action, truncate);
  break;
  case OFPP_TABLE:
-ovs_assert(!truncate);
  xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
 0, may_packet_in, true, false, false,
 do_xlate_actions);
  break;
  case OFPP_NORMAL:
-ovs_assert(!truncate);
  xlate_normal(ctx);
  break;
  case OFPP_FLOOD:
-ovs_assert(!truncate);
  flood_packets(ctx, false, is_last_action);
  break;
  case OFPP_ALL:
-ovs_assert(!truncate);
  flood_packets(ctx, true, is_last_action);
  break;
  case OFPP_CONTROLLER:


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Remove assertion for truncated

2017-10-09 Thread Ben Pfaff

You submitted the patch correctly.  It is my fault because I've been
slow about reviews lately.

I think that Andy Zhou should take a look at this patch since he added
the assertions.  I sent him a message asking about it.

On Tue, Oct 10, 2017 at 11:39:42AM +0900, Iwase Yusuke wrote:
> Hi,
> 
> I'm very sorry for disturbing you.
> 
> Could someone review this patch? Or are there some more procedures for 
> submitting patch?
> 
> Thanks,
> Iwase
> 
> 
> On 2017年10月04日 22:54, IWASE Yusuke wrote:
> >Because OpenFlow Spec does not clearly stipulate that "max_len" in
> >OUTPUT action must be zero when "port" is other than OFPP_CONTROLLER,
> >it is too strict assertion that confirm "max_len" is not zero, and
> >"max_len" should be ignored when not used.
> >Also this assertion causes the lack of the interoperability with some
> >controller implementations.
> >
> >This patch removes these redundant assertions of if truncated or not.
> >
> >Signed-off-by: IWASE Yusuke 
> >---
> >  ofproto/ofproto-dpif-xlate.c | 5 -
> >  1 file changed, 5 deletions(-)
> >
> >diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> >index d320d57..c5ed6a0 100644
> >--- a/ofproto/ofproto-dpif-xlate.c
> >+++ b/ofproto/ofproto-dpif-xlate.c
> >@@ -3700,7 +3700,6 @@ compose_output_action__(struct xlate_ctx *ctx, 
> >ofp_port_t ofp_port,
> >  }
> >  if (xport->peer) {
> >-   ovs_assert(!truncate)
> > patch_port_output(ctx, xport, xport->peer);
> > return;
> >  }
> >@@ -4839,21 +4838,17 @@ xlate_output_action(struct xlate_ctx *ctx,
> >is_last_action, truncate);
> >  break;
> >  case OFPP_TABLE:
> >-ovs_assert(!truncate);
> >  xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
> > 0, may_packet_in, true, false, false,
> > do_xlate_actions);
> >  break;
> >  case OFPP_NORMAL:
> >-ovs_assert(!truncate);
> >  xlate_normal(ctx);
> >  break;
> >  case OFPP_FLOOD:
> >-ovs_assert(!truncate);
> >  flood_packets(ctx, false, is_last_action);
> >  break;
> >  case OFPP_ALL:
> >-ovs_assert(!truncate);
> >  flood_packets(ctx, true, is_last_action);
> >  break;
> >  case OFPP_CONTROLLER:
> >
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Remove assertion for truncated

2017-10-09 Thread Ben Pfaff

Andy, can you take a quick look at this?  It appears to me that it is
trivial to trigger this assertion by specifying a max_len in any
OpenFlow output action.  I guess that wasn't the intention, but maybe
there is some other importance purpose for the assertion that should be
retained.

Thanks,

Ben.

On Wed, Oct 04, 2017 at 10:54:16PM +0900, IWASE Yusuke wrote:
> Because OpenFlow Spec does not clearly stipulate that "max_len" in
> OUTPUT action must be zero when "port" is other than OFPP_CONTROLLER,
> it is too strict assertion that confirm "max_len" is not zero, and
> "max_len" should be ignored when not used.
> Also this assertion causes the lack of the interoperability with some
> controller implementations.
> 
> This patch removes these redundant assertions of if truncated or not.
> 
> Signed-off-by: IWASE Yusuke 
> ---
>  ofproto/ofproto-dpif-xlate.c | 5 -
>  1 file changed, 5 deletions(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index d320d57..c5ed6a0 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -3700,7 +3700,6 @@ compose_output_action__(struct xlate_ctx *ctx, 
> ofp_port_t ofp_port,
>  }
>  
>  if (xport->peer) {
> -   ovs_assert(!truncate)
> patch_port_output(ctx, xport, xport->peer);
> return;
>  }
> @@ -4839,21 +4838,17 @@ xlate_output_action(struct xlate_ctx *ctx,
>is_last_action, truncate);
>  break;
>  case OFPP_TABLE:
> -ovs_assert(!truncate);
>  xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
> 0, may_packet_in, true, false, false,
> do_xlate_actions);
>  break;
>  case OFPP_NORMAL:
> -ovs_assert(!truncate);
>  xlate_normal(ctx);
>  break;
>  case OFPP_FLOOD:
> -ovs_assert(!truncate);
>  flood_packets(ctx, false, is_last_action);
>  break;
>  case OFPP_ALL:
> -ovs_assert(!truncate);
>  flood_packets(ctx, true, is_last_action);
>  break;
>  case OFPP_CONTROLLER:
> -- 
> 2.7.4
> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Remove assertion for truncated

2017-10-09 Thread Iwase Yusuke


Hi,

I'm very sorry for disturbing you.

Could someone review this patch? Or are there some more procedures for 
submitting patch?

Thanks,
Iwase


On 2017年10月04日 22:54, IWASE Yusuke wrote:

Because OpenFlow Spec does not clearly stipulate that "max_len" in
OUTPUT action must be zero when "port" is other than OFPP_CONTROLLER,
it is too strict assertion that confirm "max_len" is not zero, and
"max_len" should be ignored when not used.
Also this assertion causes the lack of the interoperability with some
controller implementations.

This patch removes these redundant assertions of if truncated or not.

Signed-off-by: IWASE Yusuke 
---
  ofproto/ofproto-dpif-xlate.c | 5 -
  1 file changed, 5 deletions(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index d320d57..c5ed6a0 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -3700,7 +3700,6 @@ compose_output_action__(struct xlate_ctx *ctx, ofp_port_t 
ofp_port,
  }
  
  if (xport->peer) {

-   ovs_assert(!truncate)
 patch_port_output(ctx, xport, xport->peer);
 return;
  }
@@ -4839,21 +4838,17 @@ xlate_output_action(struct xlate_ctx *ctx,
is_last_action, truncate);
  break;
  case OFPP_TABLE:
-ovs_assert(!truncate);
  xlate_table_action(ctx, ctx->xin->flow.in_port.ofp_port,
 0, may_packet_in, true, false, false,
 do_xlate_actions);
  break;
  case OFPP_NORMAL:
-ovs_assert(!truncate);
  xlate_normal(ctx);
  break;
  case OFPP_FLOOD:
-ovs_assert(!truncate);
  flood_packets(ctx, false, is_last_action);
  break;
  case OFPP_ALL:
-ovs_assert(!truncate);
  flood_packets(ctx, true, is_last_action);
  break;
  case OFPP_CONTROLLER:


___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev-dpdk: New option 'hw_strip_crc' fields for DPDK interfaces

2017-10-09 Thread wenxu

Hi Zhenyu,


I think add a option maybe better.


1. It keep unchange the action with default value is false
2. It can be try one more time but if there some other different config, there 
should be more times to try
3. For some user who want to enable this feature, it can be configurable




在 2017-10-09 21:13:12，"Gao Zhenyu"  写道：

Thanks for working on it.


I want to know if we can have a patch to try one more time if we hit the 
hw_strip_crc error since we had introduce many options in ovs-dpdk now. (Just 
like function dpdk_eth_dev_queue_setup, it figures out queue number by retrying)


Thanks
Zhenyu Gao


2017-10-09 17:09 GMT+08:00 :
From: wenxu 

Some vf driver like i40evf can't disable the hw_strip_crc, with err
'dpdk|ERR|i40evf_dev_configure(): VF can't disable HW CRC Strip'
so hw_strip_crc feature should can be configurable

Signed-off-by: wenxu 
---
 NEWS |  2 ++
 lib/netdev-dpdk.c| 30 ++
 vswitchd/vswitch.xml |  9 +
 3 files changed, 41 insertions(+)

diff --git a/NEWS b/NEWS
index a3c1a02..d913847 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,8 @@ Post-v2.8.0
chassis "hostname" in addition to a chassis "name".
- Linux kernel 4.13
  * Add support for compiling OVS with the latest Linux 4.13 kernel
+   - DPDK:
+ * New option 'hw_strip_crc' fields for DPDK interfaces

 v2.8.0 - xx xxx 
 -
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index c60f46f..23c2a72 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -402,11 +402,15 @@ struct netdev_dpdk {
 int requested_n_rxq;
 int requested_rxq_size;
 int requested_txq_size;
+bool requested_hw_strip_crc;

 /* Number of rx/tx descriptors for physical devices */
 int rxq_size;
 int txq_size;

+/*crc strip feature config*/
+bool hw_strip_crc;
+
 /* Socket ID detected when vHost device is brought up */
 int requested_socket_id;

@@ -700,6 +704,7 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int 
n_rxq, int n_txq)

 conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
   NETDEV_RX_CHECKSUM_OFFLOAD) != 0;
+conf.rxmode.hw_strip_crc = dev->hw_strip_crc;
 /* A device may report more queues than it makes available (this has
  * been observed for Intel xl710, which reserves some of them for
  * SRIOV):  rte_eth_*_queue_setup will fail if a queue is not
@@ -914,6 +919,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
 dev->requested_n_txq = NR_QUEUE;
 dev->requested_rxq_size = NIC_PORT_DEFAULT_RXQ_SIZE;
 dev->requested_txq_size = NIC_PORT_DEFAULT_TXQ_SIZE;
+dev->requested_hw_strip_crc = false;
+dev->hw_strip_crc = false;

 /* Initialize the flow control to NULL */
 memset(>fc_conf, 0, sizeof dev->fc_conf);
@@ -1192,6 +1199,11 @@ netdev_dpdk_get_config(const struct netdev *netdev, 
struct smap *args)
 } else {
 smap_add(args, "rx_csum_offload", "false");
 }
+if (dev->hw_strip_crc) {
+smap_add(args, "hw_strip_crc", "true");
+} else {
+smap_add(args, "hw_strip_crc", "false");
+}
 }
 ovs_mutex_unlock(>mutex);

@@ -1255,6 +1267,19 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const 
struct smap *args)
 }

 static void
+dpdk_set_hw_strip_crc_config(struct netdev_dpdk *dev, const struct smap *args)
+ OVS_REQUIRES(dev->mutex)
+{
+int new_hw_strip_crc;
+
+new_hw_strip_crc = smap_get_bool(args, "hw_strip_crc", false);
+if (new_hw_strip_crc != dev->requested_hw_strip_crc) {
+dev->requested_hw_strip_crc = new_hw_strip_crc;
+netdev_request_reconfigure(>up);
+}
+}
+
+static void
 dpdk_process_queue_size(struct netdev *netdev, const struct smap *args,
 const char *flag, int default_size, int *new_size)
 {
@@ -1290,6 +1315,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args,

 dpdk_set_rxq_config(dev, args);

+dpdk_set_hw_strip_crc_config(dev, args);
+
 dpdk_process_queue_size(netdev, args, "n_rxq_desc",
 NIC_PORT_DEFAULT_RXQ_SIZE,
 >requested_rxq_size);
@@ -3196,6 +3223,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
 if (netdev->n_txq == dev->requested_n_txq
 && netdev->n_rxq == dev->requested_n_rxq
 && dev->mtu == dev->requested_mtu
+&& dev->hw_strip_crc == dev->requested_hw_strip_crc
 && dev->rxq_size == dev->requested_rxq_size
 && dev->txq_size == dev->requested_txq_size
 && dev->socket_id == dev->requested_socket_id) {
@@ -3217,6 +3245,8 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
 dev->rxq_size = dev->requested_rxq_size;
 dev->txq_size = dev->requested_txq_size;

+dev->hw_strip_crc = dev->requested_hw_strip_crc;
+
 rte_free(dev->tx_q);

[ovs-dev] Rania_Hassan

2017-10-09 Thread Anaïs

   


Madame, Monsieur, Cher(e) Adhérent(e),
A companhia Lhe anuncia seus (ganho) tem a loteria de um soma 
quatrocentos cinqüenta mil euro. Para mais informação adquira em contato
 com Mr Giroux Alex Sgiroux.alex(At)gmail.com





MO meu nome é Rania Hassan.
É viúvo e sem família. Solicito o bom Deus abençoá-los, e se acreditando então 
for solicitada para a minha saúde porque é desesperado
Leiam o ficheiro em anexo para ter cerca de informações suplementares.
Deixo-vos o meu endereço eletrónico, contacto-me 
:hassan-rania1*ARROBA*outlook.com
Obrigado















































Si vous ne visualisez pas correctement cet email, veuillezcliquer ici





















FÉDÉRATION FRANÇAISE DE RUGBY











Bonjour à tous,





Nous vous invitons à découvrir ci-dessous le film officiel de la candidature 
#FRANCE2023

Bon visionnage, partagez cette vidéo sur les réseaux sociaux !





Sportivement,




L’équipe FFR



















































 ___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] include/openvswitch/utils.h:

2017-10-09 Thread Greg Rose


On 10/09/2017 04:34 PM, Ben Pfaff wrote:

On Mon, Oct 09, 2017 at 03:59:37PM -0700, Greg Rose wrote:
> Fix macro for case where build is done with NDEBUG
>
> Signed-off-by: Greg Rose 
> ---
>   include/openvswitch/util.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
> index abebf96..72299e7 100644
> --- a/include/openvswitch/util.h
> +++ b/include/openvswitch/util.h
> @@ -52,7 +52,7 @@ const char *ovs_get_program_version(void);
>   ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION);   
\
>   }
>   #else
> -#define ovs_assert(CONDITION) ((void) (CONDITION))
> +#define ovs_assert(CONDITION) ((void) (CONDITION));
>   #endif

We normally invoke ovs_assert() like a function call, followed by a
semicolon.  If there's a missing ; somewhere in a user of ovs_assert(),
we should fix that.

The other case does look weird to me, because in normal usage it will
expand to something like:
 if (...) {
 ...
 };
so it might be a good idea to make it a single expression, maybe like
this:

diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
index abebf96fa3ac..84d7e07ff370 100644
--- a/include/openvswitch/util.h
+++ b/include/openvswitch/util.h
@@ -48,9 +48,9 @@ const char *ovs_get_program_version(void);
   * log. */
  #ifndef NDEBUG
  #define ovs_assert(CONDITION)   \
-if (!OVS_LIKELY(CONDITION)) {   \
-ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION);   \
-}
+(OVS_LIKELY(CONDITION)  \
+ ? (void) 0 \
+ : ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION))
  #else
  #define ovs_assert(CONDITION) ((void) (CONDITION))
  #endif


Yep, that makes more sense to me... I was just trying to do something else and 
ran into this
so didn't think on it much.

Looks good to me!

Thanks Ben.

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] include/openvswitch/utils.h:

2017-10-09 Thread Ben Pfaff

On Mon, Oct 09, 2017 at 03:59:37PM -0700, Greg Rose wrote:
> Fix macro for case where build is done with NDEBUG
> 
> Signed-off-by: Greg Rose 
> ---
>  include/openvswitch/util.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
> index abebf96..72299e7 100644
> --- a/include/openvswitch/util.h
> +++ b/include/openvswitch/util.h
> @@ -52,7 +52,7 @@ const char *ovs_get_program_version(void);
>  ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION);   \
>  }
>  #else
> -#define ovs_assert(CONDITION) ((void) (CONDITION))
> +#define ovs_assert(CONDITION) ((void) (CONDITION));
>  #endif

We normally invoke ovs_assert() like a function call, followed by a
semicolon.  If there's a missing ; somewhere in a user of ovs_assert(),
we should fix that.

The other case does look weird to me, because in normal usage it will
expand to something like:
if (...) {
...
};
so it might be a good idea to make it a single expression, maybe like
this:

diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
index abebf96fa3ac..84d7e07ff370 100644
--- a/include/openvswitch/util.h
+++ b/include/openvswitch/util.h
@@ -48,9 +48,9 @@ const char *ovs_get_program_version(void);
  * log. */
 #ifndef NDEBUG
 #define ovs_assert(CONDITION)   \
-if (!OVS_LIKELY(CONDITION)) {   \
-ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION);   \
-}
+(OVS_LIKELY(CONDITION)  \
+ ? (void) 0 \
+ : ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION))
 #else
 #define ovs_assert(CONDITION) ((void) (CONDITION))
 #endif
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] include/openvswitch/utils.h:

2017-10-09 Thread Greg Rose

Fix macro for case where build is done with NDEBUG

Signed-off-by: Greg Rose 
---
 include/openvswitch/util.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/openvswitch/util.h b/include/openvswitch/util.h
index abebf96..72299e7 100644
--- a/include/openvswitch/util.h
+++ b/include/openvswitch/util.h
@@ -52,7 +52,7 @@ const char *ovs_get_program_version(void);
 ovs_assert_failure(OVS_SOURCE_LOCATOR, __func__, #CONDITION);   \
 }
 #else
-#define ovs_assert(CONDITION) ((void) (CONDITION))
+#define ovs_assert(CONDITION) ((void) (CONDITION));
 #endif
 OVS_NO_RETURN void ovs_assert_failure(const char *, const char *, const char 
*);
 
-- 
1.8.3.1

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 13/13] json: New function json_object_put_format().

2017-10-09 Thread Ben Pfaff

On Mon, Oct 09, 2017 at 04:08:54PM -0400, Russell Bryant wrote:
> On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> > This will acquire users in an upcoming commit.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  include/openvswitch/json.h |  6 +-
> >  lib/json.c | 12 
> >  2 files changed, 17 insertions(+), 1 deletion(-)
> 
> Acked-by: Russell Bryant 

Thanks for the review.

> Is it required to put the printf function attribute in both json.h and
> json.c?  I figure it's harmless, but wasn't sure if it was necessary.

I don't know.  On a related issue, I've noticed that Clang forgets about
thread safety annotations if they're not in both places.  (Maybe that is
a bug, and maybe it's been fixed since I noticed it years ago.)
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 07/13] jsonrpc: Allow jsonrpc_session to have more than one remote.

2017-10-09 Thread Russell Bryant

On Mon, Oct 9, 2017 at 4:11 PM, Ben Pfaff  wrote:
> On Mon, Oct 09, 2017 at 03:57:18PM -0400, Russell Bryant wrote:
>> On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
>> > The implementation cycles through the remotes in random order.  This allows
>> > clients to perform some load balancing across alternative implementations
>> > of a service.
>> >
>> > Signed-off-by: Ben Pfaff 
>> > ---
>> >  lib/jsonrpc.c | 53 -
>> >  lib/jsonrpc.h |  6 +-
>> >  lib/svec.c| 18 ++
>> >  lib/svec.h|  1 +
>> >  4 files changed, 72 insertions(+), 6 deletions(-)
>>
>> > diff --git a/lib/svec.c b/lib/svec.c
>> > index 297a60ce14f9..c1b986bab108 100644
>> > --- a/lib/svec.c
>> > +++ b/lib/svec.c
>> > @@ -20,6 +20,7 @@
>> >  #include 
>> >  #include 
>> >  #include "openvswitch/dynamic-string.h"
>> > +#include "random.h"
>> >  #include "util.h"
>> >  #include "openvswitch/vlog.h"
>> >
>> > @@ -174,6 +175,23 @@ svec_compact(struct svec *svec)
>> >  svec->n = j;
>> >  }
>> >
>> > +static void
>> > +swap_strings(char **a, char **b)
>> > +{
>> > +char *tmp = *a;
>> > +*a = *b;
>> > +*b = tmp;
>> > +}
>> > +
>> > +void
>> > +svec_shuffle(struct svec *svec)
>> > +{
>> > +for (size_t i = 0; i < svec->n; i++) {
>> > +size_t j = i + random_range(svec->n - i);
>> > +swap_strings(>names[i], >names[j]);
>> > +}
>> > +}
>> > +
>>
>> I'm not sure this is as random as we'd like.
>>
>> Even if there are 10 elements, the first element has a 50% chance of
>> staying there, since it's only considered for a swap when i == 0.
>> That extends to the general behavior that the closer an element is to
>> the beginning, the better chance it has of staying near the beginning.
>>
>> Or am I reading it wrong?
>
> I don't think that's right.  When 'n' is 10 and 'i' == 0, the first
> element is swapped with a randomly chosen element, whose index is
> random_range(10).
>
> This is the standard shuffling algorithm (unless I implemented it wrong,
> which is possible).  When 'i' is 0, it randomly selects any of the
> elements in the array and makes that the first element.  When 'i' is 1,
> it randomly select any of the remaining elements in the array and makes
> that the second element.  In general, at each step, it randomly chooses
> any of the elements that haven't been chosen yet as the next element.
>
> Did I get it wrong?  It's easy to do that since shuffles are difficult
> to test.

No, I got it wrong.  Sorry.

Acked-by: Russell Bryant 

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 07/13] jsonrpc: Allow jsonrpc_session to have more than one remote.

2017-10-09 Thread Ben Pfaff

On Mon, Oct 09, 2017 at 03:57:18PM -0400, Russell Bryant wrote:
> On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> > The implementation cycles through the remotes in random order.  This allows
> > clients to perform some load balancing across alternative implementations
> > of a service.
> >
> > Signed-off-by: Ben Pfaff 
> > ---
> >  lib/jsonrpc.c | 53 -
> >  lib/jsonrpc.h |  6 +-
> >  lib/svec.c| 18 ++
> >  lib/svec.h|  1 +
> >  4 files changed, 72 insertions(+), 6 deletions(-)
> 
> > diff --git a/lib/svec.c b/lib/svec.c
> > index 297a60ce14f9..c1b986bab108 100644
> > --- a/lib/svec.c
> > +++ b/lib/svec.c
> > @@ -20,6 +20,7 @@
> >  #include 
> >  #include 
> >  #include "openvswitch/dynamic-string.h"
> > +#include "random.h"
> >  #include "util.h"
> >  #include "openvswitch/vlog.h"
> >
> > @@ -174,6 +175,23 @@ svec_compact(struct svec *svec)
> >  svec->n = j;
> >  }
> >
> > +static void
> > +swap_strings(char **a, char **b)
> > +{
> > +char *tmp = *a;
> > +*a = *b;
> > +*b = tmp;
> > +}
> > +
> > +void
> > +svec_shuffle(struct svec *svec)
> > +{
> > +for (size_t i = 0; i < svec->n; i++) {
> > +size_t j = i + random_range(svec->n - i);
> > +swap_strings(>names[i], >names[j]);
> > +}
> > +}
> > +
> 
> I'm not sure this is as random as we'd like.
> 
> Even if there are 10 elements, the first element has a 50% chance of
> staying there, since it's only considered for a swap when i == 0.
> That extends to the general behavior that the closer an element is to
> the beginning, the better chance it has of staying near the beginning.
> 
> Or am I reading it wrong?

I don't think that's right.  When 'n' is 10 and 'i' == 0, the first
element is swapped with a randomly chosen element, whose index is
random_range(10).

This is the standard shuffling algorithm (unless I implemented it wrong,
which is possible).  When 'i' is 0, it randomly selects any of the
elements in the array and makes that the first element.  When 'i' is 1,
it randomly select any of the remaining elements in the array and makes
that the second element.  In general, at each step, it randomly chooses
any of the elements that haven't been chosen yet as the next element.

Did I get it wrong?  It's easy to do that since shuffles are difficult
to test.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 13/13] json: New function json_object_put_format().

2017-10-09 Thread Russell Bryant

On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> This will acquire users in an upcoming commit.
>
> Signed-off-by: Ben Pfaff 
> ---
>  include/openvswitch/json.h |  6 +-
>  lib/json.c | 12 
>  2 files changed, 17 insertions(+), 1 deletion(-)

Acked-by: Russell Bryant 

Is it required to put the printf function attribute in both json.h and
json.c?  I figure it's harmless, but wasn't sure if it was necessary.

> diff --git a/include/openvswitch/json.h b/include/openvswitch/json.h
> index edf53e594eb0..61b9a02cfc19 100644
> --- a/include/openvswitch/json.h
> +++ b/include/openvswitch/json.h
> @@ -1,5 +1,5 @@
>  /*
> - * Copyright (c) 2009, 2010, 2015 Nicira, Inc.
> + * Copyright (c) 2009, 2010, 2015, 2016 Nicira, Inc.
>   *
>   * Licensed under the Apache License, Version 2.0 (the "License");
>   * you may not use this file except in compliance with the License.
> @@ -38,6 +38,7 @@ extern "C" {
>  #endif
>
>  struct ds;
> +struct uuid;
>
>  /* Type of a JSON value. */
>  enum json_type {
> @@ -92,6 +93,9 @@ struct json *json_object_create(void);
>  void json_object_put(struct json *, const char *name, struct json *value);
>  void json_object_put_string(struct json *,
>  const char *name, const char *value);
> +void json_object_put_format(struct json *,
> +const char *name, const char *format, ...)
> +OVS_PRINTF_FORMAT(3, 4);
>
>  const char *json_string(const struct json *);
>  struct json_array *json_array(const struct json *);
> diff --git a/lib/json.c b/lib/json.c
> index b98e60f87f4b..5e93190b8a03 100644
> --- a/lib/json.c
> +++ b/lib/json.c
> @@ -29,6 +29,7 @@
>  #include "openvswitch/shash.h"
>  #include "unicode.h"
>  #include "util.h"
> +#include "uuid.h"
>
>  /* The type of a JSON token. */
>  enum json_token_type {
> @@ -284,6 +285,17 @@ json_object_put_string(struct json *json, const char 
> *name, const char *value)
>  json_object_put(json, name, json_string_create(value));
>  }
>
> +void OVS_PRINTF_FORMAT(3, 4)
> +json_object_put_format(struct json *json,
> +   const char *name, const char *format, ...)
> +{
> +va_list args;
> +va_start(args, format);
> +json_object_put(json, name,
> +json_string_create_nocopy(xvasprintf(format, args)));
> +va_end(args);
> +}
> +
>  const char *
>  json_string(const struct json *json)
>  {
> --
> 2.10.2
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev



-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 12/13] json: New function json_nullable_clone().

2017-10-09 Thread Russell Bryant

On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 
> ---
>  include/openvswitch/json.h | 1 +
>  lib/json.c | 8 +++-
>  2 files changed, 8 insertions(+), 1 deletion(-)

Acked-by: Russell Bryant 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 11/13] uuid: New function uuid_random().

2017-10-09 Thread Russell Bryant

On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> Signed-off-by: Ben Pfaff 
> ---
>  lib/uuid.c | 8 
>  lib/uuid.h | 1 +
>  2 files changed, 9 insertions(+)

Acked-by: Russell Bryant 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 10/13] tests: Add support for 1-argument 'seq' in emulation.

2017-10-09 Thread Russell Bryant

On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> The testsuite has an emulation of the common utility 'seq' that only
> supported 2- and 3-argument forms.  This commit adds support for the
> 1-argument form.
>
> Signed-off-by: Ben Pfaff 

Acked-by: Russell Bryant 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 09/13] socket-util: Make parse_bracketed_token() public, as inet_parse_token().

2017-10-09 Thread Russell Bryant

On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> An upcoming commit will introduce a new user outside socket-util.
>
> Signed-off-by: Ben Pfaff 

Acked-by: Russell Bryant 

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 08/13] jsonrpc: Increment sequence number when connection actually made.

2017-10-09 Thread Russell Bryant

On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> The purpose of the sequence number is to allow the client to figure out
> when the connection status has changed.  The significant event for the
> client is when a connection completes, not when a connection attempt
> starts.  Thus, this commit changes the code to increment the sequence
> number at completion, not at the attempt.
>
> Signed-off-by: Ben Pfaff 

Acked-by: Russell Bryant 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 07/13] jsonrpc: Allow jsonrpc_session to have more than one remote.

2017-10-09 Thread Russell Bryant

On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> The implementation cycles through the remotes in random order.  This allows
> clients to perform some load balancing across alternative implementations
> of a service.
>
> Signed-off-by: Ben Pfaff 
> ---
>  lib/jsonrpc.c | 53 -
>  lib/jsonrpc.h |  6 +-
>  lib/svec.c| 18 ++
>  lib/svec.h|  1 +
>  4 files changed, 72 insertions(+), 6 deletions(-)

> diff --git a/lib/svec.c b/lib/svec.c
> index 297a60ce14f9..c1b986bab108 100644
> --- a/lib/svec.c
> +++ b/lib/svec.c
> @@ -20,6 +20,7 @@
>  #include 
>  #include 
>  #include "openvswitch/dynamic-string.h"
> +#include "random.h"
>  #include "util.h"
>  #include "openvswitch/vlog.h"
>
> @@ -174,6 +175,23 @@ svec_compact(struct svec *svec)
>  svec->n = j;
>  }
>
> +static void
> +swap_strings(char **a, char **b)
> +{
> +char *tmp = *a;
> +*a = *b;
> +*b = tmp;
> +}
> +
> +void
> +svec_shuffle(struct svec *svec)
> +{
> +for (size_t i = 0; i < svec->n; i++) {
> +size_t j = i + random_range(svec->n - i);
> +swap_strings(>names[i], >names[j]);
> +}
> +}
> +

I'm not sure this is as random as we'd like.

Even if there are 10 elements, the first element has a 50% chance of
staying there, since it's only considered for a swap when i == 0.
That extends to the general behavior that the closer an element is to
the beginning, the better chance it has of staying near the beginning.

Or am I reading it wrong?

Thanks,

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 06/13] reconnect: Add ability to do a number of retries without backoff.

2017-10-09 Thread Russell Bryant

On Fri, Oct 6, 2017 at 8:44 PM, Ben Pfaff  wrote:
> This is aimed at an upcoming database clustering implementation, where it's
> desirable to try all of the cluster members quickly before backing off to
> retry them again in sequence.
>
> Signed-off-by: Ben Pfaff 

Acked-by: Russell Bryant 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] Introduce Emeritus Committer status.

2017-10-09 Thread Russell Bryant

On Mon, Oct 9, 2017 at 1:42 PM, Ben Pfaff  wrote:
> On Mon, Oct 09, 2017 at 01:25:57PM -0400, Russell Bryant wrote:
>> On Sat, Oct 7, 2017 at 12:22 AM, Ben Pfaff  wrote:
>> > 2. Do we need a vote of the committers to approve this change?  I
>> >think that I would be more comfortable if we did have one.
>>
>> Yes, I think that would be best.
>>
>> Shall we record the vote as a set of acks on the patch?
>
> I think that would be harmless in this case, but I believe that it would
> set a bad precedent.  We do not use a secret voting system among the
> committers but we have never (as far as I know) released the details of
> a vote result to a wider community.  So I'd prefer, slightly, to do the
> vote on the committers list in the usual way.

OK, sure.  I didn't think about it enough since votes don't happen
often.  I didn't mean to stray from the norm.

-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] locks for clustered OVSDB

2017-10-09 Thread Ben Pfaff

On Mon, Oct 09, 2017 at 01:13:56PM -0400, Russell Bryant wrote:
> On Mon, Sep 25, 2017 at 2:29 PM, Ben Pfaff  wrote:
> > On Mon, Sep 25, 2017 at 11:09:49AM -0700, Han Zhou wrote:
> >> On Mon, Sep 25, 2017 at 2:36 AM, Miguel Angel Ajo Pelayo <
> >> majop...@redhat.com> wrote:
> >> >
> >> > I believe Lucas Alvares could give you valuable feedback on this as
> >> > he was planning to use this as a mechanism for synchronization on
> >> > the networking-ovn side (if I didn't get it wrong).
> >> >
> >> > I believe he's back by October.
> >> >
> >> > Best regards.
> >> > Miguel Ángel.
> >> >
> >> > On Fri, Sep 22, 2017 at 6:58 PM, Ben Pfaff  wrote:
> >> >
> >> > > We've had a couple of brief discussions during the OVN meeting about
> >> > > locks in OVSDB.  As I understand it, a few services use OVSDB locks to
> >> > > avoid duplicating work.  The question is whether and how to extend 
> >> > > OVSDB
> >> > > locks to a distributed context.
> >> > >
> >> > > First, I think it's worth reviewing how OVSDB locks work, filling in
> >> > > some of the implications that aren't covered by RFC 7047.  OVSDB locks
> >> > > are server-level (not database-level) objects that can be owned by at
> >> > > most one client at a time.  Clients can obtain them either through a
> >> > > "lock" operation, in which case they get queued to obtain the lock when
> >> > > it's no longer owned by anyone else, or through a "steal" operation 
> >> > > that
> >> > > always succeeds immediately, kicking out whoever (if anyone) previously
> >> > > owned the lock.  A client loses a lock whenever it releases it with an
> >> > > "unlock" operation or whenever its connection to the server drops.  The
> >> > > server notifies a client whenever it acquires a lock or whenever it is
> >> > > stolen by another client.
> >> > >
> >> > > This scheme works perfectly for one particular scenario: where the
> >> > > resource protected by the lock is an OVSDB database (or part of one) on
> >> > > the same server as the lock.  This is because OVSDB transactions 
> >> > > include
> >> > > an "assert" operation that names a lock and aborts the transaction if
> >> > > the client does not hold the lock.  Since the server is both the lock
> >> > > manager and the implementer of the transaction, it can always make the
> >> > > correct decision.  This scenario could be extended to distributed locks
> >> > > with the same guarantee.
> >> > >
> >> > > Another scenario that could work acceptably with distributed OVSDB 
> >> > > locks
> >> > > is one where the lock guards against duplicated work.  For example,
> >> > > suppose a couple of ovn-northd instances both try to grab a lock, with
> >> > > only the winner actually running, to avoid having both of them spend a
> >> > > lot of CPU time recomputing the southbound flow table.  A distributed
> >> > > version of OVSDB locks would probably work fine in practice for this,
> >> > > although occasionally due to network propagation delays, "steal"
> >> > > operations, or different ideas between client and server of when a
> >> > > session has dropped, both ovn-northd might think they have the lock.
> >> > > (If, however, they combined this with "assert" when they actually
> >> > > committed their changes to the southbound database, then they would
> >> > > never actually interfere with each other in database commits.)
> >> > >
> >> > > A scenario that would not work acceptably with distributed OVSDB locks,
> >> > > without a change to the model, is where the lock ensures correctness,
> >> > > that is, if two clients both think they have the lock then bad things
> >> > > happen.  I believe that this requires clients to understand a concept 
> >> > > of
> >> > > leases, which OVSDB doesn't currently have.  The "steal" operation is
> >> > > also problematic in this model since it would require canceling a
> >> > > lease.  (This scenario also does not work acceptably with single-server
> >> > > OVSDB locks.)
> >> > >
> >> > > I'd appreciate anyone's thoughts on the topic.
> >> > >
> >> > > This webpage is good reading:
> >> > >
> >> https://martin.kleppmann.com/2016/02/08/how-to-do-distributed-locking.html
> >> > >
> >> > > Thanks,
> >> > >
> >> > > Ben.
> >>
> >> Hi Ben,
> >>
> >> If I understand correctly, you are saying that the clustering wouldn't
> >> introduce any new restriction to the locking mechanism, comparing with the
> >> current single node implementation. Both new and old approach support
> >> avoiding redundant work, but not for correctness (unless "assert" or some
> >> other "fence" is used). Is this correct?
> >
> > It's accurate that clustering would not technically introduce new
> > restrictions.  It will increase race windows, especially over Unix
> > sockets, so anyone who is currently (incorrectly) relying on OVSDB
> > locking for correctness will probably start seeing failures that they
> > did not see before.  I'd be pleased to hear that no one is doing this.
> 
> You

Re: [ovs-dev] [PATCH] Introduce Emeritus Committer status.

2017-10-09 Thread Ben Pfaff

On Mon, Oct 09, 2017 at 01:25:57PM -0400, Russell Bryant wrote:
> On Sat, Oct 7, 2017 at 12:22 AM, Ben Pfaff  wrote:
> > 2. Do we need a vote of the committers to approve this change?  I
> >think that I would be more comfortable if we did have one.
> 
> Yes, I think that would be best.
> 
> Shall we record the vote as a set of acks on the patch?

I think that would be harmless in this case, but I believe that it would
set a bad precedent.  We do not use a secret voting system among the
committers but we have never (as far as I know) released the details of
a vote result to a wider community.  So I'd prefer, slightly, to do the
vote on the committers list in the usual way.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] util: Fix style in ovs_hex_dump().

2017-10-09 Thread Ben Pfaff

On Mon, Oct 09, 2017 at 01:15:01PM -0400, Russell Bryant wrote:
> On Sat, Oct 7, 2017 at 12:01 AM, Ben Pfaff  wrote:
> > Reported-by: Russell Bryant 
> > Signed-off-by: Ben Pfaff 
> 
> Thanks :-)
> 
> Acked-by: Russell Bryant 

Thanks for the review, applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] faq: Better document how to add vendor extensions.

2017-10-09 Thread Ben Pfaff

Signed-off-by: Ben Pfaff 
---
 Documentation/faq/contributing.rst | 44 +++---
 include/openvswitch/ofp-errors.h   |  4 +++-
 2 files changed, 34 insertions(+), 14 deletions(-)

diff --git a/Documentation/faq/contributing.rst 
b/Documentation/faq/contributing.rst
index 6dfc8bc4d436..d59376cd615c 100644
--- a/Documentation/faq/contributing.rst
+++ b/Documentation/faq/contributing.rst
@@ -33,22 +33,28 @@ Q: How do I implement a new OpenFlow message?
 as needed.  (If you configure with ``--enable-Werror``, as described in
 :doc:`/intro/install/general`, then it is impossible to miss any warnings.)
 
-If you need to add an OpenFlow vendor extension message for a vendor that
-doesn't yet have any extension messages, then you will also need to edit
-``build-aux/extract-ofp-msgs``.
+To add an OpenFlow vendor extension message (aka experimenter message) for
+a vendor that doesn't yet have any extension messages, you will also need
+to edit ``build-aux/extract-ofp-msgs`` and at least ``ofphdrs_decode()``
+and ``ofpraw_put__()`` in ``lib/ofp-msgs.c``.  OpenFlow doesn't standardize
+vendor extensions very well, so it's hard to make the process simpler than
+that.  (If you have a choice of how to design your vendor extension
+messages, it will be easier if you make them resemble the ONF and OVS
+extension messages.)
 
 Q: How do I add support for a new field or header?
 
 A: Add new members for your field to ``struct flow`` in ``lib/flow.h``, and
 add new enumerations for your new field to ``enum mf_field_id`` in
-``lib/meta-flow.h``, following the existing pattern.  Also, add support to
-``miniflow_extract()`` in ``lib/flow.c`` for extracting your new field from
-a packet into struct miniflow, and to ``nx_put_raw()`` in
-``lib/nx-match.c`` to output your new field in OXM matches.  Then recompile
-and fix all of the new warnings, implementing new functionality for the new
-field or header as needed.  (If you configure with ``--enable-Werror``, as
-described in :doc:`/intro/install/general`, then it is impossible to miss
-any warnings.)
+``lib/meta-flow.h``, following the existing pattern.  If the field uses a
+new OXM class, add it to OXM_CLASSES in ``build-aux/extract-ofp-fields``.
+Also, add support to ``miniflow_extract()`` in ``lib/flow.c`` for
+extracting your new field from a packet into struct miniflow, and to
+``nx_put_raw()`` in ``lib/nx-match.c`` to output your new field in OXM
+matches.  Then recompile and fix all of the new warnings, implementing new
+functionality for the new field or header as needed.  (If you configure
+with ``--enable-Werror``, as described in :doc:`/intro/install/general`,
+then it is impossible to miss any warnings.)
 
 If you want kernel datapath support for your new field, you also need to
 modify the kernel module for the operating systems you are interested in.
@@ -71,5 +77,17 @@ Q: How do I add support for a new OpenFlow action?
 warnings.)
 
 If you need to add an OpenFlow vendor extension action for a vendor that
-doesn't yet have any extension actions, then you will also need to edit
-``build-aux/extract-ofp-actions``.
+doesn't yet have any extension actions, then you will also need to add the
+vendor to ``vendor_map`` in ``build-aux/extract-ofp-actions``.  Also, you
+will need to add support for the vendor to ``ofpact_decode_raw()`` and
+``ofpact_put_raw()`` in ``lib/ofp-actions.c``.  (If you have a choice of
+how to design your vendor extension actions, it will be easier if you make
+them resemble the ONF and OVS extension actions.)
+
+Q: How do I add support for a new OpenFlow error message?
+
+A: Add your new error to ``enum ofperr`` in
+``include/openvswitch/ofp-errors.h``.  Read the large comment at the top of
+the file for details.  If you need to add an OpenFlow vendor extension
+error for a vendor that doesn't yet have any, first add the vendor ID to
+the ``_VENDOR_ID`` list in ``include/openflow/openflow-common.h``.
diff --git a/include/openvswitch/ofp-errors.h b/include/openvswitch/ofp-errors.h
index 3d09be354af9..5e20e1adb7cd 100644
--- a/include/openvswitch/ofp-errors.h
+++ b/include/openvswitch/ofp-errors.h
@@ -60,7 +60,9 @@ struct ofpbuf;
  * type, and sometimes a code for each protocol that supports the error:
  *
  * # The vendor is OF for standard OpenFlow error codes.  Otherwise it
- *   is one of the *_VENDOR_ID codes defined in openflow-common.h.
+ *   is one of the *_VENDOR_ID codes defined in openflow-common.h.  (To
+ *   add support for a new vendor, add a VENDOR_ID code to that
+ *   header.)
  *
  * # The version can specify a specific OpenFlow version, a version
  *   range delimited by "-", or an open-ended range with "+".
-- 
2.10.2

[ovs-dev] [PATCH v2] Introduce Emeritus Committer status.

2017-10-09 Thread Russell Bryant

This patch introduces an Emeritus status for OVS committers. An
Emeritus Committer is recognized as having made a significant impact
to the project and having been a committer in the past.  It is
intended as an option for those that do not currently have the time or
interest to fulfill committer responsibilities based on their current
responsibilities.  While in this status, they are not included in
voting for governance purposes.

An emeritus committer may be re-instated as a full committer at any
time.

See documentation contents for full details.

Suggested-by: Ethan J. Jackson 
Signed-off-by: Russell Bryant 
---
 Documentation/automake.mk  |  1 +
 Documentation/index.rst|  3 +-
 .../internals/committer-emeritus-status.rst| 63 ++
 Documentation/internals/index.rst  |  1 +
 MAINTAINERS.rst| 14 -
 5 files changed, 80 insertions(+), 2 deletions(-)
 create mode 100644 Documentation/internals/committer-emeritus-status.rst


v1->v2:
 - incorporate suggested changes from Ben.


diff --git a/Documentation/automake.mk b/Documentation/automake.mk
index 6f38912f2..8adce852e 100644
--- a/Documentation/automake.mk
+++ b/Documentation/automake.mk
@@ -78,6 +78,7 @@ DOC_SOURCE = \
Documentation/internals/index.rst \
Documentation/internals/authors.rst \
Documentation/internals/bugs.rst \
+   Documentation/internals/committer-emeritus-status.rst \
Documentation/internals/committer-grant-revocation.rst \
Documentation/internals/committer-responsibilities.rst \
Documentation/internals/documentation.rst \
diff --git a/Documentation/index.rst b/Documentation/index.rst
index 836c37fc3..b7a792b0d 100644
--- a/Documentation/index.rst
+++ b/Documentation/index.rst
@@ -107,7 +107,8 @@ Learn more about the Open vSwitch project and about how you 
can contribute:
 
 - **Maintaining:** :doc:`internals/maintainers` |
   :doc:`internals/committer-responsibilities` |
-  :doc:`internals/committer-grant-revocation`
+  :doc:`internals/committer-grant-revocation` |
+  :doc:`internals/committer-emeritus-status`
 
 - **Documentation:** :doc:`internals/contributing/documentation-style` |
   :doc:`Building Open vSwitch Documentation ` |
diff --git a/Documentation/internals/committer-emeritus-status.rst 
b/Documentation/internals/committer-emeritus-status.rst
new file mode 100644
index 0..ad9dca2c3
--- /dev/null
+++ b/Documentation/internals/committer-emeritus-status.rst
@@ -0,0 +1,63 @@
+..
+  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.
+
+  Convention for heading levels in Open vSwitch documentation:
+
+  ===  Heading 0 (reserved for the title in a document)
+  ---  Heading 1
+  ~~~  Heading 2
+  +++  Heading 3
+  '''  Heading 4
+
+  Avoid deeper levels because they do not render well.
+
+==
+Emeritus Status for OVS Committers
+==
+
+OVS committers are nominated and elected based on their impact on the Open
+vSwitch project.  Over time, as committers' responsibilities change, some may
+become unable or uninterested to actively participate in project governance.
+Committer "emeritus" status provides a way for committers to take a leave of
+absence from OVS governance responsibilities.  The following guidelines clarify
+the process around the emeritus status for committers:
+
+* An committer may choose to transition from active to emeritus, or from
+  emeritus to active, by sending an email to the committers mailing list.
+
+* If a committer hasn't been heard from in 6 months, and does not respond to
+  reasonable attempts to contact him or her, the other committers can vote as a
+  majority to transition the committer from active to emeritus.  (If the
+  committer resurfaces, he or she can transition back to active by sending an
+  email to the committers mailing list.)
+
+* Emeritus committers may stay on the committers mailing list to continue to
+  follow any discussions there.
+
+* Emeritus committers do not nominate or vote in committer elections.  From a
+  governance perspective, they are equivalent to a non-committer.
+
+* Emeritus committers cannot merge patches to the OVS repository.
+
+* Emeritus committers will be listed in a separate section in the
+

Re: [ovs-dev] [PATCH] Introduce Emeritus Committer status.

2017-10-09 Thread Russell Bryant

On Sat, Oct 7, 2017 at 12:22 AM, Ben Pfaff  wrote:
> On Fri, Oct 06, 2017 at 08:19:04PM -0400, Russell Bryant wrote:
>> This patch introduces an Emeritus status for OVS committers. An
>> Emeritus Committer is recognized as having made a significant impact
>> to the project and having been a committer in the past.  It is
>> intended as an option for those that do not currently have the time or
>> interest to fulfill committer responsibilities based on their current
>> responsibilities.  While in this status, they are not included in
>> voting for governance purposes.
>>
>> An emeritus committer may be re-instated as a full committer at any
>> time.
>>
>> See documentation contents for full details.
>>
>> Suggested-by: Ethan J. Jackson 
>> Signed-off-by: Russell Bryant 
>
> Thank you.
>
> I felt like doing some editing.  Here is my version.  You don't have to
> use it though.

I like your edits.  I'll use them and post a v2.

>
> I have procedural questions:
>
> 1. Do we need to get Linux Foundation approval for this change?  My
>inclination is to believe that we do not, because 2.c.iii in the
>project charter says that the committers may "amend, adjust and
>refine the roles of Contributors and Committers listed in Section
>2.b., create new roles and publicly document responsibilities and
>expectations for such roles, as it sees fit" and I think that
>this falls squarely in that category.

For others following along, see http://openvswitch.org/charter/

I agree with your interpretation.

> 2. Do we need a vote of the committers to approve this change?  I
>think that I would be more comfortable if we did have one.

Yes, I think that would be best.

Shall we record the vote as a set of acks on the patch?

>
> Thanks,
>
> Ben.
>
> --8<--cut here-->8--
>
> OVS committers are nominated and elected based on their impact on the Open
> vSwitch project.  Over time, as committers' responsibilities change, some may
> become unable or uninterested to actively participate in project governance.
> Committer "emeritus" status provides a way for committers to take a leave of
> absence from OVS governance responsibilities.  The following guidelines 
> clarify
> the process around the emeritus status for committers:
>
> * An committer may choose to transition from active to emeritus, or from
>   emeritus to active, by sending an email to the committers mailing list.
>
> * If a committer hasn't been heard from in 6 months, and does not respond to
>   reasonable attempts to contact him or her, the other committers can vote as 
> a
>   majority to transition the committer from active to emeritus.  (If the
>   committer resurfaces, he or she can transition back to active by sending an
>   email to the committers mailing list.)
>
> * Emeritus committers may stay on the committers mailing list to continue to
>   follow any discussions there.
>
> * Emeritus committers do not nominate or vote in committer elections.  From a
>   governance perspective, they are equivalent to a non-committer.
>
> * Emeritus committers cannot merge patches to the OVS repository.
>
> * Emeritus committers will be listed in a separate section in the
>   MAINTAINERS.rst file to continue to recognize their contributions to the
>   project.
>
> Emeritus status does not replace the procedures for forcibly removing a
> committer.
>
> Note that just because a committer is not able to work on the project on a
> day-to-day basis, we feel they are still capable of providing input on the
> direction of the project.  No committer should feel pressured to move
> themselves to this status.  Again, it's just an option for those that do not
> currently have the time or interest.
>



-- 
Russell Bryant
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] util: Fix style in ovs_hex_dump().

2017-10-09 Thread Russell Bryant

On Sat, Oct 7, 2017 at 12:01 AM, Ben Pfaff  wrote:
> Reported-by: Russell Bryant 
> Signed-off-by: Ben Pfaff 

Thanks :-)

Acked-by: Russell Bryant 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] locks for clustered OVSDB

2017-10-09 Thread Russell Bryant

On Mon, Sep 25, 2017 at 2:29 PM, Ben Pfaff  wrote:
> On Mon, Sep 25, 2017 at 11:09:49AM -0700, Han Zhou wrote:
>> On Mon, Sep 25, 2017 at 2:36 AM, Miguel Angel Ajo Pelayo <
>> majop...@redhat.com> wrote:
>> >
>> > I believe Lucas Alvares could give you valuable feedback on this as
>> > he was planning to use this as a mechanism for synchronization on
>> > the networking-ovn side (if I didn't get it wrong).
>> >
>> > I believe he's back by October.
>> >
>> > Best regards.
>> > Miguel Ángel.
>> >
>> > On Fri, Sep 22, 2017 at 6:58 PM, Ben Pfaff  wrote:
>> >
>> > > We've had a couple of brief discussions during the OVN meeting about
>> > > locks in OVSDB.  As I understand it, a few services use OVSDB locks to
>> > > avoid duplicating work.  The question is whether and how to extend OVSDB
>> > > locks to a distributed context.
>> > >
>> > > First, I think it's worth reviewing how OVSDB locks work, filling in
>> > > some of the implications that aren't covered by RFC 7047.  OVSDB locks
>> > > are server-level (not database-level) objects that can be owned by at
>> > > most one client at a time.  Clients can obtain them either through a
>> > > "lock" operation, in which case they get queued to obtain the lock when
>> > > it's no longer owned by anyone else, or through a "steal" operation that
>> > > always succeeds immediately, kicking out whoever (if anyone) previously
>> > > owned the lock.  A client loses a lock whenever it releases it with an
>> > > "unlock" operation or whenever its connection to the server drops.  The
>> > > server notifies a client whenever it acquires a lock or whenever it is
>> > > stolen by another client.
>> > >
>> > > This scheme works perfectly for one particular scenario: where the
>> > > resource protected by the lock is an OVSDB database (or part of one) on
>> > > the same server as the lock.  This is because OVSDB transactions include
>> > > an "assert" operation that names a lock and aborts the transaction if
>> > > the client does not hold the lock.  Since the server is both the lock
>> > > manager and the implementer of the transaction, it can always make the
>> > > correct decision.  This scenario could be extended to distributed locks
>> > > with the same guarantee.
>> > >
>> > > Another scenario that could work acceptably with distributed OVSDB locks
>> > > is one where the lock guards against duplicated work.  For example,
>> > > suppose a couple of ovn-northd instances both try to grab a lock, with
>> > > only the winner actually running, to avoid having both of them spend a
>> > > lot of CPU time recomputing the southbound flow table.  A distributed
>> > > version of OVSDB locks would probably work fine in practice for this,
>> > > although occasionally due to network propagation delays, "steal"
>> > > operations, or different ideas between client and server of when a
>> > > session has dropped, both ovn-northd might think they have the lock.
>> > > (If, however, they combined this with "assert" when they actually
>> > > committed their changes to the southbound database, then they would
>> > > never actually interfere with each other in database commits.)
>> > >
>> > > A scenario that would not work acceptably with distributed OVSDB locks,
>> > > without a change to the model, is where the lock ensures correctness,
>> > > that is, if two clients both think they have the lock then bad things
>> > > happen.  I believe that this requires clients to understand a concept of
>> > > leases, which OVSDB doesn't currently have.  The "steal" operation is
>> > > also problematic in this model since it would require canceling a
>> > > lease.  (This scenario also does not work acceptably with single-server
>> > > OVSDB locks.)
>> > >
>> > > I'd appreciate anyone's thoughts on the topic.
>> > >
>> > > This webpage is good reading:
>> > >
>> https://martin.kleppmann.com/2016/02/08/how-to-do-distributed-locking.html
>> > >
>> > > Thanks,
>> > >
>> > > Ben.
>>
>> Hi Ben,
>>
>> If I understand correctly, you are saying that the clustering wouldn't
>> introduce any new restriction to the locking mechanism, comparing with the
>> current single node implementation. Both new and old approach support
>> avoiding redundant work, but not for correctness (unless "assert" or some
>> other "fence" is used). Is this correct?
>
> It's accurate that clustering would not technically introduce new
> restrictions.  It will increase race windows, especially over Unix
> sockets, so anyone who is currently (incorrectly) relying on OVSDB
> locking for correctness will probably start seeing failures that they
> did not see before.  I'd be pleased to hear that no one is doing this.

You discussed the ovn-northd use case in your original post (thanks!).

The existing Neutron integration use case should be fine.  In that
case, it's not committing any transactions.  The lock is only used to
ensure that only one server is processing logical switch port "up"
state.  If more than one

[ovs-dev] Black Tigers Peeled and HLSO

2017-10-09 Thread Bonesca Import en Export BV

    [ View in browser ]( http://r.newsletter.bonescamail.nl/7xa28k6seoatrf.html 
)   
 
[
]( http://r.newsletter.bonescamail.nl/track/click/vp48y8r59aoatrd ) 
 
SPECIAL PROMO JONA & BONEMER 
 
NEW ARRIVALS BLACK TIGER SHRIMPS FROM BANGLADESH :
 
JONA PEELED DEVEINED BLACK TIGER SHRIMPS SIZE 26/30 
10 X 1000 GRS+ / 700 GRS+ NETTO
1 BOX 10.25 - 10 BOX 9.75 - 1 PALET 9.25 - 3 PALETS € 8,95 P/KG!!!
 
BONEMER HEADLESS SHELL ON EASY PEEL BLACK TIGER SHRIMPS
10 X 1000 GRS+ / 650 GRS+ NETTO
16/20 1 BOX 9.50 - 10 BOX 8.95 - 1 PALET 8.50 - 3 PALETS € 8,25 P/KG!!!
26/30 1 BOX 7.95 - 10 BOX 7.50 - 1 PALET 6.95 P/KG!!!
 
Scroll down to find some pictures, frozen and thawed.    












































   Click here for complete overview latest offers
Haga clic aquí para ver la lista completa de las últimas ofertas
Klicken Sie hier für die komplette Liste der letzten Angebote
Klik hier voor het complete overzicht recente aanbiedngen
Cliquez ici pour la liste complète des offres récentes   
 

Kind regards,
 
Bertus Brouwer
Director & Purchase - Dutch / English / German
Tel.: +31 (0) 527 70 10 63
Mail: ber...@bonesca.nl
 
Nedal Muhtaseb
Sales - Dutch / Arab / English / German
Tel: +31 (0) 527 68 07 83
Mail: ne...@bonesca.nl
 
Marianne Kramer
Sales - Dutch / French / English / German
Tel: +31 (0) 527 68 07 82
Mail: maria...@bonesca.nl
 
Henry Bakker
Administration/Planning
Tel: +31 (0) 527 68 07 85
Mail: he...@bonesca.nl; i...@bonesca.nl;invo...@bonesca.nl
 
Thu Hanh
Sales Vietnamese / Thai / Laothian / German / English
Tel: +49 (0) 32221097676
Tel: +31 (0) 527 68 07 88
Mail: t...@bonesca.nl
 
Ramesh Raja
Sales Tamil / Dutch / English
Tel: +31 (0) 527 68 07 84
Mail: r...@bonesca.nl

Policarpo J. Olivas
Sales Spanish, French, Portugese, Italian, English
Tel Spain: +34 (0) 649 566 367
Mail: poli...@bonesca.nl
 
Marberta Korf
Office assistant Dutch / English
Tel: +31 (0) 527 68 07 86
Mail:marbe...@bonesca.nl
  Warehouse 
Tel: +31 (0) 527 68 07 89
Mail: expedi...@bonesca.nl     
This email was sent to d...@openvswitch.org
You received this email because you are registered with Bonesca Import en 
Export BV
 
[ Unsubscribe here ]( http://r.newsletter.bonescamail.nl/7xa28k6seoatrg.html )  

Sent by
[  ]( http://r.newsletter.bonescamail.nl/track/click/vp48y8r61qoatrd )     
© 2017 Bonesca Import en Export BV  

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [ovs-discuss] Internal Ports and Queues

2017-10-09 Thread Ben Pfaff

On Fri, Oct 06, 2017 at 07:24:15PM -0500, Prabhu wrote:
> I am evaluating openvswitch for implementing a load balancing based on Queue
> length.
> 
> I know OVS can set queue to ports attached to the bridge using linux tc
> utility.
> 
> I would like to know whether I can assign queues to Internal ports to
> measure the backlog.Before I split the traffic to two interfaces using group
> select bucket method.

I don't think that internal ports have queues.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] SWIFT COPY OF PAYMENT

2017-10-09 Thread alsoja trading

Urgent!!! please confirm below payment . 

VIEW BELOW | DOWNLOAD BELOW 

:102KB SWIFT COPY OF PAYMENT. 

 SAMUEL KOMORA. 

Medical Department 
Jubilee Insurance Company of Kenya 

Mob: +254 709901176
Tel: +254 20 3281176 

WWW.JUBILEEINSURANCE.COM [1] 

 

Links:
--
[1] http://www.jubileeinsurance.com/___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] netdev-dpdk: New option 'hw_strip_crc' fields for DPDK interfaces

2017-10-09 Thread Gao Zhenyu

Thanks for working on it.

I want to know if we can have a patch to try one more time if we hit the
hw_strip_crc error since we had introduce many options in ovs-dpdk now.
(Just like function dpdk_eth_dev_queue_setup, it figures out queue number
by retrying)

Thanks
Zhenyu Gao

2017-10-09 17:09 GMT+08:00 :

> From: wenxu 
>
> Some vf driver like i40evf can't disable the hw_strip_crc, with err
> 'dpdk|ERR|i40evf_dev_configure(): VF can't disable HW CRC Strip'
> so hw_strip_crc feature should can be configurable
>
> Signed-off-by: wenxu 
> ---
>  NEWS |  2 ++
>  lib/netdev-dpdk.c| 30 ++
>  vswitchd/vswitch.xml |  9 +
>  3 files changed, 41 insertions(+)
>
> diff --git a/NEWS b/NEWS
> index a3c1a02..d913847 100644
> --- a/NEWS
> +++ b/NEWS
> @@ -5,6 +5,8 @@ Post-v2.8.0
> chassis "hostname" in addition to a chassis "name".
> - Linux kernel 4.13
>   * Add support for compiling OVS with the latest Linux 4.13 kernel
> +   - DPDK:
> + * New option 'hw_strip_crc' fields for DPDK interfaces
>
>  v2.8.0 - xx xxx 
>  -
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index c60f46f..23c2a72 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -402,11 +402,15 @@ struct netdev_dpdk {
>  int requested_n_rxq;
>  int requested_rxq_size;
>  int requested_txq_size;
> +bool requested_hw_strip_crc;
>
>  /* Number of rx/tx descriptors for physical devices */
>  int rxq_size;
>  int txq_size;
>
> +/*crc strip feature config*/
> +bool hw_strip_crc;
> +
>  /* Socket ID detected when vHost device is brought up */
>  int requested_socket_id;
>
> @@ -700,6 +704,7 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int
> n_rxq, int n_txq)
>
>  conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
>NETDEV_RX_CHECKSUM_OFFLOAD) != 0;
> +conf.rxmode.hw_strip_crc = dev->hw_strip_crc;
>  /* A device may report more queues than it makes available (this has
>   * been observed for Intel xl710, which reserves some of them for
>   * SRIOV):  rte_eth_*_queue_setup will fail if a queue is not
> @@ -914,6 +919,8 @@ common_construct(struct netdev *netdev, dpdk_port_t
> port_no,
>  dev->requested_n_txq = NR_QUEUE;
>  dev->requested_rxq_size = NIC_PORT_DEFAULT_RXQ_SIZE;
>  dev->requested_txq_size = NIC_PORT_DEFAULT_TXQ_SIZE;
> +dev->requested_hw_strip_crc = false;
> +dev->hw_strip_crc = false;
>
>  /* Initialize the flow control to NULL */
>  memset(>fc_conf, 0, sizeof dev->fc_conf);
> @@ -1192,6 +1199,11 @@ netdev_dpdk_get_config(const struct netdev *netdev,
> struct smap *args)
>  } else {
>  smap_add(args, "rx_csum_offload", "false");
>  }
> +if (dev->hw_strip_crc) {
> +smap_add(args, "hw_strip_crc", "true");
> +} else {
> +smap_add(args, "hw_strip_crc", "false");
> +}
>  }
>  ovs_mutex_unlock(>mutex);
>
> @@ -1255,6 +1267,19 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const
> struct smap *args)
>  }
>
>  static void
> +dpdk_set_hw_strip_crc_config(struct netdev_dpdk *dev, const struct smap
> *args)
> + OVS_REQUIRES(dev->mutex)
> +{
> +int new_hw_strip_crc;
> +
> +new_hw_strip_crc = smap_get_bool(args, "hw_strip_crc", false);
> +if (new_hw_strip_crc != dev->requested_hw_strip_crc) {
> +dev->requested_hw_strip_crc = new_hw_strip_crc;
> +netdev_request_reconfigure(>up);
> +}
> +}
> +
> +static void
>  dpdk_process_queue_size(struct netdev *netdev, const struct smap *args,
>  const char *flag, int default_size, int *new_size)
>  {
> @@ -1290,6 +1315,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const
> struct smap *args,
>
>  dpdk_set_rxq_config(dev, args);
>
> +dpdk_set_hw_strip_crc_config(dev, args);
> +
>  dpdk_process_queue_size(netdev, args, "n_rxq_desc",
>  NIC_PORT_DEFAULT_RXQ_SIZE,
>  >requested_rxq_size);
> @@ -3196,6 +3223,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>  if (netdev->n_txq == dev->requested_n_txq
>  && netdev->n_rxq == dev->requested_n_rxq
>  && dev->mtu == dev->requested_mtu
> +&& dev->hw_strip_crc == dev->requested_hw_strip_crc
>  && dev->rxq_size == dev->requested_rxq_size
>  && dev->txq_size == dev->requested_txq_size
>  && dev->socket_id == dev->requested_socket_id) {
> @@ -3217,6 +3245,8 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
>  dev->rxq_size = dev->requested_rxq_size;
>  dev->txq_size = dev->requested_txq_size;
>
> +dev->hw_strip_crc = dev->requested_hw_strip_crc;
> +
>  rte_free(dev->tx_q);
>  err = dpdk_eth_dev_init(dev);
>  dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
> diff --git a/vswitchd/vswitch.xml

Re: [ovs-dev] [PATCH v2 0/4] Conntrack: add commands to r/w CT parameters.

2017-10-09 Thread Fischetti, Antonio

Thanks Kevin, I'll rework a v3.

Antonio

> -Original Message-
> From: Kevin Traynor [mailto:ktray...@redhat.com]
> Sent: Tuesday, October 3, 2017 11:11 AM
> To: Fischetti, Antonio ; d...@openvswitch.org
> Subject: Re: [ovs-dev] [PATCH v2 0/4] Conntrack: add commands to r/w CT
> parameters.
> 
> On 10/03/2017 10:11 AM, Fischetti, Antonio wrote:
> > Thanks Kevin, comments inline.
> >
> > -Antonio
> >
> >> -Original Message-
> >> From: Kevin Traynor [mailto:ktray...@redhat.com]
> >> Sent: Monday, October 2, 2017 11:46 AM
> >> To: Fischetti, Antonio ; d...@openvswitch.org
> >> Subject: Re: [ovs-dev] [PATCH v2 0/4] Conntrack: add commands to r/w CT
> >> parameters.
> >>
> >> On 09/26/2017 01:35 PM, antonio.fische...@intel.com wrote:
> >>> This series adds two new commands to allow read/write of
> >>> some of the CT configuration parameters. This could be
> >>> used for maintenance purposes or to find a better tuning
> >>> of the current setup.
> >>>
> >>
> >> Hi Antonio. I don't think that helps people not too familiar with
> >> conntrack understand why the commands are needed and what cases they
> >> will help with.
> >
> > [Antonio]
> > I can rephrase it like:
> > This change comes from the consideration that when the CT is enabled
> > the overall performance can be deeply affected, even with simple
> > firewall rules and with stateless protocols like UDP.
> > This implementation adds a basic infrastructure that allows the user
> > to adjust the CT configuration parameters at run-time in order to
> > find a better tuning.
> > For example - depending on the traffic profile - the user could decrease
> > at run-time the maximum number of tracked connections, so to mitigate
> > the impact on performance.
> >
> 
> Sounds much better, thanks.
> 
> >
> >> Also, I think there should be some documentation to
> >> guide the user on when to use the new commands.
> >
> > [Antonio]
> > Sure, I'll update the dpctl.man and possibly other docs too, like some
> > new doc inside Documentation/howto/ ?
> > If you think other docs should be updated/added please let me know.
> >
> 
> You could add to the 'performance tuning' section if it's just about
> getting better performance. I don't really mind where, just that user
> has enough info to know what they are and why they would use them.
> 
> thanks,
> Kevin.
> 
> >> I'm not making comment
> >> on the usefulness or not of the commands but there's a need to explain
> >> why you are making the changes and guide the user on them.
> >>
> >> thanks,
> >> Kevin.
> >>
> >>> V2: Reworked based on comments.
> >>> V1: First implementation.
> >>>
> >>> Fischetti, Antonio (4):
> >>>   dpctl: Add a comment to functions retrieving the datapath name.
> >>>   conntrack: add commands to r/w CT parameters.
> >>>   conntrack: r/w upper limit connection value.
> >>>   conntrack: read current nr of connections.
> >>>
> >>>  lib/conntrack.c |  90 +
> >>>  lib/conntrack.h |   3 ++
> >>>  lib/ct-dpif.c   |  28 ++
> >>>  lib/ct-dpif.h   |   2 +
> >>>  lib/dpctl.c | 104
> >> +++-
> >>>  lib/dpif-netdev.c   |  19 ++
> >>>  lib/dpif-netlink.c  |   2 +
> >>>  lib/dpif-provider.h |   4 ++
> >>>  8 files changed, 251 insertions(+), 1 deletion(-)
> >>>
> >

___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 2/6] netdev-dpdk: Fix mempool names to reflect socket id.

2017-10-09 Thread Loftus, Ciara

> 
> Create mempool names by considering also the NUMA socket number.
> So a name reflects on what socket the mempool is allocated on.
> This change is needed for the NUMA-awareness feature.
> 
> CC: Kevin Traynor 
> CC: Aaron Conole 
> Reported-by: Ciara Loftus 
> Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each
> port.")
> Signed-off-by: Antonio Fischetti 
> ---
> Mempool names now contains the requested socket id and become like:
> "ovs_4adb057e_1_2030_20512".
> 
> Tested with DPDK 17.05.2 (from dpdk-stable branch).
> NUMA-awareness feature enabled (DPDK/config/common_base).
> 
> Created 1 single dpdkvhostuser port type.
> OvS pmd-cpu-mask=FF3 # enable cores on both numa nodes
> QEMU core mask = 0xFC000 # cores for qemu on numa node 1 only
> 
>  Before launching the VM:
>  
> ovs-appctl dpif-netdev/pmd-rxq-show
> shows core #1 is serving the vhu port.
> 
> pmd thread numa_id 0 core_id 1:
> isolated : false
> port: dpdkvhostuser0queue-id: 0
> 
>  After launching the VM:
>  ---
> the vhu port is now managed by core #27
> pmd thread numa_id 1 core_id 27:
> isolated : false
> port: dpdkvhostuser0queue-id: 0
> 
> and the log shows a new mempool is allocated on NUMA node 1, while
> the previous one is deleted:
> 
> 2017-10-06T14:04:55Z|00105|netdev_dpdk|DBG|Allocated
> "ovs_4adb057e_1_2030_20512" mempool with 20512 mbufs
> 2017-10-06T14:04:55Z|00106|netdev_dpdk|DBG|Releasing
> "ovs_4adb057e_0_2030_20512" mempool
> ---
>  lib/netdev-dpdk.c | 9 +
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 80a6ff3..0cf47cb 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -499,8 +499,8 @@ dpdk_mp_name(struct dpdk_mp *dmp)
>  {
>  uint32_t h = hash_string(dmp->if_name, 0);
>  char *mp_name = xcalloc(RTE_MEMPOOL_NAMESIZE, sizeof *mp_name);
> -int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
> "ovs_%x_%d_%u",
> -   h, dmp->mtu, dmp->mp_size);
> +int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE,
> "ovs_%x_%d_%d_%u",
> +   h, dmp->socket_id, dmp->mtu, dmp->mp_size);
>  if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
>  return NULL;
>  }
> @@ -534,9 +534,10 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu,
> bool *mp_exists)
>  char *mp_name = dpdk_mp_name(dmp);
> 
>  VLOG_DBG("Requesting a mempool of %u mbufs for netdev %s "
> - "with %d Rx and %d Tx queues.",
> + "with %d Rx and %d Tx queues, socket id:%d.",
>   dmp->mp_size, dev->up.name,
> - dev->requested_n_rxq, dev->requested_n_txq);
> + dev->requested_n_rxq, dev->requested_n_txq,
> + dev->requested_socket_id);
> 
>  dmp->mp = rte_pktmbuf_pool_create(mp_name, dmp->mp_size,
>MP_CACHE_SZ,
> --
> 2.4.11

Thanks for this fix Antonio. I've verified that the vHost User NUMA Awareness 
feature works again with this change.

Tested-by: Ciara Loftus 

> 
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] netdev-dpdk: New option 'hw_strip_crc' fields for DPDK interfaces

2017-10-09 Thread wenxu

From: wenxu 

Some vf driver like i40evf can't disable the hw_strip_crc, with err
'dpdk|ERR|i40evf_dev_configure(): VF can't disable HW CRC Strip'
so hw_strip_crc feature should can be configurable

Signed-off-by: wenxu 
---
 NEWS |  2 ++
 lib/netdev-dpdk.c| 30 ++
 vswitchd/vswitch.xml |  9 +
 3 files changed, 41 insertions(+)

diff --git a/NEWS b/NEWS
index a3c1a02..d913847 100644
--- a/NEWS
+++ b/NEWS
@@ -5,6 +5,8 @@ Post-v2.8.0
chassis "hostname" in addition to a chassis "name".
- Linux kernel 4.13
  * Add support for compiling OVS with the latest Linux 4.13 kernel
+   - DPDK:
+ * New option 'hw_strip_crc' fields for DPDK interfaces
 
 v2.8.0 - xx xxx 
 -
diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index c60f46f..23c2a72 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -402,11 +402,15 @@ struct netdev_dpdk {
 int requested_n_rxq;
 int requested_rxq_size;
 int requested_txq_size;
+bool requested_hw_strip_crc;
 
 /* Number of rx/tx descriptors for physical devices */
 int rxq_size;
 int txq_size;
 
+/*crc strip feature config*/
+bool hw_strip_crc;
+
 /* Socket ID detected when vHost device is brought up */
 int requested_socket_id;
 
@@ -700,6 +704,7 @@ dpdk_eth_dev_queue_setup(struct netdev_dpdk *dev, int 
n_rxq, int n_txq)
 
 conf.rxmode.hw_ip_checksum = (dev->hw_ol_features &
   NETDEV_RX_CHECKSUM_OFFLOAD) != 0;
+conf.rxmode.hw_strip_crc = dev->hw_strip_crc;
 /* A device may report more queues than it makes available (this has
  * been observed for Intel xl710, which reserves some of them for
  * SRIOV):  rte_eth_*_queue_setup will fail if a queue is not
@@ -914,6 +919,8 @@ common_construct(struct netdev *netdev, dpdk_port_t port_no,
 dev->requested_n_txq = NR_QUEUE;
 dev->requested_rxq_size = NIC_PORT_DEFAULT_RXQ_SIZE;
 dev->requested_txq_size = NIC_PORT_DEFAULT_TXQ_SIZE;
+dev->requested_hw_strip_crc = false;
+dev->hw_strip_crc = false;
 
 /* Initialize the flow control to NULL */
 memset(>fc_conf, 0, sizeof dev->fc_conf);
@@ -1192,6 +1199,11 @@ netdev_dpdk_get_config(const struct netdev *netdev, 
struct smap *args)
 } else {
 smap_add(args, "rx_csum_offload", "false");
 }
+if (dev->hw_strip_crc) {
+smap_add(args, "hw_strip_crc", "true");
+} else {
+smap_add(args, "hw_strip_crc", "false");
+}
 }
 ovs_mutex_unlock(>mutex);
 
@@ -1255,6 +1267,19 @@ dpdk_set_rxq_config(struct netdev_dpdk *dev, const 
struct smap *args)
 }
 
 static void
+dpdk_set_hw_strip_crc_config(struct netdev_dpdk *dev, const struct smap *args)
+ OVS_REQUIRES(dev->mutex)
+{
+int new_hw_strip_crc;
+
+new_hw_strip_crc = smap_get_bool(args, "hw_strip_crc", false);
+if (new_hw_strip_crc != dev->requested_hw_strip_crc) {
+dev->requested_hw_strip_crc = new_hw_strip_crc;
+netdev_request_reconfigure(>up);
+}
+}
+
+static void
 dpdk_process_queue_size(struct netdev *netdev, const struct smap *args,
 const char *flag, int default_size, int *new_size)
 {
@@ -1290,6 +1315,8 @@ netdev_dpdk_set_config(struct netdev *netdev, const 
struct smap *args,
 
 dpdk_set_rxq_config(dev, args);
 
+dpdk_set_hw_strip_crc_config(dev, args);
+
 dpdk_process_queue_size(netdev, args, "n_rxq_desc",
 NIC_PORT_DEFAULT_RXQ_SIZE,
 >requested_rxq_size);
@@ -3196,6 +3223,7 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
 if (netdev->n_txq == dev->requested_n_txq
 && netdev->n_rxq == dev->requested_n_rxq
 && dev->mtu == dev->requested_mtu
+&& dev->hw_strip_crc == dev->requested_hw_strip_crc
 && dev->rxq_size == dev->requested_rxq_size
 && dev->txq_size == dev->requested_txq_size
 && dev->socket_id == dev->requested_socket_id) {
@@ -3217,6 +3245,8 @@ netdev_dpdk_reconfigure(struct netdev *netdev)
 dev->rxq_size = dev->requested_rxq_size;
 dev->txq_size = dev->requested_txq_size;
 
+dev->hw_strip_crc = dev->requested_hw_strip_crc;
+
 rte_free(dev->tx_q);
 err = dpdk_eth_dev_init(dev);
 dev->tx_q = netdev_dpdk_alloc_txq(netdev->n_txq);
diff --git a/vswitchd/vswitch.xml b/vswitchd/vswitch.xml
index 074535b..d1e7a6b 100644
--- a/vswitchd/vswitch.xml
+++ b/vswitchd/vswitch.xml
@@ -2589,6 +2589,15 @@
 
   
 
+  
+
+  Specifies DPDK interfaces whether enable hw_strip_crc feature or 
not. 
+  If not specified, the default is false.
+  Not supported by DPDK vHost interfaces.
+
+  
+
   
 Specifies mapping of RX queues of this interface to CPU cores.
 Value should be set in the following form:
-- 
1.8.3.1

Re: [ovs-dev] [PATCH v4 3/7] netdev: Remove unused may_steal.

2017-10-09 Thread Ilya Maximets

On 09.10.2017 09:42, Gao Zhenyu wrote:
> But the netdev_dpdk_send__ function may release whole batch packets.

Yes, sure. Look at reply to my previous message.

> 
> static inline void
> netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,
>    struct dp_packet_batch *batch, bool may_steal,
>    bool concurrent_txq)
> {
> ...
>     if (OVS_UNLIKELY(!may_steal ||
>  batch->packets[0]->source != DPBUF_DPDK)) {
>     struct netdev *netdev = >up;
> 
>     dpdk_do_tx_copy(netdev, qid, batch);  <---your patch releases 
> some packets in a batch
>     dp_packet_delete_batch(batch, may_steal); <---it  releases 
> all packets in this batch, may hit issue I think
>     }
> 
> 2017-10-09 14:16 GMT+08:00 Ilya Maximets  >:
> 
> On 08.10.2017 12:32, Gao Zhenyu wrote:
> > Hi llya,
> >
> >   Thanks for working it. Your patch tried to eliminate the may_steal in 
> dpdk qos, because may_steal handled on dpif-netdev layer and always true.
> >   But in function dpdk_do_tx_copy, it set the may_steal to false, 
> because the packet buffer were not allocated by dpdk side so cannot released 
> by netdev_dpdk_policer_run(). Otherwise it may hit coredump. Did you test 
> that scenario?
> >
> > Thanks
> > Zhenyu Gao
> 
> Good catch. Thanks.
> 
> Following incremental can be used to fix the issue:
> 
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 300a0ae..3352ae2 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1538,7 +1538,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm 
> *meter,
>              }
>              cnt++;
>          } else {
> -            rte_pktmbuf_free(pkt);
> +            /* In case of calling from 'dpdk_do_tx_copy' 'pkt' could be 
> not
> +             * a DPDK allocated mbuf. */
> +            dp_packet_delete((struct dp_packet *) pkt);
>          }
>      }
> 
> 
> 
> 
> 
> Best regards, Ilya Maximets.
> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 3/7] netdev: Remove unused may_steal.

2017-10-09 Thread Ilya Maximets

On 09.10.2017 09:16, Ilya Maximets wrote:
> On 08.10.2017 12:32, Gao Zhenyu wrote:
>> Hi llya,
>>
>>   Thanks for working it. Your patch tried to eliminate the may_steal in dpdk 
>> qos, because may_steal handled on dpif-netdev layer and always true.
>>   But in function dpdk_do_tx_copy, it set the may_steal to false, because 
>> the packet buffer were not allocated by dpdk side so cannot released by 
>> netdev_dpdk_policer_run(). Otherwise it may hit coredump. Did you test that 
>> scenario?
>>
>> Thanks
>> Zhenyu Gao
> 
> Good catch. Thanks.
> 
> Following incremental can be used to fix the issue:
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 300a0ae..3352ae2 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1538,7 +1538,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,
>  }
>  cnt++;
>  } else {
> -rte_pktmbuf_free(pkt);
> +/* In case of calling from 'dpdk_do_tx_copy' 'pkt' could be not
> + * a DPDK allocated mbuf. */
> +dp_packet_delete((struct dp_packet *) pkt);
>  }
>  }
>  
> 
> 
> 
> Best regards, Ilya Maximets.
> 


Sorry, one more change is required to avoid double free:


diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 3352ae2..bb8bbf3 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1851,6 +1851,7 @@ dpdk_do_tx_copy(struct netdev *netdev, int qid, struct 
dp_packet_batch *batch)
 cnt = netdev_dpdk_qos_run(dev, (struct rte_mbuf **) batch->packets,
   batch_cnt);
 dropped += batch_cnt - cnt;
+batch->count = cnt;
 }
 
 for (uint32_t i = 0; i < cnt; i++) {



OTOH, 'may_steal' in QoS is a different entity. This patch is intended to
remove 'may_steal' from netdev API, but this is an internal to netdev-dpdk
variable. So, we can keep it in this patch set and use patch from v3.

Beside that, I think, some cleanup is needed here anyway.

Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 3/7] netdev: Remove unused may_steal.

2017-10-09 Thread Gao Zhenyu

But the netdev_dpdk_send__ function may release whole batch packets.

static inline void
netdev_dpdk_send__(struct netdev_dpdk *dev, int qid,
   struct dp_packet_batch *batch, bool may_steal,
   bool concurrent_txq)
{
...
if (OVS_UNLIKELY(!may_steal ||
 batch->packets[0]->source != DPBUF_DPDK)) {
struct netdev *netdev = >up;

dpdk_do_tx_copy(netdev, qid, batch);  <---your patch releases
some packets in a batch
dp_packet_delete_batch(batch, may_steal); <---it  releases
all packets in this batch, may hit issue I think
}

2017-10-09 14:16 GMT+08:00 Ilya Maximets :

> On 08.10.2017 12:32, Gao Zhenyu wrote:
> > Hi llya,
> >
> >   Thanks for working it. Your patch tried to eliminate the may_steal in
> dpdk qos, because may_steal handled on dpif-netdev layer and always true.
> >   But in function dpdk_do_tx_copy, it set the may_steal to false,
> because the packet buffer were not allocated by dpdk side so cannot
> released by netdev_dpdk_policer_run(). Otherwise it may hit coredump. Did
> you test that scenario?
> >
> > Thanks
> > Zhenyu Gao
>
> Good catch. Thanks.
>
> Following incremental can be used to fix the issue:
> 
> 
> diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
> index 300a0ae..3352ae2 100644
> --- a/lib/netdev-dpdk.c
> +++ b/lib/netdev-dpdk.c
> @@ -1538,7 +1538,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm
> *meter,
>  }
>  cnt++;
>  } else {
> -rte_pktmbuf_free(pkt);
> +/* In case of calling from 'dpdk_do_tx_copy' 'pkt' could be
> not
> + * a DPDK allocated mbuf. */
> +dp_packet_delete((struct dp_packet *) pkt);
>  }
>  }
>
> 
> 
>
>
> Best regards, Ilya Maximets.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4 3/7] netdev: Remove unused may_steal.

2017-10-09 Thread Ilya Maximets

On 08.10.2017 12:32, Gao Zhenyu wrote:
> Hi llya,
> 
>   Thanks for working it. Your patch tried to eliminate the may_steal in dpdk 
> qos, because may_steal handled on dpif-netdev layer and always true.
>   But in function dpdk_do_tx_copy, it set the may_steal to false, because the 
> packet buffer were not allocated by dpdk side so cannot released by 
> netdev_dpdk_policer_run(). Otherwise it may hit coredump. Did you test that 
> scenario?
> 
> Thanks
> Zhenyu Gao

Good catch. Thanks.

Following incremental can be used to fix the issue:

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 300a0ae..3352ae2 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -1538,7 +1538,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,
 }
 cnt++;
 } else {
-rte_pktmbuf_free(pkt);
+/* In case of calling from 'dpdk_do_tx_copy' 'pkt' could be not
+ * a DPDK allocated mbuf. */
+dp_packet_delete((struct dp_packet *) pkt);
 }
 }
 



Best regards, Ilya Maximets.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

42 matches

Mail list logo