Re: [ovs-dev] [PATCH] ovn: Add LB flows for logical router with gateway port

[Numan Siddique]

On Fri, Oct 27, 2017 at 11:19 PM, Ben Pfaff  wrote:

> On Fri, Oct 27, 2017 at 10:03:07AM -0700, Guru Shetty wrote:
> > On 27 October 2017 at 08:35, Ben Pfaff  wrote:
> >
> > > This patch is one where I need help.  I asked Guru because it's an area
> > > he knows, but if you feel confident about it, Mark, then I'll merge it.
> > >
> >
> > No objections from me. If I find issues, I will let know even after the
> > patch is merged.
> >
> >
> > >
> > > Do you have a preference on ordering?
>
> OK, thanks Numan, Guru, Mark.  I applied this to master.  It is not
> obviously (to me) a bug fix, so I did not backport it.
>

Thanks Ben.
Yeah its not a bug fix. So I think it's fine not to backport.

Numan



> ___
> 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] [ovs-dpdk bug] Failing to release memory when QEMU exits?

[Sam]

add dpdk

2017-10-31 10:43 GMT+08:00 Sam :

> Hi all,
>
> I found in ovs-dpdk it is failing to release memory when QEMU exits. Is
> this a clear bug in ovs-dpdk? If it is, when did this bug fixed? Detail is
> bellow:
>
>
> For qemu-2.6.0 and ovs-dpdk, in huge page (1G) environment, after kill the
> qemu process, memory which is alloc for the vm could not be released.
>
> The start up command is:
>
> CMD1="$QEMU_CMD -D qemu.log -trace events=qemu-events-all -enable-kvm -cpu
> qemu64,+vmx,+ssse3,+sse4.1,+sse4.2,+x2apic,+aes,+avx,+vme,+p
> at,+ss,+pclmulqdq,+xsave,level=13 -machine pc,accel=kvm -chardev
> socket,id=hmqmondev,port=55908,host=127.0.0.1,nodelay,server,nowait -mon
> chardev=hmqmondev,id=hmqmon,mode=readline -rtc
> base=utc,clock=host,driftfix=none -usb -device usb-tablet -daemonize
> -nodefaults -nodefconfig -no-kvm-pit-reinjection -global
> kvm-pit.lost_tick_policy=discard -vga std -k en-us -smp 8 -name
> gangyewei-qemutime-1 -m 40960 -boot order=cdn -vnc :8,password -drive
> file=$DISK_0,if=none,id=drive_0,format=qcow2,cache=none,aio=native
> -device virtio-blk-pci,id=dev_drive_0,drive=drive_0,bus=pci.0,addr=0x5
> -drive file=$DISK_1,if=none,id=drive_1,format=qcow2,cache=none,aio=native
> -device virtio-blk-pci,id=dev_drive_1,drive=drive_1,bus=pci.0,addr=0x6
> -drive file=$DISK_2,if=none,id=drive_2,format=qcow2,cache=none,aio=native
> -device virtio-blk-pci,id=dev_drive_2,drive=drive_2,bus=pci.0,addr=0x7
> -device ide-cd,drive=ide0-cd0,bus=ide.1,unit=1 -drive
> id=ide0-cd0,media=cdrom,if=none -chardev socket,id=char-n-52b49b80,path
> =/usr/local/var/run/openvswitch/n-52b49b80,server -netdev
> type=vhost-user,id=n-52b49b80,chardev=char-n-52b49b80,vhostforce=on
> -device virtio-net-pci,netdev=n-52b49b80,mac=00:22:52:b4:9b:80,id=ne
> tdev-n-52b49b80,addr=0xf$(nic_speed 1) -object
> memory-backend-file,id=mem,size=40960M,mem-path=/mnt/huge,share=on -numa
> node,memdev=mem -pidfile $PID_FILE -chardev socket,path=/opt/cloud/workspa
> ce/servers/4511f52a-f450-40d3-9417-a1e0a27ed507/qga.sock,server,nowait,id=qga0
> -device virtio-serial -device virtserialport,chardev=qga0,na
> me=org.qemu.guest_agent.0"
>
> The stop script is just kill this process.
>
> the result of `cat /proc/meminfo` show memory is still there.
>
> After restart ovs-dpdk(which is openvswitch with dpdk lib), memory is
> released.
>
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [ovs-dpdk bug] Failing to release memory when QEMU exits?

[Sam]

Hi all,

I found in ovs-dpdk it is failing to release memory when QEMU exits. Is
this a clear bug in ovs-dpdk? If it is, when did this bug fixed? Detail is
bellow:


For qemu-2.6.0 and ovs-dpdk, in huge page (1G) environment, after kill the
qemu process, memory which is alloc for the vm could not be released.

The start up command is:

CMD1="$QEMU_CMD -D qemu.log -trace events=qemu-events-all -enable-kvm -cpu
qemu64,+vmx,+ssse3,+sse4.1,+sse4.2,+x2apic,+aes,+avx,+vme,+
pat,+ss,+pclmulqdq,+xsave,level=13 -machine pc,accel=kvm -chardev
socket,id=hmqmondev,port=55908,host=127.0.0.1,nodelay,server,nowait -mon
chardev=hmqmondev,id=hmqmon,mode=readline -rtc
base=utc,clock=host,driftfix=none -usb -device usb-tablet -daemonize
-nodefaults -nodefconfig -no-kvm-pit-reinjection -global
kvm-pit.lost_tick_policy=discard -vga std -k en-us -smp 8 -name
gangyewei-qemutime-1 -m 40960 -boot order=cdn -vnc :8,password -drive
file=$DISK_0,if=none,id=drive_0,format=qcow2,cache=none,aio=native -device
virtio-blk-pci,id=dev_drive_0,drive=drive_0,bus=pci.0,addr=0x5 -drive
file=$DISK_1,if=none,id=drive_1,format=qcow2,cache=none,aio=native -device
virtio-blk-pci,id=dev_drive_1,drive=drive_1,bus=pci.0,addr=0x6 -drive
file=$DISK_2,if=none,id=drive_2,format=qcow2,cache=none,aio=native -device
virtio-blk-pci,id=dev_drive_2,drive=drive_2,bus=pci.0,addr=0x7 -device
ide-cd,drive=ide0-cd0,bus=ide.1,unit=1 -drive id=ide0-cd0,media=cdrom,if=none
-chardev 
socket,id=char-n-52b49b80,path=/usr/local/var/run/openvswitch/n-52b49b80,server
-netdev type=vhost-user,id=n-52b49b80,chardev=char-n-52b49b80,vhostforce=on
-device virtio-net-pci,netdev=n-52b49b80,mac=00:22:52:b4:9b:80,id=
netdev-n-52b49b80,addr=0xf$(nic_speed 1) -object
memory-backend-file,id=mem,size=40960M,mem-path=/mnt/huge,share=on -numa
node,memdev=mem -pidfile $PID_FILE -chardev socket,path=/opt/cloud/workspa
ce/servers/4511f52a-f450-40d3-9417-a1e0a27ed507/qga.sock,server,nowait,id=qga0
-device virtio-serial -device virtserialport,chardev=qga0,na
me=org.qemu.guest_agent.0"

The stop script is just kill this process.

the result of `cat /proc/meminfo` show memory is still there.

After restart ovs-dpdk(which is openvswitch with dpdk lib), memory is
released.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2 1/3] Add multipath static router in OVN northd and north-db

[Gao Zhenyu]

Hi Ben,

   Thanks for the comments!
   The multipath feature can separete traffics to multi-gateways. If you
get two gateway routers and both of them pin on chassis. (gateway1 pin on
chassis-A, gateway2 pin on chassis-B) The multipath can seperate traffic to
those chassis automaticly.
   Otherwise, user should config many static routes to make it.

   The problem you mentioned about CMS part has not previously considered.


Thanks
Zhenyu Gao





2017-10-31 5:54 GMT+08:00 Ben Pfaff :

> Thank you for sending this series.  I feel bad that this series did not
> receive a timely review.  Let me help a little bit.
>
> I don't entirely understand the series.  This is mostly because of a
> lack of context, which the series could be revised to provide.
>
> One issue I have is that the term "multipath" is used for a few
> different purposes in networking.  Sometimes, multiple paths are used
> for reliability, and sometimes they are used to take advantage of
> bandwidth available along multiple paths.  I don't know which meaning is
> intended here.
>
> I also suffer a bit from not understanding the purpose to which these
> patches will be put at a higher level.  At the cloud management system
> (CMS) level, such as the level of Neutron or equivalent, how will the
> user use this feature?
>
> Once I understand the purpose a bit better, perhaps I can contribute to
> providing reviews, or help direct the patches to someone who can better
> review them.
>
> (The answers to these questions should probably not be just in email,
> but also added in appropriate places in the documentation for the
> patches themselves.)
>
> Thanks,
>
> Ben.
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] [PATCH] datapath: Remove redundant check for IFF_NO_QUEUE

[fukaige]

From: Kaige Fu 

IFF_NO_QUEUE is checked twice. This cause a situation
that HAVE_IFF_NO_QUEUE is defined no matter whether
the flag IFF_NO_QUEUE is defined in kernel or not.

Signed-off-by: Kaige Fu 
---
 datapath/linux/compat/include/linux/netdevice.h | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/datapath/linux/compat/include/linux/netdevice.h 
b/datapath/linux/compat/include/linux/netdevice.h
index a5d4ee8..3c3cf42 100644
--- a/datapath/linux/compat/include/linux/netdevice.h
+++ b/datapath/linux/compat/include/linux/netdevice.h
@@ -22,9 +22,6 @@ struct net;
 #define IFF_LIVE_ADDR_CHANGE 0
 #endif
 
-#ifndef IFF_NO_QUEUE
-#define IFF_NO_QUEUE   0
-#endif
 #ifndef IFF_OPENVSWITCH
 #define IFF_OPENVSWITCH 0
 #endif
-- 
1.8.3.1


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

Re: [ovs-dev] [PATCH] ovn: reduce logical flow applied to ovn-controller

[Ben Pfaff]

On Thu, Sep 21, 2017 at 07:45:23PM +0800, wang.qia...@zte.com.cn wrote:
> From 80260a2950f10544e307d6f20cb1cfe8c9bb885f Mon Sep 17 00:00:00 2001
> From: wang qianyu 
> Date: Thu, 21 Sep 2017 18:05:16 +0800
> Subject: [PATCH] ovn: reduce logical flow applied to ovn-controller
> 
> Add a logical_port column in Logical_Flow table. If logical flow generated 
> by
> logical_switch_port, set the logical_port of Logical_Flow with value of 
> port
> name. In ovn-controller, do not do expr_parse for non-local Logical_Flow.
> This can reduce the calculation and memory usage of ovn-controller.
> 
> Signed-off-by: wang qianyu 

Thank you for contributing to OVN!

This patch is wordwrapped and cannot be applied.  Can you send a version
that is not wordwrapped, or make this available as a Git branch?

Thanks,

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

Re: [ovs-dev] [PATCH v3] ovn: Support for taas(tap-as-a-service) function

[Ben Pfaff]

On Tue, Sep 19, 2017 at 04:38:04PM +0800, wang.qia...@zte.com.cn wrote:
> To support taas function, this patch add two type of logica_switch_port, 
> "mirror" and "taas". port with type "mirror" is used as inport for monitor 
> flow in logica_switch, and port with type "taas" is used as outport for 
> monitor flow in logica_switch.
> 
> The ovn-controller will make the relations of the ports in tap_service and 
> tap_flow to mirror port and taas port.
> 
> Signed-off-by: wang qianyu 

Thanks for contributing to OVN!

This patch is wordwrapped and cannot be applied.  Can you send it
without wordwrapping, or make it available as a Git branch?

It's somewhat difficult to understand the purpose here.  Some more
context would be helpful.  For example, you could supply high-level
documentation that explains the purpose of the feature.

This should add an item in NEWS mentioning the new feature.

Thanks,

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

Re: [ovs-dev] [PATCH] sparse: eliminate "duplicate initialization" warning

[Ben Pfaff]

On Tue, Aug 29, 2017 at 10:06:54AM -0700, Greg Rose wrote:
> On 08/28/2017 08:23 AM, Lance Richardson wrote:
> >Sparse version 0.5.1 will be released in the near future. Prepare
> >for it by eliminating the only new warning (as of 0.5.1-rc5):
> >
> >ofproto/fail-open.c:134:22: error: Initializer entry defined twice
> >ofproto/fail-open.c:135:22:   also defined here
> >
> >MATCH_CATCHALL_INITIALIZER effectively sets all fields of .flow to
> >zero, which is redundant because according to the C99 semantics for
> >structure initialization, initializing any single member of a
> >structure results in all other members being initialized to zero,
> >and the next line initializes a member of the same structure.
> >
> >Signed-off-by: Lance Richardson 
> >---
> >  ofproto/fail-open.c | 1 -
> >  1 file changed, 1 deletion(-)
> >
> >diff --git a/ofproto/fail-open.c b/ofproto/fail-open.c
> >index 914a51b4b..819bdadc3 100644
> >--- a/ofproto/fail-open.c
> >+++ b/ofproto/fail-open.c
> >@@ -131,7 +131,6 @@ send_bogus_packet_ins(struct fail_open *fo)
> >  .base = {
> >  .packet = dp_packet_data(),
> >  .packet_len = dp_packet_size(),
> >-.flow_metadata = MATCH_CATCHALL_INITIALIZER,
> >  .flow_metadata.flow.in_port.ofp_port = OFPP_LOCAL,
> >  .flow_metadata.wc.masks.in_port.ofp_port
> >  = u16_to_ofp(UINT16_MAX),
> >
> Reviewed-by: Greg Rose 

Thanks Lance and Greg, I applied this to master and branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] dpif: Remove duplicated word in comment for dpif_recv()

[Ben Pfaff]

