Re: [ovs-dev] [ovs-dev,v6] Improved Packet Drop Statistics in OVS

[Anju Thomas]

Thanks Ben and Ilya.
I will incorporate the other changes. 

I just have below comments.

>> @@ -2427,8 +2474,13 @@ odp_actions_from_string(const char *s, const struct 
>> simap *port_names,
>>  struct ofpbuf *actions)
>>  {
>>  size_t old_size;
>> +struct ovs_action_drop drop_action;
>>  
>> -if (!strcasecmp(s, "drop")) {
>> +if ((!strcasecmp(s, "drop") ||
>> +!strcasecmp(s, "drop:pipeline-drop"))) {
> 
> Why you're adding parse only for this type of drop action ?
> The rest are all below error scenarios which we hit during processing 
> and hence I thought perhaps may not make sense as an input?? . Do you think 
> it make sense to do that ?

I think, Ben already answered this questin.
IMHO, that parsing of only one type makes no sence. There should be all
or nothing. As we think that OVS should be able to parse flows it generates,
there should be all.


 In that case, i propose removing this to ensure no subtypes are parsed.  
Can we agree on this ? In case in future there is a use case,we can revisit 
this .




> 
> Above counters could be defined inside odp-execute.c. You already have
> 2 other counters there. In this case there will be no need to introduce
> additional 'dp_update_drop_action_counter_cb' callback and pass it around.
> All the required logic will also be moved to odp-execute.c.
> 
>  But I want the callback to be NULL when called from 
> dpif_execute_with_help

Why? This seems strange to count drops only for actions that do not need
help from the dpif layer.

  For now we would want to add only for drop in actual dpdk datapath. We 
will revist this
Later




Regards
Anju



-Original Message-
From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
Sent: Wednesday, January 23, 2019 8:27 PM
To: Anju Thomas ; d...@openvswitch.org
Cc: Keshav Gupta ; Stokes, Ian ; 
Ben Pfaff 
Subject: Re: [ovs-dev,v6] Improved Packet Drop Statistics in OVS

On 23.01.2019 16:53, Anju Thomas wrote:
> Hi Ilya ,
> Some queries w.r.t your comments .
> 
> Regards
> Anju
> 
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
> Sent: Friday, January 18, 2019 5:33 PM
> To: Anju Thomas ; d...@openvswitch.org
> Cc: Keshav Gupta ; Stokes, Ian ; 
> Ben Pfaff 
> Subject: Re: [ovs-dev,v6] Improved Packet Drop Statistics in OVS
> 
> Hi.
> Thanks for working on this.
> Some general comments:
> 
> 1. This patch consists of two separate features:
>- Reporting the drop reason in flow dumps.
>- Counters for different drop types.
>I still think that there should be patch-set of two separate patches.
>This will simplify the review and allow to apply them separately/speed
>up accepting.
> 
> 2. There are some issues with the patch formatting. The commit message is
>indented like in the output of 'git show' command.  So, it's double
>shifted after applying in git. Please remove the additional indentation.
>You probably should start using 'git format-patch'.
> 
>Second issue:
> 
>Applying: Improved Packet Drop Statistics in OVS
>.git/rebase-apply/patch:415: new blank line at EOF.
>+
>warning: 1 line adds whitespace errors.
> 
>One more thing is that for better readability it's a common practice to
>limit the width of lines in commit-message to 72 characters. For example,
>you may strip not important fields of the dump-flows output. Like this:
> 
>recirc_id(0),in_port(5),<...>, actions:drop:recursion too deep
> 
> 
> Other comments inline.
> 
>  I use 
> a)git format-patch -1 -s 
> b)  ./utilities/checkpatch.py -1 
> 
> Are these the correct parameters that I should be using ?

Commands looks fine. If the patch looks same in your git repo, I'd suggest you
to re-format the patch by hands.

Also, I just spotted that you have a lot of duplicated sign-offs in the patch.

> 
> Best regards, Ilya Maximets. 
> 
> On 17.01.2019 7:49, Anju Thomas wrote:
>> Currently OVS maintains explicit packet drop/error counters only on port
>> level. Packets that are dropped as part of normal OpenFlow processing are
>> counted in flow stats of “drop” flows or as table misses in table stats.
>> These can only be interpreted by controllers that know the semantics of
>> the configured OpenFlow pipeline. Without that knowledge, it is 
>> impossible
>> for an OVS user to obtain e.g. the total number of packets dropped due to
>> OpenFlow rules.
>>
>> Furthermore, there are numerous other reasons for which packets can be
>> dropped by OVS slow path that are not related to the OpenFlow pipeline.
>> The generated datapath flow entries include a drop action to avoid 
>> further
>> expensive upcalls to the slow path, but subsequent packets dropped by the
>> datapath are not accounted anywhere.
>>
>> Finally, the datapath itself drops packets in certain error situations.
>> Also, these drops are today not accounted for.
>>
>> This makes it difficult for OVS 

Re: [ovs-dev] [Intel-gfx] [PATCH 1/3] treewide: Lift switch variables out of switches

[Greg KH]

On Thu, Jan 24, 2019 at 07:55:51AM +1300, Kees Cook wrote:
> On Thu, Jan 24, 2019 at 4:44 AM Jani Nikula  
> wrote:
> >
> > On Wed, 23 Jan 2019, Edwin Zimmerman  wrote:
> > > On Wed, 23 Jan 2019, Jani Nikula  wrote:
> > >> On Wed, 23 Jan 2019, Greg KH  wrote:
> > >> > On Wed, Jan 23, 2019 at 03:03:47AM -0800, Kees Cook wrote:
> > >> >> Variables declared in a switch statement before any case statements
> > >> >> cannot be initialized, so move all instances out of the switches.
> > >> >> After this, future always-initialized stack variables will work
> > >> >> and not throw warnings like this:
> > >> >>
> > >> >> fs/fcntl.c: In function ‘send_sigio_to_task’:
> > >> >> fs/fcntl.c:738:13: warning: statement will never be executed 
> > >> >> [-Wswitch-unreachable]
> > >> >>siginfo_t si;
> > >> >>  ^~
> > >> >
> > >> > That's a pain, so this means we can't have any new variables in { }
> > >> > scope except for at the top of a function?
> 
> Just in case this wasn't clear: no, it's just the switch statement
> before the first "case". I cannot imagine how bad it would be if we
> couldn't have block-scoped variables! Heh. :)

Sorry, it was not clear at first glance.  So no more objection from me
for this change.

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

[ovs-dev] [PATCH net-next 1/1] openvswitch: Declare ovs key structures using macros

[Eli Britstein]

Declare ovs key structures using macros to enable retrieving fields
information, with no functional change.

Signed-off-by: Eli Britstein 
Reviewed-by: Roi Dayan 
---
 include/uapi/linux/openvswitch.h | 102 ++-
 1 file changed, 69 insertions(+), 33 deletions(-)

diff --git a/include/uapi/linux/openvswitch.h b/include/uapi/linux/openvswitch.h
index dbe0cbe4f1b7..dc8246f871fd 100644
--- a/include/uapi/linux/openvswitch.h
+++ b/include/uapi/linux/openvswitch.h
@@ -387,73 +387,109 @@ enum ovs_frag_type {
 
 #define OVS_FRAG_TYPE_MAX (__OVS_FRAG_TYPE_MAX - 1)
 
+#define OVS_KEY_FIELD_ARR(type, name, elements) type name[elements];
+#define OVS_KEY_FIELD(type, name) type name;
+
+#define OVS_KEY_ETHERNET_FIELDS \
+OVS_KEY_FIELD_ARR(__u8, eth_src, ETH_ALEN) \
+OVS_KEY_FIELD_ARR(__u8, eth_dst, ETH_ALEN)
+
 struct ovs_key_ethernet {
-   __u8 eth_src[ETH_ALEN];
-   __u8 eth_dst[ETH_ALEN];
+OVS_KEY_ETHERNET_FIELDS
 };
 
 struct ovs_key_mpls {
__be32 mpls_lse;
 };
 
+#define OVS_KEY_IPV4_FIELDS \
+OVS_KEY_FIELD(__be32, ipv4_src) \
+OVS_KEY_FIELD(__be32, ipv4_dst) \
+OVS_KEY_FIELD(__u8, ipv4_proto) \
+OVS_KEY_FIELD(__u8, ipv4_tos) \
+OVS_KEY_FIELD(__u8, ipv4_ttl) \
+OVS_KEY_FIELD(__u8, ipv4_frag /* One of OVS_FRAG_TYPE_*. */)
+
 struct ovs_key_ipv4 {
-   __be32 ipv4_src;
-   __be32 ipv4_dst;
-   __u8   ipv4_proto;
-   __u8   ipv4_tos;
-   __u8   ipv4_ttl;
-   __u8   ipv4_frag;   /* One of OVS_FRAG_TYPE_*. */
+OVS_KEY_IPV4_FIELDS
 };
 
+#define OVS_KEY_IPV6_FIELDS \
+OVS_KEY_FIELD_ARR(__be32, ipv6_src, 4) \
+OVS_KEY_FIELD_ARR(__be32, ipv6_dst, 4) \
+OVS_KEY_FIELD(__be32, ipv6_label /* 20-bits in least-significant bits. */) 
\
+OVS_KEY_FIELD(__u8, ipv6_proto) \
+OVS_KEY_FIELD(__u8, ipv6_tclass) \
+OVS_KEY_FIELD(__u8, ipv6_hlimit) \
+OVS_KEY_FIELD(__u8, ipv6_frag /* One of OVS_FRAG_TYPE_*. */)
+
 struct ovs_key_ipv6 {
-   __be32 ipv6_src[4];
-   __be32 ipv6_dst[4];
-   __be32 ipv6_label;  /* 20-bits in least-significant bits. */
-   __u8   ipv6_proto;
-   __u8   ipv6_tclass;
-   __u8   ipv6_hlimit;
-   __u8   ipv6_frag;   /* One of OVS_FRAG_TYPE_*. */
+OVS_KEY_IPV6_FIELDS
 };
 
+#define OVS_KEY_TCP_FIELDS \
+OVS_KEY_FIELD(__be16, tcp_src) \
+OVS_KEY_FIELD(__be16, tcp_dst)
+
 struct ovs_key_tcp {
-   __be16 tcp_src;
-   __be16 tcp_dst;
+OVS_KEY_TCP_FIELDS
 };
 
+#define OVS_KEY_UDP_FIELDS \
+OVS_KEY_FIELD(__be16, udp_src) \
+OVS_KEY_FIELD(__be16, udp_dst)
+
 struct ovs_key_udp {
-   __be16 udp_src;
-   __be16 udp_dst;
+OVS_KEY_UDP_FIELDS
 };
 
+#define OVS_KEY_SCTP_FIELDS \
+OVS_KEY_FIELD(__be16, sctp_src) \
+OVS_KEY_FIELD(__be16, sctp_dst)
+
 struct ovs_key_sctp {
-   __be16 sctp_src;
-   __be16 sctp_dst;
+OVS_KEY_SCTP_FIELDS
 };
 
+#define OVS_KEY_ICMP_FIELDS \
+OVS_KEY_FIELD(__u8, icmp_type) \
+OVS_KEY_FIELD(__u8, icmp_code)
+
 struct ovs_key_icmp {
-   __u8 icmp_type;
-   __u8 icmp_code;
+OVS_KEY_ICMP_FIELDS
 };
 
+#define OVS_KEY_ICMPV6_FIELDS \
+OVS_KEY_FIELD(__u8, icmpv6_type) \
+OVS_KEY_FIELD(__u8, icmpv6_code)
+
 struct ovs_key_icmpv6 {
-   __u8 icmpv6_type;
-   __u8 icmpv6_code;
+OVS_KEY_ICMPV6_FIELDS
 };
 
+#define OVS_KEY_ARP_FIELDS \
+OVS_KEY_FIELD(__be32, arp_sip) \
+OVS_KEY_FIELD(__be32, arp_tip) \
+OVS_KEY_FIELD(__be16, arp_op) \
+OVS_KEY_FIELD_ARR(__u8, arp_sha, ETH_ALEN) \
+OVS_KEY_FIELD_ARR(__u8, arp_tha, ETH_ALEN)
+
 struct ovs_key_arp {
-   __be32 arp_sip;
-   __be32 arp_tip;
-   __be16 arp_op;
-   __u8   arp_sha[ETH_ALEN];
-   __u8   arp_tha[ETH_ALEN];
+OVS_KEY_ARP_FIELDS
 };
 
+#define OVS_KEY_ND_FIELDS \
+OVS_KEY_FIELD_ARR(__be32, nd_target, 4) \
+OVS_KEY_FIELD_ARR(__u8, nd_sll, ETH_ALEN) \
+OVS_KEY_FIELD_ARR(__u8, nd_tll, ETH_ALEN)
+
 struct ovs_key_nd {
-   __be32  nd_target[4];
-   __u8nd_sll[ETH_ALEN];
-   __u8nd_tll[ETH_ALEN];
+OVS_KEY_ND_FIELDS
 };
 
+#undef OVS_KEY_FIELD_ARR
+#undef OVS_KEY_FIELD
+
 #define OVS_CT_LABELS_LEN_32   4
 #define OVS_CT_LABELS_LEN  (OVS_CT_LABELS_LEN_32 * sizeof(__u32))
 struct ovs_key_ct_labels {
-- 
2.14.4

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

Re: [ovs-dev] [PATCH 1/3] treewide: Lift switch variables out of switches

[Edwin Zimmerman]

On Wednesday, January 23, 2019 6:04 AM, Kees Cook wrote
> 
> Variables declared in a switch statement before any case statements
> cannot be initialized, so move all instances out of the switches.
> After this, future always-initialized stack variables will work
> and not throw warnings like this:
> 
> fs/fcntl.c: In function ‘send_sigio_to_task’:
> fs/fcntl.c:738:13: warning: statement will never be executed 
> [-Wswitch-unreachable]
>siginfo_t si;
>  ^~
> 
> Signed-off-by: Kees Cook 

Reviewed by: Edwin Zimmerman 

> ---
>  arch/x86/xen/enlighten_pv.c   |  7 ---
>  drivers/char/pcmcia/cm4000_cs.c   |  2 +-
>  drivers/char/ppdev.c  | 20 ---
>  drivers/gpu/drm/drm_edid.c|  4 ++--
>  drivers/gpu/drm/i915/intel_display.c  |  2 +-
>  drivers/gpu/drm/i915/intel_pm.c   |  4 ++--
>  drivers/net/ethernet/intel/e1000/e1000_main.c |  3 ++-
>  drivers/tty/n_tty.c   |  3 +--
>  drivers/usb/gadget/udc/net2280.c  |  5 ++---
>  fs/fcntl.c|  3 ++-
>  mm/shmem.c|  5 +++--
>  net/core/skbuff.c |  4 ++--
>  net/ipv6/ip6_gre.c|  4 ++--
>  net/ipv6/ip6_tunnel.c |  4 ++--
>  net/openvswitch/flow_netlink.c|  7 +++
>  security/tomoyo/common.c  |  3 ++-
>  security/tomoyo/condition.c   |  7 ---
>  security/tomoyo/util.c|  4 ++--
>  18 files changed, 45 insertions(+), 46 deletions(-)
> 
> diff --git a/arch/x86/xen/enlighten_pv.c b/arch/x86/xen/enlighten_pv.c
> index c54a493e139a..a79d4b548a08 100644
> --- a/arch/x86/xen/enlighten_pv.c
> +++ b/arch/x86/xen/enlighten_pv.c
> @@ -907,14 +907,15 @@ static u64 xen_read_msr_safe(unsigned int msr, int *err)
>  static int xen_write_msr_safe(unsigned int msr, unsigned low, unsigned high)
>  {
>   int ret;
> +#ifdef CONFIG_X86_64
> + unsigned which;
> + u64 base;
> +#endif
> 
>   ret = 0;
> 
>   switch (msr) {
>  #ifdef CONFIG_X86_64
> - unsigned which;
> - u64 base;
> -
>   case MSR_FS_BASE:   which = SEGBASE_FS; goto set;
>   case MSR_KERNEL_GS_BASE:which = SEGBASE_GS_USER; goto set;
>   case MSR_GS_BASE:   which = SEGBASE_GS_KERNEL; goto set;
> diff --git a/drivers/char/pcmcia/cm4000_cs.c b/drivers/char/pcmcia/cm4000_cs.c
> index 7a4eb86aedac..7211dc0e6f4f 100644
> --- a/drivers/char/pcmcia/cm4000_cs.c
> +++ b/drivers/char/pcmcia/cm4000_cs.c
> @@ -663,6 +663,7 @@ static void monitor_card(struct timer_list *t)
>  {
>   struct cm4000_dev *dev = from_timer(dev, t, timer);
>   unsigned int iobase = dev->p_dev->resource[0]->start;
> + unsigned char flags0;
>   unsigned short s;
>   struct ptsreq ptsreq;
>   int i, atrc;
> @@ -731,7 +732,6 @@ static void monitor_card(struct timer_list *t)
>   }
> 
>   switch (dev->mstate) {
> - unsigned char flags0;
>   case M_CARDOFF:
>   DEBUGP(4, dev, "M_CARDOFF\n");
>   flags0 = inb(REG_FLAGS0(iobase));
> diff --git a/drivers/char/ppdev.c b/drivers/char/ppdev.c
> index 1ae77b41050a..d77c97e4f996 100644
> --- a/drivers/char/ppdev.c
> +++ b/drivers/char/ppdev.c
> @@ -359,14 +359,19 @@ static int pp_do_ioctl(struct file *file, unsigned int 
> cmd, unsigned long arg)
>   struct pp_struct *pp = file->private_data;
>   struct parport *port;
>   void __user *argp = (void __user *)arg;
> + struct ieee1284_info *info;
> + unsigned char reg;
> + unsigned char mask;
> + int mode;
> + s32 time32[2];
> + s64 time64[2];
> + struct timespec64 ts;
> + int ret;
> 
>   /* First handle the cases that don't take arguments. */
>   switch (cmd) {
>   case PPCLAIM:
>   {
> - struct ieee1284_info *info;
> - int ret;
> -
>   if (pp->flags & PP_CLAIMED) {
>   dev_dbg(&pp->pdev->dev, "you've already got it!\n");
>   return -EINVAL;
> @@ -517,15 +522,6 @@ static int pp_do_ioctl(struct file *file, unsigned int 
> cmd, unsigned long arg)
> 
>   port = pp->pdev->port;
>   switch (cmd) {
> - struct ieee1284_info *info;
> - unsigned char reg;
> - unsigned char mask;
> - int mode;
> - s32 time32[2];
> - s64 time64[2];
> - struct timespec64 ts;
> - int ret;
> -
>   case PPRSTATUS:
>   reg = parport_read_status(port);
>   if (copy_to_user(argp, ®, sizeof(reg)))
> diff --git a/drivers/gpu/drm/drm_edid.c b/drivers/gpu/drm/drm_edid.c
> index b506e3622b08..8f93956c1628 100644
> --- a/drivers/gpu/drm/drm_edid.c
> +++ b/drivers/gpu/drm/drm_edid.c
> @@ -3942,12 +3942,12 @@ static voi

Re: [ovs-dev] [ovs-dev,v6] Improved Packet Drop Statistics in OVS

[Ilya Maximets]

On 24.01.2019 11:04, Anju Thomas wrote:
> Thanks Ben and Ilya.
> I will incorporate the other changes. 
> 
> I just have below comments.
> 
>>> @@ -2427,8 +2474,13 @@ odp_actions_from_string(const char *s, const struct 
>>> simap *port_names,
>>>  struct ofpbuf *actions)
>>>  {
>>>  size_t old_size;
>>> +struct ovs_action_drop drop_action;
>>>  
>>> -if (!strcasecmp(s, "drop")) {
>>> +if ((!strcasecmp(s, "drop") ||
>>> +!strcasecmp(s, "drop:pipeline-drop"))) {
>>
>> Why you're adding parse only for this type of drop action ?
>> The rest are all below error scenarios which we hit during processing 
>> and hence I thought perhaps may not make sense as an input?? . Do you think 
>> it make sense to do that ?
> 
> I think, Ben already answered this questin.
> IMHO, that parsing of only one type makes no sence. There should be all
> or nothing. As we think that OVS should be able to parse flows it generates,
> there should be all.
> 
> 
>  In that case, i propose removing this to ensure no subtypes are 
> parsed.  
> Can we agree on this ? In case in future there is a use case,we can revisit 
> this .

IMHO, ovs should not fail to parse flows it generates. So, at least, even if
you will not set proper error value in the structure, you need to allow 
successful
parsing of "drop:.*" strings there. But still fully correct parsing is 
preferred.
I'd like to leave the decision to Ben.

> 
> 
> 
> 
>>
>> Above counters could be defined inside odp-execute.c. You already have
>> 2 other counters there. In this case there will be no need to introduce
>> additional 'dp_update_drop_action_counter_cb' callback and pass it around.
>> All the required logic will also be moved to odp-execute.c.
>>
>>  But I want the callback to be NULL when called from 
>> dpif_execute_with_help
> 
> Why? This seems strange to count drops only for actions that do not need
> help from the dpif layer.
> 
>   For now we would want to add only for drop in actual dpdk datapath. 
> We will revist this
> Later

I still do not understand that. Why not? You're writing a big amount of code
just to avoid counting drops for actions executed with help. If you're
planning to count them in the future you'll need to revert those changes later.
Anyway, these packets are part of the same datapath and should be accounted.

> 
> 
> 
> 
> Regards
> Anju
> 
> 
> 
> -Original Message-
> From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
> Sent: Wednesday, January 23, 2019 8:27 PM
> To: Anju Thomas ; d...@openvswitch.org
> Cc: Keshav Gupta ; Stokes, Ian ; 
> Ben Pfaff 
> Subject: Re: [ovs-dev,v6] Improved Packet Drop Statistics in OVS
> 
> On 23.01.2019 16:53, Anju Thomas wrote:
>> Hi Ilya ,
>> Some queries w.r.t your comments .
>>
>> Regards
>> Anju
>>
>> -Original Message-
>> From: Ilya Maximets [mailto:i.maxim...@samsung.com] 
>> Sent: Friday, January 18, 2019 5:33 PM
>> To: Anju Thomas ; d...@openvswitch.org
>> Cc: Keshav Gupta ; Stokes, Ian 
>> ; Ben Pfaff 
>> Subject: Re: [ovs-dev,v6] Improved Packet Drop Statistics in OVS
>>
>> Hi.
>> Thanks for working on this.
>> Some general comments:
>>
>> 1. This patch consists of two separate features:
>>- Reporting the drop reason in flow dumps.
>>- Counters for different drop types.
>>I still think that there should be patch-set of two separate patches.
>>This will simplify the review and allow to apply them separately/speed
>>up accepting.
>>
>> 2. There are some issues with the patch formatting. The commit message is
>>indented like in the output of 'git show' command.  So, it's double
>>shifted after applying in git. Please remove the additional indentation.
>>You probably should start using 'git format-patch'.
>>
>>Second issue:
>>
>>Applying: Improved Packet Drop Statistics in OVS
>>.git/rebase-apply/patch:415: new blank line at EOF.
>>+
>>warning: 1 line adds whitespace errors.
>>
>>One more thing is that for better readability it's a common practice to
>>limit the width of lines in commit-message to 72 characters. For example,
>>you may strip not important fields of the dump-flows output. Like this:
>>
>>recirc_id(0),in_port(5),<...>, actions:drop:recursion too deep
>>
>>
>> Other comments inline.
>>
>>  I use 
>> a)git format-patch -1 -s 
>> b)  ./utilities/checkpatch.py -1 
>>
>> Are these the correct parameters that I should be using ?
> 
> Commands looks fine. If the patch looks same in your git repo, I'd suggest you
> to re-format the patch by hands.
> 
> Also, I just spotted that you have a lot of duplicated sign-offs in the patch.
> 
>>
>> Best regards, Ilya Maximets. 
>>
>> On 17.01.2019 7:49, Anju Thomas wrote:
>>> Currently OVS maintains explicit packet drop/error counters only on port
>>> level. Packets that are dropped as part of normal OpenFlow processing 
>>> are
>>> counted in flow stats of “drop” flows or as table misses in table stats.
>>> These c

Re: [ovs-dev] [PATCH] treewide: Get rid of // comments, even inside comments.

[Ilya Maximets]

On 23.01.2019 23:09, Ben Pfaff wrote:
> Just a style fix.
> 
> With this patch, the following reports no hits:
> 
> git ls-files | grep '\.[ch]$' | grep -vE 'datapath|sflow' | xargs grep -n // 
> | grep -vE "http|s/|'|\""

I'd like this line wrapped. For example, like this:

  git ls-files | grep '\.[ch]$' | grep -vE 'datapath|sflow' \
   | xargs grep -n // | grep -vE "http|s/|'|\""

It's still a valid shell command.

Beside that,
Acked-by: Ilya Maximets 

P.S. sflow_receiver.c is just a bunch of bad style examples.

> 
> Reported-by: Ilya Maximets 
> Signed-off-by: Ben Pfaff 
> ---
>  include/openvswitch/ofpbuf.h | 2 +-
>  lib/backtrace.h  | 4 ++--
>  lib/hash.h   | 4 ++--
>  lib/netdev-bsd.c | 2 +-
>  lib/ofp-actions.c| 6 +++---
>  lib/ovs-atomic.h | 3 +--
>  lib/rtbsd.c  | 6 +++---
>  lib/seq.h| 2 +-
>  lib/socket-util.c| 2 +-
>  ofproto/ofproto-dpif-ipfix.c | 2 +-
>  ofproto/ofproto-provider.h   | 6 +++---
>  11 files changed, 19 insertions(+), 20 deletions(-)
> 
> diff --git a/include/openvswitch/ofpbuf.h b/include/openvswitch/ofpbuf.h
> index 71ee0c9534a3..e4cf0883ceec 100644
> --- a/include/openvswitch/ofpbuf.h
> +++ b/include/openvswitch/ofpbuf.h
> @@ -72,7 +72,7 @@ struct ofpbuf {
>   *
>   * Usage example:
>   *
> - * uint64_t stub[1024 / 8]; // 1 kB stub properly aligned for 64-bit 
> data.
> + * uint64_t stub[1024 / 8]; <-- 1 kB stub aligned for 64-bit 
> data.
>   * struct ofpbuf ofpbuf = OFPBUF_STUB_INITIALIZER(stub);
>   */
>  #define OFPBUF_STUB_INITIALIZER(STUB) { \
> diff --git a/lib/backtrace.h b/lib/backtrace.h
> index 3eb92f7ad23d..384f2700d94c 100644
> --- a/lib/backtrace.h
> +++ b/lib/backtrace.h
> @@ -27,8 +27,8 @@
>   * desired:
>   *   #include "backtrace.h"
>   *
> - *   log_backtrace();
> - *   // A message can be added with log_backtrace_msg("your message")
> + *   log_backtrace();   <-- plain
> + *   log_backtrace_msg("your message"); <-- with a message
>   *
>   *
>   * A typical log will look like the following. The hex numbers listed after
> diff --git a/lib/hash.h b/lib/hash.h
> index a642a1e97954..01e8c52de8b9 100644
> --- a/lib/hash.h
> +++ b/lib/hash.h
> @@ -55,8 +55,8 @@ static inline uint32_t hash_string(const char *, uint32_t 
> basis);
>   *
>   * The upstream license there says:
>   *
> - * // MurmurHash3 was written by Austin Appleby, and is placed in the public
> - * // domain. The author hereby disclaims copyright to this source code.
> + *MurmurHash3 was written by Austin Appleby, and is placed in the public
> + *domain. The author hereby disclaims copyright to this source code.
>   *
>   * See hash_words() for sample usage. */
>  
> diff --git a/lib/netdev-bsd.c b/lib/netdev-bsd.c
> index 46698d54748f..7875636cc3cc 100644
> --- a/lib/netdev-bsd.c
> +++ b/lib/netdev-bsd.c
> @@ -996,7 +996,7 @@ netdev_bsd_get_stats(const struct netdev *netdev_, struct 
> netdev_stats *stats)
>  mib[3] = IFMIB_IFDATA;
>  len = sizeof(ifmd);
>  for (i = 1; i <= if_count; i++) {
> -mib[4] = i; //row
> +mib[4] = i; /* row */
>  if (sysctl(mib, 6, &ifmd, &len, (void *)0, 0) == -1) {
>  VLOG_DBG_RL(&rl, "%s: sysctl failed: %s",
>  netdev_get_name(netdev_), ovs_strerror(errno));
> diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
> index 96e39d6c6c9c..f76db6c0f948 100644
> --- a/lib/ofp-actions.c
> +++ b/lib/ofp-actions.c
> @@ -3569,10 +3569,10 @@ struct nx_action_cnt_ids {
>  ovs_be16 n_controllers; /* Number of controllers. */
>  uint8_t zeros[4];   /* Must be zero. */
>  
> -/* Followed by 1 or more controller ids.
> +/* Followed by 1 or more controller ids:
>   *
> - * uint16_t cnt_ids[];// Controller ids.
> - * uint8_t pad[];   // Must be 0 to 8-byte align cnt_ids[].
> + * uint16_t cnt_ids[];  -- Controller ids.
> + * uint8_t pad[];   -- Must be 0 to 8-byte align cnt_ids[].
>   */
>  };
>  OFP_ASSERT(sizeof(struct nx_action_cnt_ids) == 16);
> diff --git a/lib/ovs-atomic.h b/lib/ovs-atomic.h
> index 4664eefaf3a1..21e230e36f6b 100644
> --- a/lib/ovs-atomic.h
> +++ b/lib/ovs-atomic.h
> @@ -509,7 +509,7 @@ ovs_refcount_ref(struct ovs_refcount *refcount)
>   * in this form:
>   *
>   * if (ovs_refcount_unref(&object->ref_cnt) == 1) {
> - * // ...uninitialize object...
> + * ...uninitialize object...
>   * free(object);
>   * }
>   *
> @@ -593,7 +593,6 @@ ovs_refcount_try_ref_rcu(struct ovs_refcount *refcount)
>   * For example:
>   *
>   * if (ovs_refcount_unref_relaxed(&object->ref_cnt) == 1) {
> - * // Schedule uninitialization and freeing of the object:
>   * ovsrcu_postpone(destructor_function, object);
>   * }
>   *
> diff --git a/lib/rtbsd.c b/lib/rtbsd.c
> index c98f3052f386..564595c3a511 100644
> ---

Re: [ovs-dev] [PATCH 0/3] revert port duplicate checking optimization

[Flavio Leitner]

On Wed, Jan 23, 2019 at 12:13:25PM -0800, Ben Pfaff wrote:
> I'm not so happy about reverting without having the followup ready.  How
> close are we to having the followup?  Basically we've got two problems
> here.  Without the revert, we have one of them; with the revert, we have
> the other one.  I'd rather not trade one for the other, that's not
> ideal.  It would be much better to fix both in one shot.

Hi Ben,

I guess one is a performance problem and the other is a broken
environment that has no workaround (to my knowledge).

The kernel fix and possibly a follow up in userspace is on my
ToDo list, but I haven't had a chance to get to it yet.

Thanks,
fbl

> 
> On Wed, Jan 23, 2019 at 07:55:36AM -0200, Flavio Leitner wrote:
> > 
> > Hi,
> > 
> > Just a reminder about this revert.
> > 
> > On Thu, Dec 13, 2018 at 12:30:47PM -0200, Flavio Leitner wrote:
> > > 
> > > This should be applied to branch-2.10 as well.
> > 
> > And now branch-2.11
> > 
> > Thanks,
> > fbl
> > 
> > > 
> > > (BTW, I had CC few folks in the patchset, but I am only seeing Guru, so
> > >  I am adding them to this email just in case)
> > > 
> > > Thanks,
> > > fbl
> > > 
> > > On Thu, Dec 13, 2018 at 12:24:45PM -0200, Flavio Leitner wrote:
> > > > The optimization introduced a regression in OSP environments using
> > > > internal ports in other netns. Their networking configuration is lost
> > > > when the service is restarted because the ports are recreated now.
> > > > 
> > > > Before the patch it checked using netlink if the port with a specific
> > > > "name" was already there. The check is a lookup in all ports attached
> > > > to the DP regardless of the port's netns.
> > > > 
> > > > After the patch it relies on the kernel to identify that situation.
> > > > Unfortunately the only protection there is register_netdevice() which
> > > > fails only if the port with that name exists in the current netns.
> > > > 
> > > > If the port is in another netns, it will get a new dp_port and because
> > > > of that userspace will delete the old port. At this point the original
> > > > port is gone from the other netns and there a fresh port in the current
> > > > netns.
> > > > 
> > > > This patchset reverts the original commit and the two other follow ups.
> > > > 
> > > > Flavio Leitner (3):
> > > >   Revert "dpif-netlink: Don't destroy and recreate port if it exists"
> > > >   Revert "ofproto-dpif: Check for EBUSY as well"
> > > >   Revert "ofproto-dpif: Let the dpif report when a port is a duplicate."
> > > > 
> > > >  lib/dpif-netlink.c | 4 ++--
> > > >  lib/dpif.c | 9 ++---
> > > >  ofproto/ofproto-dpif.c | 7 ---
> > > >  3 files changed, 8 insertions(+), 12 deletions(-)
> > > > 
> > > > -- 
> > > > 2.17.2
> > > 
> > > ___
> > > 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 v5] ovn: Support a new Logical_Switch_Port.type - 'external'

[Han Zhou]

On Mon, Jan 21, 2019 at 7:06 AM Miguel Angel Ajo Pelayo
 wrote:
>
>
>
> On Mon, Jan 21, 2019 at 4:02 PM Numan Siddique  wrote:
>>
>>
>> Hi Han,
>>
>> I have addressed your comments. But before posting the patch I wanted to get 
>> an opinion
>> on the HA support for these external ports.
>>
>> The proposed patch doesn't support HA. If the requested chassis goes down 
>> for some reason
>> it is expected that CMS would detect it and change the requested-chassis 
>> option to other
>> suitable chassis.
>>
>> The openstack OVN folks think this would be too much for the CMS to handle 
>> and it would
>> complicate the code in networking-ovn which I agree with.
>>
>
> Not only the complexity part. If we implement this from the CMS, then every 
> CMS using ovn
> will need to replicate that behaviour.
>
> That's in my opinion a good reason why it's better to handle HA within OVN 
> itself.
>
>>
>> I am thinking to add the HA support on the lines of gateway chassis support 
>> and I want to
>> submit this patch after adding the HA support. I think this would be better 
>> as we won't add
>> more options in OVN (first requested-chassis for external ports and then 
>> later HA chassis support).
>> Thoughts?

I thought it would be easier to support outside of OVN combining with
chassis life-cycle management, but I didn't go deeper in any CMS
implementation. I agree it is better to handle HA in OVN than
implementing it in every CMS. But I am also worring about the
complexity in OVN itself. Could you describe briefly how would you
support it in OVN? For example, how to detect if a chassis failed? It
is different from gateway chassis because the major use case of
external port is for bridged networks (vlan/flat), so I think the BFD
mechanism for tunnel health monitoring may not be a good fit here.

Thanks,
Han
>>
>> Thanks
>> Numan
>>
>>
>> On Sat, Jan 19, 2019 at 12:42 AM Numan Siddique  wrote:
>>>
>>>
>>>
>>> On Sat, Jan 19, 2019, 12:32 AM Han Zhou >>>
 On Fri, Jan 18, 2019 at 10:16 AM Numan Siddique  
 wrote:
 >
 >
 >
 > On Fri, Jan 18, 2019 at 2:11 AM Han Zhou  wrote:
 >>
 >> On Thu, Jan 17, 2019 at 11:32 AM Han Zhou  wrote:
 >> >
 >> > On Thu, Jan 17, 2019 at 11:25 AM Numan Siddique  
 >> > wrote:
 >> > >
 >> > >
 >> > >
 >> > > On Fri, Jan 18, 2019 at 12:21 AM Han Zhou  wrote:
 >> > >>
 >> > >> Hi Numan,
 >> > >>
 >> > >> With v5 the new test case "external logical port" fails.
 >> > >> And please see more comments inlined.
 >> > >>
 >> > >> On Tue, Jan 15, 2019 at 12:09 PM  wrote:
 >> > >> >
 >> > >> > From: Numan Siddique 
 >> > >> >
 >> > >> > In the case of OpenStack + OVN, when the VMs are booted on
 >> > >> > hypervisors supporting SR-IOV nics, there are no OVS ports
 >> > >> > for these VMs. When these VMs sends DHCPv4, DHPCv6 or IPv6
 >> > >> > Router Solicitation requests, the local ovn-controller
 >> > >> > cannot reply to these packets. OpenStack Neutron dhcp agent
 >> > >> > service needs to be run to serve these requests.
 >> > >> >
 >> > >> > With the new logical port type - 'external', OVN itself can
 >> > >> > handle these requests avoiding the need to deploy any
 >> > >> > external services like neutron dhcp agent.
 >> > >> >
 >> > >> > To make use of this feature, CMS has to
 >> > >> >  - create a logical port for such VMs
 >> > >> >  - set the type to 'external'
 >> > >> >  - set requested-chassis="" in the options
 >> > >> >column.
 >> > >> >  - create a localnet port for the logical switch
 >> > >> >  - configure the ovn-bridge-mappings option in the OVS db.
 >> > >> >
 >> > >> > When the ovn-controller running in that 'chassis', detects
 >> > >> > the Port_Binding row, it adds the necessary DHCPv4/v6 OF
 >> > >> > flows. Since the packet enters the logical switch pipeline
 >> > >> > via the localnet port, the inport register (reg14) is set
 >> > >> > to the tunnel key of localnet port in the match conditions.
 >> > >> >
 >> > >> > In case the chassis goes down for some reason, it is the
 >> > >> > responsibility of CMS to change the 'requested-chassis'
 >> > >> > option to some other active chassis, so that it can serve
 >> > >> > these requests.
 >> > >> >
 >> > >> > When the VM with the external port, sends an ARP request for
 >> > >> > the router ips, only the chassis which has claimed the port,
 >> > >> > will reply to the ARP requests. Rest of the chassis on
 >> > >> > receiving these packets drop them in the ingress switch
 >> > >> > datapath stage - S_SWITCH_IN_EXTERNAL_PORT which is just
 >> > >> > before S_SWITCH_IN_L2_LKUP.
 >> > >> >
 >> > >> > This would guarantee that only the chassis which has claimed
 >> > >> > the external ports will run the router datapath pipeline.
 >> > >> >
 >> > >> > Signed-off-by: Numan Sidd

[ovs-dev] [PATCH] acinclude: Also use LIBS from dpkg pkg-config

[Christian Ehrhardt]

DPDK 18.11 builds using the more modern meson build system no more
provide the -ldpdk linker script. Instead it is expected to use
pkgconfig for linker options as well.

This change will set DPDK_LIB from pkg-config (if pkg-config was
available) and since that already carries the whole-archive flags around
the PMDs skips the further wrapping in more whole-archive if that is
already part of DPDK_LIB.

Signed-off-by: Christian Ehrhardt 
---
 acinclude.m4 | 17 -
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/acinclude.m4 b/acinclude.m4
index f038fd457..a45411860 100644
--- a/acinclude.m4
+++ b/acinclude.m4
@@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
 case "$with_dpdk" in
   yes)
 DPDK_AUTO_DISCOVER="true"
-PKG_CHECK_MODULES([DPDK], [libdpdk],
-  [DPDK_INCLUDE="$DPDK_CFLAGS"],
-  [DPDK_INCLUDE="-I/usr/local/include/dpdk 
-I/usr/include/dpdk"])
+PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
+ [DPDK_INCLUDE="$DPDK_CFLAGS", 
DPDK_LIB="$DPDK_LIBS"],
+ [DPDK_INCLUDE="-I/usr/local/include/dpdk 
-I/usr/include/dpdk", DPDK_LIB="-ldpdk"])
 ;;
   *)
 DPDK_AUTO_DISCOVER="false"
@@ -238,10 +238,10 @@ AC_DEFUN([OVS_CHECK_DPDK], [
DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk"
 fi
 DPDK_LIB_DIR="$with_dpdk/lib"
+DPDK_LIB="-ldpdk"
 ;;
 esac
 
-DPDK_LIB="-ldpdk"
 DPDK_EXTRA_LIB=""
 
 ovs_save_CFLAGS="$CFLAGS"
@@ -346,7 +346,14 @@ AC_DEFUN([OVS_CHECK_DPDK], [
 #
 # These options are specified inside a single -Wl directive to prevent
 # autotools from reordering them.
-DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive
+#
+# OTOH newer versions of dpdk pkg-config (generated with Meson)
+# will already have flagged just the right set of libs with
+# --whole-archive - in those cases do not wrap it once more.
+case "$DPDK_LIB" in
+  *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;;
+  *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-archive
+esac
 AC_SUBST([DPDK_vswitchd_LDFLAGS])
 AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.])
   fi
-- 
2.17.1

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

Re: [ovs-dev] [PATCH] acinclude: Also use LIBS from dpkg pkg-config

[Christian Ehrhardt]

On Thu, Jan 24, 2019 at 5:38 PM Christian Ehrhardt
 wrote:
>
> DPDK 18.11 builds using the more modern meson build system no more
> provide the -ldpdk linker script. Instead it is expected to use
> pkgconfig for linker options as well.

A single patch didn't seem right for a 0/1 into, but let me add some
further data to help your review in a reply.

Debian/Ubuntu already use the meson build system and therefore will
need this or a similar change.
FYI here an Ubuntu Openvswitch build log of OVS branch-2.11 with dpdk
enabled and this patch applied to get it building.
amd64: 
https://launchpadlibrarian.net/407992531/buildlog_ubuntu-disco-amd64.openvswitch_2.11.0~git20190121.4e4f80ec2-0ubuntu1~ppa4_BUILDING.txt.gz

> This change will set DPDK_LIB from pkg-config (if pkg-config was
> available) and since that already carries the whole-archive flags around
> the PMDs skips the further wrapping in more whole-archive if that is
> already part of DPDK_LIB.

FYI this is an example content a meson generated pkg-config file for DPDK:

prefix=/usr
libdir=${prefix}/lib/x86_64-linux-gnu
includedir=${prefix}/include/dpdk

Name: DPDK
Description: The Data Plane Development Kit (DPDK).
Note that CFLAGS might contain an -march flag higher than typical baseline.
This is required for a number of static inline functions in the public headers.
Version: 18.11.0
Requires.private: libbsd, zlib, libmnl, libmlx4, libibverbs, libmlx5, libcrypto
Libs: -L${libdir} -lrte_telemetry -lrte_bpf -lrte_flow_classify
-lrte_pipeline -lrte_table -lrte_port -lrte_vhost -lrte_security
-lrte_sched -lrte_reorder -lrte_rawdev -lrte_pdump -lrte_power
-lrte_meter -lrte_member -lrte_lpm -lrte_latencystats -lrte_kni
-lrte_jobstats -
lrte_ip_frag -lrte_gso -lrte_gro -lrte_eventdev -lrte_efd
-lrte_distributor -lrte_cryptodev -lrte_compressdev -lrte_cfgfile
-lrte_bitratestats -lrte_bbdev -lrte_acl -lrte_timer -lrte_hash
-lrte_metrics -lrte_pci -lrte_ethdev -lrte_net -lrte_mbuf
-lrte_mempool -lrte_ring -
lrte_cmdline -lrte_eal -lrte_kvargs
Libs.private: -L${libdir} -lrte_kvargs -lrte_eal -lrte_ring
-lrte_mempool -lrte_mbuf -lrte_pci -lrte_cryptodev -lrte_net
-lrte_cmdline -lrte_ethdev -lrte_hash -lrte_timer -lrte_common_dpaax
-lrte_eventdev -lrte_rawdev -lrte_bus_dpaa -lrte_bus_fslmc
-lrte_bus_pci -lrte_com
mon_octeontx -lrte_bus_vdev -lrte_meter -lrte_sched -lrte_ip_frag
-lrte_mempool_dpaa -lrte_mempool_dpaa2 -lrte_vhost -lrte_security
-lrte_kni -lrte_bus_vmbus -lrte_mempool_octeontx -lpcap -lrte_port
-lrte_lpm -lrte_acl -lrte_table -lrte_pipeline -lrte_gso -lIPSec_MB
-lIPS
ec_MB -lrte_common_cpt -lrte_reorder -lrte_compressdev -lrte_pmd_dpaa
-lrte_pmd_dpaa2 -lrte_pmd_dpaa2_sec -lrte_pmd_octeontx -lrte_bbdev
-lrte_bus_ifpga -Wl,--whole-archive -lrte_mempool_bucket
-lrte_mempool_ring -lrte_mempool_stack -lrte_pmd_af_packet
-lrte_pmd_ark -lrte
_pmd_atlantic -lrte_pmd_avf -lrte_pmd_avp -lrte_pmd_axgbe
-lrte_pmd_bond -lrte_pmd_bnx2x -lrte_pmd_bnxt -lrte_pmd_cxgbe
-lrte_pmd_e1000 -lrte_pmd_ena -lrte_pmd_enetc -lrte_pmd_enic
-lrte_pmd_failsafe -lrte_pmd_fm10k -lrte_pmd_i40e -lrte_pmd_ifc
-lrte_pmd_ixgbe -lrte_pmd_k
ni -lrte_pmd_liquidio -lrte_pmd_mlx4 -lrte_pmd_mlx5 -lrte_pmd_netvsc
-lrte_pmd_nfp -lrte_pmd_null -lrte_pmd_pcap -lrte_pmd_qede
-lrte_pmd_ring -lrte_pmd_sfc -lrte_pmd_softnic -lrte_pmd_tap
-lrte_pmd_thunderx -lrte_pmd_vdev_netvsc -lrte_pmd_vhost
-lrte_pmd_virtio -lrte_pmd
_vmxnet3 -lrte_pmd_aesni_gcm -lrte_pmd_aesni_mb -lrte_pmd_caam_jr
-lrte_pmd_ccp -lrte_pmd_dpaa_sec -lrte_pmd_null_crypto
-lrte_pmd_octeontx_crypto -lrte_pmd_openssl -lrte_pmd_crypto_scheduler
-lrte_pmd_virtio_crypto -lrte_pmd_octeontx_compress -lrte_pmd_qat
-lrte_pmd_zlib
-lrte_pmd_dpaa_event -lrte_pmd_dpaa2_event -lrte_pmd_octeontx_event
-lrte_pmd_opdl_event -lrte_pmd_skeleton_event -lrte_pmd_sw_event
-lrte_pmd_dsw_event -lrte_pmd_bbdev_null -lrte_pmd_skeleton_rawdev
-lrte_pmd_dpaa2_cmdif -lrte_pmd_dpaa2_qdma -lrte_pmd_ifpga_rawdev
-Wl,-
-no-whole-archive -Wl,-Bdynamic -Wl,--no-as-needed -pthread -lm -ldl -lnuma
Cflags: -I${includedir}/../x86_64-linux-gnu/dpdk -I${includedir}
-include rte_config.h -march=corei7



>
> Signed-off-by: Christian Ehrhardt 
> ---
>  acinclude.m4 | 17 -
>  1 file changed, 12 insertions(+), 5 deletions(-)
>
> diff --git a/acinclude.m4 b/acinclude.m4
> index f038fd457..a45411860 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>  case "$with_dpdk" in
>yes)
>  DPDK_AUTO_DISCOVER="true"
> -PKG_CHECK_MODULES([DPDK], [libdpdk],
> -  [DPDK_INCLUDE="$DPDK_CFLAGS"],
> -  [DPDK_INCLUDE="-I/usr/local/include/dpdk 
> -I/usr/include/dpdk"])
> +PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
> + [DPDK_INCLUDE="$DPDK_CFLAGS", 
> DPDK_LIB="$DPDK_LIBS"],
> + [DPDK_INCLUDE="-I/usr/local/include/dpdk 
> -I/usr/include/dpdk", DPDK_LIB="-ldpdk"])
>  ;;
>*

Re: [ovs-dev] [PATCH] acinclude: Also use LIBS from dpkg pkg-config

[Luca Boccassi]

On Thu, 2019-01-24 at 17:38 +0100, Christian Ehrhardt wrote:
> DPDK 18.11 builds using the more modern meson build system no more
> provide the -ldpdk linker script. Instead it is expected to use
> pkgconfig for linker options as well.
> 
> This change will set DPDK_LIB from pkg-config (if pkg-config was
> available) and since that already carries the whole-archive flags
> around
> the PMDs skips the further wrapping in more whole-archive if that is
> already part of DPDK_LIB.
> 
> Signed-off-by: Christian Ehrhardt 
> ---
>  acinclude.m4 | 17 -
>  1 file changed, 12 insertions(+), 5 deletions(-)
> 
> diff --git a/acinclude.m4 b/acinclude.m4
> index f038fd457..a45411860 100644
> --- a/acinclude.m4
> +++ b/acinclude.m4
> @@ -223,9 +223,9 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>  case "$with_dpdk" in
>    yes)
>  DPDK_AUTO_DISCOVER="true"
> -PKG_CHECK_MODULES([DPDK], [libdpdk],
> -  [DPDK_INCLUDE="$DPDK_CFLAGS"],
> -  [DPDK_INCLUDE="-I/usr/local/include/dpdk
> -I/usr/include/dpdk"])
> +PKG_CHECK_MODULES_STATIC([DPDK], [libdpdk],
> + [DPDK_INCLUDE="$DPDK_CFLAGS",
> DPDK_LIB="$DPDK_LIBS"],
> + [DPDK_INCLUDE="-
> I/usr/local/include/dpdk -I/usr/include/dpdk", DPDK_LIB="-ldpdk"])
>  ;;
>    *)
>  DPDK_AUTO_DISCOVER="false"
> @@ -238,10 +238,10 @@ AC_DEFUN([OVS_CHECK_DPDK], [
> DPDK_INCLUDE="-I$DPDK_INCLUDE_PATH/dpdk"
>  fi
>  DPDK_LIB_DIR="$with_dpdk/lib"
> +DPDK_LIB="-ldpdk"
>  ;;
>  esac
>  
> -DPDK_LIB="-ldpdk"
>  DPDK_EXTRA_LIB=""
>  
>  ovs_save_CFLAGS="$CFLAGS"
> @@ -346,7 +346,14 @@ AC_DEFUN([OVS_CHECK_DPDK], [
>  #
>  # These options are specified inside a single -Wl directive to
> prevent
>  # autotools from reordering them.
> -DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-whole-
> archive
> +#
> +# OTOH newer versions of dpdk pkg-config (generated with Meson)
> +# will already have flagged just the right set of libs with
> +# --whole-archive - in those cases do not wrap it once more.
> +case "$DPDK_LIB" in
> +  *whole-archive*) DPDK_vswitchd_LDFLAGS=$DPDK_LIB;;
> +  *) DPDK_vswitchd_LDFLAGS=-Wl,--whole-archive,$DPDK_LIB,--no-
> whole-archive
> +esac
>  AC_SUBST([DPDK_vswitchd_LDFLAGS])
>  AC_DEFINE([DPDK_NETDEV], [1], [System uses the DPDK module.])
>    fi

Acked-by: Luca Boccassi 

-- 
Kind regards,
Luca Boccassi
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] output:NXM_OF_ETH_SRC[0..3]

[GEORGIOS PAPATHANAIL]

Hello,

I'm trying to reduce the forwarding state in switches.
I have a simple topology consists of 2 switches and 2 servers

H1>S1>S2>H2


Ι have entered those 2 flows

*Routing header insertion at s1*



sudo ovs-ofctl add-flow s1
dl_type=0x800,nw_dst=10.0.0.2,actions=mod_dl_src:20:00:00:00:00:00,output:2



*Source routing at s2*



sudo ovs-ofctl add-flow s2 actions=output:NXM_OF_ETH_SRC[0..3]


Unfortunately I couldn't ping the two hosts.
Is there any issue with the output:NXM_OF_ETH_SRC[0..3] command?


Thank you in advance





Απαλλαγμένο
από ιούς. www.avast.com

<#DAB4FAD8-2DD7-40BB-A1B8-4E2AA1F9FDF2>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] output:NXM_OF_ETH_SRC[0..3]

[Ben Pfaff]

On Thu, Jan 24, 2019 at 07:34:02PM +0200, GEORGIOS PAPATHANAIL wrote:
> Hello,
> 
> I'm trying to reduce the forwarding state in switches.
> I have a simple topology consists of 2 switches and 2 servers
> 
> H1>S1>S2>H2
> 
> 
> Ι have entered those 2 flows
> 
> *Routing header insertion at s1*
> 
> 
> 
> sudo ovs-ofctl add-flow s1
> dl_type=0x800,nw_dst=10.0.0.2,actions=mod_dl_src:20:00:00:00:00:00,output:2
> 
> 
> 
> *Source routing at s2*
> 
> 
> 
> sudo ovs-ofctl add-flow s2 actions=output:NXM_OF_ETH_SRC[0..3]
> 
> 
> Unfortunately I couldn't ping the two hosts.
> Is there any issue with the output:NXM_OF_ETH_SRC[0..3] command?

It should work.

For debugging flow tables, I recommend this approach from the FAQ.

Q: I have a sophisticated network setup involving Open vSwitch, VMs or multiple
hosts, and other components.  The behavior isn't what I expect.  Help!

A: To debug network behavior problems, trace the path of a packet,
hop-by-hop, from its origin in one host to a remote host.  If that's
correct, then trace the path of the response packet back to the origin.

The open source tool called ``plotnetcfg`` can help to understand the
relationship between the networking devices on a single host.

Usually a simple ICMP echo request and reply (``ping``) packet is good
enough.  Start by initiating an ongoing ``ping`` from the origin host to a
remote host.  If you are tracking down a connectivity problem, the "ping"
will not display any successful output, but packets are still being sent.
(In this case the packets being sent are likely ARP rather than ICMP.)

Tools available for tracing include the following:

- ``tcpdump`` and ``wireshark`` for observing hops across network devices,
  such as Open vSwitch internal devices and physical wires.

- ``ovs-appctl dpif/dump-flows `` in Open vSwitch 1.10 and later or
  ``ovs-dpctl dump-flows `` in earlier versions.  These tools allow one
  to observe the actions being taken on packets in ongoing flows.

  See ovs-vswitchd(8) for ``ovs-appctl dpif/dump-flows`` documentation,
  ovs-dpctl(8) for ``ovs-dpctl dump-flows`` documentation, and "Why are
  there so many different ways to dump flows?" above for some background.

- ``ovs-appctl ofproto/trace`` to observe the logic behind how ovs-vswitchd
  treats packets.  See ovs-vswitchd(8) for documentation.  You can out more
  details about a given flow that ``ovs-dpctl dump-flows`` displays, by
  cutting and pasting a flow from the output into an ``ovs-appctl
  ofproto/trace`` command.

- SPAN, RSPAN, and ERSPAN features of physical switches, to observe what
  goes on at these physical hops.

Starting at the origin of a given packet, observe the packet at each hop in
turn.  For example, in one plausible scenario, you might:

1. ``tcpdump`` the ``eth`` interface through which an ARP egresses a VM,
   from inside the VM.

2. ``tcpdump`` the ``vif`` or ``tap`` interface through which the ARP
   ingresses the host machine.

3. Use ``ovs-dpctl dump-flows`` to spot the ARP flow and observe the host
   interface through which the ARP egresses the physical machine.  You may
   need to use ``ovs-dpctl show`` to interpret the port numbers.  If the
   output seems surprising, you can use ``ovs-appctl ofproto/trace`` to
   observe details of how ovs-vswitchd determined the actions in the
   ``ovs-dpctl dump-flows`` output.

4. ``tcpdump`` the ``eth`` interface through which the ARP egresses the
   physical machine.

5. ``tcpdump`` the ``eth`` interface through which the ARP ingresses the
   physical machine, at the remote host that receives the ARP.

6. Use ``ovs-dpctl dump-flows`` to spot the ARP flow on the remote host
   remote host that receives the ARP and observe the VM ``vif`` or ``tap``
   interface to which the flow is directed.  Again, ``ovs-dpctl show`` and
   ``ovs-appctl ofproto/trace`` might help.

7. ``tcpdump`` the ``vif`` or ``tap`` interface to which the ARP is
   directed.

8. ``tcpdump`` the ``eth`` interface through which the ARP ingresses a VM,
   from inside the VM.

It is likely that during one of these steps you will figure out the
problem.  If not, then follow the ARP reply back to the origin, in reverse.

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

Re: [ovs-dev] compat: Fixup ipv6 fragmentation on 4.9.135+ kernels

[Aaron Conole]

Hi Greg,

0-day Robot  writes:

> Bleep bloop.  Greetings Greg Rose, I am a robot and I have tried out your 
> patch.
> Thanks for your contribution.
>
> I encountered some error that I wasn't expecting.  See the details below.
>
>
> git-am:
> Failed to merge in the changes.
> Patch failed at 0001 compat: Fixup ipv6 fragmentation on 4.9.135+ kernels
> The copy of the patch that failed is found in:
>
> /var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
> When you have resolved this problem, run "git am --resolved".
> If you prefer to skip this patch, run "git am --skip" instead.
> To restore the original branch and stop patching, run "git am --abort".
>

In the future, you can avoid this bleep-bloop by using the branch name.
IE: branch-2.10 rather than Branch 2.10

I'll add looking at improving the branch detection as well, but not sure
when I'll get around to that.

> Please check this out.  If you feel there has been an error, please
> email acon...@bytheb.org
>
> Thanks,
> 0-day Robot
> ___
> 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 RFC] stt: fix return code during xmit

[Justin Pettit]

> On Jan 21, 2019, at 10:23 AM, Gregory Rose  wrote:
> 
> On 1/16/2019 8:03 AM, Aaron Conole wrote:
>> Following code looks like it might be wrong.  I don't know much about
>> the way the stt infrastructure is being used, so feel free to ignore if
>> it is expected to return NETDEV_TX_OK even in error cases (just seems
>> strange).
>> 
>> Caught by compiler warning:
>> 
>>   /home/travis/build/ovsrobot/ovs/datapath/linux/stt.c: In function 
>> ‘ovs_stt_xmit’:
>>   /home/travis/build/ovsrobot/ovs/datapath/linux/stt.c:1005:6: warning: 
>> variable ‘err’ set but not used [-Wunused-but-set-variable]
>> int err;
>> ^
>> 
>> If not used, then consider alternatively just dropping the variable.
>> 
>> Signed-off-by: Aaron Conole 
>> ---
>> 
>> diff --git a/datapath/linux/compat/stt.c b/datapath/linux/compat/stt.c
>> index 506f1d90a..5f045120e 100644
>> --- a/datapath/linux/compat/stt.c
>> +++ b/datapath/linux/compat/stt.c
>> @@ -1029,7 +1029,7 @@ netdev_tx_t ovs_stt_xmit(struct sk_buff *skb)
>>  error:
>> kfree_skb(skb);
>> dev->stats.tx_errors++;
>> -   return NETDEV_TX_OK;
>> +   return err;
>>  }
>>  EXPORT_SYMBOL(ovs_stt_xmit);
> 
> Looks good to me.  I don't know if you need to resend without the RFC tag or 
> not but in any case...
> 
> Reviewed-by: Greg Rose 

Thanks, Aaron and Greg.  I updated the commit message to remove the doubts from 
it and pushed it all the way back to branch-2.5.

--Justin


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

Re: [ovs-dev] [PATCH v5] ovn: Support a new Logical_Switch_Port.type - 'external'

[Numan Siddique]

On Thu, Jan 24, 2019 at 9:20 PM Han Zhou  wrote:

> On Mon, Jan 21, 2019 at 7:06 AM Miguel Angel Ajo Pelayo
>  wrote:
> >
> >
> >
> > On Mon, Jan 21, 2019 at 4:02 PM Numan Siddique 
> wrote:
> >>
> >>
> >> Hi Han,
> >>
> >> I have addressed your comments. But before posting the patch I wanted
> to get an opinion
> >> on the HA support for these external ports.
> >>
> >> The proposed patch doesn't support HA. If the requested chassis goes
> down for some reason
> >> it is expected that CMS would detect it and change the
> requested-chassis option to other
> >> suitable chassis.
> >>
> >> The openstack OVN folks think this would be too much for the CMS to
> handle and it would
> >> complicate the code in networking-ovn which I agree with.
> >>
> >
> > Not only the complexity part. If we implement this from the CMS, then
> every CMS using ovn
> > will need to replicate that behaviour.
> >
> > That's in my opinion a good reason why it's better to handle HA within
> OVN itself.
> >
> >>
> >> I am thinking to add the HA support on the lines of gateway chassis
> support and I want to
> >> submit this patch after adding the HA support. I think this would be
> better as we won't add
> >> more options in OVN (first requested-chassis for external ports and
> then later HA chassis support).
> >> Thoughts?
>
> I thought it would be easier to support outside of OVN combining with
> chassis life-cycle management, but I didn't go deeper in any CMS
> implementation. I agree it is better to handle HA in OVN than
> implementing it in every CMS. But I am also worring about the
> complexity in OVN itself. Could you describe briefly how would you
> support it in OVN? For example, how to detect if a chassis failed? It
> is different from gateway chassis because the major use case of
> external port is for bridged networks (vlan/flat), so I think the BFD
> mechanism for tunnel health monitoring may not be a good fit here.
>

At present, as you know, ovn-controller's do establish geneve tunnels
even if there are only logical switches representing bridged networks.
So I feel we can leverage it unless we have a better mechanism
to detect health monitoring.

Do you have any thoughts/ideas of any other possibilities ?

I am also trying to make the HA support more generic and a bit simpler
(hopefully :))
so that it can be used either for "external" ports or for the
redirectchassis router ports.


Thanks
Numan

Thanks,
> Han
> >>
> >> Thanks
> >> Numan
> >>
> >>
> >> On Sat, Jan 19, 2019 at 12:42 AM Numan Siddique 
> wrote:
> >>>
> >>>
> >>>
> >>> On Sat, Jan 19, 2019, 12:32 AM Han Zhou  
>  On Fri, Jan 18, 2019 at 10:16 AM Numan Siddique 
> wrote:
>  >
>  >
>  >
>  > On Fri, Jan 18, 2019 at 2:11 AM Han Zhou  wrote:
>  >>
>  >> On Thu, Jan 17, 2019 at 11:32 AM Han Zhou 
> wrote:
>  >> >
>  >> > On Thu, Jan 17, 2019 at 11:25 AM Numan Siddique <
> nusid...@redhat.com> wrote:
>  >> > >
>  >> > >
>  >> > >
>  >> > > On Fri, Jan 18, 2019 at 12:21 AM Han Zhou 
> wrote:
>  >> > >>
>  >> > >> Hi Numan,
>  >> > >>
>  >> > >> With v5 the new test case "external logical port" fails.
>  >> > >> And please see more comments inlined.
>  >> > >>
>  >> > >> On Tue, Jan 15, 2019 at 12:09 PM  wrote:
>  >> > >> >
>  >> > >> > From: Numan Siddique 
>  >> > >> >
>  >> > >> > In the case of OpenStack + OVN, when the VMs are booted on
>  >> > >> > hypervisors supporting SR-IOV nics, there are no OVS ports
>  >> > >> > for these VMs. When these VMs sends DHCPv4, DHPCv6 or IPv6
>  >> > >> > Router Solicitation requests, the local ovn-controller
>  >> > >> > cannot reply to these packets. OpenStack Neutron dhcp agent
>  >> > >> > service needs to be run to serve these requests.
>  >> > >> >
>  >> > >> > With the new logical port type - 'external', OVN itself can
>  >> > >> > handle these requests avoiding the need to deploy any
>  >> > >> > external services like neutron dhcp agent.
>  >> > >> >
>  >> > >> > To make use of this feature, CMS has to
>  >> > >> >  - create a logical port for such VMs
>  >> > >> >  - set the type to 'external'
>  >> > >> >  - set requested-chassis="" in the options
>  >> > >> >column.
>  >> > >> >  - create a localnet port for the logical switch
>  >> > >> >  - configure the ovn-bridge-mappings option in the OVS db.
>  >> > >> >
>  >> > >> > When the ovn-controller running in that 'chassis', detects
>  >> > >> > the Port_Binding row, it adds the necessary DHCPv4/v6 OF
>  >> > >> > flows. Since the packet enters the logical switch pipeline
>  >> > >> > via the localnet port, the inport register (reg14) is set
>  >> > >> > to the tunnel key of localnet port in the match conditions.
>  >> > >> >
>  >> > >> > In case the chassis goes down for some reason, it is the
>  >> > >> > responsibility of CMS to change the 'requested-chassis'

Re: [ovs-dev] [RFC v3 0/5] Address MTU issue for larger packets in OVN

[Numan Siddique]

Just a gentle reminder on some feedback about the approach in the first two
patches in the series :)

Thanks
Numan


On Thu, Jan 10, 2019 at 11:29 PM  wrote:

> From: Numan Siddique 
>
> This is an RFC series to address the MTU issues for OVN reported
> here [1].
>
> To address this issue, a new OVS action - check_pkt_larger is added.
> A new datapath action is also added - check_pkt_len.
>
> The datapath patch is submitted here to get feedback before submitting
> to the kernel net-dev ML.
>
> v2 --> v3
> -
> In v2, the check_pkt_len implementation in odp-execute.c (in p2) was
> missing,
> which is added in v3.
>
> 
>
> v1 of RFC included patches for datapath and OVS. This v2 series also
> includes the corresponding OVN changes.
>
> In v1, check_pkt_len datapath action was implemented as
>   - check_pkt_len(pkt_len, condition, userspace action if condition
> matches)
>Eg.
> check_pkt_len( > 1500 ? userspace(pid=,slow_path(check_pkt_len))
> If the packet length is greater than 1500 bytes, send it to userspace.
>
> In v2 a different approach is taken
>   - check_pkt_len(> pkt_len ? (set of action to apply) : (another set of
> actions to apply))
> Eg. check_pkt_len(> 1500 ? (2): (3))
> If the packet length is greater than 1500, output to port 2, else
> output to port 3.
>
> ovs-vswitchd is modified accordingly.
>
> The ovs action - check_pkt_larger is unchanged. It looks like
>   - check_pkt_larger(pkt_len)->REGISTER_BIT.
> Eg.  check_pkt_larger(1500)->NXM_NX_REG0[0]
> If the packet is larger than 1500 bytes, REG0[0] will be set to 1.
>
> More details in the patches.
>
> If the approach seems reasonable, I will submit the datapath patch first
> to net-dev ML first.
>
> [1] -
> https://mail.openvswitch.org/pipermail/ovs-discuss/2018-July/047039.html
>
> Datapath patch
> -
> Numan Siddique (1):
>  datapath: Add a new action check_pkt_len
>
>  include/uapi/linux/openvswitch.h |  25 -
>  net/openvswitch/actions.c|  55 +-
>  net/openvswitch/flow_netlink.c   | 175 +++
>  3 files changed, 253 insertions(+), 2 deletions(-)
>
>
> OVS patches
> ---
> Numan Siddique (4):
>   Add a new OVS action check_pkt_larger
>   ovn: Add a new OVN field icmp4.frag_mtu
>   ovn: Support OVS action 'check_pkt_larger' in OVN
>   ovn: Generate ICMPv4 packet in router pipeline for larger packets
>
>  .../linux/compat/include/linux/openvswitch.h  |  30 ++-
>  include/openvswitch/ofp-actions.h |  18 ++
>  include/ovn/actions.h |  33 ++-
>  include/ovn/automake.mk   |   3 +-
>  include/ovn/expr.h|   5 +
>  {ovn/lib => include/ovn}/logical-fields.h |  41 
>  lib/dpif-netdev.c |   1 +
>  lib/dpif.c|   1 +
>  lib/odp-execute.c |   4 +
>  lib/odp-util.c|  36 
>  lib/ofp-actions.c |  98 -
>  lib/ofp-parse.c   |  10 +
>  ofproto/ofproto-dpif-ipfix.c  |   1 +
>  ofproto/ofproto-dpif-sflow.c  |   1 +
>  ofproto/ofproto-dpif-xlate.c  | 108 ++
>  ofproto/ofproto-dpif.c|  47 +
>  ofproto/ofproto-dpif.h|   5 +-
>  ovn/controller/lflow.c|   1 +
>  ovn/controller/lflow.h|   2 +-
>  ovn/controller/pinctrl.c  |  38 +++-
>  ovn/lib/actions.c | 188 ++---
>  ovn/lib/automake.mk   |   1 -
>  ovn/lib/expr.c|  17 +-
>  ovn/lib/logical-fields.c  |  36 +++-
>  ovn/northd/ovn-northd.8.xml   |  83 +++-
>  ovn/northd/ovn-northd.c   |  91 -
>  ovn/ovn-sb.xml|  32 +++
>  ovn/utilities/ovn-trace.c |   9 +-
>  tests/ovn.at  | 193 +-
>  tests/test-ovn.c  |   2 +-
>  30 files changed, 1072 insertions(+), 63 deletions(-)
>  rename {ovn/lib => include/ovn}/logical-fields.h (71%)
>
> --
> 2.20.1
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5] ovn: Support a new Logical_Switch_Port.type - 'external'

[Han Zhou]

On Thu, Jan 24, 2019 at 10:56 AM Numan Siddique  wrote:
>
>
>
> On Thu, Jan 24, 2019 at 9:20 PM Han Zhou  wrote:
>>
>> On Mon, Jan 21, 2019 at 7:06 AM Miguel Angel Ajo Pelayo
>>  wrote:
>> >
>> >
>> >
>> > On Mon, Jan 21, 2019 at 4:02 PM Numan Siddique  wrote:
>> >>
>> >>
>> >> Hi Han,
>> >>
>> >> I have addressed your comments. But before posting the patch I wanted to 
>> >> get an opinion
>> >> on the HA support for these external ports.
>> >>
>> >> The proposed patch doesn't support HA. If the requested chassis goes down 
>> >> for some reason
>> >> it is expected that CMS would detect it and change the requested-chassis 
>> >> option to other
>> >> suitable chassis.
>> >>
>> >> The openstack OVN folks think this would be too much for the CMS to 
>> >> handle and it would
>> >> complicate the code in networking-ovn which I agree with.
>> >>
>> >
>> > Not only the complexity part. If we implement this from the CMS, then 
>> > every CMS using ovn
>> > will need to replicate that behaviour.
>> >
>> > That's in my opinion a good reason why it's better to handle HA within OVN 
>> > itself.
>> >
>> >>
>> >> I am thinking to add the HA support on the lines of gateway chassis 
>> >> support and I want to
>> >> submit this patch after adding the HA support. I think this would be 
>> >> better as we won't add
>> >> more options in OVN (first requested-chassis for external ports and then 
>> >> later HA chassis support).
>> >> Thoughts?
>>
>> I thought it would be easier to support outside of OVN combining with
>> chassis life-cycle management, but I didn't go deeper in any CMS
>> implementation. I agree it is better to handle HA in OVN than
>> implementing it in every CMS. But I am also worring about the
>> complexity in OVN itself. Could you describe briefly how would you
>> support it in OVN? For example, how to detect if a chassis failed? It
>> is different from gateway chassis because the major use case of
>> external port is for bridged networks (vlan/flat), so I think the BFD
>> mechanism for tunnel health monitoring may not be a good fit here.
>
>
> At present, as you know, ovn-controller's do establish geneve tunnels
> even if there are only logical switches representing bridged networks.
> So I feel we can leverage it unless we have a better mechanism
> to detect health monitoring.
>
> Do you have any thoughts/ideas of any other possibilities ?
>
> I am also trying to make the HA support more generic and a bit simpler 
> (hopefully :))
> so that it can be used either for "external" ports or for the redirectchassis 
> router ports.
>
One example that the BFD monitoring may not be good for external port
HA is, in purely bridged environment there is a potential optimization
to not creat the tunnel mesh at all, but this BFD dependency makes it
harder. However it seems not a strong blocker, since the optimization
can selectively create tunnels.

If we have to implement HV in OVN, I don't have any better idea than
BFD for now. It may be good in practice. We'd better hear opinion from
more people. It may worth abstracting data plane monitoring mechanism,
and implement new mechanisms in the future, but it doesn't need to be
in this version of code if BFD is the only mechanism for now anyway.

Outside of OVN, I believe all chassis manage systems should have their
own ways of monitoring. Would it be sufficient just to provide an
interface from OVN (and even CMS) so that the failures detected by
external systems can be used to trigger the failover?

In addition, the current priority based gateway chassis HA mechanism
has the split-brain problem upon network partitioning. This may be a
independent topic ;)

>
> Thanks
> Numan
>
>> Thanks,
>> Han
>> >>
>> >> Thanks
>> >> Numan
>> >>
>> >>
>> >> On Sat, Jan 19, 2019 at 12:42 AM Numan Siddique  
>> >> wrote:
>> >>>
>> >>>
>> >>>
>> >>> On Sat, Jan 19, 2019, 12:32 AM Han Zhou > 
>>  On Fri, Jan 18, 2019 at 10:16 AM Numan Siddique  
>>  wrote:
>>  >
>>  >
>>  >
>>  > On Fri, Jan 18, 2019 at 2:11 AM Han Zhou  wrote:
>>  >>
>>  >> On Thu, Jan 17, 2019 at 11:32 AM Han Zhou  wrote:
>>  >> >
>>  >> > On Thu, Jan 17, 2019 at 11:25 AM Numan Siddique 
>>  >> >  wrote:
>>  >> > >
>>  >> > >
>>  >> > >
>>  >> > > On Fri, Jan 18, 2019 at 12:21 AM Han Zhou  
>>  >> > > wrote:
>>  >> > >>
>>  >> > >> Hi Numan,
>>  >> > >>
>>  >> > >> With v5 the new test case "external logical port" fails.
>>  >> > >> And please see more comments inlined.
>>  >> > >>
>>  >> > >> On Tue, Jan 15, 2019 at 12:09 PM  wrote:
>>  >> > >> >
>>  >> > >> > From: Numan Siddique 
>>  >> > >> >
>>  >> > >> > In the case of OpenStack + OVN, when the VMs are booted on
>>  >> > >> > hypervisors supporting SR-IOV nics, there are no OVS ports
>>  >> > >> > for these VMs. When these VMs sends DHCPv4, DHPCv6 or IPv6
>>  >> > >> > Router Solicitation requests, the local ovn-controller
>>  >> > >

Re: [ovs-dev] compat: Fixup ipv6 fragmentation on 4.9.135+ kernels

[Gregory Rose]

On 1/24/2019 10:21 AM, Aaron Conole wrote:

Hi Greg,

0-day Robot  writes:


Bleep bloop.  Greetings Greg Rose, I am a robot and I have tried out your patch.
Thanks for your contribution.

I encountered some error that I wasn't expecting.  See the details below.


git-am:
Failed to merge in the changes.
Patch failed at 0001 compat: Fixup ipv6 fragmentation on 4.9.135+ kernels
The copy of the patch that failed is found in:

/var/lib/jenkins/jobs/upstream_build_from_pw/workspace/.git/rebase-apply/patch
When you have resolved this problem, run "git am --resolved".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".


In the future, you can avoid this bleep-bloop by using the branch name.
IE: branch-2.10 rather than Branch 2.10

I'll add looking at improving the branch detection as well, but not sure
when I'll get around to that.


Thanks for the tip!  Seems minor and I'm sure there's lots of other 
productive things for you to work

on.

;^)

- Greg


Please check this out.  If you feel there has been an error, please
email acon...@bytheb.org

Thanks,
0-day Robot
___
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] [RFC 1/7] ovsdb-client.c: fix typo

[Han Zhou]

From: Han Zhou 

Signed-off-by: Han Zhou 
---
 ovsdb/ovsdb-client.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovsdb/ovsdb-client.c b/ovsdb/ovsdb-client.c
index 83c3c12..0215357 100644
--- a/ovsdb/ovsdb-client.c
+++ b/ovsdb/ovsdb-client.c
@@ -1267,7 +1267,7 @@ ovsdb_client_cond_change(struct unixctl_conn *conn, int 
argc OVS_UNUSED,
 jsonrpc_send(rpc, request);
 
 VLOG_DBG("cond change %s %s", argv[1], argv[2]);
-unixctl_command_reply(conn, "condiiton changed");
+unixctl_command_reply(conn, "condition changed");
 }
 
 static void
-- 
2.1.0

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

[ovs-dev] [RFC 2/7] ovsdb_monitor: Fix style of prototypes.

[Han Zhou]

From: Han Zhou 

Ommiting the parameter names in prototypes, as suggested by coding
style: Omit parameter names from function prototypes when the names
do not give useful information.

Adjust orders of parameters as suggested by coding style.

Signed-off-by: Han Zhou 
---
 ovsdb/jsonrpc-server.c |  4 +--
 ovsdb/monitor.c| 14 +--
 ovsdb/monitor.h| 66 +-
 3 files changed, 42 insertions(+), 42 deletions(-)

diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index 7c7a277..77f15d9 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -1578,7 +1578,7 @@ ovsdb_jsonrpc_monitor_cond_change(struct 
ovsdb_jsonrpc_session *s,
 struct json *update_json;
 
 update_json = ovsdb_monitor_get_update(m->dbmon, false, true,
-&m->unflushed, m->condition, m->version);
+m->condition, m->version, &m->unflushed);
 if (update_json) {
 struct jsonrpc_msg *msg;
 struct json *p;
@@ -1653,7 +1653,7 @@ ovsdb_jsonrpc_monitor_compose_update(struct 
ovsdb_jsonrpc_monitor *m,
 }
 
 return ovsdb_monitor_get_update(m->dbmon, initial, false,
-&m->unflushed, m->condition, m->version);
+m->condition, m->version, &m->unflushed);
 }
 
 static bool
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index dd06e26..bf130ad 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -158,14 +158,14 @@ typedef struct json *
  const void *,
  bool initial, unsigned long int *changed);
 
-static void ovsdb_monitor_destroy(struct ovsdb_monitor *dbmon);
+static void ovsdb_monitor_destroy(struct ovsdb_monitor *);
 static struct ovsdb_monitor_changes * ovsdb_monitor_table_add_changes(
-struct ovsdb_monitor_table *mt, uint64_t next_txn);
+struct ovsdb_monitor_table *, uint64_t next_txn);
 static struct ovsdb_monitor_changes *ovsdb_monitor_table_find_changes(
-struct ovsdb_monitor_table *mt, uint64_t unflushed);
+struct ovsdb_monitor_table *, uint64_t unflushed);
 static void ovsdb_monitor_changes_destroy(
-  struct ovsdb_monitor_changes *changes);
-static void ovsdb_monitor_table_track_changes(struct ovsdb_monitor_table *mt,
+  struct ovsdb_monitor_changes *);
+static void ovsdb_monitor_table_track_changes(struct ovsdb_monitor_table *,
   uint64_t unflushed);
 
 static uint32_t
@@ -1103,9 +1103,9 @@ struct json *
 ovsdb_monitor_get_update(
  struct ovsdb_monitor *dbmon,
  bool initial, bool cond_updated,
- uint64_t *unflushed_,
  struct ovsdb_monitor_session_condition *condition,
- enum ovsdb_monitor_version version)
+ enum ovsdb_monitor_version version,
+ uint64_t *unflushed_)
 {
 struct ovsdb_monitor_json_cache_node *cache_node = NULL;
 struct shash_node *node;
diff --git a/ovsdb/monitor.h b/ovsdb/monitor.h
index eb3ff27..9bc0613 100644
--- a/ovsdb/monitor.h
+++ b/ovsdb/monitor.h
@@ -44,73 +44,73 @@ enum ovsdb_monitor_version {
   OVSDB_MONITOR_VERSION_MAX
 };
 
-struct ovsdb_monitor *ovsdb_monitor_create(struct ovsdb *db,
-   struct ovsdb_jsonrpc_monitor *jsonrpc_monitor);
+struct ovsdb_monitor *ovsdb_monitor_create(struct ovsdb *,
+   struct ovsdb_jsonrpc_monitor *);
 void ovsdb_monitors_remove(struct ovsdb *);
 void ovsdb_monitors_commit(struct ovsdb *, const struct ovsdb_txn *);
 
 void ovsdb_monitor_prereplace_db(struct ovsdb *);
 
-struct ovsdb_monitor *ovsdb_monitor_add(struct ovsdb_monitor *dbmon);
+struct ovsdb_monitor *ovsdb_monitor_add(struct ovsdb_monitor *);
 
-void ovsdb_monitor_add_jsonrpc_monitor(struct ovsdb_monitor *dbmon,
-   struct ovsdb_jsonrpc_monitor *jsonrpc_monitor);
+void ovsdb_monitor_add_jsonrpc_monitor(struct ovsdb_monitor *,
+   struct ovsdb_jsonrpc_monitor *);
 
-void ovsdb_monitor_remove_jsonrpc_monitor(struct ovsdb_monitor *dbmon,
-   struct ovsdb_jsonrpc_monitor *jsonrpc_monitor,
+void ovsdb_monitor_remove_jsonrpc_monitor(struct ovsdb_monitor *,
+   struct ovsdb_jsonrpc_monitor *,
uint64_t unflushed);
 
-void ovsdb_monitor_add_table(struct ovsdb_monitor *m,
- const struct ovsdb_table *table);
+void ovsdb_monitor_add_table(struct ovsdb_monitor *,
+ const struct ovsdb_table *);
 
-const char * ovsdb_monitor_add_column(struct ovsdb_monitor *dbmon,
-  const struct ovsdb_table *table,
-  const struct ovsdb_column *column,
-  enum ovsdb_monitor_selection select,
+const char * ovsdb_monitor_add_column(struct ovsdb_monitor *,
+ 

[ovs-dev] [RFC 3/7] ovsdb-monitor: Refactor ovsdb monitor implementation.

[Han Zhou]

From: Han Zhou 

Current ovsdb monitor maintains pending changes through an incremental
integer to figure out if the set of changes should be flushed. And it
uses number 0 to represent that the change set contains all data for
initial client population.  It is a smart way but it prevents further
extension of the monitoring mechanism to support future use case of
monitoring starting from an arbitory history point. This patch
refactors the structures so that change sets are tracked directly,
instead of through calculated version numbers based on implicite
rules.

Signed-off-by: Han Zhou 
---
 ovsdb/jsonrpc-server.c |  18 ++-
 ovsdb/monitor.c| 365 +
 ovsdb/monitor.h|  10 +-
 3 files changed, 204 insertions(+), 189 deletions(-)

diff --git a/ovsdb/jsonrpc-server.c b/ovsdb/jsonrpc-server.c
index 77f15d9..f9b7c27 100644
--- a/ovsdb/jsonrpc-server.c
+++ b/ovsdb/jsonrpc-server.c
@@ -1216,8 +1216,7 @@ struct ovsdb_jsonrpc_monitor {
 struct ovsdb *db;
 struct json *monitor_id;
 struct ovsdb_monitor *dbmon;
-uint64_t unflushed; /* The first transaction that has not been
-   flushed to the jsonrpc remote client. */
+struct ovsdb_monitor_change_set *change_set;
 enum ovsdb_monitor_version version;
 struct ovsdb_monitor_session_condition *condition;/* Session's condition */
 };
@@ -1389,7 +1388,6 @@ ovsdb_jsonrpc_monitor_create(struct ovsdb_jsonrpc_session 
*s, struct ovsdb *db,
 if (version == OVSDB_MONITOR_V2) {
 m->condition = ovsdb_monitor_session_condition_create();
 }
-m->unflushed = 0;
 m->version = version;
 hmap_insert(&s->monitors, &m->node, json_hash(monitor_id, 0));
 m->monitor_id = json_clone(monitor_id);
@@ -1436,7 +1434,7 @@ ovsdb_jsonrpc_monitor_create(struct ovsdb_jsonrpc_session 
*s, struct ovsdb *db,
 dbmon = ovsdb_monitor_add(m->dbmon);
 if (dbmon != m->dbmon) {
 /* Found an exisiting dbmon, reuse the current one. */
-ovsdb_monitor_remove_jsonrpc_monitor(m->dbmon, m, m->unflushed);
+ovsdb_monitor_remove_jsonrpc_monitor(m->dbmon, m, NULL);
 ovsdb_monitor_add_jsonrpc_monitor(dbmon, m);
 m->dbmon = dbmon;
 }
@@ -1446,7 +1444,7 @@ ovsdb_jsonrpc_monitor_create(struct ovsdb_jsonrpc_session 
*s, struct ovsdb *db,
 ovsdb_monitor_condition_bind(m->dbmon, m->condition);
 }
 
-ovsdb_monitor_get_initial(m->dbmon);
+ovsdb_monitor_get_initial(m->dbmon, &m->change_set);
 json = ovsdb_jsonrpc_monitor_compose_update(m, true);
 json = json ? json : json_object_create();
 return jsonrpc_create_reply(json, request_id);
@@ -1578,7 +1576,7 @@ ovsdb_jsonrpc_monitor_cond_change(struct 
ovsdb_jsonrpc_session *s,
 struct json *update_json;
 
 update_json = ovsdb_monitor_get_update(m->dbmon, false, true,
-m->condition, m->version, &m->unflushed);
+m->condition, m->version, &m->change_set);
 if (update_json) {
 struct jsonrpc_msg *msg;
 struct json *p;
@@ -1648,12 +1646,12 @@ ovsdb_jsonrpc_monitor_compose_update(struct 
ovsdb_jsonrpc_monitor *m,
  bool initial)
 {
 
-if (!ovsdb_monitor_needs_flush(m->dbmon, m->unflushed)) {
+if (!ovsdb_monitor_needs_flush(m->dbmon, m->change_set)) {
 return NULL;
 }
 
 return ovsdb_monitor_get_update(m->dbmon, initial, false,
-m->condition, m->version, &m->unflushed);
+m->condition, m->version, &m->change_set);
 }
 
 static bool
@@ -1662,7 +1660,7 @@ ovsdb_jsonrpc_monitor_needs_flush(struct 
ovsdb_jsonrpc_session *s)
 struct ovsdb_jsonrpc_monitor *m;
 
 HMAP_FOR_EACH (m, node, &s->monitors) {
-if (ovsdb_monitor_needs_flush(m->dbmon, m->unflushed)) {
+if (ovsdb_monitor_needs_flush(m->dbmon, m->change_set)) {
 return true;
 }
 }
@@ -1686,7 +1684,7 @@ ovsdb_jsonrpc_monitor_destroy(struct 
ovsdb_jsonrpc_monitor *m,
 
 json_destroy(m->monitor_id);
 hmap_remove(&m->session->monitors, &m->node);
-ovsdb_monitor_remove_jsonrpc_monitor(m->dbmon, m, m->unflushed);
+ovsdb_monitor_remove_jsonrpc_monitor(m->dbmon, m, m->change_set);
 ovsdb_monitor_session_condition_destroy(m->condition);
 free(m);
 }
diff --git a/ovsdb/monitor.c b/ovsdb/monitor.c
index bf130ad..d42dddf 100644
--- a/ovsdb/monitor.c
+++ b/ovsdb/monitor.c
@@ -69,17 +69,31 @@ struct ovsdb_monitor {
 struct shash tables; /* Holds "struct ovsdb_monitor_table"s. */
 struct ovs_list jsonrpc_monitors;  /* Contains "jsonrpc_monitor_node"s. */
 struct ovsdb *db;
-uint64_t n_transactions;  /* Count number of committed transactions. */
-struct hmap_node hmap_node;   /* Elements within ovsdb_monitors.  */
-struct hmap json_cache;   /* Contains 
"ovsdb_monitor_json_cache_node

[ovs-dev] [RFC 4/7] ovsdb-server: Transaction history tracking.

[Han Zhou]

From: Han Zhou 

Maintaining last N (n = 100) transactions in memory, which will be
used for future patches for generating monitor data from any point
in this N transactions.

Signed-off-by: Han Zhou 
---
 ovsdb/ovsdb-server.c |  8 +
 ovsdb/ovsdb.h| 10 ++
 ovsdb/transaction.c  | 97 +++-
 ovsdb/transaction.h  |  2 ++
 4 files changed, 116 insertions(+), 1 deletion(-)

diff --git a/ovsdb/ovsdb-server.c b/ovsdb/ovsdb-server.c
index 65a47a4..2e3a84a 100644
--- a/ovsdb/ovsdb-server.c
+++ b/ovsdb/ovsdb-server.c
@@ -219,6 +219,7 @@ main_loop(struct server_config *config,
 struct shash_node *next;
 SHASH_FOR_EACH_SAFE (node, next, all_dbs) {
 struct db *db = node->data;
+ovsdb_txn_history_run(db->db);
 if (ovsdb_trigger_run(db->db, time_msec())) {
 /* The message below is currently the only reason to disconnect
  * all clients. */
@@ -568,6 +569,7 @@ parse_txn(struct server_config *config, struct db *db,
 
 error = ovsdb_file_txn_from_json(db->db, txn_json, false, &txn);
 if (!error) {
+ovsdb_txn_set_txnid(txnid, txn);
 log_and_free_error(ovsdb_txn_replay_commit(txn));
 }
 if (!error && !uuid_is_zero(txnid)) {
@@ -658,6 +660,10 @@ open_db(struct server_config *config, const char *filename)
 db->db = ovsdb_create(schema, storage);
 ovsdb_jsonrpc_server_add_db(config->jsonrpc, db->db);
 
+db->db->need_txn_history = true;
+db->db->n_txn_history = 0;
+ovs_list_init(&db->db->txn_history);
+
 read_db(config, db);
 
 error = (db->db->name[0] == '_'
@@ -695,6 +701,8 @@ add_server_db(struct server_config *config)
 json_destroy(schema_json);
 
 struct db *db = xzalloc(sizeof *db);
+/* We don't need txn_history for server_db. */
+
 db->filename = xstrdup("");
 db->db = ovsdb_create(schema, ovsdb_storage_create_unbacked());
 bool ok OVS_UNUSED = ovsdb_jsonrpc_server_add_db(config->jsonrpc, db->db);
diff --git a/ovsdb/ovsdb.h b/ovsdb/ovsdb.h
index d96b1c2..32e5333 100644
--- a/ovsdb/ovsdb.h
+++ b/ovsdb/ovsdb.h
@@ -67,6 +67,11 @@ bool ovsdb_parse_version(const char *, struct ovsdb_version 
*);
 bool ovsdb_is_valid_version(const char *);
 
 /* Database. */
+struct ovsdb_txn_history_node {
+struct ovs_list node; /* Element in struct ovsdb's txn_history list */
+struct ovsdb_txn *txn;
+};
+
 struct ovsdb {
 char *name;
 struct ovsdb_schema *schema;
@@ -80,6 +85,11 @@ struct ovsdb {
 bool run_triggers;
 
 struct ovsdb_table *rbac_role;
+
+/* History trasanctions for incremental monitor transfer. */
+bool need_txn_history; /* Need to maintain history of transactions. */
+unsigned int n_txn_history; /* Current number of history transactions. */
+struct ovs_list txn_history; /* Contains "struct ovsdb_txn_history_node. */
 };
 
 struct ovsdb *ovsdb_create(struct ovsdb_schema *, struct ovsdb_storage *);
diff --git a/ovsdb/transaction.c b/ovsdb/transaction.c
index 5a43132..18b646b 100644
--- a/ovsdb/transaction.c
+++ b/ovsdb/transaction.c
@@ -40,6 +40,7 @@ struct ovsdb_txn {
 struct ovsdb *db;
 struct ovs_list txn_tables; /* Contains "struct ovsdb_txn_table"s. */
 struct ds comment;
+struct uuid txnid; /* For clustered mode only. It is the eid. */
 };
 
 /* A table modified by a transaction. */
@@ -106,13 +107,19 @@ static unsigned int serial;
 struct ovsdb_txn *
 ovsdb_txn_create(struct ovsdb *db)
 {
-struct ovsdb_txn *txn = xmalloc(sizeof *txn);
+struct ovsdb_txn *txn = xzalloc(sizeof *txn);
 txn->db = db;
 ovs_list_init(&txn->txn_tables);
 ds_init(&txn->comment);
 return txn;
 }
 
+void
+ovsdb_txn_set_txnid(const struct uuid *txnid, struct ovsdb_txn *txn)
+{
+txn->txnid = *txnid;
+}
+
 static void
 ovsdb_txn_free(struct ovsdb_txn *txn)
 {
@@ -881,11 +888,79 @@ ovsdb_txn_precommit(struct ovsdb_txn *txn)
 return error;
 }
 
+static struct ovsdb_txn*
+ovsdb_txn_clone(const struct ovsdb_txn *txn)
+{
+struct ovsdb_txn *txn_cloned = xzalloc(sizeof *txn_cloned);
+ovs_list_init(&txn_cloned->txn_tables);
+txn_cloned->txnid = txn->txnid;
+
+struct ovsdb_txn_table *t;
+LIST_FOR_EACH (t, node, &txn->txn_tables) {
+struct ovsdb_txn_table *t_cloned = xmalloc(sizeof *t_cloned);
+ovs_list_push_back(&txn_cloned->txn_tables, &t_cloned->node);
+hmap_init(&t_cloned->txn_rows);
+
+struct ovsdb_txn_row *r;
+HMAP_FOR_EACH (r, hmap_node, &t->txn_rows) {
+size_t n_columns = shash_count(&t->table->schema->columns);
+struct ovsdb_txn_row *r_cloned =
+xzalloc(offsetof(struct ovsdb_txn_row, changed)
++ bitmap_n_bytes(n_columns));
+
+r_cloned->uuid = r->uuid;
+r_cloned->table = r->table;
+r_cloned->old = r->old ? ovsdb_row_clone(r->old) : NULL;
+r_clone

[ovs-dev] [RFC 7/7] ovsdb-idl.c: Fast resync from server when connection reset.

[Han Zhou]

From: Han Zhou 

Use monitor_cond_since to request changes after last version of local
data when connection to server is reset, without clearing the local
data. It falls back to clearing and repopulate all the data when
the requested id cannot be fulfilled by the server.

Signed-off-by: Han Zhou 
---
 lib/ovsdb-idl.c | 31 +++
 1 file changed, 27 insertions(+), 4 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index 7ef4ed0..8cfb201 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -216,6 +216,9 @@ struct ovsdb_idl_db {
 bool has_lock;  /* Has db server told us we have the lock? */
 bool is_lock_contended; /* Has db server told us we can't get lock? */
 struct json *lock_request_id; /* JSON-RPC ID of in-flight lock request. */
+
+/* Last db txn id, used for fast resync through monitor_cond_since */
+struct uuid last_id;
 };
 
 static void ovsdb_idl_db_track_clear(struct ovsdb_idl_db *);
@@ -2070,9 +2073,8 @@ ovsdb_idl_send_monitor_request(struct ovsdb_idl *idl, 
struct ovsdb_idl_db *db,
 break;
 case OVSDB_IDL_MM_MONITOR_COND_SINCE:
 method = "monitor_cond_since";
-struct uuid last_id = UUID_ZERO;
 struct json *json_last_id = json_string_create_nocopy(
-xasprintf(UUID_FMT, UUID_ARGS(&last_id)));
+xasprintf(UUID_FMT, UUID_ARGS(&db->last_id)));
 json_array_add(params, json_last_id);
 break;
 default:
@@ -2100,6 +2102,7 @@ ovsdb_idl_db_parse_monitor_reply(struct ovsdb_idl_db *db,
 {
 db->change_seqno++;
 const struct json *table_updates = result;
+bool clear_db = true;
 if (method == OVSDB_IDL_MM_MONITOR_COND_SINCE) {
 if (result->type != JSON_ARRAY || result->array.n != 3) {
 struct ovsdb_error *error = ovsdb_syntax_error(result, NULL,
@@ -2110,12 +2113,24 @@ ovsdb_idl_db_parse_monitor_reply(struct ovsdb_idl_db 
*db,
 }
 
 bool found = json_boolean(result->array.elems[0]);
+if (found) {
+clear_db = false;
+}
+
 const char *last_id = json_string(result->array.elems[1]);
+if (!uuid_from_string(&db->last_id, last_id)) {
+struct ovsdb_error *error = ovsdb_syntax_error(result, NULL,
+ "Last-id %s is not in UUID format.",
+ last_id);
+log_parse_update_error(error);
+return;
+}
 
 table_updates = result->array.elems[2];
-
 }
-ovsdb_idl_db_clear(db);
+if (clear_db) {
+ovsdb_idl_db_clear(db);
+}
 ovsdb_idl_db_parse_update(db, table_updates, method);
 }
 
@@ -2158,6 +2173,14 @@ ovsdb_idl_db_parse_update_rpc(struct ovsdb_idl_db *db,
 struct json *table_updates = params->array.elems[1];
 if (!strcmp(msg->method, "update3")) {
 table_updates = params->array.elems[2];
+const char *last_id = json_string(params->array.elems[1]);
+if (!uuid_from_string(&db->last_id, last_id)) {
+struct ovsdb_error *error = ovsdb_syntax_error(params, NULL,
+ "Last-id %s is not in UUID format.",
+ last_id);
+log_parse_update_error(error);
+return false;
+}
 }
 ovsdb_idl_db_parse_update(db, table_updates, mm);
 return true;
-- 
2.1.0

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

[ovs-dev] [RFC 6/7] ovsdb-idl.c: Support monitor_cond_since method in C IDL.

[Han Zhou]

From: Han Zhou 

Use monitor_cond_since in C IDL. If it is not supported by server,
fall back to old method (monitor_cond, or monitor).

Signed-off-by: Han Zhou 
---
 lib/ovsdb-idl.c | 206 +++-
 1 file changed, 158 insertions(+), 48 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index a7274de..7ef4ed0 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -106,10 +106,10 @@ struct ovsdb_idl_arc {
  * connection.  The next connection attempt has a chance at \
  * picking a connected server.  \
  *  \
- *   * Otherwise, sends a "monitor_cond" request for the IDL\
+ *   * Otherwise, sends a "monitor_cond_since" request for the IDL  \
  * database whose details are informed by the schema\
  * (obtained from the row), and transitions to  \
- * IDL_S_DATA_MONITOR_COND_REQUESTED.   \
+ * IDL_S_DATA_MONITOR_COND_SINCE_REQUESTED. \
  *  \
  * - If the reply indicates success, but the Database table does\
  *   not have a row for the IDL database, transitions to\
@@ -125,6 +125,13 @@ struct ovsdb_idl_arc {
  * transitions to IDL_S_DATA_MONITOR_COND_REQUESTED. */ \
 OVSDB_IDL_STATE(DATA_SCHEMA_REQUESTED)  \
 \
+/* Waits for "monitor_cond_since" reply.  If successful, replaces   \
+ * the IDL contents by the data carried in the reply and\
+ * transitions to IDL_S_MONITORING.  On failure, sends a\
+ * "monitor_cond" request and transitions to\
+ * IDL_S_DATA_MONITOR_COND_REQUESTED. */\
+OVSDB_IDL_STATE(DATA_MONITOR_COND_SINCE_REQUESTED)  \
+\
 /* Waits for "monitor_cond" reply.  If successful, replaces the \
  * IDL contents by the data carried in the reply and transitions\
  * to IDL_S_MONITORING.  On failure, sends a "monitor" request  \
@@ -136,9 +143,9 @@ struct ovsdb_idl_arc {
  * IDL_S_MONITORING.  On failure, transitions to IDL_S_ERROR. */\
 OVSDB_IDL_STATE(DATA_MONITOR_REQUESTED) \
 \
-/* State that processes "update" or "update2" notifications for \
- * the main database (and the Database table in _Server if  \
- * available).  \
+/* State that processes "update", "update2" or "update3"\
+ * notifications for the main database (and the Database table  \
+ * in _Server if available).\
  *  \
  * If we're monitoring the Database table and we get notified   \
  * that the IDL database has been deleted, we close the \
@@ -167,10 +174,18 @@ enum ovsdb_idl_state {
 
 static const char *ovsdb_idl_state_to_string(enum ovsdb_idl_state);
 
+enum ovsdb_idl_monitor_method {
+OVSDB_IDL_MM_MONITOR,
+OVSDB_IDL_MM_MONITOR_COND,
+OVSDB_IDL_MM_MONITOR_COND_SINCE
+};
+
 enum ovsdb_idl_monitoring {
 OVSDB_IDL_NOT_MONITORING,   /* Database is not being monitored. */
 OVSDB_IDL_MONITORING,   /* Database has "monitor" outstanding. */
 OVSDB_IDL_MONITORING_COND,  /* Database has "monitor_cond" outstanding. */
+OVSDB_IDL_MONITORING_COND_SINCE,  /* Database has "monitor_cond_since"
+ outstanding. */
 };
 
 struct ovsdb_idl_db {
@@ -220,7 +235,7 @@ static void ovsdb_idl_send_db_change_aware(struct ovsdb_idl 
*);
 static bool ovsdb_idl_check_server_db(struct ovsdb_idl *);
 static void ovsdb_idl_send_monitor_request(struct ovsdb_idl *,
struct ovsdb_idl_db *,
-   bool use_monitor_cond);
+   enum ovsdb_idl_monitor_method);
 
 struct ovsdb_idl {
 struct ovsdb_idl_db server;
@@ -287,8 +302,8 @@ static struct vlog_rate_limit other_rl = 
VLOG_RATE_LIMIT_INIT(1, 5);
 
 static void ovsdb_idl_clear(struct ovsdb_idl *);
 static void ovsdb_idl_db_parse_monitor_reply(struct ovsdb_idl_db *,
- const struct json *result,
- bool is_monitor_cond);
+ const struct json *result,
+ enum ovsdb_idl_monitor_method method);
 static bool ovsdb_idl

[ovs-dev] [RFC 5/7] ovsdb-monitor: Support monitor_cond_since.

[Han Zhou]

From: Han Zhou 

Support the new monitor method monitor_cond_since so that a client
can request monitoring start from a specific point instead of always
from beginning. This will reduce the cost at scenarios when server
is restarted/failed-over but client still has all existing data. In
these scenarios only new changes (and in most cases no change) needed
to be transfered to client.

This change includes both server side and ovsdb-client side changes
with the new protocol. IDLs using this capability will be added in
future patches.

Note: the feature takes effect only for cluster mode of ovsdb-server,
because cluster mode is the only mode that supports unique transcation
uuid today. For other modes, the monitor_cond_since always fall back
to transfer all data with found = false.

For more details of the protocol change, see
Documentation/ref/ovsdb-server.7.rst.

Signed-off-by: Han Zhou 
---
 Documentation/ref/ovsdb-server.7.rst |  78 +-
 ovsdb/jsonrpc-server.c   |  85 --
 ovsdb/monitor.c  | 113 +-
 ovsdb/monitor.h  |   6 +
 ovsdb/ovsdb-client.c | 104 -
 ovsdb/transaction.c  |   6 +
 ovsdb/transaction.h  |   1 +
 tests/ovsdb-monitor.at   | 294 +++
 8 files changed, 659 insertions(+), 28 deletions(-)

diff --git a/Documentation/ref/ovsdb-server.7.rst 
b/Documentation/ref/ovsdb-server.7.rst
index 14c7da8..4312aab 100644
--- a/Documentation/ref/ovsdb-server.7.rst
+++ b/Documentation/ref/ovsdb-server.7.rst
@@ -364,7 +364,79 @@ Initial views of rows are not presented in update2 
notifications, but in the
 response object to the ``monitor_cond`` request.  The formatting of the
  object, however, is the same in either case.
 
-4.1.15 Get Server ID
+4.1.15 Monitor_cond_since
+-
+
+A new monitor method added in Open vSwitch version 2.12.  The
+``monitor_cond_since`` request enables a client to request changes that
+happened after a specific transaction id. A client can use this feature to
+request only latest changes after a server connection reset instead of
+re-transfer all data from the server again.
+
+The ``monitor_cond`` method described in Section 4.1.12 also applies to
+``monitor_cond_since``, with the following exceptions:
+
+* RPC request method becomes ``monitor_cond_since``.
+
+* Reply result includes extra parameters.
+
+* Subsequent changes are sent to the client using the ``update3`` monitor
+  notification, described in Section 4.1.16
+
+The request object has the following members::
+
+"method": "monitor_cond_since"
+"params": [, , , ]
+"id": 
+
+The  parameter is the transaction id that identifies the latest
+data the client already has, and it requests server to send changes AFTER this
+transaction (exclusive).
+
+All other parameters are the same as ``monitor_cond`` method.
+
+The response object has the following members::
+
+"result": [, , ]
+"error": null
+"id": same "id" as request
+
+The  is a boolean value that tells if the  requested by
+client is found in server's history or not. If true, the changes after that
+version up to current is sent. Otherwise, all data is sent.
+
+The  is the transaction id that identifies the latest transaction
+included in the changes in  of this response, so that client
+can keep tracking.  If there is no change involved in this response, it is the
+same as the  in the request if  is true, or zero uuid if
+ is false.  If the server does not support transaction uuid, it will
+be zero uuid as well.
+
+All other parameters are the same as in response object of ``monitor_cond``
+method.
+
+Like in ``monitor_cond``, subsequent changes that match conditions in
+ are automatically sent to the client, but using
+``update3`` monitor notification (see Section 4.1.16), instead of ``update2``.
+
+4.1.16 Update3 notification
+---
+
+The ``update3`` notification is sent by the server to the client to report
+changes in tables that are being monitored following a ``monitor_cond_since``
+request as described above. The notification has the following members::
+
+"method": "update3"
+"params": [, , ]
+"id": null
+
+The  is the same as described in the response object of
+``monitor_cond_since``.
+
+All other parameters are the same as in ``update2`` monitor notification (see
+Section 4.1.14).
+
+4.1.17 Get Server ID
 
 
 A new RPC method added in Open vSwitch version 2.7.  The request contains the
@@ -384,7 +456,7 @@ The response object contains the following members::
 running OVSDB server process.  A fresh UUID is generated when the process
 restarts.
 
-4.1.16 Database Change Awareness
+4.1.18 Database Change Awareness
 
 
 RFC 7047 does not provide a way for a client to find out about some kinds of
@@ -414,7 +486,7 @@ The reply is always the same::
 "erro

[ovs-dev] [RFC 0/7] Fast OVSDB resync after restart or failover.

[Han Zhou]

In scalability test with ovn-scale-test, ovsdb-server SB load is not a
problem at least with 1k HVs. However, if we restart the ovsdb-server,
depending on the number of HVs and scale of logical objects, e.g. the
number of logical ports, ovsdb-server of SB become an obvious bottleneck.

In our test with 1k HVs and 20k logical ports (200 lport * 100 lswitches
connected by one single logical router). Restarting ovsdb-server of SB
resulted in 100% CPU of ovsdb-server for more than 1 hour. All HVs (and
northd) are reconnecting and resyncing the big amount of data at the same
time.

Similar problem would happen in failover scenario. With active-active
cluster, the problem can be aleviated slightly, because only 1/3 (assuming
it is 3-node cluster) of the HVs will need to resync data from new servers,
but it is still a serious problem.

For detailed discussions for the problem and solutions, see:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047591.html

The patches implements the proposal in that discussion. It introduces
a new method monitor_cond_since to enable client to request changes that
happened after a specific point so that the data has been cached already
in client are not re-transfered.

The current patches supports all 3 modes of ovsdb-server, but only clustered
mode can benefit from it, since it is the only one that supports transaction
id out of the box.

Han Zhou (7):
  ovsdb-client.c: fix typo
  ovsdb_monitor: Fix style of prototypes.
  ovsdb-monitor: Refactor ovsdb monitor implementation.
  ovsdb-server: Transaction history tracking.
  ovsdb-monitor: Support monitor_cond_since.
  ovsdb-idl.c: Support monitor_cond_since method in C IDL.
  ovsdb-idl.c: Fast resync from server when connection reset.

 Documentation/ref/ovsdb-server.7.rst |  78 +-
 lib/ovsdb-idl.c  | 229 ++
 ovsdb/jsonrpc-server.c   | 101 ++--
 ovsdb/monitor.c  | 458 ++-
 ovsdb/monitor.h  |  78 +++---
 ovsdb/ovsdb-client.c | 106 +++-
 ovsdb/ovsdb-server.c |   8 +
 ovsdb/ovsdb.h|  10 +
 ovsdb/transaction.c  | 103 +++-
 ovsdb/transaction.h  |   3 +
 tests/ovsdb-monitor.at   | 294 ++
 11 files changed, 1181 insertions(+), 287 deletions(-)

-- 
2.1.0

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

[ovs-dev] [RFC 0/7] OVSDB client-server fast re-sync after restart or failover.

[Han Zhou]

(re-send this cover letter because it was blocked by the mailing-list saying:
"The message headers matched a filter rule")

In scalability test with ovn-scale-test, ovsdb-server SB load is not a
problem at least with 1k HVs. However, if we restart the ovsdb-server,
depending on the number of HVs and scale of logical objects, e.g. the
number of logical ports, ovsdb-server of SB become an obvious bottleneck.

In our test with 1k HVs and 20k logical ports (200 lport * 100 lswitches
connected by one single logical router). Restarting ovsdb-server of SB
resulted in 100% CPU of ovsdb-server for more than 1 hour. All HVs (and
northd) are reconnecting and resyncing the big amount of data at the same
time.

Similar problem would happen in failover scenario. With active-active
cluster, the problem can be aleviated slightly, because only 1/3 (assuming
it is 3-node cluster) of the HVs will need to resync data from new servers,
but it is still a serious problem.

For detailed discussions for the problem and solutions, see:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047591.html

The patches implements the proposal in that discussion. It introduces
a new method monitor_cond_since to enable client to request changes that
happened after a specific point so that the data has been cached already
in client are not re-transfered.

The current patches supports all 3 modes of ovsdb-server, but only clustered
mode can benefit from it, since it is the only one that supports transaction
id out of the box.

Han Zhou (7):
  ovsdb-client.c: fix typo
  ovsdb_monitor: Fix style of prototypes.
  ovsdb-monitor: Refactor ovsdb monitor implementation.
  ovsdb-server: Transaction history tracking.
  ovsdb-monitor: Support monitor_cond_since.
  ovsdb-idl.c: Support monitor_cond_since method in C IDL.
  ovsdb-idl.c: Fast resync from server when connection reset.

 Documentation/ref/ovsdb-server.7.rst |  78 +-
 lib/ovsdb-idl.c  | 229 ++
 ovsdb/jsonrpc-server.c   | 101 ++--
 ovsdb/monitor.c  | 458 ++-
 ovsdb/monitor.h  |  78 +++---
 ovsdb/ovsdb-client.c | 106 +++-
 ovsdb/ovsdb-server.c |   8 +
 ovsdb/ovsdb.h|  10 +
 ovsdb/transaction.c  | 103 +++-
 ovsdb/transaction.h  |   3 +
 tests/ovsdb-monitor.at   | 294 ++
 11 files changed, 1181 insertions(+), 287 deletions(-)

-- 
2.1.0

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

[ovs-dev] [RFC 0/7] ovsdb client-server incremental re-sync after restart or failover.

[Han Zhou]

(re-send this cover letter because it was blocked by the mailing-list saying:
"The message headers matched a filter rule")

In scalability test with ovn-scale-test, ovsdb-server SB load is not a
problem at least with 1k HVs. However, if we restart the ovsdb-server,
depending on the number of HVs and scale of logical objects, e.g. the
number of logical ports, ovsdb-server of SB become an obvious bottleneck.

In our test with 1k HVs and 20k logical ports (200 lport * 100 lswitches
connected by one single logical router). Restarting ovsdb-server of SB
resulted in 100% CPU of ovsdb-server for more than 1 hour. All HVs (and
northd) are reconnecting and resyncing the big amount of data at the same
time.

Similar problem would happen in failover scenario. With active-active
cluster, the problem can be aleviated slightly, because only 1/3 (assuming
it is 3-node cluster) of the HVs will need to resync data from new servers,
but it is still a serious problem.

For detailed discussions for the problem and solutions, see:
https://mail.openvswitch.org/pipermail/ovs-discuss/2018-October/047591.html

The patches implements the proposal in that discussion. It introduces
a new method monitor_cond_since to enable client to request changes that
happened after a specific point so that the data has been cached already
in client are not re-transfered.

The current patches supports all 3 modes of ovsdb-server, but only clustered
mode can benefit from it, since it is the only one that supports transaction
id out of the box.

Han Zhou (7):
  ovsdb-client.c: fix typo
  ovsdb_monitor: Fix style of prototypes.
  ovsdb-monitor: Refactor ovsdb monitor implementation.
  ovsdb-server: Transaction history tracking.
  ovsdb-monitor: Support monitor_cond_since.
  ovsdb-idl.c: Support monitor_cond_since method in C IDL.
  ovsdb-idl.c: Fast resync from server when connection reset.

 Documentation/ref/ovsdb-server.7.rst |  78 +-
 lib/ovsdb-idl.c  | 229 ++
 ovsdb/jsonrpc-server.c   | 101 ++--
 ovsdb/monitor.c  | 458 ++-
 ovsdb/monitor.h  |  78 +++---
 ovsdb/ovsdb-client.c | 106 +++-
 ovsdb/ovsdb-server.c |   8 +
 ovsdb/ovsdb.h|  10 +
 ovsdb/transaction.c  | 103 +++-
 ovsdb/transaction.h  |   3 +
 tests/ovsdb-monitor.at   | 294 ++
 11 files changed, 1181 insertions(+), 287 deletions(-)

-- 
2.1.0

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

[ovs-dev] [PATCH v4] Monitor Database table to manage lifecycle of IDL client.

[Ted Elhourani]

The Python IDL implementation supports ovsdb cluster connections.
This patch is a follow up to commit 31e434fc98, it adds the option of
connecting to the leader (the default) in the Raft-based cluster. It mimics
the exisiting C IDL support for clusters introduced in commit 1b1d2e6daa.

The _Server database schema is first requested, then a monitor of the
Database table in the _Server Database. Method __check_server_db verifies
the eligibility of the server. If the attempt to obtain a monitor of the
_Server database fails and a cluster id was not provided this implementation
proceeds to request the data monitor. If a cluster id was provided via the
set_cluster_id method then the connection is aborted and a connection to a
different node is instead attempted, until a valid cluster node is found.
Thus, when supplied, cluster id is interpreted as the intention to only
allow connections to a clustered database. If not supplied, connections to
standalone nodes, or nodes that do not have the _Server database are
allowed. change_seqno is not incremented in the case of Database table
updates.

Signed-off-by: Ted Elhourani 
---

v3 -> v4

* export -f is not compatible with FreeBSD, modify tests to avoid shell
function export.
* Re-add a line that was removed by mistake.

v2 -> v3

* Add 2 tests, treat cluster_id as a string, mv arg till end, pep8 fixes.

v1 -> v2

* Modify for backward compatibility with _Server-less ovsdb servers.

 python/ovs/db/idl.py| 219 
 python/ovs/reconnect.py |   3 +
 tests/ovsdb-idl.at  | 128 +---
 tests/test-ovsdb.py |  67 ++-
 4 files changed, 371 insertions(+), 46 deletions(-)

diff --git a/python/ovs/db/idl.py b/python/ovs/db/idl.py
index 250e897..84af978 100644
--- a/python/ovs/db/idl.py
+++ b/python/ovs/db/idl.py
@@ -38,6 +38,8 @@ ROW_DELETE = "delete"
 OVSDB_UPDATE = 0
 OVSDB_UPDATE2 = 1
 
+CLUSTERED = "clustered"
+
 
 class Idl(object):
 """Open vSwitch Database Interface Definition Language (OVSDB IDL).
@@ -92,10 +94,13 @@ class Idl(object):
 """
 
 IDL_S_INITIAL = 0
-IDL_S_MONITOR_REQUESTED = 1
-IDL_S_MONITOR_COND_REQUESTED = 2
+IDL_S_SERVER_SCHEMA_REQUESTED = 1
+IDL_S_SERVER_MONITOR_REQUESTED = 2
+IDL_S_DATA_MONITOR_REQUESTED = 3
+IDL_S_DATA_MONITOR_COND_REQUESTED = 4
 
-def __init__(self, remote, schema_helper, probe_interval=None):
+def __init__(self, remote, schema_helper, probe_interval=None,
+ leader_only=True):
 """Creates and returns a connection to the database named 'db_name' on
 'remote', which should be in a form acceptable to
 ovs.jsonrpc.session.open().  The connection will maintain an in-memory
@@ -119,6 +124,9 @@ class Idl(object):
 
 The IDL uses and modifies 'schema' directly.
 
+If 'leader_only' is set to True (default value) the IDL will only
+monitor and transact with the leader of the cluster.
+
 If "probe_interval" is zero it disables the connection keepalive
 feature. If non-zero the value will be forced to at least 1000
 milliseconds. If None it will just use the default value in OVS.
@@ -137,6 +145,20 @@ class Idl(object):
 self._last_seqno = None
 self.change_seqno = 0
 self.uuid = uuid.uuid1()
+
+# Server monitor.
+self._server_schema_request_id = None
+self._server_monitor_request_id = None
+self._db_change_aware_request_id = None
+self._server_db_name = '_Server'
+self._server_db_table = 'Database'
+self.server_tables = None
+self._server_db = None
+self.server_monitor_uuid = uuid.uuid1()
+self.leader_only = leader_only
+self.cluster_id = None
+self._min_index = 0
+
 self.state = self.IDL_S_INITIAL
 
 # Database locking.
@@ -172,6 +194,12 @@ class Idl(object):
 remotes.append(r)
 return remotes
 
+def set_cluster_id(self, cluster_id):
+"""Set the id of the cluster that this idl must connect to."""
+self.cluster_id = cluster_id
+if self.state != self.IDL_S_INITIAL:
+self.force_reconnect()
+
 def index_create(self, table, name):
 """Create a named multi-column index on a table"""
 return self.tables[table].rows.index_create(name)
@@ -222,7 +250,7 @@ class Idl(object):
 if seqno != self._last_seqno:
 self._last_seqno = seqno
 self.__txn_abort_all()
-self.__send_monitor_request()
+self.__send_server_schema_request()
 if self.lock_name:
 self.__send_lock_request()
 break
@@ -230,6 +258,7 @@ class Idl(object):
 msg = self._session.recv()
 if msg is None:
 break
+
 if (msg.type == ovs.jsonrpc.Message.T_NOTIFY