On Tue, Aug 29, 2017 at 08:25:28AM -0700, Greg Rose wrote:
> On 08/25/2017 07:27 PM, fukaige wrote:
> >From: Kaige Fu 
> >
> >Signed-off-by: Kaige Fu 
> >---
> >  lib/dpif.c | 10 +-
> >  1 file changed, 5 insertions(+), 5 deletions(-)
> >
> >diff --git a/lib/dpif.c b/lib/dpif.c
> >index 79b2e6c..a4471d5 100644
> >--- a/lib/dpif.c
> >+++ b/lib/dpif.c
> >@@ -1547,11 +1547,11 @@ dpif_set_config(struct dpif *dpif, const struct smap 
> >*cfg)
> >  return error;
> >  }
> >
> >-/* Polls for an upcall from 'dpif' for an upcall handler.  Since there
> >- * there can be multiple poll loops, 'handler_id' is needed as index to
> >- * identify the corresponding poll loop.  If successful, stores the upcall
> >- * into '*upcall', using 'buf' for storage.  Should only be called if
> >- * 'recv_set' has been used to enable receiving packets from 'dpif'.
> >+/* Polls for an upcall from 'dpif' for an upcall handler.  Since there can
> >+ * be multiple poll loops, 'handler_id' is needed as index to identify the
> >+ * corresponding poll loop.  If successful, stores the upcall into 
> >'*upcall',
> >+ * using 'buf' for storage.  Should only be called if 'recv_set' has been 
> >used
> >+ * to enable receiving packets from 'dpif'.
> >   *
> >   * 'upcall->key' and 'upcall->userdata' point into data in the 
> > caller-provided
> >   * 'buf', so their memory cannot be freed separately from 'buf'.
> >
> LGTM
> 
> Reviewed-by: Greg Rose 

Thanks fukaige and Greg, I applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v4] route-table: Remove netdevs in netdev_hash when deleted

[Ben Pfaff]

On Thu, Aug 17, 2017 at 03:10:34PM -0700, Greg Rose wrote:
> On 08/14/2017 12:11 AM, fukaige wrote:
> >From: Kaige Fu 
> >
> >Start a virtual machine with its backend tap device attached to a brought up 
> >linux bridge.
> >If we delete the linux bridge when vm is still running, we'll get the 
> >following error when
> >trying to create a ovs bridge with the same name.
> >
> >The reason is that ovs-router subsystem add the linux bridge into 
> >netdev_shash, but does
> >not remove it when the bridge is deleted in the situation. When the bridge 
> >is deleted, ovs
> >will receive a RTM_DELLINK msg, take this chance to remove the bridge in 
> >netdev_shash.

Applied, thanks Greg and fukaige.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] controller: Remote connection option to OpenFlow switch.

[Ben Pfaff]

On Mon, Aug 07, 2017 at 08:48:07PM +0530, Jai Singh Rana wrote:
> Currently ovn-controller uses default unix domain socket in Open
> vSwitch's "run" directory to connect to OpenFlow switch. If
> ovn-controller/ovsdb-server and vswitchd are running on different
> systems, remote connection method needs to be provided.
> 
> Added configuration option to override default connection by using OVSDB
> external-ids "ovn-ofswitch-remote". Using this external-id, desired tcp
> or unix connection to OpenFlow switch can be specified.
> 
> Tested this by using tcp/unix method configured through external-id
> "ovn-ofswitch-remote" and confirmed connection as flows getting updated
> in Open vSwitch.
> 
> Signed-off-by: Jai Singh Rana 

Thanks for the revised patch.

With this change, ofctrl_run() and pinctrl_run() both use xstrdup() to
make a copy of the ovn_ofswitch_remote string, and later free it.  I
don't see any reason to make the copy.

Please try to maintain the general principle of parameter ordering in
Open vSwitch, where read-only parameters go before read/write and
write-only parameters.  In this case, ofctrl_run() and pinctrl_run()
only read from ovn_ofswitch_remote, so it should go before the
parameters that it writes.

It looks like get_ovn_ofswitch_remote() will log the default remote on
every single trip through ovn-controller's main loop.  I don't think
it's necessary to log it at all.

This new feature seems to be pretty specialized.  The documentation
should explain that it's not normally necessary to set the new setting,
that it is only for use in situations where the most common
configuration will not work.

Thanks,

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

Re: [ovs-dev] [PATCH] dpif-netdev: Optimize the exact match lookup.

[Ben Pfaff]

On Thu, Jul 27, 2017 at 11:38:00PM -0700, Tonghao Zhang wrote:
> When inserting or updating (e.g. emc_insert) a flow to EMC,
> we compare (e.g the hash and miniflow ) the netdev_flow_key.
> If the key is matched, we will update it. If we didn’t find
> the miniflow in the cache, the new flow will be stored.
> 
> But when looking up the flow, we compare the hash and miniflow
> of key and make sure it is alive. If a flow is not alive but
> the key is matched, we still will go to next loop. More important,
> we can’t find the flow in the next loop (the flow is not alive in
> the previous loop). This patch simply compares the miniflows of
> the packets.
> 
> The topo is shown as below. VM01 sends TCP packets to VM02, and
> OvS forwards packtets.
> 
>   VM01 -- OVS+DPDK VM02 -- VM03
> 
> With this patch, the TCP throughput between VMs is
> 5.37, 5.45, 5.48, 5.59, 5.65, 5.60 Gbs/sec avg: 5.52 Gbs/sec
> 
> up to:
> 5.64, 5.65, 5.66, 5.67, 5.62, 5.67 Gbs/sec avg: 5.65 Gbs/sec
> 
> (maybe ~2.3% performance improve, but it is hard to tell exactly
> due to variance in the test results).
> 
> Signed-off-by: Tonghao Zhang 

Thank you for the patch.  I haven't spotted any reviews for this on the
mailing list.  I apologize for that--usually I expect to see a review
much more quickly than this.  I hope that someone who understands the
dpif-netdev code well will provide a review soon.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v5 1/2] netdev-dpdk: extend netdev_dpdk_get_status to include if_type and if_descr

[Ben Pfaff]

On Mon, Oct 02, 2017 at 03:49:28PM +0100, Michal Weglicki wrote:
> This commit extends netdev_dpdk_get_status API to include additional
> driver-related information: if_type and if_descr.
> 
> v2->v3: Code rebase.
> v3->v4: Minor comments applied.
> 
> Co-authored-by: Michal Weglicki 
> Signed-off-by: Michal Weglicki 
> Signed-off-by: Przemyslaw Szczerbik 

Thank you for this series.  It's always great to have more IPFIX support
and more status information for an interface.

I'm happy with this series, except for one thing that I noticed in this
particular patch: there is no documentation.  The various status keys
supported by dpdk should be documented in vswitch.xml near the existing
status keys, which currently start around line 2781:

  
Key-value pairs that report port status.  Supported status values are
-dependent; some interfaces may not have a valid
, for example.
  

  
The name of the device driver controlling the network adapter.
  

  
The version string of the device driver controlling the network
adapter.
  

and so on.  Since there are several for dpdk, maybe they should be
grouped together in a 

I see that the existing dpdk status keys aren't documented either.  If
you can figure out what they are for, then it would be helpful to
document them too.

Thanks,

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

Re: [ovs-dev] [PATCH 1/4] netdev-dpdk: Fix mp_name leak on snprintf failure.

[Ben Pfaff]

On Mon, Oct 30, 2017 at 04:00:20PM -0700, Ben Pfaff wrote:
> On Mon, Oct 30, 2017 at 03:52:38PM +0300, Ilya Maximets wrote:
> > CC: Robert Wojciechowicz 
> > CC: Antonio Fischetti 
> > Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each 
> > port.")
> > Fixes: 65056fd79694 ("netdev-dpdk: manage failure in mempool name 
> > creation.")
> > Signed-off-by: Ilya Maximets 
> 
> Thanks, applied to master.  Let me know if this needs backporting.

This patch was obviously correct to me, but I don't feel qualified to
review the remainder of the series, so I will leave that to Ian.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/4] netdev-dpdk: Fix mp_name leak on snprintf failure.

[Ben Pfaff]

On Mon, Oct 30, 2017 at 03:52:38PM +0300, Ilya Maximets wrote:
> CC: Robert Wojciechowicz 
> CC: Antonio Fischetti 
> Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each 
> port.")
> Fixes: 65056fd79694 ("netdev-dpdk: manage failure in mempool name creation.")
> Signed-off-by: Ilya Maximets 

Thanks, applied to master.  Let me know if this needs backporting.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 01/11] ovs/dp-cls: fetching the mark id from hw

[Ben Pfaff]

Great, thanks for the info.

On Mon, Oct 30, 2017 at 09:57:49PM +, Darrell Ball wrote:
>  I believe this series is superceded by the HWOL series from Yuanhan Liu 
> y...@fridaylinux.org
> 
> 
> On 10/30/17, 2:22 PM, "ovs-dev-boun...@openvswitch.org on behalf of Ben 
> Pfaff"  wrote:
> 
> On Wed, Jul 05, 2017 at 12:27:08PM +, Shachar Beiser wrote:
> > The HW set the mark id that represents matching rule.
> > The hw-pipeline reads the mark id from fdir.hi
> > 
> > Signed-off-by: Shachar Beiser 
> 
> I'm currently going through old patches.  It looks like this series was
> never reviewed.  I apologize for that.  Should it get reviewed now, or
> do you want to re-post it, or do I misunderstand the status?
> 
> Thanks,
> 
> Ben.
> ___
> dev mailing list
> d...@openvswitch.org
> 
> https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=I4tZkysObRxv01uzMfLp7leJOlfBb7j1Jflz_NnP5og=kvk_xVzd9LISdss_SD7dokLllbBsjeXthg5S_YI28Io=
> 
> 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 01/11] ovs/dp-cls: fetching the mark id from hw

[Darrell Ball]

 I believe this series is superceded by the HWOL series from Yuanhan Liu 
y...@fridaylinux.org


On 10/30/17, 2:22 PM, "ovs-dev-boun...@openvswitch.org on behalf of Ben Pfaff" 
 wrote:

On Wed, Jul 05, 2017 at 12:27:08PM +, Shachar Beiser wrote:
> The HW set the mark id that represents matching rule.
> The hw-pipeline reads the mark id from fdir.hi
> 
> Signed-off-by: Shachar Beiser 

I'm currently going through old patches.  It looks like this series was
never reviewed.  I apologize for that.  Should it get reviewed now, or
do you want to re-post it, or do I misunderstand the status?

Thanks,

Ben.
___
dev mailing list
d...@openvswitch.org

https://urldefense.proofpoint.com/v2/url?u=https-3A__mail.openvswitch.org_mailman_listinfo_ovs-2Ddev=DwICAg=uilaK90D4TOVoH58JNXRgQ=BVhFA09CGX7JQ5Ih-uZnsw=I4tZkysObRxv01uzMfLp7leJOlfBb7j1Jflz_NnP5og=kvk_xVzd9LISdss_SD7dokLllbBsjeXthg5S_YI28Io=


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

Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to mbuf

[Ben Pfaff]

OK, great, thanks.

On Mon, Oct 30, 2017 at 09:53:01PM +, Darrell Ball wrote:
> This patch was merged to dpdk_merge on Sept 6 and later merged to ovs.
> 
> https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338413.html
> 
> 
> On 10/30/17, 2:38 PM, "ovs-dev-boun...@openvswitch.org on behalf of Ben 
> Pfaff"  wrote:
> 
> Hi Ian and Gao, it looks like this patch never got applied.  Gao, is it
> still relevant and, if so, Ian, will you add it to your next batch of
> patches?
> 
> Thanks,
> 
> Ben.
> 
> On Sun, Sep 03, 2017 at 10:46:24AM +, Stokes, Ian wrote:
> > OK,
> > 
> > In my own testing I’m seeing the same behavior with a tap interface 
> pinging to a bridge with a DPDK device. As such I think this should be ok.
> > 
> > Acked-by: ian.sto...@intel.com
> > 
> > Ian
> > 
> > From: Gao Zhenyu [mailto:sysugaozhe...@gmail.com]
> > Sent: Thursday, August 31, 2017 2:56 AM
> > To: Stokes, Ian 
> > Cc: d...@openvswitch.org
> > Subject: Re: [PATCH v2] netdev-dpdk: Execute QoS Checking before 
> copying to mbuf
> > 
> > Here is the the testing I just did:
> > 1. Add log in function netdev_dpdk_policer_pkt_handle
> > static inline bool
> > netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter,
> >struct rte_mbuf *pkt, uint64_t time)
> > {
> > uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct 
> ether_hdr);
> > VLOG_WARN(", payload_len:%u, rte_pkt_len:%u",
> >   pkt_len, rte_pktmbuf_pkt_len(pkt)); 
>  > 
> > return rte_meter_srtcm_color_blind_check(meter, time, pkt_len) ==
> > e_RTE_METER_GREEN;
> > }
> > 2. rebuild a rpm, install in machine, enable DPDK.
> > 3. Create a container on the bridge(userspace bridge). (The container 
> use veth device)
> > 4. Config qos on eth dpdk eth by using : ovs-vsctl set port 
> dpdk-uplink-eth3 qos=@newqos -- --id=@newqos create qos  type=egress-policer 
> other-config:cir=125 other-config:cbs=2048
> > 5. execute ping on the container and monitor ovs-vswitchd.log. Then I 
> can see:
> > 
> > 2017-08-31T01:39:28.036Z|00133|netdev_dpdk|WARN|, payload_len:88, 
> rte_pkt_len:102
> > 2017-08-31T01:39:29.036Z|00134|netdev_dpdk|WARN|, payload_len:88, 
> rte_pkt_len:102
> > 2017-08-31T01:39:30.036Z|00135|netdev_dpdk|WARN|, payload_len:88, 
> rte_pkt_len:102
> > 2017-08-31T01:39:31.036Z|00136|netdev_dpdk|WARN|, payload_len:88, 
> rte_pkt_len:102
> > So the mbuf->pkt_len has a correct length.
> > On the code side, we have function dp_packet_set_size(), if ovs 
> compiled with dpdk, this function set the mbuf.data_len and mbuf.pkt_len. 
> Many functions consume dp_packet_set_size() like netdev_linux_rxq_recv_sock 
> in Netdev-linux.c
> > 
> > #ifdef DPDK_NETDEV
> > ..
> > 
> > static inline void
> > dp_packet_set_size(struct dp_packet *b, uint32_t v)
> > {
> > .
> > b->mbuf.data_len = (uint16_t)v;  /* Current seg length. */
> > b->mbuf.pkt_len = v; /* Total length of all segments 
> linked to
> >   * this segment. */
> > }
> > Please let me know if you have any concern on it.
> > 
> > Thanks
> > Zhenyu Gao
> > 
> > 2017-08-30 23:18 GMT+08:00 Stokes, Ian 
> >:
> > > In dpdk_do_tx_copy function, all packets were copied to mbuf first, 
> but
> > > QoS checking may drop some of them.
> > > Move the QoS checking in front of copying data to mbuf, it helps to 
> reduce
> > > useless copy.
> > 
> > Hi Zhenyu, thanks for the patch, I agree with the objective of the 
> patch but have some comments below I'd like to see addressed/tested before 
> signing off.
> > 
> > >
> > > Signed-off-by: Zhenyu Gao 
> >
> > > ---
> > >  lib/netdev-dpdk.c | 55 
> --
> > > -
> > >  1 file changed, 36 insertions(+), 19 deletions(-)
> > >
> > > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 
> 1aaf6f7..50874b8
> > > 100644
> > > --- a/lib/netdev-dpdk.c
> > > +++ b/lib/netdev-dpdk.c
> > > @@ -279,7 +279,7 @@ struct dpdk_qos_ops {
> > >   * For all QoS implementations it should always be non-null.
> > >   */
> > >  int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,
> > > -   int pkt_cnt);
> > > +   int pkt_cnt, bool may_steal);
> > >  };
> > >
> > >  /* 

Re: [ovs-dev] [PATCH v2 1/3] Add multipath static router in OVN northd and north-db

[Ben Pfaff]

Thank you for sending this series.  I feel bad that this series did not
receive a timely review.  Let me help a little bit.

I don't entirely understand the series.  This is mostly because of a
lack of context, which the series could be revised to provide.

One issue I have is that the term "multipath" is used for a few
different purposes in networking.  Sometimes, multiple paths are used
for reliability, and sometimes they are used to take advantage of
bandwidth available along multiple paths.  I don't know which meaning is
intended here.

I also suffer a bit from not understanding the purpose to which these
patches will be put at a higher level.  At the cloud management system
(CMS) level, such as the level of Neutron or equivalent, how will the
user use this feature?

Once I understand the purpose a bit better, perhaps I can contribute to
providing reviews, or help direct the patches to someone who can better
review them.

(The answers to these questions should probably not be just in email,
but also added in appropriate places in the documentation for the
patches themselves.)

Thanks,

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

Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to mbuf

[Darrell Ball]

This patch was merged to dpdk_merge on Sept 6 and later merged to ovs.

https://mail.openvswitch.org/pipermail/ovs-dev/2017-September/338413.html


On 10/30/17, 2:38 PM, "ovs-dev-boun...@openvswitch.org on behalf of Ben Pfaff" 
 wrote:

Hi Ian and Gao, it looks like this patch never got applied.  Gao, is it
still relevant and, if so, Ian, will you add it to your next batch of
patches?

Thanks,

Ben.

On Sun, Sep 03, 2017 at 10:46:24AM +, Stokes, Ian wrote:
> OK,
> 
> In my own testing I’m seeing the same behavior with a tap interface 
pinging to a bridge with a DPDK device. As such I think this should be ok.
> 
> Acked-by: ian.sto...@intel.com
> 
> Ian
> 
> From: Gao Zhenyu [mailto:sysugaozhe...@gmail.com]
> Sent: Thursday, August 31, 2017 2:56 AM
> To: Stokes, Ian 
> Cc: d...@openvswitch.org
> Subject: Re: [PATCH v2] netdev-dpdk: Execute QoS Checking before copying 
to mbuf
> 
> Here is the the testing I just did:
> 1. Add log in function netdev_dpdk_policer_pkt_handle
> static inline bool
> netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter,
>struct rte_mbuf *pkt, uint64_t time)
> {
> uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct 
ether_hdr);
> VLOG_WARN(", payload_len:%u, rte_pkt_len:%u",
>   pkt_len, rte_pktmbuf_pkt_len(pkt)); 
 
> return rte_meter_srtcm_color_blind_check(meter, time, pkt_len) ==
> e_RTE_METER_GREEN;
> }
> 2. rebuild a rpm, install in machine, enable DPDK.
> 3. Create a container on the bridge(userspace bridge). (The container use 
veth device)
> 4. Config qos on eth dpdk eth by using : ovs-vsctl set port 
dpdk-uplink-eth3 qos=@newqos -- --id=@newqos create qos  type=egress-policer 
other-config:cir=125 other-config:cbs=2048
> 5. execute ping on the container and monitor ovs-vswitchd.log. Then I can 
see:
> 
> 2017-08-31T01:39:28.036Z|00133|netdev_dpdk|WARN|, payload_len:88, 
rte_pkt_len:102
> 2017-08-31T01:39:29.036Z|00134|netdev_dpdk|WARN|, payload_len:88, 
rte_pkt_len:102
> 2017-08-31T01:39:30.036Z|00135|netdev_dpdk|WARN|, payload_len:88, 
rte_pkt_len:102
> 2017-08-31T01:39:31.036Z|00136|netdev_dpdk|WARN|, payload_len:88, 
rte_pkt_len:102
> So the mbuf->pkt_len has a correct length.
> On the code side, we have function dp_packet_set_size(), if ovs compiled 
with dpdk, this function set the mbuf.data_len and mbuf.pkt_len. Many functions 
consume dp_packet_set_size() like netdev_linux_rxq_recv_sock in Netdev-linux.c
> 
> #ifdef DPDK_NETDEV
> ..
> 
> static inline void
> dp_packet_set_size(struct dp_packet *b, uint32_t v)
> {
> .
> b->mbuf.data_len = (uint16_t)v;  /* Current seg length. */
> b->mbuf.pkt_len = v; /* Total length of all segments 
linked to
>   * this segment. */
> }
> Please let me know if you have any concern on it.
> 
> Thanks
> Zhenyu Gao
> 
> 2017-08-30 23:18 GMT+08:00 Stokes, Ian 
>:
> > In dpdk_do_tx_copy function, all packets were copied to mbuf first, but
> > QoS checking may drop some of them.
> > Move the QoS checking in front of copying data to mbuf, it helps to 
reduce
> > useless copy.
> 
> Hi Zhenyu, thanks for the patch, I agree with the objective of the patch 
but have some comments below I'd like to see addressed/tested before signing 
off.
> 
> >
> > Signed-off-by: Zhenyu Gao 
>
> > ---
> >  lib/netdev-dpdk.c | 55 
--
> > -
> >  1 file changed, 36 insertions(+), 19 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 
1aaf6f7..50874b8
> > 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -279,7 +279,7 @@ struct dpdk_qos_ops {
> >   * For all QoS implementations it should always be non-null.
> >   */
> >  int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,
> > -   int pkt_cnt);
> > +   int pkt_cnt, bool may_steal);
> >  };
> >
> >  /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -
> > 1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm
> > *meter,
> >
> >  static int
> >  netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,
> > -struct rte_mbuf **pkts, int pkt_cnt)
> > +   

Re: [ovs-dev] [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to mbuf

[Ben Pfaff]

Hi Ian and Gao, it looks like this patch never got applied.  Gao, is it
still relevant and, if so, Ian, will you add it to your next batch of
patches?

Thanks,

Ben.

On Sun, Sep 03, 2017 at 10:46:24AM +, Stokes, Ian wrote:
> OK,
> 
> In my own testing I’m seeing the same behavior with a tap interface pinging 
> to a bridge with a DPDK device. As such I think this should be ok.
> 
> Acked-by: ian.sto...@intel.com
> 
> Ian
> 
> From: Gao Zhenyu [mailto:sysugaozhe...@gmail.com]
> Sent: Thursday, August 31, 2017 2:56 AM
> To: Stokes, Ian 
> Cc: d...@openvswitch.org
> Subject: Re: [PATCH v2] netdev-dpdk: Execute QoS Checking before copying to 
> mbuf
> 
> Here is the the testing I just did:
> 1. Add log in function netdev_dpdk_policer_pkt_handle
> static inline bool
> netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm *meter,
>struct rte_mbuf *pkt, uint64_t time)
> {
> uint32_t pkt_len = rte_pktmbuf_pkt_len(pkt) - sizeof(struct ether_hdr);
> VLOG_WARN(", payload_len:%u, rte_pkt_len:%u",
>   pkt_len, rte_pktmbuf_pkt_len(pkt)); 
>  
> return rte_meter_srtcm_color_blind_check(meter, time, pkt_len) ==
> e_RTE_METER_GREEN;
> }
> 2. rebuild a rpm, install in machine, enable DPDK.
> 3. Create a container on the bridge(userspace bridge). (The container use 
> veth device)
> 4. Config qos on eth dpdk eth by using : ovs-vsctl set port dpdk-uplink-eth3 
> qos=@newqos -- --id=@newqos create qos  type=egress-policer 
> other-config:cir=125 other-config:cbs=2048
> 5. execute ping on the container and monitor ovs-vswitchd.log. Then I can see:
> 
> 2017-08-31T01:39:28.036Z|00133|netdev_dpdk|WARN|, payload_len:88, 
> rte_pkt_len:102
> 2017-08-31T01:39:29.036Z|00134|netdev_dpdk|WARN|, payload_len:88, 
> rte_pkt_len:102
> 2017-08-31T01:39:30.036Z|00135|netdev_dpdk|WARN|, payload_len:88, 
> rte_pkt_len:102
> 2017-08-31T01:39:31.036Z|00136|netdev_dpdk|WARN|, payload_len:88, 
> rte_pkt_len:102
> So the mbuf->pkt_len has a correct length.
> On the code side, we have function dp_packet_set_size(), if ovs compiled with 
> dpdk, this function set the mbuf.data_len and mbuf.pkt_len. Many functions 
> consume dp_packet_set_size() like netdev_linux_rxq_recv_sock in Netdev-linux.c
> 
> #ifdef DPDK_NETDEV
> ..
> 
> static inline void
> dp_packet_set_size(struct dp_packet *b, uint32_t v)
> {
> .
> b->mbuf.data_len = (uint16_t)v;  /* Current seg length. */
> b->mbuf.pkt_len = v; /* Total length of all segments linked to
>   * this segment. */
> }
> Please let me know if you have any concern on it.
> 
> Thanks
> Zhenyu Gao
> 
> 2017-08-30 23:18 GMT+08:00 Stokes, Ian 
> >:
> > In dpdk_do_tx_copy function, all packets were copied to mbuf first, but
> > QoS checking may drop some of them.
> > Move the QoS checking in front of copying data to mbuf, it helps to reduce
> > useless copy.
> 
> Hi Zhenyu, thanks for the patch, I agree with the objective of the patch but 
> have some comments below I'd like to see addressed/tested before signing off.
> 
> >
> > Signed-off-by: Zhenyu Gao 
> > >
> > ---
> >  lib/netdev-dpdk.c | 55 --
> > -
> >  1 file changed, 36 insertions(+), 19 deletions(-)
> >
> > diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index 1aaf6f7..50874b8
> > 100644
> > --- a/lib/netdev-dpdk.c
> > +++ b/lib/netdev-dpdk.c
> > @@ -279,7 +279,7 @@ struct dpdk_qos_ops {
> >   * For all QoS implementations it should always be non-null.
> >   */
> >  int (*qos_run)(struct qos_conf *qos_conf, struct rte_mbuf **pkts,
> > -   int pkt_cnt);
> > +   int pkt_cnt, bool may_steal);
> >  };
> >
> >  /* dpdk_qos_ops for each type of user space QoS implementation */ @@ -
> > 1501,7 +1501,8 @@ netdev_dpdk_policer_pkt_handle(struct rte_meter_srtcm
> > *meter,
> >
> >  static int
> >  netdev_dpdk_policer_run(struct rte_meter_srtcm *meter,
> > -struct rte_mbuf **pkts, int pkt_cnt)
> > +struct rte_mbuf **pkts, int pkt_cnt,
> > +bool may_steal)
> >  {
> >  int i = 0;
> >  int cnt = 0;
> > @@ -1517,7 +1518,9 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm
> > *meter,
> >  }
> >  cnt++;
> >  } else {
> > -rte_pktmbuf_free(pkt);
> > +if (may_steal) {
> > +rte_pktmbuf_free(pkt);
> > +}
> >  }
> >  }
> >
> > @@ -1526,12 +1529,13 @@ netdev_dpdk_policer_run(struct rte_meter_srtcm
> > *meter,
> >
> >  static int
> >  ingress_policer_run(struct ingress_policer *policer, struct rte_mbuf
> > **pkts,
> > -

Re: [ovs-dev] [PATCH v2] tests: fix PTAP system test to check only OF stats

[Ben Pfaff]

On Wed, Jul 12, 2017 at 07:22:58AM +, Zoltán Balogh wrote:
> 
> It turned out, checking datapath flow statistics during system-userspace
> test is not reliable. Unwanted packets can be injected depending on
> system configuration. As a workaround, this commit removes checking
> statistics of datapath flows and does check OpenFlow statistics of the
> integrator bridges. Datapath flows can be checked in normal PTAP unit 
> tests by running 'make check'.
> 
> Reported-by: Darrell Ball 
> Suggested-by: Jan Scheurich 
> Tested-by: Darrell Ball 
> Signed-off-by: Zoltán Balogh 

It seems that this patch was never properly reviewed and applied, but it
no longer applies cleanly.  Is there any chance you'd be willing to
rebase and re-post it?

Thanks,

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

Re: [ovs-dev] [PATCH 01/11] ovs/dp-cls: fetching the mark id from hw

[Ben Pfaff]

On Wed, Jul 05, 2017 at 12:27:08PM +, Shachar Beiser wrote:
> The HW set the mark id that represents matching rule.
> The hw-pipeline reads the mark id from fdir.hi
> 
> Signed-off-by: Shachar Beiser 

I'm currently going through old patches.  It looks like this series was
never reviewed.  I apologize for that.  Should it get reviewed now, or
do you want to re-post it, or do I misunderstand the status?

Thanks,

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

[ovs-dev] Cobranza Altamente Eficaces

[Comunicación y negociación eficaz]

Las mejores técnicas para la recuperación de su cartera vencida

Habilidades de cobranza altamente eficaces
10 de noviembre - MCE. Abdón Guzmán Santana Zárate9am-6pm

El arte de la cobranza requiere de técnicas de negociación y habilidades para 
la interacción y el trato humano. El éxito de esta actividad, depende en gran 
parte de las habilidades y destrezas alcanzadas en el campo de la negociación. 
Un ejecutivo de cobranza exitoso, debe saber llevar un diálogo con el deudor y 
atraer el pago, a través del uso de palabras adecuadas sin llegar a las 
amenazas, buscando un acuerdo entre la empresa y los deudores.

BENEFICIOS DE ASISTIR: 

- Aprenderá a manejar excusas, mentiras y quejas de los deudores, así como a 
clientes difíciles. - Conocerá cuáles son los límites que marca la ley en 
relación a la cobranza. - Sabrá como tomar el control de la llamada telefónica 
con deudores que intentan desviar la conversación. - Identificará cuándo y cómo 
es posible llevar un proceso judicial con su cartera vencida. 

¿Requiere la información a la Brevedad? responda este email con la palabra: 
Cobranza + nombre - teléfono - correo.


centro telefónico:018002120744


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

Re: [ovs-dev] [PATCH] rhel: Use python2-sphinx as BuildRequires on Fedora

[Ben Pfaff]

On Fri, Oct 27, 2017 at 02:18:43PM +0200, Timothy Redaelli wrote:
> python-* package names are deprecated on Fedora
> (https://fedoraproject.org/wiki/Packaging:Naming#Python2_binary_package_naming)
> so use python2-sphinx, when available, instead.
> 
> CC: Lance Richardson 
> Fixes: cd6121410b52 ("rhel: add python-sphinx as a build dependency")
> Signed-off-by: Timothy Redaelli 

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

Re: [ovs-dev] [PATCH] fedora: Use python2-sphinx as dependency instead of python-sphinx

[Ben Pfaff]

On Fri, Oct 27, 2017 at 02:03:15PM +0200, Timothy Redaelli wrote:
> python-* package names are deprecated
> (https://fedoraproject.org/wiki/Packaging:Naming#Python2_binary_package_naming)
> so use python2-sphinx instead.
> 
> CC: Gurucharan Shetty 
> Fixes: fc9669525f3f ("rhel, fedora: Add python-sphinx as a dependency.")
> Signed-off-by: Timothy Redaelli 

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

Re: [ovs-dev] [PATCH] Add configuration option to link ovs binaries statically

[Ben Pfaff]

On Tue, Jul 11, 2017 at 11:22:26AM -0700, Ben Pfaff wrote:
> On Tue, Jun 20, 2017 at 05:43:56PM +0200, Timothy Redaelli wrote:
> > Add --enable-static-binaries configuration option for enabling or disabling
> > static linking with libopenvswitch, libsflow, libovsdb, libvtep, libovn and
> > libofproto also when --enable-shared is specified.
> > 
> > This is needed to avoid link binaries with position-independent code (PIC)
> > that generates slower code, but to permit building the ovs shared librares,
> > since it's needed, for example, by the C extension wrapper for Python JSON
> > parsing.
> > 
> > --enable-static-binaries option can only be used when both --enable-static
> > (default) and --enable-shared options are specified since if only
> > --enable-static is specified the binaries are already statically linked and 
> > if
> > only --enable-shared is specified the binaries cannot link with nonbuilt 
> > static
> > libraries.
> > 
> > Signed-off-by: Timothy Redaelli 
> 
> This strikes me as an unusual configuration option.  Is there precedent
> for this in other projects?  I've always taken the cost of dynamic
> linking as a given when a project generates shared libraries.

I don't know whether this patch is still important to you.  If it is,
then you should follow up with more information.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH V5 1/1] ovs-vtep: vtep-ctl and ovs-vtep support of adding explicit tunnel key

[Ben Pfaff]

Justin, I don't think there's been a review of this yet.  It's been
almost 8 months.  Are you planning to take a look?

On Wed, Mar 08, 2017 at 03:58:28PM -0800, Ben Pfaff wrote:
> Justin, you've reviewed previous versions of this patch.  Will you look
> at this one?
> 
> On Sun, Feb 19, 2017 at 01:45:15PM +0200, itamaro wrote:
> > From: itamarofek 
> > 
> > This patch adds support for handeling a per-tunnel tunnel key in the
> > ovs-vtep and vtep-ctl.
> > 
> > The aim of this patch is to support the usage of hardware vtep switch as
> > an inter-cloud gateway, which is used to connect two separated l2 broadcast
> > domains
> > 
> > Requested-by: "Ofer Ben-Yacov" 
> > Signed-off-by: "Itamar Ofek" 
> > Co-authored-by: "Darrell Ball" 
> > ---
> >  tests/vtep-ctl.at   | 217 
> >  vtep/ovs-vtep   | 122 -
> >  vtep/vtep-ctl.8.in  |  25 --
> >  vtep/vtep-ctl.c | 252 
> > 
> >  vtep/vtep.ovsschema |   9 +-
> >  vtep/vtep.xml   |  13 +++
> >  6 files changed, 488 insertions(+), 150 deletions(-)
> > 
> > diff --git a/tests/vtep-ctl.at b/tests/vtep-ctl.at
> > index 2b0df67..4290025 100644
> > --- a/tests/vtep-ctl.at
> > +++ b/tests/vtep-ctl.at
> > @@ -433,12 +433,20 @@ AT_CHECK([RUN_VTEP_CTL(
> >  CHECK_LSWITCHES([ls1])
> >  AT_CHECK([RUN_VTEP_CTL(
> > [add-ucast-local ls1 00:11:22:33:44:55 10.0.0.10],
> > -   [add-ucast-local ls1 00:11:22:33:44:66 vxlan_over_ipv4 10.0.0.11])
> > +   [add-ucast-local ls1 00:11:22:33:44:66 vxlan_over_ipv4 10.0.0.11],
> > +   [add-ucast-local ls1 00:11:22:33:55:66 10.0.0.12 100],
> > +   [add-ucast-local ls1 00:11:22:33:55:77 vxlan_over_ipv4 10.0.0.13 200],
> > +   [add-ucast-local ls1 00:11:22:33:55:88 10.0.0.14 300 level0],
> > +   [add-ucast-local ls1 00:11:22:33:55:99 vxlan_over_ipv4 10.0.0.15 400 
> > level1])
> >  ], [0], [], [], [VTEP_CTL_CLEANUP])
> >  AT_CHECK([RUN_VTEP_CTL([list-local-macs ls1])], [0],
> > [ucast-mac-local
> >00:11:22:33:44:55 -> vxlan_over_ipv4/10.0.0.10
> >00:11:22:33:44:66 -> vxlan_over_ipv4/10.0.0.11
> > +  00:11:22:33:55:66 -> vxlan_over_ipv4/10.0.0.12 100
> > +  00:11:22:33:55:77 -> vxlan_over_ipv4/10.0.0.13 200
> > +  00:11:22:33:55:88 -> vxlan_over_ipv4/10.0.0.14 300 level0
> > +  00:11:22:33:55:99 -> vxlan_over_ipv4/10.0.0.15 400 level1
> >  
> >  mcast-mac-local
> >  
> > @@ -460,11 +468,26 @@ AT_CHECK([RUN_VTEP_CTL(
> >  CHECK_LSWITCHES([ls1])
> >  AT_CHECK([RUN_VTEP_CTL(
> > [add-ucast-local ls1 00:11:22:33:44:55 10.0.0.10],
> > -   [add-ucast-local ls1 00:11:22:33:44:55 10.0.0.11])
> > +   [add-ucast-local ls1 00:11:22:33:44:55 10.0.0.11],
> > +   [add-ucast-local ls1 00:11:22:33:55:66 10.0.0.12 100],
> > +   [add-ucast-local ls1 00:11:22:33:55:66 10.0.0.13 200],
> > +   [add-ucast-local ls1 00:11:22:33:55:77 10.0.0.14 300],
> > +   [add-ucast-local ls1 00:11:22:33:55:77 10.0.0.15],
> > +   [add-ucast-local ls1 00:11:22:33:55:88 10.0.0.16],
> > +   [add-ucast-local ls1 00:11:22:33:55:88 10.0.0.17 400],
> > +   [add-ucast-local ls1 00:11:22:33:55:99 10.0.0.18 500 level0],
> > +   [add-ucast-local ls1 00:11:22:33:55:99 10.0.0.19 600 level1],
> > +   [add-ucast-local ls1 00:11:22:33:56:11 10.0.0.20 700 level1],
> > +   [add-ucast-local ls1 00:11:22:33:56:11 10.0.0.21])
> >  ], [0], [], [], [VTEP_CTL_CLEANUP])
> >  AT_CHECK([RUN_VTEP_CTL([list-local-macs ls1])], [0],
> > [ucast-mac-local
> >00:11:22:33:44:55 -> vxlan_over_ipv4/10.0.0.11
> > +  00:11:22:33:55:66 -> vxlan_over_ipv4/10.0.0.13 200
> > +  00:11:22:33:55:77 -> vxlan_over_ipv4/10.0.0.15
> > +  00:11:22:33:55:88 -> vxlan_over_ipv4/10.0.0.17 400
> > +  00:11:22:33:55:99 -> vxlan_over_ipv4/10.0.0.19 600 level1
> > +  00:11:22:33:56:11 -> vxlan_over_ipv4/10.0.0.21
> >  
> >  mcast-mac-local
> >  
> > @@ -480,22 +503,34 @@ AT_CHECK([RUN_VTEP_CTL(
> >  CHECK_LSWITCHES([ls1])
> >  AT_CHECK([RUN_VTEP_CTL(
> > [add-ucast-local ls1 00:11:22:33:44:55 10.0.0.10],
> > -   [add-ucast-local ls1 00:11:22:33:44:66 vxlan_over_ipv4 10.0.0.11])
> > +   [add-ucast-local ls1 00:11:22:33:44:66 vxlan_over_ipv4 10.0.0.11],
> > +   [add-ucast-local ls1 00:11:22:33:55:66 10.0.0.12 100],
> > +   [add-ucast-local ls1 00:11:22:33:55:77 vxlan_over_ipv4 10.0.0.13 200],
> > +   [add-ucast-local ls1 00:11:22:33:55:88 10.0.0.14 300 level0],
> > +   [add-ucast-local ls1 00:11:22:33:55:99 vxlan_over_ipv4 10.0.0.15 400 
> > level1])
> >  ], [0], [], [], [VTEP_CTL_CLEANUP])
> >  AT_CHECK([RUN_VTEP_CTL([list-local-macs ls1])], [0],
> > [ucast-mac-local
> >00:11:22:33:44:55 -> vxlan_over_ipv4/10.0.0.10
> >00:11:22:33:44:66 -> vxlan_over_ipv4/10.0.0.11
> > +  00:11:22:33:55:66 -> vxlan_over_ipv4/10.0.0.12 100
> > +  00:11:22:33:55:77 -> vxlan_over_ipv4/10.0.0.13 200
> > +  00:11:22:33:55:88 -> vxlan_over_ipv4/10.0.0.14 300 level0
> > +  00:11:22:33:55:99 -> 

Re: [ovs-dev] [PATCH] ovn-controller: support configurable acl log file rate limit

[Ben Pfaff]

On Tue, Aug 29, 2017 at 03:27:53PM -0700, Han Zhou wrote:
> Add parameters in local Open_vSwitch DB external-ids for rate-
> limiting the log file writing:
> ovn-acl-log-rl-rate
> ovn-acl-log-rl-burst
> 
> Note: this has nothing to do with packet-in rate-limiting.
> Signed-off-by: Han Zhou 

Thanks for working on this.

Justin is probably the right person to review this, but here are a few
preliminary comments.

I'd prefer to see a new vlog_rate_limit_set() function, which could do
the actual work of changing the rate limit.  This is partly just to
improve the abstraction, but also partly because vlog_rate_limit
contains a mutex that should be taken whenever its data is accessed.  I
don't think that thread safety is a problem in this particular case, but
I don't know of a downside to being careful here.

The commit message doesn't explain the change to tests/automake.mk.

In acl-log.h, we normally write prototypes without a line break.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/4] util: Fix potential leak of memory.

[William Tu]

On Mon, Oct 30, 2017 at 12:49 PM, Ben Pfaff  wrote:
> On Mon, Oct 30, 2017 at 10:27:55AM -0700, William Tu wrote:
>> Clang reports potiential leak of memory pointed to by 'pname'.
>> We already free the previous subporgram name when setting the
>> new subprogram name, and once the running thread exits, the
>> name of the process will be free by the system.  Thus, we don't
>> need to explicitly free it, so exclude the clang analyzer as a
>> workaround.
>>
>> Signed-off-by: William Tu 
>
> Thanks for reporting this.
>
> I don't think it's worthwhile adding an #if to satisfy clang-analyzer
> here.  I don't think it should be our goal to make clang-analyzer
> produce no output; it does not have enough context to understand
> everything that OVS does.

Hi Ben,
Thanks for the review. Yes, I agree clang-analyzer in many cases
doesn't have enough context. I will leave these warnings there.
William
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 4/4] util: Fix potential leak of memory.

[Ben Pfaff]

On Mon, Oct 30, 2017 at 10:27:55AM -0700, William Tu wrote:
> Clang reports potiential leak of memory pointed to by 'pname'.
> We already free the previous subporgram name when setting the
> new subprogram name, and once the running thread exits, the
> name of the process will be free by the system.  Thus, we don't
> need to explicitly free it, so exclude the clang analyzer as a
> workaround.
> 
> Signed-off-by: William Tu 

Thanks for reporting this.

I don't think it's worthwhile adding an #if to satisfy clang-analyzer
here.  I don't think it should be our goal to make clang-analyzer
produce no output; it does not have enough context to understand
everything that OVS does.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 3/4] ovn-sbctl: Fix dead assignment.

[Ben Pfaff]

On Mon, Oct 30, 2017 at 10:27:54AM -0700, William Tu wrote:
> Clang reports value stored to 'sb' is never read.
> Fix it by removing it.
> 
> Signed-off-by: William Tu 

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

Re: [ovs-dev] [PATCH 2/4] ofp-actions: Fix dead assignment.

[Ben Pfaff]

On Mon, Oct 30, 2017 at 10:27:53AM -0700, William Tu wrote:
> Clang reports value stored to 'eah' in never read.
> Fix it by removing it.
> 
> Signed-off-by: William Tu 

Thanks a lot!  Applied to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 1/4] ofproto-dpif-xlate: Fix bad memory free.

[Ben Pfaff]

On Mon, Oct 30, 2017 at 10:27:52AM -0700, William Tu wrote:
> Clang reports possibly bad free of 'ofm' when it comes from the stack
> instead of malloc because Clang is not able to verify whether the previous
> if condition 'ctx->xin->xcache' still hold the same.  Fix it by
> adding additional condition.
> 
> Signed-off-by: William Tu 
> ---
>  ofproto/ofproto-dpif-xlate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
> index ddcaf059ded2..b8a4986061a9 100644
> --- a/ofproto/ofproto-dpif-xlate.c
> +++ b/ofproto/ofproto-dpif-xlate.c
> @@ -5128,7 +5128,7 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct 
> ofpact_learn *learn)
>  }
>  }
>  
> -if (ctx->xin->xcache) {
> +if (ctx->xin->xcache && ofm != __) {
>  free(ofm);
>  }

Hmm, this is interesting.  Can we just change this to:
if (ofm != __)
though?  (free(NULL) is harmless.)

Thanks,

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

Re: [ovs-dev] [PATCH 09/11] tnl-neigh-cache: Fix possible null pointer.

[Ben Pfaff]

On Sat, Oct 28, 2017 at 10:31:56AM -0700, William Tu wrote:
> Clang reports possible null pointer '>masks.ipv6_src' to memset.
> Workaround it by adding extra pointer check.
> 
> Signed-off-by: William Tu 
> ---
>  lib/tnl-neigh-cache.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/lib/tnl-neigh-cache.c b/lib/tnl-neigh-cache.c
> index a28ce1de8855..b3024848b8d5 100644
> --- a/lib/tnl-neigh-cache.c
> +++ b/lib/tnl-neigh-cache.c
> @@ -178,6 +178,9 @@ tnl_nd_snoop(const struct flow *flow, struct 
> flow_wildcards *wc,
>  return EINVAL;
>  }
>  
> +if (OVS_UNLIKELY(!wc))
> +return EINVAL;
> +
>  memset(>masks.ipv6_src, 0xff, sizeof wc->masks.ipv6_src);
>  memset(>masks.ipv6_dst, 0xff, sizeof wc->masks.ipv6_dst);
>  memset(>masks.nd_target, 0xff, sizeof wc->masks.nd_target);

Thanks for taking a look at this.

In practice, I believe that there is no bug because this function is
currently always called with nonnull 'wc'.  If it were to be called with
a null 'wc', however, we'd still want the function to do its work
instead of aborting.  So I think that the correct change would be to
surround the memsets with "if (wc) { ... }";

(In addition, OVS coding style calls for {} around conditional
statements.)

Thanks,

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

Re: [ovs-dev] [PATCH 08/11] ovn-sbctl: Fix possible null pointer to qsort.

[Ben Pfaff]

I'd recommend just something like "if (n_flows) { qsort(...); }".

On Mon, Oct 30, 2017 at 12:05:55PM -0700, William Tu wrote:
> Hi Mark,
> Thanks for the review. I will fix it and resubmit v2.
> William
> 
> On Mon, Oct 30, 2017 at 8:12 AM, Mark Michelson  wrote:
> > This is not the proper solution. If you start ovn-northd and do not add any
> > logical switches or routers, then there will be no logical flows created.
> > Currently, if you run `ovs-sbctl lflow-list` in this circumstance, it lists
> > nothing and exits cleanly. With this change, the command hits the assertion
> > and crashes.
> > it's not an erroneous case to have no logical flows to sort. I think the
> > proper thing to do in this case is to check the value of n_flows after the
> > loop that populates lflows. If it is 0, then close the vconn and exit the
> > function early.
> >
> > On Sat, Oct 28, 2017 at 12:37 PM William Tu  wrote:
> >>
> >> Clang reports possible null pointer 'lflows' passed to qsort.
> >> This is due to the checker unable to make sure whether 'lflows'
> >> gets malloc or not in the previous loop.  Thus, fix it by adding
> >> ovs_assert to pass clang checker.
> >>
> >> Signed-off-by: William Tu 
> >> ---
> >>  ovn/utilities/ovn-sbctl.c | 1 +
> >>  1 file changed, 1 insertion(+)
> >>
> >> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
> >> index 7af5863b08fc..b1f106002842 100644
> >> --- a/ovn/utilities/ovn-sbctl.c
> >> +++ b/ovn/utilities/ovn-sbctl.c
> >> @@ -860,6 +860,7 @@ cmd_lflow_list(struct ctl_context *ctx)
> >>  lflows[n_flows] = lflow;
> >>  n_flows++;
> >>  }
> >> +ovs_assert(lflows);
> >>  qsort(lflows, n_flows, sizeof *lflows, lflow_cmp);
> >>
> >>  bool print_uuid = shash_find(>options, "--uuid") != NULL;
> >> --
> >> 2.7.4
> >>
> >> ___
> >> dev mailing list
> >> d...@openvswitch.org
> >> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 07/11] packets: Fix possible null pointer to memcmp.

[Ben Pfaff]

On Sat, Oct 28, 2017 at 10:31:54AM -0700, William Tu wrote:
> Clang reports possible null pointer passes to dst of memcmp, >ip6_src.
> This is due to *nh comes from dp_packet_l3, and dp_packet_l3 might
> return NULL.  Fix it by adding ovs_assert check.
> 
> Signed-off-by: William Tu 
> ---
>  lib/packets.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/packets.c b/lib/packets.c
> index 230a62553ff6..7b147bbfb880 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -1191,6 +1191,7 @@ packet_set_ipv6(struct dp_packet *packet, const struct 
> in6_addr *src,
>  
>  rh_present = packet_rh_present(packet, );
>  
> +ovs_assert(>ip6_src);
>  if (memcmp(>ip6_src, src, sizeof(ovs_be32[4]))) {
>  packet_set_ipv6_addr(packet, proto, nh->ip6_src.be32, src, true);
>  }

In this case, Clang doesn't have enough context to understand the
situation.  packet_set_ipv6() is only called if the packet is already an
ipv6 packet, so the assertion is not helpful.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 06/11] packets: Fix possible null pointer argument to memmove.

[Ben Pfaff]

On Sat, Oct 28, 2017 at 10:31:53AM -0700, William Tu wrote:
> Clang reports possible null pointer to memmove due to
> dp_packet_data might retun null.  Fix it by adding
> ovs_assert check.
> 
> Signed-off-by: William Tu 
> ---
>  lib/packets.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/lib/packets.c b/lib/packets.c
> index 74d87eda89e1..230a62553ff6 100644
> --- a/lib/packets.c
> +++ b/lib/packets.c
> @@ -397,6 +397,7 @@ pop_mpls(struct dp_packet *packet, ovs_be16 ethtype)
>  dp_packet_set_l2_5(packet, NULL);
>  }
>  /* Shift the l2 header forward. */
> +ovs_assert(dp_packet_data(packet));
>  memmove((char*)dp_packet_data(packet) + MPLS_HLEN, 
> dp_packet_data(packet), len);
>  dp_packet_resize_l2_5(packet, -MPLS_HLEN);
>  }

Clang is just wrong here.  The packet has to be nonnull because it is an
MPLS packet.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 03/11] ovs-appctl: Fix possible null pointer argument.

[Ben Pfaff]

On Sat, Oct 28, 2017 at 10:31:50AM -0700, William Tu wrote:
> Clang reports possible optarg as null pointer passed to atoi.
> Fix it by adding ovs_assert check before.
> 
> Signed-off-by: William Tu 
> ---
>  utilities/ovs-appctl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index 8f87cc4f6c6e..cc1326bc9478 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -165,6 +165,7 @@ parse_command_line(int argc, char *argv[])
>  exit(EXIT_SUCCESS);
>  
>  case 'T':
> +ovs_assert(optarg);
>  time_alarm(atoi(optarg));
>  break;

This is an example of "converting SIGSEGV to SIGABRT" as I mentioned
earlier.  I don't think it is valuable; both the computer and the human
reader can understand that atoi(optarg) implies that optarg is nonnull.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 01/11] dp-packet: fix possible null pointer argument

[Ben Pfaff]

On Sat, Oct 28, 2017 at 10:31:48AM -0700, William Tu wrote:
> Clang reports possible null pointer argument to the memcpy src.
> This is due to at dp_packet_clone_data_with_headroom, the
> dp_packet *b might have a NULL base due to allocating a dp_packet
> with size = 0.  Fix it by adding ovs_assert to satisfy clang.
> 
> Signed-off-by: William Tu 

Thanks for working on this.

If 'old_base' might really be null, because of some path into the
function, then the most likely issue here is a very minor technical one:
the C specification requires that the pointer arguments to memcpy() be
nonnull even if the number of bytes to be copied is 0.  Most actual
implementations of memcpy() do nothing in that case, which is the
expected result.  In this case, in the situation where 'old_base' is
null, I guess that the number of bytes to be copied should be 0.  That
means that, if this ever actually occurs, this patch will cause
dp_packet_copy__() to assert-fail instead of doing nothing.  That is not
an improvement.

The right solution is probably to use nullable_memcpy() instead of
memcpy().

Thanks,

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

Re: [ovs-dev] [PATCH v2] ovsdb-idl: fix index row setting with references.

[Han Zhou]

On Mon, Oct 30, 2017 at 11:27 AM, Ben Pfaff  wrote:
>
> On Fri, Oct 27, 2017 at 05:40:56PM -0700, Han Zhou wrote:
> > IDL index should be able to be used without having to be in a
> > transaction. However, current implementation leads to crash if
> > a reference type column is being set in an index row for querying
> > purpose when it is not in a transaction. It is because of the
> > uninitialized arcs and unnecessary updates of the arcs. This patch
> > fixes it by identifying index rows by a magic uuid, so that when
> > parsing index row, the arcs are not updated. A new test case is
> > added to cover this scenario.
> >
> > Signed-off-by: Han Zhou 
> > ---
> >
> > Notes:
> > v1 -> v2: use magic UUID to identify index rows to avoid introducing
> >   a parameter everywhere.
>
> Thanks for the revision!
>
> I still see a spelling error "udpate" in the commit.  Please fix it.
>
> Instead of using memcpy() to copy a UUID, please use struct assignment
> (it is probably not necessary to have a function to do this).
>
> Our usual practice is to write:
>   static void
>   set_index_row(struct ovsdb_idl_row *row)
> instead of:
>   static void set_index_row(struct ovsdb_idl_row *row)
>
> Thanks,
>
> Ben.

Sorry for missing the typo. I submitted v3.

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

[ovs-dev] [PATCH v3] ovsdb-idl: fix index row setting with references.

[Han Zhou]

IDL index should be able to be used without having to be in a
transaction. However, current implementation leads to crash if
a reference type column is being set in an index row for querying
purpose when it is not in a transaction. It is because of the
uninitialized arcs and unnecessary updates of the arcs. This patch
fixes it by identifying index rows by a magic uuid, so that when
parsing index row, the arcs are not updated. A new test case is
added to cover this scenario.

Signed-off-by: Han Zhou 
---

Notes:
v2 -> v3:
- fix typos and code style
- change memcpy to struct assignment, and remove the function

 lib/ovsdb-idl.c | 32 ---
 tests/idltest.ovsschema | 18 +++
 tests/ovsdb-idl.at  | 26 
 tests/test-ovsdb.c  | 83 +
 4 files changed, 149 insertions(+), 10 deletions(-)

diff --git a/lib/ovsdb-idl.c b/lib/ovsdb-idl.c
index af1821b..5617e08 100644
--- a/lib/ovsdb-idl.c
+++ b/lib/ovsdb-idl.c
@@ -2298,6 +2298,20 @@ ovsdb_idl_index_write_(struct ovsdb_idl_row *const_row,
 (column->parse)(row, >new_datum[column_idx]);
 }
 
+/* Magic UUID for index rows */
+static const struct uuid index_row_uuid = {
+.parts = {0xdeadbeef,
+  0xdeadbeef,
+  0xdeadbeef,
+  0xdeadbeef}};
+
+/* Check if a row is an index row */
+static bool
+is_index_row(struct ovsdb_idl_row *row)
+{
+return uuid_equals(>uuid, _row_uuid);
+}
+
 /* Initializes a row for use in an indexed query.
  * Not intended for direct usage.
  */
@@ -2307,9 +2321,13 @@ ovsdb_idl_index_init_row(struct ovsdb_idl * idl,
 {
 struct ovsdb_idl_row *row = xzalloc(class->allocation_size);
 class->row_init(row);
+row->uuid = index_row_uuid;
 row->new_datum = xmalloc(class->n_columns * sizeof *row->new_datum);
 row->written = bitmap_allocate(class->n_columns);
 row->table = ovsdb_idl_table_from_class(idl, class);
+/* arcs are not used for index row, but it doesn't harm to initialize */
+ovs_list_init(>src_arcs);
+ovs_list_init(>dst_arcs);
 return row;
 }
 
@@ -2325,6 +2343,8 @@ ovsdb_idl_index_destroy_row__(const struct ovsdb_idl_row 
*row_)
 const struct ovsdb_idl_column *c;
 size_t i;
 
+ovs_assert(ovs_list_is_empty(_->src_arcs));
+ovs_assert(ovs_list_is_empty(_->dst_arcs));
 BITMAP_FOR_EACH_1 (i, class->n_columns, row->written) {
 c = >columns[i];
 (c->unparse) (row);
@@ -2726,13 +2746,17 @@ ovsdb_idl_get_row_arc(struct ovsdb_idl_row *src,
 
 dst_table = ovsdb_idl_table_from_class(idl, dst_table_class);
 dst = ovsdb_idl_get_row(dst_table, dst_uuid);
-if (idl->txn) {
-/* We're being called from ovsdb_idl_txn_write().  We must not update
+if (idl->txn || is_index_row(src)) {
+/* There are two cases we should not update any arcs:
+ *
+ * 1. We're being called from ovsdb_idl_txn_write(). We must not update
  * any arcs, because the transaction will be backed out at commit or
  * abort time and we don't want our graph screwed up.
  *
- * Just return the destination row, if there is one and it has not been
- * deleted. */
+ * 2. The row is used as an index for querying purpose only.
+ *
+ * In these cases, just return the destination row, if there is one and
+ * it has not been deleted. */
 if (dst && (hmap_node_is_null(>txn_node) || dst->new_datum)) {
 return dst;
 }
diff --git a/tests/idltest.ovsschema b/tests/idltest.ovsschema
index 21d8118..57b6bde 100644
--- a/tests/idltest.ovsschema
+++ b/tests/idltest.ovsschema
@@ -34,7 +34,8 @@
 "min": 0
   }
 }
-  }
+  },
+  "isRoot" : true
 },
 "link2": {
   "columns": {
@@ -50,7 +51,8 @@
 "min": 0
   }
 }
-  }
+  },
+  "isRoot" : true
 },
 "simple": {
   "columns": {
@@ -104,7 +106,8 @@
 "min": 0
   }
 }
-  }
+  },
+  "isRoot" : true
 },
 "simple2" : {
   "columns" : {
@@ -133,7 +136,8 @@
 "max": "unlimited"
   }
 }
-  }
+  },
+  "isRoot" : true
 },
 "simple3" : {
   "columns" : {
@@ -156,14 +160,16 @@
  "max": "unlimited"
   }
 }
-  }
+  },
+  "isRoot" : true
 },
 "simple4" : {
   "columns" : {
 "name" : {
   "type": "string"
 }
-  }
+  },
+  "isRoot" : false
 }
   }
 }
diff --git a/tests/ovsdb-idl.at b/tests/ovsdb-idl.at
index a5f75fe..21745ea 100644
--- a/tests/ovsdb-idl.at
+++ b/tests/ovsdb-idl.at
@@ -1596,3 +1596,29 @@ 
OVSDB_CHECK_IDL_COMPOUND_INDEX_DOUBLE_COLUMN_C([Compound_index, double column te
 006: i=4 s=List001 b=True r=130.00
 006: i=5 s=List005 b=True r=130.00
 ])
+

Re: [ovs-dev] [PATCH 00/11] Fix clang static analysis null pointer bugs.

[Ben Pfaff]

On Sat, Oct 28, 2017 at 10:31:47AM -0700, William Tu wrote:
> Before the patch, the scan-build reports 46 bugs found.
> The patch series fix the clang static checker bug group:
> "Argument with nonnull attribute passed null"
> Most of the fixes are adding "ovs_assert" to tell the checker
> that the pointer won't be null. Now the bugs count reduces to 35.
> Tested it by doing "make clang-analyze"

Thanks for taking a look at all those warnings.

In general, I'm not a fan of adding assertions for nonnull pointers that
are dereferenced soon after.  This kind of an assertion doesn't really
help much, because it just converts a SIGSEGV signal into a SIGABRT
signal.  I agree that it makes sense to add them when the assertion is
long before the dereference, or if the pointer should always be nonnull
but is not necessarily dereferenced.

I don't think it should be a goal to eliminate all clang-analyzer
warnings.  clang-analyzer simply doesn't have enough context in many
cases, so it gives incorrect warnings.

I'll make some more specific comments on individual patches.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 02/11] ovn-controller: Fix possible null pointer in memcmp.

[Ben Pfaff]

On Sat, Oct 28, 2017 at 10:31:49AM -0700, William Tu wrote:
> Clang reports possible null pointer in_dhcp_opt passing to memcmp.
> This might due to dp_packet_get_udp_payload retuning null.  Fix it
> by adding ovs_assert.
> 
> Signed-off-by: William Tu 
> ---
>  ovn/controller/pinctrl.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 469a35586b8a..3fdd01182a52 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -270,6 +270,7 @@ pinctrl_handle_put_dhcp_opts(
>  sizeof (struct dhcp_header);
>  
>  ovs_be32 magic_cookie = htonl(DHCP_MAGIC_COOKIE);
> +ovs_assert(in_dhcp_opt);

I don't see how this makes sense.  in_dhcp_opt is a pointer plus a
nonzero constant.  That's only null if it wraps around.  I don't want to
"fix" this one.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 08/11] ovn-sbctl: Fix possible null pointer to qsort.

[William Tu]

Hi Mark,
Thanks for the review. I will fix it and resubmit v2.
William

On Mon, Oct 30, 2017 at 8:12 AM, Mark Michelson  wrote:
> This is not the proper solution. If you start ovn-northd and do not add any
> logical switches or routers, then there will be no logical flows created.
> Currently, if you run `ovs-sbctl lflow-list` in this circumstance, it lists
> nothing and exits cleanly. With this change, the command hits the assertion
> and crashes.
> it's not an erroneous case to have no logical flows to sort. I think the
> proper thing to do in this case is to check the value of n_flows after the
> loop that populates lflows. If it is 0, then close the vconn and exit the
> function early.
>
> On Sat, Oct 28, 2017 at 12:37 PM William Tu  wrote:
>>
>> Clang reports possible null pointer 'lflows' passed to qsort.
>> This is due to the checker unable to make sure whether 'lflows'
>> gets malloc or not in the previous loop.  Thus, fix it by adding
>> ovs_assert to pass clang checker.
>>
>> Signed-off-by: William Tu 
>> ---
>>  ovn/utilities/ovn-sbctl.c | 1 +
>>  1 file changed, 1 insertion(+)
>>
>> diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
>> index 7af5863b08fc..b1f106002842 100644
>> --- a/ovn/utilities/ovn-sbctl.c
>> +++ b/ovn/utilities/ovn-sbctl.c
>> @@ -860,6 +860,7 @@ cmd_lflow_list(struct ctl_context *ctx)
>>  lflows[n_flows] = lflow;
>>  n_flows++;
>>  }
>> +ovs_assert(lflows);
>>  qsort(lflows, n_flows, sizeof *lflows, lflow_cmp);
>>
>>  bool print_uuid = shash_find(>options, "--uuid") != NULL;
>> --
>> 2.7.4
>>
>> ___
>> dev mailing list
>> d...@openvswitch.org
>> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH v2] ovsdb-idl: fix index row setting with references.

[Ben Pfaff]

On Fri, Oct 27, 2017 at 05:40:56PM -0700, Han Zhou wrote:
> IDL index should be able to be used without having to be in a
> transaction. However, current implementation leads to crash if
> a reference type column is being set in an index row for querying
> purpose when it is not in a transaction. It is because of the
> uninitialized arcs and unnecessary updates of the arcs. This patch
> fixes it by identifying index rows by a magic uuid, so that when
> parsing index row, the arcs are not updated. A new test case is
> added to cover this scenario.
> 
> Signed-off-by: Han Zhou 
> ---
> 
> Notes:
> v1 -> v2: use magic UUID to identify index rows to avoid introducing
>   a parameter everywhere.

Thanks for the revision!

I still see a spelling error "udpate" in the commit.  Please fix it.

Instead of using memcpy() to copy a UUID, please use struct assignment
(it is probably not necessary to have a function to do this).

Our usual practice is to write:
  static void
  set_index_row(struct ovsdb_idl_row *row)
instead of:
  static void set_index_row(struct ovsdb_idl_row *row)

Thanks,

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

Re: [ovs-dev] [PATCH] faq: Fix header path of ofp-msgs.h.

[Ben Pfaff]

On Mon, Oct 30, 2017 at 10:46:42AM -0700, William Tu wrote:
> Replace path 'lib/ofp-msgs.h' with 'include/openvswitch/ofp-msgs.h'
> 
> Signed-off-by: William Tu 

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

[ovs-dev] [PATCH] faq: Fix header path of ofp-msgs.h.

[William Tu]

Replace path 'lib/ofp-msgs.h' with 'include/openvswitch/ofp-msgs.h'

Signed-off-by: William Tu 
---
 Documentation/faq/contributing.rst | 9 +
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/Documentation/faq/contributing.rst 
b/Documentation/faq/contributing.rst
index df4eb9fac2c2..e4a37708fbca 100644
--- a/Documentation/faq/contributing.rst
+++ b/Documentation/faq/contributing.rst
@@ -28,10 +28,11 @@ Development
 Q: How do I implement a new OpenFlow message?
 
 A: Add your new message to ``enum ofpraw`` and ``enum ofptype`` in
-``lib/ofp-msgs.h``, following the existing pattern.  Then recompile and fix
-all of the new warnings, implementing new functionality for the new message
-as needed.  (If you configure with ``--enable-Werror``, as described in
-:doc:`/intro/install/general`, then it is impossible to miss any warnings.)
+``include/openvswitch/ofp-msgs.h``, following the existing pattern.
+Then recompile and fix all of the new warnings, implementing new 
functionality
+for the new message as needed.  (If you configure with 
``--enable-Werror``, as
+described in :doc:`/intro/install/general`, then it is impossible to miss 
any
+warnings.)
 
 To add an OpenFlow vendor extension message (aka experimenter message) for
 a vendor that doesn't yet have any extension messages, you will also need
-- 
2.7.4

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

Re: [ovs-dev] [PATCH] dpif-netlink-rtnl: Fix ovs_geneve probing after restart

[William Tu]

Thanks for the review.

Guru and I had some offline discussion. We have concern about possible
packet lost when unconditionally remove the interface and bring it
again. Currently if the userspace ovs-vswitchd dies, since the kernel
datapath module still remains, packets can still go through.
If restarting ovs-vswitch restarts the geneve device, then we might
see more packets lost. So I'm thinking about another way to fix it
without deleting the device.

Thanks
William

On Thu, Oct 26, 2017 at 1:20 PM, Eric Garver  wrote:
> On Thu, Oct 26, 2017 at 12:31:03PM -0700, William Tu wrote:
>> When using the out-of-tree (openvswitch compat) geneve module,
>> the first time oot tunnel probing returns true (correct).
>> Without unloading the geneve module, if the userspace ovs-vswitchd
>> restarts, because the 'geneve_sys_6081' still exists, the probing
>> incorrectly returns false and loads the in-tree (upstream kernel)
>> geneve module.  The issue also exists the other way around, where
>> existing geneve module is in-tree and ovs-vswitchd restarts.
>>
>> The patch fixes it by unconditionally removing the tunnel device
>> before the probing.  To reproduce the issue, start the ovs
>> > /etc/init.d/openvswitch-switch start
>> > creat a bridge and attach a geneve port using out-of-tree geneve
>> > /etc/init.d/openvswitch-switch restart
>>
>> Fixes: 921c370a9df5 ("dpif-netlink: Probe for out-of-tree tunnels, decides 
>> used interface")
>> Signed-off-by: William Tu 
>> Cc: Eric Garver 
>> Cc: Gurucharan Shetty 
>> ---
>>  lib/dpif-netlink-rtnl.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/lib/dpif-netlink-rtnl.c b/lib/dpif-netlink-rtnl.c
>> index 0c32e7d8ccb4..148ce5ff3a3d 100644
>> --- a/lib/dpif-netlink-rtnl.c
>> +++ b/lib/dpif-netlink-rtnl.c
>> @@ -448,6 +448,8 @@ dpif_netlink_rtnl_probe_oot_tunnels(void)
>>  }
>>
>>  name = netdev_vport_get_dpif_port(netdev, namebuf, sizeof namebuf);
>> +dpif_netlink_rtnl_destroy(name);
>> +
>>  error = dpif_netlink_rtnl_create(tnl_cfg, name, 
>> OVS_VPORT_TYPE_GENEVE,
>>   "ovs_geneve",
>>   (NLM_F_REQUEST | NLM_F_ACK
>> --
>> 2.7.4
>
> Thanks for the fix!
>
> Acked-by: Eric Garver 
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] system-traffic: Fix conntrack tests

[William Tu]

On Thu, Oct 26, 2017 at 2:24 PM, Yi-Hung Wei  wrote:
> Three conntrack system-traffic tests are broken because of a recent
> change 7827edcaebd8 ("Add dl_type to flow metadata for correct
> interpretation of conntrack metadata"). It can be reproduced by
>   $ make check-system-userspace TESTSUITEFLAGS='18 19 37'
>
> This patch modifies the check messages to fix the breakage.
>
> Fixes: 7827edcaebd8 ("Add dl_type to flow metadata for correct interpretation 
> of conntrack metadata")
> CC: Daniel Alvarez 
> Signed-off-by: Yi-Hung Wei 
> ---

Looks good to me.

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

[ovs-dev] [PATCH 4/4] util: Fix potential leak of memory.

[William Tu]

Clang reports potiential leak of memory pointed to by 'pname'.
We already free the previous subporgram name when setting the
new subprogram name, and once the running thread exits, the
name of the process will be free by the system.  Thus, we don't
need to explicitly free it, so exclude the clang analyzer as a
workaround.

Signed-off-by: William Tu 
---
 lib/util.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/lib/util.c b/lib/util.c
index a26cd51dcf6d..7153e8b8a5b6 100644
--- a/lib/util.c
+++ b/lib/util.c
@@ -534,6 +534,7 @@ get_subprogram_name(void)
 void
 set_subprogram_name(const char *subprogram_name)
 {
+#ifndef __clang_analyzer__
 char *pname = xstrdup(subprogram_name ? subprogram_name : program_name);
 free(subprogram_name_set(pname));
 
@@ -544,6 +545,7 @@ set_subprogram_name(const char *subprogram_name)
 #elif HAVE_PTHREAD_SET_NAME_NP
 pthread_set_name_np(pthread_self(), pname);
 #endif
+#endif
 }
 
 unsigned int
-- 
2.7.4

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

[ovs-dev] [PATCH 3/4] ovn-sbctl: Fix dead assignment.

[William Tu]

Clang reports value stored to 'sb' is never read.
Fix it by removing it.

Signed-off-by: William Tu 
---
 ovn/utilities/ovn-sbctl.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ovn/utilities/ovn-sbctl.c b/ovn/utilities/ovn-sbctl.c
index 7af5863b08fc..c5ec4e6eaf24 100644
--- a/ovn/utilities/ovn-sbctl.c
+++ b/ovn/utilities/ovn-sbctl.c
@@ -1235,7 +1235,7 @@ do_sbctl(const char *args, struct ctl_command *commands, 
size_t n_commands,
 const struct sbrec_sb_global *sb = sbrec_sb_global_first(idl);
 if (!sb) {
 /* XXX add verification that table is empty */
-sb = sbrec_sb_global_insert(txn);
+sbrec_sb_global_insert(txn);
 }
 
 symtab = ovsdb_symbol_table_create();
-- 
2.7.4

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

[ovs-dev] [PATCH 2/4] ofp-actions: Fix dead assignment.

[William Tu]

Clang reports value stored to 'eah' in never read.
Fix it by removing it.

Signed-off-by: William Tu 
---
 lib/ofp-actions.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/ofp-actions.c b/lib/ofp-actions.c
index 71eb70c3c239..79041c281ed0 100644
--- a/lib/ofp-actions.c
+++ b/lib/ofp-actions.c
@@ -5535,7 +5535,7 @@ encode_CLONE(const struct ofpact_nest *clone,
 const size_t ofs = out->size;
 struct ext_action_header *eah;
 
-eah = put_NXAST_CLONE(out);
+put_NXAST_CLONE(out);
 len = ofpacts_put_openflow_actions(clone->actions,
ofpact_nest_get_action_len(clone),
out, ofp_version);
-- 
2.7.4

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

[ovs-dev] [PATCH 0/4] Fix clang static checker errors.

[William Tu]

The patch series fix three bug types reported by clang,
dead assignment, bad free, and memory leak.

William Tu (4):
  ofproto-dpif-xlate: Fix bad memory free.
  ofp-actions: Fix dead assignment.
  ovn-sbctl: Fix dead assignment.
  util: Fix potential leak of memory.

 lib/ofp-actions.c| 2 +-
 lib/util.c   | 2 ++
 ofproto/ofproto-dpif-xlate.c | 2 +-
 ovn/utilities/ovn-sbctl.c| 2 +-
 4 files changed, 5 insertions(+), 3 deletions(-)

-- 
2.7.4

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

[ovs-dev] [PATCH 1/4] ofproto-dpif-xlate: Fix bad memory free.

[William Tu]

Clang reports possibly bad free of 'ofm' when it comes from the stack
instead of malloc because Clang is not able to verify whether the previous
if condition 'ctx->xin->xcache' still hold the same.  Fix it by
adding additional condition.

Signed-off-by: William Tu 
---
 ofproto/ofproto-dpif-xlate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ddcaf059ded2..b8a4986061a9 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -5128,7 +5128,7 @@ xlate_learn_action(struct xlate_ctx *ctx, const struct 
ofpact_learn *learn)
 }
 }
 
-if (ctx->xin->xcache) {
+if (ctx->xin->xcache && ofm != __) {
 free(ofm);
 }
 
-- 
2.7.4

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

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

[Ben Pfaff]

On Thu, Oct 26, 2017 at 03:08:00PM -0700, Yi-Hung Wei wrote:
> On Mon, Oct 9, 2017 at 10:33 AM, Ben Pfaff  wrote:
> > Signed-off-by: Ben Pfaff 
> > ---
> >  Documentation/faq/contributing.rst | 44 
> > +++---
> >  include/openvswitch/ofp-errors.h   |  4 +++-
> >  2 files changed, 34 insertions(+), 14 deletions(-)
> >
> > diff --git a/Documentation/faq/contributing.rst 
> > b/Documentation/faq/contributing.rst
> > index 6dfc8bc4d436..d59376cd615c 100644
> > --- a/Documentation/faq/contributing.rst
> > +++ b/Documentation/faq/contributing.rst
> > @@ -33,22 +33,28 @@ Q: How do I implement a new OpenFlow message?
> >  as needed.  (If you configure with ``--enable-Werror``, as described in
> >  :doc:`/intro/install/general`, then it is impossible to miss any 
> > warnings.)
> >
> > -If you need to add an OpenFlow vendor extension message for a vendor 
> > that
> > -doesn't yet have any extension messages, then you will also need to 
> > edit
> > -``build-aux/extract-ofp-msgs``.
> > +To add an OpenFlow vendor extension message (aka experimenter message) 
> > for
> > +a vendor that doesn't yet have any extension messages, you will also 
> > need
> > +to edit ``build-aux/extract-ofp-msgs`` and at least 
> > ``ofphdrs_decode()``
> > +and ``ofpraw_put__()`` in ``lib/ofp-msgs.c``.  OpenFlow doesn't 
> > standardize
> > +vendor extensions very well, so it's hard to make the process simpler 
> > than
> > +that.  (If you have a choice of how to design your vendor extension
> > +messages, it will be easier if you make them resemble the ONF and OVS
> > +extension messages.)
> >
> >  Q: How do I add support for a new field or header?
> >
> >  A: Add new members for your field to ``struct flow`` in 
> > ``lib/flow.h``, and
> >  add new enumerations for your new field to ``enum mf_field_id`` in
> > -``lib/meta-flow.h``, following the existing pattern.  Also, add 
> > support to
> Instead of ``lib/meta-flow.h``, maybe
> ``include/openvswitch/meta-flow.h``? Otherwise, looks good to me.
> 
> Acked-by: Yi-Hung Wei 

Thanks, I made that fix and applied this to master.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] timeval: Check for OS-provided clock_gettime on macOS

[Ben Pfaff]

On Sat, Oct 28, 2017 at 04:38:30PM +0100, Richard Oliver wrote:
> [Problem]
> Compilation error on newer versions of macOS (Sierra onwards) due to
> multiple declarations of clock_gettime.
> 
> [Solution]
> Have configure check for clock_gettime and check this result in
> timeval to avoid incorrectly declaring/defining clock_gettime again.
> 
> [Testing]
> Source code now successfully builds on macOS.
> 
> Signed-off-by: Richard Oliver 

Thanks a lot, I applied this to master and branch-2.8.
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH] treewide: Get rid of "echo -n", and add a test to prevent regression.

[Ben Pfaff]

On Mon, Oct 30, 2017 at 10:51:46AM -0200, Flavio Leitner wrote:
> On Fri, 27 Oct 2017 09:59:33 -0700
> Ben Pfaff  wrote:
> 
> > "echo -n" is not POSIX and has spotty support in shells.
> > 
> > CC: Timothy Redaelli 
> > CC: Flavio Leitner 
> > Signed-off-by: Ben Pfaff 
> > ---
> >  Makefile.am   | 11 +++
> >  tests/automake.mk |  2 +-
> >  tests/ofp-print.at| 10 +-
> >  tests/ofproto-dpif.at |  6 +++---
> >  tutorial/ovs-sandbox  |  2 +-
> >  5 files changed, 21 insertions(+), 10 deletions(-)
> > 
> > diff --git a/Makefile.am b/Makefile.am
> > index 31d6331792af..5ceb3822c976 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -303,6 +303,17 @@ check-endian:
> > fi
> >  .PHONY: check-endian
> >  
> > +ALL_LOCAL += check-echo-n
> > +check-echo-n:
> > +   @if test -e $(srcdir)/.git && (git --version) >/dev/null 2>&1 && \
> > + git --no-pager grep -n 'echo'' -n' $(srcdir); \
> > +   then \
> > + echo "See above for uses for \"echo"" -n\", which is non-POSIX"; \
> > + echo "and does not work with all shells.  Use \"printf\" instead."; \
> > + exit 1; \
> > +   fi
> > +.PHONY: check-echo-n
> > +
> >  ALL_LOCAL += thread-safety-check
> >  thread-safety-check:
> > @cd $(srcdir); \
> > diff --git a/tests/automake.mk b/tests/automake.mk
> > index 156b40f58d7c..198ba64e5cc2 100644
> > --- a/tests/automake.mk
> > +++ b/tests/automake.mk
> > @@ -201,7 +201,7 @@ $(valgrind_wrappers): tests/valgrind-wrapper.in
> >  CLEANFILES += $(valgrind_wrappers)
> >  EXTRA_DIST += tests/valgrind-wrapper.in
> >  
> > -VALGRIND = valgrind --log-file=valgrind.%p --leak-check=full \
> > +VALGRIND = valgrind --log-file=valgrind.%p --leak-check=no \
> > --suppressions=$(abs_top_srcdir)/tests/glibc.supp \
> > --suppressions=$(abs_top_srcdir)/tests/openssl.supp --num-callers=20
> >  HELGRIND = valgrind --log-file=helgrind.%p --tool=helgrind \
> > diff --git a/tests/ofp-print.at b/tests/ofp-print.at
> 
> 
> The above change seems unrelated, otherwise the patch looks good to me.
> fbl

You are right.  I dropped this part of the patch.

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

Re: [ovs-dev] DNS support feature (was: Re: DNS support options)

[Ben Pfaff]

One possible use for this DNS feature is to identify cluster members for
OVSDB clusters.  That might be an appropriate use for a low TTL.

On Mon, Oct 30, 2017 at 10:52:14AM +0100, Miguel Angel Ajo Pelayo wrote:
> I don't believe rounding up TTLs would be a good practice. The
> administrators
> are aware of the risks of having a very low TTL, so if they decide to pick
> a low TTL they may have good reasons (like the possibility of a very close
> event of the IP being moved, or some sort of failover mechanism, or a
> way for dealing with dynamic dns). If you round it up you add unexpected
> extra downtime.
> 
> 
> 
> On Fri, Oct 27, 2017 at 6:38 PM, Yifeng Sun  wrote:
> 
> > Thanks a lot. I will keep your guidance in mind.
> >
> > On Fri, Oct 27, 2017 at 9:29 AM, Mark Michelson 
> > wrote:
> >
> > > Usually the software that performs DNS lookups and caches results is
> > > referred to as a "DNS forwarder". You configure resolv.conf's nameserver
> > as
> > > 127.0.0.1. This way, DNS queries go to the DNS forwarder running on
> > > localhost. The forwarder then has real nameservers configured to send the
> > > queries to. The forwarder receives the DNS response (or lack of response
> > if
> > > the nameserver is unreachable), caches the result, and sends the result
> > > back to the system resolver. From the perspective of OVS, all it is doing
> > > is performing a simple DNS lookup. Further DNS lookups hit the forwarder,
> > > which has cached the previous queries and can immediately return the
> > cached
> > > records (or cached failure).
> > >
> > > One common DNS forwarder that works this way is dnsmasq. There are likely
> > > lots of others. I believe that bind can be configured to act as a DNS
> > > forwarder as well. I recommend searching for "DNS forwarder" and seeing
> > > what you can find. The common thread here, though, is that OVS would not
> > > actually package or depend on a DNS forwarder. Rather, we would add
> > > documentation that strongly suggests that applications that consider DNS
> > > resolution to be critical configure their favorite DNS forwarder on
> > systems
> > > running OVS.
> > >
> > > Since we are looking into using a third party library for DNS resolution,
> > > it may also be worth looking into what sort of caching those libraries
> > > provide. I am not familiar with that information off the top of my head.
> > >
> > > Mark
> > >
> > > On Fri, Oct 27, 2017 at 11:14 AM Yifeng Sun 
> > > wrote:
> > >
> > >> Mark, thanks a lot for the detailed and thorough explanation.
> > >>
> > >> Do you happen to know any other projects that we can take a peek?
> > >>
> > >> On Fri, Oct 27, 2017 at 8:57 AM, Mark Michelson 
> > >> wrote:
> > >>
> > >>> Yep, that makes good sense. I'd recommend having some min and max
> > >>> threshold though. That way, if a record has a TTL of multiple days,
> > you can
> > >>> round that down to something more reasonable. Similarly, a
> > ridiculously low
> > >>> TTL can be rounded up.
> > >>>
> > >>> There's another aspect to DNS caching that I briefly mentioned in my
> > >>> last e-mail: negative caching. Consider that you attempt to look up
> > >>> exampledomain.com, and for some reason, you either can't reach your
> > DNS
> > >>> server or the DNS server returns NXDOMAIN (or some other error). Each
> > >>> additional lookup of exampledomain.com will likely hit this same
> > >>> problem until either you can restore the link to your DNS server or the
> > >>> proper records get added to DNS. If exampledomain.com is a frequent
> > >>> destination for traffic, you don't want to be doing full DNS lookups
> > of it
> > >>> each time, just to find out you can't send to it. This is especially
> > true
> > >>> in the case that the DNS server is unreachable. resolv.conf by default
> > will
> > >>> wait 5 seconds on an attempt and will attempt the query twice before
> > giving
> > >>> up. This means each DNS query will take 10 seconds just to ultimately
> > fail.
> > >>> This can lead to large backlogs of queries. With negative caching, you
> > >>> cache the fact that an attempt to reach exampledomain.com failed, and
> > >>> for some amount of time, any further attempts to query that domain will
> > >>> immediately fail rather than attempting the query again.
> > >>>
> > >>> I mention negative caching because in some ways, it's more important
> > >>> than positive caching. I've seen certain applications get completely
> > >>> crippled by having the DNS server unavailable since they'll keep
> > attempting
> > >>> to perform DNS lookups for domains that will ultimately fail. Caching
> > >>> positive results based on TTL of records won't help in a case like
> > this.
> > >>>
> > >>> I'm not going to take a hard-line stand that DNS caching absolutely
> > >>> should not be added to OVS. But I point out negative caching as one of
> > >>> those things that may seem non-obvious at 

Re: [ovs-dev] [PATCH 00/11] Fix clang static analysis null pointer bugs.

[Mark Michelson]

I've reviewed everything except for patches 1, 6, 7, and 9. I'm not very
familiar with those areas of code and do not feel as comfortable giving an
ACK to those.

On Sat, Oct 28, 2017 at 12:32 PM William Tu  wrote:

> Before the patch, the scan-build reports 46 bugs found.
> The patch series fix the clang static checker bug group:
> "Argument with nonnull attribute passed null"
> Most of the fixes are adding "ovs_assert" to tell the checker
> that the pointer won't be null. Now the bugs count reduces to 35.
> Tested it by doing "make clang-analyze"
>
> William Tu (11):
>   dp-packet: fix possible null pointer argument
>   ovn-controller: Fix possible null pointer in memcmp.
>   ovs-appctl: Fix possible null pointer argument.
>   lib/process: Fix possible null pointer argument.
>   ovs-ofctl: Fix possible null pointer.
>   packets: Fix possible null pointer argument to memmove.
>   packets: Fix possible null pointer to memcmp.
>   ovn-sbctl: Fix possible null pointer to qsort.
>   tnl-neigh-cache: Fix possible null pointer.
>   lib: Fix possible null pointer to execvp.
>   ovs-testcontroller: Fix possible null pointer to atoi.
>
>  lib/dp-packet.c| 1 +
>  lib/packets.c  | 2 ++
>  lib/process.c  | 2 ++
>  lib/tnl-neigh-cache.c  | 3 +++
>  ovn/controller/pinctrl.c   | 1 +
>  ovn/utilities/ovn-sbctl.c  | 1 +
>  utilities/ovs-appctl.c | 1 +
>  utilities/ovs-ofctl.c  | 1 +
>  utilities/ovs-testcontroller.c | 2 ++
>  9 files changed, 14 insertions(+)
>
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 11/11] ovs-testcontroller: Fix possible null pointer to atoi.

[Mark Michelson]

Looks good to me

On Sat, Oct 28, 2017 at 12:38 PM William Tu  wrote:

> Clang reports possible optarg as null passed to atoi.
> Fix it by adding ovs_assert check.
>
> Signed-off-by: William Tu 
>
Acked-by: Mark Michelson 

> ---
>  utilities/ovs-testcontroller.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/utilities/ovs-testcontroller.c
> b/utilities/ovs-testcontroller.c
> index b4a451286455..be28d5494ec0 100644
> --- a/utilities/ovs-testcontroller.c
> +++ b/utilities/ovs-testcontroller.c
> @@ -318,6 +318,7 @@ parse_options(int argc, char *argv[])
>  if (!strcmp(optarg, "permanent")) {
>  max_idle = OFP_FLOW_PERMANENT;
>  } else {
> +ovs_assert(optarg);
>  max_idle = atoi(optarg);
>  if (max_idle < 1 || max_idle > 65535) {
>  ovs_fatal(0, "--max-idle argument must be between 1
> and "
> @@ -327,6 +328,7 @@ parse_options(int argc, char *argv[])
>  break;
>
>  case 'q':
> +ovs_assert(optarg);
>  default_queue = atoi(optarg);
>  break;
>
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 10/11] lib: Fix possible null pointer to execvp.

[Mark Michelson]

This looks fine to me.
If you end up needing to re-work this patch series, I think it would be
appropriate to combine this with patch #4 since this is essentially fixing
the same possible NULL pointer.

On Sat, Oct 28, 2017 at 12:38 PM William Tu  wrote:

> Clang reports possible null pointer 'argv[0]' to execvp.
> Fix it by adding ovs_assert check.
>
> Signed-off-by: William Tu 
>
Acked-by: Mark Michelson 

> ---
>  lib/process.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/process.c b/lib/process.c
> index 254052f2c27d..0b8f994f9b75 100644
> --- a/lib/process.c
> +++ b/lib/process.c
> @@ -275,6 +275,7 @@ process_start(char **argv, struct process **pp)
>  close(fd);
>  }
>  xpthread_sigmask(SIG_SETMASK, _mask, NULL);
> +ovs_assert(argv[0]);
>  execvp(argv[0], argv);
>  fprintf(stderr, "execvp(\"%s\") failed: %s\n",
>  argv[0], ovs_strerror(errno));
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 05/11] ovs-ofctl: Fix possible null pointer.

[Mark Michelson]

Looks good to me.

On Sat, Oct 28, 2017 at 12:35 PM William Tu  wrote:

> Clang reports possible null pointer of optarg as argument to strtoul.
> Fix it by adding ovs_assert check.
>
> Signed-off-by: William Tu 
>
Acked-by: Mark Michelson 

> ---
>  utilities/ovs-ofctl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/utilities/ovs-ofctl.c b/utilities/ovs-ofctl.c
> index 5b7f1b02104f..05c5995fa775 100644
> --- a/utilities/ovs-ofctl.c
> +++ b/utilities/ovs-ofctl.c
> @@ -262,6 +262,7 @@ parse_options(int argc, char *argv[])
>
>  switch (c) {
>  case 't':
> +ovs_assert(optarg);
>  timeout = strtoul(optarg, NULL, 10);
>  if (timeout <= 0) {
>  ovs_fatal(0, "value %s on -t or --timeout is not at least
> 1",
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 04/11] lib/process: Fix possible null pointer argument.

[Mark Michelson]

Looks good to me.

On Sat, Oct 28, 2017 at 12:35 PM William Tu  wrote:

> Clang reports possible null pointer due to process_register could
> take the name from argv[0].  Fix it by adding ovs_assert check.
>
> Signed-off-by: William Tu 
>
Acked-by: Mark Michelson 

> ---
>  lib/process.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/lib/process.c b/lib/process.c
> index 3e119b59bfbc..254052f2c27d 100644
> --- a/lib/process.c
> +++ b/lib/process.c
> @@ -172,6 +172,7 @@ process_register(const char *name, pid_t pid)
>  struct process *p;
>  const char *slash;
>
> +ovs_assert(name);
>  p = xzalloc(sizeof *p);
>  p->pid = pid;
>  slash = strrchr(name, '/');
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 03/11] ovs-appctl: Fix possible null pointer argument.

[Mark Michelson]

Looks good to me.

On Sat, Oct 28, 2017 at 12:34 PM William Tu  wrote:

> Clang reports possible optarg as null pointer passed to atoi.
> Fix it by adding ovs_assert check before.
>
> Signed-off-by: William Tu 
>
Acked-by: Mark Michelson 

> ---
>  utilities/ovs-appctl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/utilities/ovs-appctl.c b/utilities/ovs-appctl.c
> index 8f87cc4f6c6e..cc1326bc9478 100644
> --- a/utilities/ovs-appctl.c
> +++ b/utilities/ovs-appctl.c
> @@ -165,6 +165,7 @@ parse_command_line(int argc, char *argv[])
>  exit(EXIT_SUCCESS);
>
>  case 'T':
> +ovs_assert(optarg);
>  time_alarm(atoi(optarg));
>  break;
>
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

[ovs-dev] Greeting from Mrs Fofana Maladho

[Mrs Fofana Maladho Boukhary via dev]

I am Mrs Fofana Maladho Boukhary, I was born on June 8, 1969. I am a widow, I 
am so happy to be writing this message because it rejoices my heart if you can 
share my passion to get this project I want to get started.

I want you to understand that my current medical status will not allow me to 
move to make this project a reality, which is why I need your help to make this 
project go through. My late husband was a precious stone dealer, he buys from 
Ghana and Asia and I joined him in business about 25 years ago and since then 
we have been able to make lots of money.

The problem is that my husband is now late and  I was diagnosed with a deadly 
syndrome and I also have brain tumor as at now and the doctor is saying that I 
only have only a few months to live as to be realistic with you I have to say 
for about month or so that's why I want you to do it for me as a charity job as 
there is no one to deal with that money here for security reasons.

Now the fund is worth four million eight hundred thousand United States dollars 
($4.8m)is currently in a bank. I asked you to do this because I want you to 
recover the money from the bank and half the money should be used in setting up 
the orphanage in your country and other facilities to help less privilege 
because the other half of the money should be used in the investment in your 
country.

Note that this investment will be in your name and 50% of this investment only 
while the rest should be for charity. Also you will own share of the investment.

Note that when you accept this offer and I will introduce you to the bank as a 
beneficiary to the account where the fund is and make you a signatory to the 
account.

Answer me with a copy of your identity card if you accept this offer and you 
must create the infrastructure to meet A Needy and less privileged.

Yours faithful.
Mrs Fofana Maladho Boukhary
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH 02/11] ovn-controller: Fix possible null pointer in memcmp.

[Mark Michelson]

This looks fine to me. Usually I don't like assertions based on contents of
incoming packets. However, in this case, ovn-northd has set up the logical
flow so that only UDP packets can reach here.

On Sat, Oct 28, 2017 at 12:34 PM William Tu  wrote:

> Clang reports possible null pointer in_dhcp_opt passing to memcmp.
> This might due to dp_packet_get_udp_payload retuning null.  Fix it
> by adding ovs_assert.
>
> Signed-off-by: William Tu 
>
Acked-by: Mark Michelson

> ---
>  ovn/controller/pinctrl.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/ovn/controller/pinctrl.c b/ovn/controller/pinctrl.c
> index 469a35586b8a..3fdd01182a52 100644
> --- a/ovn/controller/pinctrl.c
> +++ b/ovn/controller/pinctrl.c
> @@ -270,6 +270,7 @@ pinctrl_handle_put_dhcp_opts(
>  sizeof (struct dhcp_header);
>
>  ovs_be32 magic_cookie = htonl(DHCP_MAGIC_COOKIE);
> +ovs_assert(in_dhcp_opt);
>  if (memcmp(in_dhcp_opt, _cookie, sizeof(ovs_be32))) {
>  static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(1, 5);
>  VLOG_WARN_RL(, "DHCP magic cookie not present in the DHCP
> packet");
> --
> 2.7.4
>
> ___
> dev mailing list
> d...@openvswitch.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
>
___
dev mailing list
d...@openvswitch.org
https://mail.openvswitch.org/mailman/listinfo/ovs-dev

Re: [ovs-dev] [PATCH V2 3/4] tc: Add header rewrite using tc pedit action

[Simon Horman]

On Wed, Oct 25, 2017 at 02:24:15PM +0300, Roi Dayan wrote:
> 
> 
> On 27/09/2017 12:08, Simon Horman wrote:
> > On Mon, Sep 25, 2017 at 04:31:42PM +0300, Paul Blakey wrote:
> > > 
> > > 
> > > On 18/09/2017 18:01, Simon Horman wrote:
> > > > On Mon, Sep 18, 2017 at 07:16:03AM +0300, Roi Dayan wrote:
> > > > > From: Paul Blakey 
> > > > > 
> > > > > To be later used to implement ovs action set offloading.
> > > > > 
> > > > > Signed-off-by: Paul Blakey 
> > > > > Reviewed-by: Roi Dayan 
> > > > > ---
> > > > >   lib/tc.c | 372 
> > > > > ++-
> > > > >   lib/tc.h |  16 +++
> > > > >   2 files changed, 385 insertions(+), 3 deletions(-)
> > > > > 
> > > > > diff --git a/lib/tc.c b/lib/tc.c
> > > > > index c9cada2..743b2ee 100644
> > > > > --- a/lib/tc.c
> > > > > +++ b/lib/tc.c
> > > > > @@ -21,8 +21,10 @@
> > > > >   #include 
> > > > >   #include 
> > > > >   #include 
> > > > > +#include 
> > > > >   #include 
> > > > >   #include 
> > > > > +#include 
> > > > >   #include 
> > > > >   #include 
> > > > >   #include 
> > > > > @@ -33,11 +35,14 @@
> > > > >   #include "netlink-socket.h"
> > > > >   #include "netlink.h"
> > > > >   #include "openvswitch/ofpbuf.h"
> > > > > +#include "openvswitch/util.h"
> > > > >   #include "openvswitch/vlog.h"
> > > > >   #include "packets.h"
> > > > >   #include "timeval.h"
> > > > >   #include "unaligned.h"
> > > > > +#define MAX_PEDIT_OFFSETS 8
> > > > 
> > > > Why 8?
> > > We don't expect anything more right now (ipv6 src/dst rewrite requires 8
> > > pedits iirc). I can't think of a larger use case, maybe ipv6 + macs if
> > > that's makes sens. do you suggest we increase it? to what?
> > 
> > It seems strange to me to place a somewhat arbitrary small limit
> > when none exists in the pedit API being used. I would at prefer if
> > it was at least a bigger, say 16 or 32.
> 
> 
> Hi Simon,
> 
> Sorry for the late reply due to holidays and vacations.
> Me & Paul going to go over this and do the fixes needed and
> also rebase over latest master and run tests again.

Likewise, sorry for not responding earlier (same reason).

> I'll answer what I'm more familiar with now and Paul will continue.
> The 8 here is too low and you right. We used this definition
> for allocation of the pedit keys on the stack in
> nl_msg_put_flower_rewrite_pedits()
> 
> It was for convenience instead of calculating the maximum possible
> keys that could exists and allocating it there and freeing it at
> the end.
> 
> Increasing it to 32 is probably more than enough and wont waste much.

Thanks, that sounds good.

> > > > >   VLOG_DEFINE_THIS_MODULE(tc);
> > > > >   static struct vlog_rate_limit error_rl = VLOG_RATE_LIMIT_INIT(60, 
> > > > > 5);
> > > > > @@ -50,6 +55,82 @@ enum tc_offload_policy {
> > > > >   static enum tc_offload_policy tc_policy = TC_POLICY_NONE;
> > > > > +struct tc_pedit_key_ex {
> > > > > +enum pedit_header_type htype;
> > > > > +enum pedit_cmd cmd;
> > > > > +};
> > > > > +
> > > > > +struct flower_key_to_pedit {
> > > > > +enum pedit_header_type htype;
> > > > > +int flower_offset;
> > > > > +int offset;
> > > > > +int size;
> > > > > +};
> > > > > +
> > > > > +static struct flower_key_to_pedit flower_pedit_map[] = {
> > > > > +{
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> > > > > +12,
> > > > > +offsetof(struct tc_flower_key, ipv4.ipv4_src),
> > > > > +MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_src)
> > > > > +}, {
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> > > > > +16,
> > > > > +offsetof(struct tc_flower_key, ipv4.ipv4_dst),
> > > > > +MEMBER_SIZEOF(struct tc_flower_key, ipv4.ipv4_dst)
> > > > > +}, {
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_IP4,
> > > > > +8,
> > > > > +offsetof(struct tc_flower_key, ipv4.rewrite_ttl),
> > > > > +MEMBER_SIZEOF(struct tc_flower_key, ipv4.rewrite_ttl)
> > > > > +}, {
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_IP6,
> > > > > +8,
> > > > > +offsetof(struct tc_flower_key, ipv6.ipv6_src),
> > > > > +MEMBER_SIZEOF(struct tc_flower_key, ipv6.ipv6_src)
> > > > > +}, {
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_IP6,
> > > > > +24,
> > > > > +offsetof(struct tc_flower_key, ipv6.ipv6_dst),
> > > > > +MEMBER_SIZEOF(struct tc_flower_key, ipv6.ipv6_dst)
> > > > > +}, {
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_ETH,
> > > > > +6,
> > > > > +offsetof(struct tc_flower_key, src_mac),
> > > > > +MEMBER_SIZEOF(struct tc_flower_key, src_mac)
> > > > > +}, {
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_ETH,
> > > > > +0,
> > > > > +offsetof(struct tc_flower_key, dst_mac),
> > > > > +MEMBER_SIZEOF(struct tc_flower_key, dst_mac)
> > > > > +}, {
> > > > > +TCA_PEDIT_KEY_EX_HDR_TYPE_ETH,
> 

[ovs-dev] YOUR RESPONSE IS NEEDED

[Y.T. Chan]

Did you receive my previous email regarding your family inheritance?

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

[ovs-dev] [PATCH 4/4] netdev-dpdk: Remove unused MAX_NB_MBUF.

[Ilya Maximets]

CC: Robert Wojciechowicz 
Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each port.")
Signed-off-by: Ilya Maximets 
---
 lib/netdev-dpdk.c | 18 --
 1 file changed, 4 insertions(+), 14 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index cdb3244..0b40966 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -89,23 +89,13 @@ static struct vlog_rate_limit rl = VLOG_RATE_LIMIT_INIT(5, 
20);
 #define NETDEV_DPDK_MBUF_ALIGN  1024
 #define NETDEV_DPDK_MAX_PKT_LEN 9728
 
-/* Max and min number of packets in the mempool.  OVS tries to allocate a
- * mempool with MAX_NB_MBUF: if this fails (because the system doesn't have
- * enough hugepages) we keep halving the number until the allocation succeeds
- * or we reach MIN_NB_MBUF */
-
-#define MAX_NB_MBUF  (4096 * 64)
+/* Min number of packets in the mempool.  OVS tries to allocate a mempool with
+ * roughly estimated number of mbufs: if this fails (because the system doesn't
+ * have enough hugepages) we keep halving the number until the allocation
+ * succeeds or we reach MIN_NB_MBUF */
 #define MIN_NB_MBUF  (4096 * 4)
 #define MP_CACHE_SZ  RTE_MEMPOOL_CACHE_MAX_SIZE
 
-/* MAX_NB_MBUF can be divided by 2 many times, until MIN_NB_MBUF */
-BUILD_ASSERT_DECL(MAX_NB_MBUF % ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF) == 0);
-
-/* The smallest possible NB_MBUF that we're going to try should be a multiple
- * of MP_CACHE_SZ. This is advised by DPDK documentation. */
-BUILD_ASSERT_DECL((MAX_NB_MBUF / ROUND_DOWN_POW2(MAX_NB_MBUF/MIN_NB_MBUF))
-  % MP_CACHE_SZ == 0);
-
 /*
  * DPDK XSTATS Counter names definition
  */
-- 
2.7.4

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

[ovs-dev] [PATCH 3/4] netdev-dpdk: Factor out struct dpdk_mp.

[Ilya Maximets]

Since commit d555d9bded5f ("netdev-dpdk: Create separate memory pool
for each port."), struct dpdk_mp is redundant because each mempool
can be used by single port only and this port already contains all
the information we store in dpdk_mp.
There is no need to duplicate the information.
Fields of this structure currently used only to generate mempool name.
But it's required only while creation and after that we can use
mp->name directly from the struct rte_mempool.

Let's remove this structure and use struct rte_mempool directly
instead.

Signed-off-by: Ilya Maximets 
---
 lib/netdev-dpdk.c | 190 +++---
 1 file changed, 65 insertions(+), 125 deletions(-)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index ba6add2..cdb3244 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -303,15 +303,6 @@ static struct ovs_list dpdk_list OVS_GUARDED_BY(dpdk_mutex)
 static struct ovs_mutex dpdk_mp_mutex OVS_ACQ_AFTER(dpdk_mutex)
 = OVS_MUTEX_INITIALIZER;
 
-struct dpdk_mp {
-struct rte_mempool *mp;
-int mtu;
-int socket_id;
-char if_name[IFNAMSIZ];
-unsigned n_mbufs;   /* Number of mbufs inside the mempool. */
-struct ovs_list list_node OVS_GUARDED_BY(dpdk_mp_mutex);
-};
-
 /* There should be one 'struct dpdk_tx_queue' created for
  * each cpu core. */
 struct dpdk_tx_queue {
@@ -359,7 +350,7 @@ struct netdev_dpdk {
 
 struct ovs_mutex mutex OVS_ACQ_AFTER(dpdk_mutex);
 
-struct dpdk_mp *dpdk_mp;
+struct rte_mempool *mp;
 int mtu;
 int socket_id;
 int buf_size;
@@ -490,38 +481,18 @@ ovs_rte_pktmbuf_init(struct rte_mempool *mp OVS_UNUSED,
 dp_packet_init_dpdk((struct dp_packet *) pkt, pkt->buf_len);
 }
 
-/*
- * Full DPDK memory pool name must be unique
- * and cannot be longer than RTE_MEMPOOL_NAMESIZE
- */
-static char *
-dpdk_mp_name(struct dpdk_mp *dmp)
-{
-uint32_t h = hash_string(dmp->if_name, 0);
-char *mp_name = xcalloc(RTE_MEMPOOL_NAMESIZE, sizeof *mp_name);
-int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
-   h, dmp->socket_id, dmp->mtu, dmp->n_mbufs);
-if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
-VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
-"name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
-ret, dmp->if_name, h, dmp->mtu, dmp->n_mbufs);
-free(mp_name);
-return NULL;
-}
-return mp_name;
-}
-
-static struct dpdk_mp *
-dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool *mp_exists)
+/* Returns a valid pointer when either of the following is true:
+ *  - a new mempool was just created;
+ *  - a matching mempool already exists. */
+static struct rte_mempool *
+dpdk_mp_create(struct netdev_dpdk *dev, int mtu)
 {
-struct dpdk_mp *dmp = dpdk_rte_mzalloc(sizeof *dmp);
-if (!dmp) {
-return NULL;
-}
-*mp_exists = false;
-dmp->socket_id = dev->requested_socket_id;
-dmp->mtu = mtu;
-ovs_strzcpy(dmp->if_name, dev->up.name, IFNAMSIZ);
+char mp_name[RTE_MEMPOOL_NAMESIZE];
+const char *netdev_name = netdev_get_name(>up);
+int socket_id = dev->requested_socket_id;
+uint32_t n_mbufs;
+uint32_t hash = hash_string(netdev_name, 0);
+struct rte_mempool *mp = NULL;
 
 /*
  * XXX: rough estimation of number of mbufs required for this port:
@@ -530,95 +501,72 @@ dpdk_mp_create(struct netdev_dpdk *dev, int mtu, bool 
*mp_exists)
  * + 
  * + 
  */
-dmp->n_mbufs = dev->requested_n_rxq * dev->requested_rxq_size
-+ dev->requested_n_txq * dev->requested_txq_size
-+ MIN(RTE_MAX_LCORE, dev->requested_n_rxq) * NETDEV_MAX_BURST
-+ MIN_NB_MBUF;
+n_mbufs = dev->requested_n_rxq * dev->requested_rxq_size
+  + dev->requested_n_txq * dev->requested_txq_size
+  + MIN(RTE_MAX_LCORE, dev->requested_n_rxq) * NETDEV_MAX_BURST
+  + MIN_NB_MBUF;
 
+ovs_mutex_lock(_mp_mutex);
 do {
-char *mp_name = dpdk_mp_name(dmp);
-if (!mp_name) {
-rte_free(dmp);
-return NULL;
+/* Full DPDK memory pool name must be unique and cannot be
+ * longer than RTE_MEMPOOL_NAMESIZE. */
+int ret = snprintf(mp_name, RTE_MEMPOOL_NAMESIZE, "ovs_%x_%d_%d_%u",
+   hash, socket_id, mtu, n_mbufs);
+if (ret < 0 || ret >= RTE_MEMPOOL_NAMESIZE) {
+VLOG_DBG("snprintf returned %d. "
+ "Failed to generate a mempool name for \"%s\". "
+ "Hash:0x%x, socket_id: %d, mtu:%d, mbufs:%u.",
+ ret, netdev_name, hash, socket_id, mtu, n_mbufs);
+break;
 }
 
 VLOG_DBG("Port %s: Requesting a mempool of %u mbufs "
   "on socket %d for %d Rx and %d Tx queues.",
-  dev->up.name, dmp->n_mbufs,
-  dev->requested_socket_id,
+  

[ovs-dev] [PATCH 2/4] netdev-dpdk: Fix dpdk_mp leak in case of EEXIST.

[Ilya Maximets]

CC: Robert Wojciechowicz 
CC: Antonio Fischetti 
Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each port.")
Fixes: b6b26021d2e2 ("netdev-dpdk: fix management of pre-existing mempools.")
Signed-off-by: Ilya Maximets 
---
 lib/netdev-dpdk.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 1e9d78f..ba6add2 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -649,6 +649,12 @@ netdev_dpdk_mempool_configure(struct netdev_dpdk *dev)
  * Update dev with the new values. */
 dev->mtu = dev->requested_mtu;
 dev->max_packet_len = MTU_TO_FRAME_LEN(dev->mtu);
+/* 'mp' should contain pointer to the mempool already owned by netdev.
+ * Otherwise something went completely wrong. */
+ovs_assert(dev->dpdk_mp);
+ovs_assert(dev->dpdk_mp->mp == mp->mp);
+/* Free the returned struct dpdk_mp because it will not be used. */
+rte_free(mp);
 return EEXIST;
 } else {
 /* A new mempool was created, release the previous one. */
-- 
2.7.4

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

[ovs-dev] [PATCH 1/4] netdev-dpdk: Fix mp_name leak on snprintf failure.

[Ilya Maximets]

CC: Robert Wojciechowicz 
CC: Antonio Fischetti 
Fixes: d555d9bded5f ("netdev-dpdk: Create separate memory pool for each port.")
Fixes: 65056fd79694 ("netdev-dpdk: manage failure in mempool name creation.")
Signed-off-by: Ilya Maximets 
---
 lib/netdev-dpdk.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c
index 82652f0..1e9d78f 100644
--- a/lib/netdev-dpdk.c
+++ b/lib/netdev-dpdk.c
@@ -505,6 +505,7 @@ dpdk_mp_name(struct dpdk_mp *dmp)
 VLOG_DBG("snprintf returned %d. Failed to generate a mempool "
 "name for \"%s\". Hash:0x%x, mtu:%d, mbufs:%u.",
 ret, dmp->if_name, h, dmp->mtu, dmp->n_mbufs);
+free(mp_name);
 return NULL;
 }
 return mp_name;
-- 
2.7.4

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

Re: [ovs-dev] [PATCH] treewide: Get rid of "echo -n", and add a test to prevent regression.

[Flavio Leitner]

On Fri, 27 Oct 2017 09:59:33 -0700
Ben Pfaff  wrote:

> "echo -n" is not POSIX and has spotty support in shells.
> 
> CC: Timothy Redaelli 
> CC: Flavio Leitner 
> Signed-off-by: Ben Pfaff 
> ---
>  Makefile.am   | 11 +++
>  tests/automake.mk |  2 +-
>  tests/ofp-print.at| 10 +-
>  tests/ofproto-dpif.at |  6 +++---
>  tutorial/ovs-sandbox  |  2 +-
>  5 files changed, 21 insertions(+), 10 deletions(-)
> 
> diff --git a/Makefile.am b/Makefile.am
> index 31d6331792af..5ceb3822c976 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -303,6 +303,17 @@ check-endian:
>   fi
>  .PHONY: check-endian
>  
> +ALL_LOCAL += check-echo-n
> +check-echo-n:
> + @if test -e $(srcdir)/.git && (git --version) >/dev/null 2>&1 && \
> +   git --no-pager grep -n 'echo'' -n' $(srcdir); \
> + then \
> +   echo "See above for uses for \"echo"" -n\", which is non-POSIX"; \
> +   echo "and does not work with all shells.  Use \"printf\" instead."; \
> +   exit 1; \
> + fi
> +.PHONY: check-echo-n
> +
>  ALL_LOCAL += thread-safety-check
>  thread-safety-check:
>   @cd $(srcdir); \
> diff --git a/tests/automake.mk b/tests/automake.mk
> index 156b40f58d7c..198ba64e5cc2 100644
> --- a/tests/automake.mk
> +++ b/tests/automake.mk
> @@ -201,7 +201,7 @@ $(valgrind_wrappers): tests/valgrind-wrapper.in
>  CLEANFILES += $(valgrind_wrappers)
>  EXTRA_DIST += tests/valgrind-wrapper.in
>  
> -VALGRIND = valgrind --log-file=valgrind.%p --leak-check=full \
> +VALGRIND = valgrind --log-file=valgrind.%p --leak-check=no \
>   --suppressions=$(abs_top_srcdir)/tests/glibc.supp \
>   --suppressions=$(abs_top_srcdir)/tests/openssl.supp --num-callers=20
>  HELGRIND = valgrind --log-file=helgrind.%p --tool=helgrind \
> diff --git a/tests/ofp-print.at b/tests/ofp-print.at


The above change seems unrelated, otherwise the patch looks good to me.
fbl



> index 50083a34d556..17682aa73db2 100644
> --- a/tests/ofp-print.at
> +++ b/tests/ofp-print.at
> @@ -1656,26 +1656,26 @@ AT_KEYWORDS([ofp-print OFPT_STATS_REPLY])
>  00 00 00 07 00 00 00 00 00 0f 42 40 "
>   tail="00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00"
>  
> - echo -n "03 13 7f 90 00 00 00 02 00 03 00 00 00 00 00 00 "
> + printf "03 13 7f 90 00 00 00 02 00 03 00 00 00 00 00 00 "
>  
>   x=0
>   printf "%02x $pad7" $x
>   printf "%s$pad32" "classifier" | od -A n -t x1 -v -N 32 | tr '\n' ' '
> - echo -n "$mid 00 00 00 01  "
> - echo -n "00 00 00 00 00 01 23 76 00 00 00 00 00 01 9e 28 "
> + printf "$mid 00 00 00 01  "
> + printf "00 00 00 00 00 01 23 76 00 00 00 00 00 01 9e 28 "
>  
>   x=1
>   while test $x -lt 254; do
> printf "%02x $pad7" $x
> printf "%s$pad32" "table$x" | od -A n -t x1 -v -N 32 | tr '\n' ' '
> -   echo -n "$mid 00 00 00 00 $tail "
> +   printf "$mid 00 00 00 00 $tail "
> x=`expr $x + 1`
>   done
>  
>   x=254
>   printf "%02x $pad7" $x
>   printf "%s$pad32" "table$x" | od -A n -t x1 -v -N 32 | tr '\n' ' '
> - echo -n "$mid 00 00 00 02 $tail") > in
> + printf "$mid 00 00 00 02 $tail") > in
>  AT_CHECK([ovs-ofctl ofp-print - < in], [0], [expout])
>  AT_CLEANUP
>  
> diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
> index c75a1acbf906..583907b9aa0d 100644
> --- a/tests/ofproto-dpif.at
> +++ b/tests/ofproto-dpif.at
> @@ -7333,7 +7333,7 @@ m4_define([OFPROTO_DPIF_GET_FLOW],
> set Open_vSwitch . other_config:max-idle=1], [], 
> [],
> [m4_if([$1], [], [], 
> [--dummy-numa="0,0,0,0,1,1,1,1"])])
>  
> -   func=`echo -n "$1_" | cut -c 4-`
> +   func=`printf '%s_' "$1" | cut -c 4-`
> add_${func}of_ports br0 1 2
>  
> AT_CHECK([ovs-appctl netdev-dummy/receive p1 
> 'in_port(1),eth(src=50:54:00:00:00:05,dst=50:54:00:00:00:07),eth_type(0x0800),ipv4(src=192.168.0.1,dst=192.168.0.2,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
> @@ -7665,7 +7665,7 @@ m4_define([OFPROTO_DPIF_MEGAFLOW_NORMAL],
>[AT_SETUP([ofproto-dpif megaflow - normal$1])
> OVS_VSWITCHD_START([], [], [], [m4_if([$1], [], [], 
> [--dummy-numa="0,0,0,0,1,1,1,1"])])
> AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> -   func=`echo -n "$1_" | cut -c 4-`
> +   func=`printf '%s_' "$1" | cut -c 4-`
> add_${func}of_ports br0 1 2
> AT_CHECK([ovs-ofctl add-flow br0 action=normal])
> AT_CHECK([ovs-appctl netdev-dummy/receive p1 
> 'in_port(1),eth(src=50:54:00:00:00:09,dst=50:54:00:00:00:0a),eth_type(0x0800),ipv4(src=10.0.0.2,dst=10.0.0.1,proto=1,tos=0,ttl=64,frag=no),icmp(type=8,code=0)'])
> @@ -8068,7 +8068,7 @@ m4_define([OFPROTO_DPIF_MEGAFLOW_DISABLED],
>[AT_SETUP([ofproto-dpif megaflow - disabled$1])
> OVS_VSWITCHD_START([], [], [], [m4_if([$1], [], [], 
> [--dummy-numa="0,0,0,0,1,1,1,1"])])
> AT_CHECK([ovs-appctl vlog/set dpif:dbg dpif_netdev:dbg])
> -   func=`echo -n "$1_" | cut -c 4-`
> +   func=`printf '%s_' "$1" | cut -c 4-`
> add_${func}of_ports br0 1 2
>

[ovs-dev] [PATCH 0/2] xlate: optimize dp flow action in case of error in multi-bridge setup

[Zoltan Balogh]

If several bridges are connected via patch ports or the packet is sent
via tunnel and an error occurs during translation in a subsequent bridge,
e.g. Ethernet header cannot be pushed to the packet due to lack of
prerequisites in the second bridge and there were flow actions already
translated in first bridge, it can happen that the installed datapath
flow contains actions which are going to be performed for all received
packets, however the packets are going to be dropped in the end. This
is waste of processing resource.

There are two points in the code where we can fix such datapath flows.
1) During translation of OF actions on a bridge, we can store the last
valid state of translated actions while iterating over the OF actions
and revert to it in case of error. This can be performed in the
do_xlate_actions() funtion.
2) When all OF actions have been translated, there could be actions at
the end of list of odp actions which are not needed to be executed.
So, the list can be normalized at the end of xlate_actions().

Let's have a config like this and inject L2 Ethernet packets into br0
via LOCAL port:

   ->-+
  | LOCAL  LOCAL   LOCAL
+-o---+   +---o-+   +-+   +---o-+
|   br0   |   |   br1   |   |   br2   |   |   br3   |
+---o-+   +-o-o-+   +-o-o-+   +-o---+
p01 |   p10 | | p12   p21 | | p23   p32 |
+---+ +---+ +---+

netdev@ovs-netdev: hit:17754528 missed:57
br0:
br0 65534/1: (tap)
p01 10/none: (patch: peer=p10)
br1:
br1 65534/2: (tap)
p10 20/none: (patch: peer=p01)
p12 30/none: (patch: peer=p21)
br2:
br2 65534/3: (tap)
p21 40/none: (patch: peer=p12)
p23 50/none: (patch: peer=p32)
br3:
br3 65534/4: (tap)
p32 60/none: (patch: peer=p23)

- OpenFlow rules:

"br0"
OFPST_FLOW reply (OF1.3) (xid=0x2):
cookie=0x0, duration=15.601s, table=0, n_packets=20739, n_bytes=2032422,
reset_counts in_port=LOCAL actions=dec_ttl,output:10

"br1"
OFPST_FLOW reply (OF1.3) (xid=0x2):
cookie=0x0, duration=15.568s, table=0, n_packets=20739, n_bytes=2032422,
reset_counts ip,in_port=20
actions=dec_ttl,dec_ttl,dec_ttl,dec_ttl,LOCAL,
set_field:ee:ee:ee:ff:ff:01->eth_dst,output:30

"br2"
OFPST_FLOW reply (OF1.3) (xid=0x2):
cookie=0x0, duration=15.536s, table=0, n_packets=20739, n_bytes=2032422,
ip,in_port=40
actions=dec_ttl,dec_ttl,dec_ttl,dec_ttl,dec_ttl,
set_field:aa:aa:aa:bb:bb:ff->eth_src,encap(ethernet),dec_ttl,output:50

"br3"
OFPST_FLOW reply (OF1.3) (xid=0x2):
cookie=0x0, duration=15.502s, table=0, n_packets=0, n_bytes=0,
reset_counts in_port=60 actions=LOCAL

- Installed datapath flow:

flow-dump from non-dpdk interfaces:
recirc_id(0),in_port(1),packet_type(ns=0,id=0),
eth(src=aa:dd:dd:ee:ee:f9,dst=aa:aa:aa:bb:bb:00),eth_type(0x0800),
ipv4(ttl=255,frag=no), packets:20738, bytes:2032324, used:5.176s,
actions:set(ipv4(ttl=250)),2,
set(eth(src=aa:aa:aa:bb:bb:ff,dst=ee:ee:ee:ff:ff:01)),set(ipv4(ttl=245))

In this case, encap(ethernet) action cannot be performed, because of the
packet is L2 Ethernet packet. By looking at the datapath flow, you can
see, that setting MAC addresses and TTL at the end is actually not needed,
since the packet is not transmitted anywhere after these actions.
With this series, these actions are omitted. 

Zoltan Balogh (2):
  xlate: rollback to valid known state in case of ctx->error on
translation
  xlate: normalize the actions after translation

 ofproto/ofproto-dpif-xlate.c | 178 +
 tests/ofproto-dpif.at| 227 +++
 2 files changed, 405 insertions(+)

-- 
1.9.1

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

[ovs-dev] [PATCH 1/2] xlate: rollback to valid known state in case of ctx->error on translation

[Zoltan Balogh]

During translation of OF actions on a bridge, we can store the last
valid state of translated actions while iterating over the OF actions
and revert to it in case of error. This can be performed in the
do_xlate_actions() funtion.

Signed-off-by: Zoltan Balogh 
Signed-off-by: Sugesh Chandran 
Co-authored-by: Sugesh Chandran 
Tested-by: Sugesh Chandran 
---
 ofproto/ofproto-dpif-xlate.c | 119 
 tests/ofproto-dpif.at| 179 +++
 2 files changed, 298 insertions(+)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index ddcaf05..0cc59e7 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6147,6 +6147,107 @@ xlate_ofpact_unroll_xlate(struct xlate_ctx *ctx,
  "cookie=%#"PRIx64, a->rule_table_id, a->rule_cookie);
 }
 
+struct xlate_backup_data {
+size_t odp_actions_size;
+struct flow wc_masks;
+struct flow flow;
+struct flow base_flow;
+};
+
+static void
+store_ctx_xlate_data(struct xlate_backup_data *data,
+ const struct xlate_ctx *ctx)
+{
+data->odp_actions_size = ctx->odp_actions->size;
+data->wc_masks = ctx->wc->masks;
+data->flow = ctx->xin->flow;
+data->base_flow = ctx->base_flow;
+}
+
+static void
+restore_ctx_xlate_data(struct xlate_ctx *ctx,
+   const struct xlate_backup_data *data)
+{
+ctx->odp_actions->size = data->odp_actions_size;
+ctx->wc->masks = data->wc_masks;
+ctx->xin->flow = data->flow;
+ctx->base_flow = data->base_flow;
+}
+
+static bool
+ofpact_validates_dp_actions(const struct ofpact *a)
+{
+switch (a->type) {
+/* Output. */
+case OFPACT_OUTPUT:
+case OFPACT_GROUP:
+case OFPACT_CONTROLLER:
+case OFPACT_ENQUEUE:
+case OFPACT_OUTPUT_REG:
+case OFPACT_BUNDLE:
+/* Metadata. */
+case OFPACT_SET_TUNNEL:
+case OFPACT_SET_QUEUE:
+case OFPACT_POP_QUEUE:
+case OFPACT_FIN_TIMEOUT:
+/* Flow table interaction. */
+case OFPACT_RESUBMIT:
+case OFPACT_LEARN:
+case OFPACT_CONJUNCTION:
+/* Arithmetic. */
+case OFPACT_MULTIPATH:
+/* Other. */
+case OFPACT_NOTE:
+case OFPACT_EXIT:
+case OFPACT_SAMPLE:
+case OFPACT_UNROLL_XLATE:
+case OFPACT_CT:
+case OFPACT_CT_CLEAR:
+case OFPACT_NAT:
+case OFPACT_OUTPUT_TRUNC:
+case OFPACT_CLONE:
+/* Instructions. */
+case OFPACT_METER:
+case OFPACT_CLEAR_ACTIONS:
+case OFPACT_WRITE_ACTIONS:
+case OFPACT_WRITE_METADATA:
+case OFPACT_GOTO_TABLE:
+return true;
+/* Header changes. */
+case OFPACT_SET_FIELD:
+case OFPACT_SET_VLAN_VID:
+case OFPACT_SET_VLAN_PCP:
+case OFPACT_STRIP_VLAN:
+case OFPACT_PUSH_VLAN:
+case OFPACT_SET_ETH_SRC:
+case OFPACT_SET_ETH_DST:
+case OFPACT_SET_IPV4_SRC:
+case OFPACT_SET_IPV4_DST:
+case OFPACT_SET_IP_DSCP:
+case OFPACT_SET_IP_ECN:
+case OFPACT_SET_IP_TTL:
+case OFPACT_SET_L4_SRC_PORT:
+case OFPACT_SET_L4_DST_PORT:
+case OFPACT_REG_MOVE:
+case OFPACT_STACK_PUSH:
+case OFPACT_STACK_POP:
+case OFPACT_DEC_TTL:
+case OFPACT_SET_MPLS_LABEL:
+case OFPACT_SET_MPLS_TC:
+case OFPACT_SET_MPLS_TTL:
+case OFPACT_DEC_MPLS_TTL:
+case OFPACT_PUSH_MPLS:
+case OFPACT_POP_MPLS:
+/* Generic encap & decap */
+case OFPACT_ENCAP:
+case OFPACT_DECAP:
+/* Debugging actions. */
+case OFPACT_DEBUG_RECIRC:
+default:
+return false;
+}
+}
+
 static void
 do_xlate_actions(const struct ofpact *ofpacts, size_t ofpacts_len,
  struct xlate_ctx *ctx, bool is_last_action)
@@ -6154,6 +6255,7 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 struct flow_wildcards *wc = ctx->wc;
 struct flow *flow = >xin->flow;
 const struct ofpact *a;
+struct xlate_backup_data xlate_data;
 
 if (ovs_native_tunneling_is_on(ctx->xbridge->ofproto)) {
 tnl_neigh_snoop(flow, wc, ctx->xbridge->name);
@@ -6165,6 +6267,9 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 return;
 }
 
+/* Initialize xlate_data. */
+store_ctx_xlate_data(_data, ctx);
+
 OFPACT_FOR_EACH (a, ofpacts, ofpacts_len) {
 struct ofpact_controller *controller;
 const struct ofpact_metadata *metadata;
@@ -6172,8 +6277,11 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 
ofpacts_len,
 const struct mf_field *mf;
 bool last = is_last_action && ofpact_last(a, ofpacts, ofpacts_len)
 && ctx->action_set.size;
+size_t old_size;
 
 if (ctx->error) {
+/* Restore xlate data. */
+restore_ctx_xlate_data(ctx, _data);
 break;
 }
 
@@ -6196,6 +6304,8 @@ do_xlate_actions(const struct ofpact *ofpacts, size_t 

[ovs-dev] [PATCH 2/2] xlate: normalize the actions after translation

[Zoltan Balogh]

When all OF actions have been translated, there could be actions at
the end of list of odp actions which are not needed to be executed.
So, the list can be normalized at the end of xlate_actions().

Signed-off-by: Zoltan Balogh 
Signed-off-by: Sugesh Chandran 
Co-authored-by: Sugesh Chandran 
Tested-by: Sugesh Chandran 
---
 ofproto/ofproto-dpif-xlate.c | 59 
 tests/ofproto-dpif.at| 48 +++
 2 files changed, 107 insertions(+)

diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c
index 0cc59e7..5f3e5f8 100644
--- a/ofproto/ofproto-dpif-xlate.c
+++ b/ofproto/ofproto-dpif-xlate.c
@@ -6938,6 +6938,62 @@ xlate_wc_finish(struct xlate_ctx *ctx)
 }
 }
 
+/* Returns true if the action stored in 'nla' can be a valid last action of a
+ * datapath flow. */
+static bool
+is_valid_last_action(struct nlattr *nla)
+{
+switch (nl_attr_type(nla)) {
+case OVS_ACTION_ATTR_USERSPACE:
+case OVS_ACTION_ATTR_SAMPLE:
+case OVS_ACTION_ATTR_TRUNC:
+case OVS_ACTION_ATTR_RECIRC:
+case OVS_ACTION_ATTR_TUNNEL_PUSH:
+case OVS_ACTION_ATTR_TUNNEL_POP:
+case OVS_ACTION_ATTR_OUTPUT:
+case OVS_ACTION_ATTR_CLONE:
+case OVS_ACTION_ATTR_CT:
+return true;
+default:
+return false;
+}
+}
+
+/* Returns offset of last netlink attribute storing valid action in array
+ * 'data'. Execution of actions beyond this last attribute does not make sense.
+ */
+static size_t
+last_action_offset(struct nlattr *data, const size_t data_len)
+{
+uint16_t left;
+struct nlattr *a, *b = NULL;
+
+NL_ATTR_FOR_EACH (a, left, data, data_len) {
+if (is_valid_last_action(a)) {
+b = a;
+}
+}
+
+if (b) {
+return NLA_ALIGN(((char *)b - (char *)data) + b->nla_len);
+} else {
+return 0;
+}
+}
+
+static void
+normalize_odp_actions(struct xlate_ctx *ctx)
+{
+struct nlattr *data = ctx->odp_actions->data;
+size_t size = ctx->odp_actions->size;
+size_t new_size = last_action_offset(data, size);
+
+/* Get rid of any unneeded actions at the tail end. */
+if (OVS_UNLIKELY(new_size != size)) {
+ctx->odp_actions->size = new_size;
+}
+}
+
 /* Translates the flow, actions, or rule in 'xin' into datapath actions in
  * 'xout'.
  * The caller must take responsibility for eventually freeing 'xout', with
@@ -7343,7 +7399,10 @@ exit:
 if (xin->odp_actions) {
 ofpbuf_clear(xin->odp_actions);
 }
+} else {
+normalize_odp_actions();
 }
+
 return ctx.error;
 }
 
diff --git a/tests/ofproto-dpif.at b/tests/ofproto-dpif.at
index 1124f0e..d9f0432 100644
--- a/tests/ofproto-dpif.at
+++ b/tests/ofproto-dpif.at
@@ -10133,3 +10133,51 @@ AT_CHECK([grep "Datapath actions" stdout], [0],
 
 OVS_VSWITCHD_STOP
 AT_CLEANUP
+
+AT_SETUP([ofproto-dpif - xlate error - normalize actions])
+
+#  ->-+
+# | p1
+#   +-o---+
+#   |   br0   |
+#   +-o-o-+
+#   patch0| |patch1
+# +-->--+
+
+OVS_VSWITCHD_START([dnl
+-- add-port br0 patch0 \
+-- set interface patch0 type=patch options:peer=patch1 ofport_request=10 \
+-- add-port br0 patch1 \
+-- set interface patch1 type=patch options:peer=patch0 ofport_request=20
+])
+
+AT_CHECK([
+ovs-vsctl add-port br0 p1 -- set interface p1 type=dummy ofport_request=1
+])
+
+AT_CHECK([
+ovs-appctl dpif/show | grep -v hit | sed "s/$(printf \\t)//g" | sed 
's./[[0-9]]\{1,\}..'
+], [0], [dnl
+br0:
+br0 65534: (dummy-internal)
+p1 1: (dummy)
+patch0 10/none: (patch: peer=patch1)
+patch1 20/none: (patch: peer=patch0)
+])
+
+# Error due to pushing too many MPLS labels.
+AT_CHECK([
+ovs-ofctl del-flows br0
+ovs-ofctl add-flow br0 
"table=0,in_port=1,actions=mod_dl_src:aa:aa:aa:bb:bb:00,goto_table:1" 
-OOpenFlow13
+ovs-ofctl add-flow br0 "table=0,in_port=patch1,actions=goto_table:2" 
-OOpenFlow13
+ovs-ofctl add-flow br0 
"table=1,actions=push_vlan:0x8100,set_field:4196->vlan_vid,output:patch0" 
-OOpenFlow13
+ovs-ofctl add-flow br0 "table=2,actions=push_mpls:0x8847,output:patch0"
+], [0])
+
+AT_CHECK([ovs-appctl ofproto/trace br0 'in_port=1,ip'], [0], [stdout])
+AT_CHECK([tail -1 stdout], [0],
+  [Datapath actions: drop
+])
+
+OVS_VSWITCHD_STOP
+AT_CLEANUP
-- 
1.9.1

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

Re: [ovs-dev] DNS support feature (was: Re: DNS support options)

[Miguel Angel Ajo Pelayo]

I don't believe rounding up TTLs would be a good practice. The
administrators
are aware of the risks of having a very low TTL, so if they decide to pick
a low TTL they may have good reasons (like the possibility of a very close
event of the IP being moved, or some sort of failover mechanism, or a
way for dealing with dynamic dns). If you round it up you add unexpected
extra downtime.



On Fri, Oct 27, 2017 at 6:38 PM, Yifeng Sun  wrote:

> Thanks a lot. I will keep your guidance in mind.
>
> On Fri, Oct 27, 2017 at 9:29 AM, Mark Michelson 
> wrote:
>
> > Usually the software that performs DNS lookups and caches results is
> > referred to as a "DNS forwarder". You configure resolv.conf's nameserver
> as
> > 127.0.0.1. This way, DNS queries go to the DNS forwarder running on
> > localhost. The forwarder then has real nameservers configured to send the
> > queries to. The forwarder receives the DNS response (or lack of response
> if
> > the nameserver is unreachable), caches the result, and sends the result
> > back to the system resolver. From the perspective of OVS, all it is doing
> > is performing a simple DNS lookup. Further DNS lookups hit the forwarder,
> > which has cached the previous queries and can immediately return the
> cached
> > records (or cached failure).
> >
> > One common DNS forwarder that works this way is dnsmasq. There are likely
> > lots of others. I believe that bind can be configured to act as a DNS
> > forwarder as well. I recommend searching for "DNS forwarder" and seeing
> > what you can find. The common thread here, though, is that OVS would not
> > actually package or depend on a DNS forwarder. Rather, we would add
> > documentation that strongly suggests that applications that consider DNS
> > resolution to be critical configure their favorite DNS forwarder on
> systems
> > running OVS.
> >
> > Since we are looking into using a third party library for DNS resolution,
> > it may also be worth looking into what sort of caching those libraries
> > provide. I am not familiar with that information off the top of my head.
> >
> > Mark
> >
> > On Fri, Oct 27, 2017 at 11:14 AM Yifeng Sun 
> > wrote:
> >
> >> Mark, thanks a lot for the detailed and thorough explanation.
> >>
> >> Do you happen to know any other projects that we can take a peek?
> >>
> >> On Fri, Oct 27, 2017 at 8:57 AM, Mark Michelson 
> >> wrote:
> >>
> >>> Yep, that makes good sense. I'd recommend having some min and max
> >>> threshold though. That way, if a record has a TTL of multiple days,
> you can
> >>> round that down to something more reasonable. Similarly, a
> ridiculously low
> >>> TTL can be rounded up.
> >>>
> >>> There's another aspect to DNS caching that I briefly mentioned in my
> >>> last e-mail: negative caching. Consider that you attempt to look up
> >>> exampledomain.com, and for some reason, you either can't reach your
> DNS
> >>> server or the DNS server returns NXDOMAIN (or some other error). Each
> >>> additional lookup of exampledomain.com will likely hit this same
> >>> problem until either you can restore the link to your DNS server or the
> >>> proper records get added to DNS. If exampledomain.com is a frequent
> >>> destination for traffic, you don't want to be doing full DNS lookups
> of it
> >>> each time, just to find out you can't send to it. This is especially
> true
> >>> in the case that the DNS server is unreachable. resolv.conf by default
> will
> >>> wait 5 seconds on an attempt and will attempt the query twice before
> giving
> >>> up. This means each DNS query will take 10 seconds just to ultimately
> fail.
> >>> This can lead to large backlogs of queries. With negative caching, you
> >>> cache the fact that an attempt to reach exampledomain.com failed, and
> >>> for some amount of time, any further attempts to query that domain will
> >>> immediately fail rather than attempting the query again.
> >>>
> >>> I mention negative caching because in some ways, it's more important
> >>> than positive caching. I've seen certain applications get completely
> >>> crippled by having the DNS server unavailable since they'll keep
> attempting
> >>> to perform DNS lookups for domains that will ultimately fail. Caching
> >>> positive results based on TTL of records won't help in a case like
> this.
> >>>
> >>> I'm not going to take a hard-line stand that DNS caching absolutely
> >>> should not be added to OVS. But I point out negative caching as one of
> >>> those things that may seem non-obvious at first but that is very
> important.
> >>> If we can implement it successfully, then that is fantastic. But I just
> >>> know that other projects have already gone through the pains of trying
> to
> >>> implement this properly and if we can piggyback on their efforts, we'll
> >>> probably end up happier in the long run.
> >>>
> >>> Mark
> >>>
> >>> PS There's actually an RFC dedicated to negative caching of 

Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface Information Elements to flow key

[Weglicki, MichalX]

Hello, 

Can we proceed further with this patch if there are no more comments? 

Thank you in advance. 

Br, 
Michal. 

> -Original Message-
> From: Weglicki, MichalX
> Sent: Thursday, October 12, 2017 10:03 AM
> To: d...@openvswitch.org
> Cc: Greg Rose ; Weglicki, MichalX 
> 
> Subject: RE: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface 
> Information Elements to flow key
> 
> Hello,
> 
> Are there are any other comments?
> 
> Thank you all in advance.
> 
> Br,
> Michal.
> 
> > -Original Message-
> > From: ovs-dev-boun...@openvswitch.org 
> > [mailto:ovs-dev-boun...@openvswitch.org] On Behalf Of Weglicki, MichalX
> > Sent: Wednesday, October 4, 2017 10:19 AM
> > To: Greg Rose ; d...@openvswitch.org
> > Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface 
> > Information Elements to flow key
> >
> > > -Original Message-
> > > From: Greg Rose [mailto:gvrose8...@gmail.com]
> > > Sent: Wednesday, October 4, 2017 12:21 AM
> > > To: Weglicki, MichalX ; d...@openvswitch.org
> > > Subject: Re: [ovs-dev] [PATCH v5 2/2] ofproto-dpif-ipfix: add interface 
> > > Information Elements to flow key
> > >
> > > On 10/02/2017 07:49 AM, Michal Weglicki wrote:
> > > > Extend flow key part of data record to include following Information 
> > > > Elements:
> > > > - ingressInterface
> > > > - ingressInterfaceType
> > > > - egressInterface
> > > > - egressInterfaceType
> > > > - interfaceName
> > > > - interfaceDescription
> > > >
> > > > In case of input sampling we don't have information about egress port.
> > > > Define templates depending not only on protocol types, but also on flow
> > > > direction. Only egress flow will include egress information elements.
> > > >
> > > > With this change, dpif_ipfix_exporter stores every port in hmap rather
> > > > than only tunnel ports. It allows to easily retrieve required
> > > > information about interfaces during sampling upcalls.
> > > >
> > > > v1->v2
> > > > * Add interfaceType and interfaceDescription
> > > > * Rework ipfix_get_iface_data_record function
> > > > v2->v3: Code rebase.
> > > > v3->v4: Minor comments applied.
> > > > v4->v5: Clang complation problem fix.
> > > >
> > > > Co-authored-by: Michal Weglicki 
> > > > Signed-off-by: Michal Weglicki 
> > > > Signed-off-by: Przemyslaw Szczerbik 
> > >
> > > Michal and Przemyslaw,
> > >
> > > There is one more small nit pick that I came across noted below.
> > >
> > > Other than that
> > >
> > > Tested-by: Greg Rose 
> > > Reviewed-by: Greg Rose 
> > >
> > > > ---
> > > >   ofproto/ofproto-dpif-ipfix.c | 358 
> > > > +++
> > > >   ofproto/ofproto-dpif-ipfix.h |   6 +-
> > > >   ofproto/ofproto-dpif-xlate.c |   4 +-
> > > >   ofproto/ofproto-dpif.c   |  19 +--
> > > >   4 files changed, 277 insertions(+), 110 deletions(-)
> > > >
> > > > diff --git a/ofproto/ofproto-dpif-ipfix.c b/ofproto/ofproto-dpif-ipfix.c
> > > > index 472c272..138c325 100644
> > > > --- a/ofproto/ofproto-dpif-ipfix.c
> > > > +++ b/ofproto/ofproto-dpif-ipfix.c
> > > > @@ -115,11 +115,12 @@ struct dpif_ipfix_global_stats {
> > > >   };
> > > >
> > > >   struct dpif_ipfix_port {
> > > > -struct hmap_node hmap_node; /* In struct dpif_ipfix's 
> > > > "tunnel_ports" hmap. */
> > > > +struct hmap_node hmap_node; /* In struct dpif_ipfix's "ports" 
> > > > hmap. */
> > > >   struct ofport *ofport;  /* To retrieve port stats. */
> > > >   odp_port_t odp_port;
> > > >   enum dpif_ipfix_tunnel_type tunnel_type;
> > > >   uint8_t tunnel_key_length;
> > > > +uint32_t ifindex;
> > > >   };
> > > >
> > > >   struct dpif_ipfix_exporter {
> > > > @@ -157,9 +158,9 @@ struct dpif_ipfix_flow_exporter_map_node {
> > > >   struct dpif_ipfix {
> > > >   struct dpif_ipfix_bridge_exporter bridge_exporter;
> > > >   struct hmap flow_exporter_map;  /* 
> > > > dpif_ipfix_flow_exporter_map_node. */
> > > > -struct hmap tunnel_ports;   /* Contains "struct 
> > > > dpif_ipfix_port"s.
> > > > - * It makes tunnel port lookups 
> > > > faster in
> > > > - * sampling upcalls. */
> > > > +struct hmap ports;  /* Contains "struct 
> > > > dpif_ipfix_port"s.
> > > > + * It makes port lookups faster in 
> > > > sampling
> > > > + * upcalls. */
> > > >   struct ovs_refcount ref_cnt;
> > > >   };
> > > >
> > > > @@ -293,7 +294,8 @@ BUILD_ASSERT_DECL(sizeof(struct 
> > > > ipfix_template_field_specifier) == 8);
> > > >   /* Cf. IETF RFC 5102 Section 5.11.6. */
> > > >   enum ipfix_flow_direction {
> > > >   INGRESS_FLOW = 0x00,
> > > > -EGRESS_FLOW