Re: [ovs-dev] [PATCH v2 5/5] netdev-dpdk: Add OVS_UNLIKELY annotations in dpdk_do_tx_copy().

You are right, turns out this was just an anomaly in performance. Please ignore the comment in the commit message. I still recommend this be pushed as these annotations also exist in netdev_dpdk_send() for similar functionality. (Again, remove the comment on performance in my commit message if you

Re: [ovs-dev] [PATCH v2 1/5] netdev-dpdk: Refactor dpdk_queue_flush().

I'll add the inline to both dpdk_queue_flush() and dpdk_queue_flush__() and repost this patch. Cheers, Ryan On 6/30/14 9:52 AM, "Daniele Di Proietto" wrote: >I don¹t know if we want to keep the inline attribute here (like Pravin >said for commit 4/5). > >Otherwise LGTM > >Daniele > >On Jun 26,

Re: [ovs-dev] [PATCH v2 4/5] netdev-dpdk: Remove inline from static function dpdk_queue_pkts().

Sorry about this, I should've been more mindful of that. Please ignore this patch. Ryan On 6/27/14 2:04 PM, "Ben Pfaff" wrote: >On Fri, Jun 27, 2014 at 1:58 PM, Pravin Shelar wrote: >> On Thu, Jun 26, 2014 at 6:16 PM, Ryan Wilson wrote: >>> Per style requirements, functions local to a module

Re: [ovs-dev] [PATCH v2 5/5] netdev-dpdk: Add OVS_UNLIKELY annotations in dpdk_do_tx_copy().

Subject: Re: [ovs-dev] [PATCH v2 5/5] netdev-dpdk: Add OVS_UNLIKELY annotations in dpdk_do_tx_copy(). .4 million or .04 million? There's a big difference. On Jun 26, 2014 6:24 PM, "Ryan Wilson 76511" mailto:wr...@vmware.com>> wrote: Its between 0.2 - 0.6 million PPS incr

Re: [ovs-dev] [PATCH v2 5/5] netdev-dpdk: Add OVS_UNLIKELY annotations in dpdk_do_tx_copy().

Its between 0.2 - 0.6 million PPS increase after running 3 tests with and without this patch. So I went with the average of 0.4 :) And we actually use these annotations elsewhere in netdev_dpdk_send() where we measure size of packets and dropped packets, so it would be nice to add these annotat

Re: [ovs-dev] [PATCH 3/3] netdev-dpdk: Fix performance issues / bugs in dpdk_do_tx_copy().

I'll submit as 3 separate patches. The reducing locking is really a byproduct of the bug fix, so its unnecessary to add performance measurements to that. I'll add performance measurements for the OVS_UNLIKELY annotations (if they do in fact help as I predict). Ryan On 6/26/14 5:48 PM, "Ethan Ja

Re: [ovs-dev] [PATCH v3] dpif-netdev: Polling threads directly call ofproto upcall functions.

So I added OVS_NO_THREAD_SAFETY_ANALYSIS to create_dp_netdev() and removed OVS_NO_THREAD_SAFETY_ANALYSIS annotations on dpif_netdev_disable_upcall() and dpif_netdev_enable_upcall(). However, I still get a warning from sparse about dp->upcall_rwlock being locked / unlocked at the end of dpif_netdev_

Re: [ovs-dev] [PATCH] vswitchd: skip right number of arguments in dpdk_init()

Sent out a new version of the patch with the correct commit message and signed-off-bys. Ryan On 6/23/14 1:57 PM, "Pravin Shelar" wrote: >I like this better, If you send me signed off line I will merge it. > > >On Thu, Jun 19, 2014 at 5:27 PM, Ryan Wilson 76511 >wrot

Re: [ovs-dev] [PATCH] dpif-netdev: Implement batched flow dumping.

Ok just kidding, I actually just sent a v3 of the patch since the other optimizations may take me a bit longer. Sorry for the spam, have a good weekend! Ryan From: Ryan Wilson mailto:wr...@vmware.com>> Date: Friday, June 20, 2014 4:15 PM To: Ryan Wilson mailto:wr...@nicira.com>>, Joe Stringer m

Re: [ovs-dev] [PATCH] dpif-netdev: Implement batched flow dumping.

Hey Joe, Not sure if you were looking at this patch, but I believe I found a more efficient way to do this. I will likely send this out as a larger series of revalidator optimizations for dpif-netdev. Ryan From: Ryan Wilson mailto:wr...@nicira.com>> Date: Thursday, June 19, 2014 5:49 PM To: Jo

Re: [ovs-dev] [PATCH] vswitchd: skip right number of arguments in dpdk_init()

MHO, we should push this (my patch fixes something, at least) and worry about the wrong naming problem later What do you think? Daniele On Jun 19, 2014, at 3:02 PM, Ryan Wilson 76511 mailto:wr...@vmware.com>> wrote: I did a bit of research and I found out what's happening here. Here&

Re: [ovs-dev] [PATCH] vswitchd: skip right number of arguments in dpdk_init()

the code always skips by default the first of the remaining arguments. With DPDK, its "--" but without DPDK, its the program name. So dpdk_init() actually does skip the program name, I believe. Anyways it doesn't really matter as long as this works. Ryan On 6/19/14 2:14 PM, "Pr

Re: [ovs-dev] [PATCH] vswitchd: skip right number of arguments in dpdk_init()

Well we're really not 'skipping' the '--dpdk' argument since that is passed to rte_eal_init() as well. We're skipping the program name which is the path to ovs-vswitchd. I'd change the comment in the patch to something like: diff --git a/lib/netdev-dpdk.c b/lib/netdev-dpdk.c index fbdb6b3..5cd4a07

Re: [ovs-dev] [PATCH v2] dpif-netdev: Polling threads directly call ofproto upcall functions.

Thanks for the review! I fixed all these things in my next version (v3). Ryan From: Daniele Di Proietto mailto:ddiproie...@vmware.com>> Date: Wednesday, June 18, 2014 9:05 AM To: Ryan Wilson mailto:wr...@nicira.com>> Cc: "dev@openvswitch.org" mailto:dev@openvswitch.o

Re: [ovs-dev] [PATCH v2] dpif-netdev: Polling threads directly call ofproto upcall functions.

I think the key issue here is that Mike believes the limiting factor for DPDK is CPU while Ethan believes it to be memory (transferring memory between cores, to be precise). I would agree Mike is right where there is a heavy load (i.e. 100% utilization of 10 GbE links) while Ethan is right in the a

Re: [ovs-dev] [PATCH] dpif-netdev: Remove an extra memcpy of packet data from datapath-upcall interface for bridges with datapath_type=netdev.

ning b is a >> copy of the header and thus we can free the original header). I figured >> I'd let you check out the patch and see if you had any other ideas. >> > >An assert to this effect would help detect/avoid bugs. > >> Cheers, >> >> Ryan >>

Re: [ovs-dev] [PATCH v2] timeval: Add monotonic time functionality for NetBSD and FreeBSD.

Thanks Yamamoto! Feel free to push this if you think its ready. Ryan From: YAMAMOTO Takashi Sent: Tuesday, June 3, 2014 7:20 PM To: wr...@nicira.com Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH v2] timeval: Add monotonic time functionality for Ne

Re: [ovs-dev] [PATCH] dpif-netdev: Remove an extra memcpy of packet data from datapath-upcall interface for bridges with datapath_type=netdev.

r and thus we can free the original header). I figured I'd let you check out the patch and see if you had any other ideas. Cheers, Ryan On 6/3/14 5:22 PM, "Pravin Shelar" wrote: >Can you combine it with second patch, otherwise it introduces a bug. > >On Tue, Jun 3,

Re: [ovs-dev] [PATCH] dpif-netdev: Remove an extra memcpy of packet data from datapath-upcall interface for bridges with datapath_type=netdev.

I've ran into some unexpected issues while perf testing this, lets hold off on looking at this. I'll submit another patch when I've had all the kinks worked out. Ryan On 6/3/14 2:21 PM, "Ryan Wilson 76511" wrote: >Hey Pravin, > >Thanks for the catch her

Re: [ovs-dev] [PATCH] dpif-netdev: Remove an extra memcpy of packet data from datapath-upcall interface for bridges with datapath_type=netdev.

Hey Pravin, Thanks for the catch here! Turns out the header is already tracked in DPDK with rte_mbuf's buffer address - sizeof(ofpbuf). Thus, I submitted another patch that, in free_dpdk_buf(), always gets this header and uses this to free memory. http://patchwork.openvswitch.org/patch/4375/ Le

Re: [ovs-dev] [PATCH] dpif-netdev: Remove an extra memcpy of packet data from datapath-upcall interface for bridges with datapath_type=netdev.

Hey Jarno, I didn't consider that possibility, but that is a much better solution which makes for a much cleaner patch. I just posted a new patch using this direct pointer assignment. Looks like I'm a bit rusty on my C :P Thanks for the review! Ryan On 6/3/14 10:10 AM, "Jarno Rajahalme" wrote:

Re: [ovs-dev] [PATCH] timeval: Add monotonic time functionality for NetBSD and FreeBSD.

Thanks for the fix! Sorry, I don't know too much about the intricacies of NetBSD. My first version seemed to work on my NetBSD instance though, so I wrongly assumed it would work for most versions of NetBSD. I tested this on FreeBSD and it works, but I don't know the intricacies of FreeBSD either.

Re: [ovs-dev] [PATCH v3] timeval: Use monotonic time in OVS Python timeval library.

Hey Yamamoto, You're right, this is too fragile. This patch I just posted checks the system's platform, so we don't run into situations like this (defaults to using time.time() if its not Linux, NetBSD or FreeBSD). It also adds CLOCK_MONOTONIC compatibility for NetBSD and FreeBSD, as well as updat

Re: [ovs-dev] [PATCH] rtbsd: Make rtbsd module thread-safe.

Sorry for the delay, I swear I haven't been ignoring this email. Ok finally set up my NetBSD environment and master (with my rtbsd change) compiles and passes all unit tests. Ryan From: Ryan Wilson mailto:wr...@vmware.com>> Date: Thursday, May 29, 2014 10:47 PM To: Ben Pfaff mailto:b...@nicira.

Re: [ovs-dev] [PATCH v3] timeval: Use monotonic time in OVS Python timeval library.

Hey Yamamoto, Sorry for the delay, I finally set up my NetBSD environment and tested this script. You're right, but NetBSD will actually return an error on clock_gettime because of this argument difference. Thus, NetBSD will continue to use time.time(), which is as expected. I can add NetBSD func

Re: [ovs-dev] [PATCH] rtbsd: Make rtbsd module thread-safe.

Working on setting up a NetBSD environment now, will report back with the results soon. Ryan From: Ben Pfaff mailto:b...@nicira.com>> Date: Thursday, May 29, 2014 10:37 PM To: Alex Wang mailto:al...@nicira.com>> Cc: YAMAMOTO Takashi mailto:yamam...@valinux.co.jp>>, Ryan Wilson mailto:wr...@nici

Re: [ovs-dev] [PATCH] route-table: Make route-table module thread-safe.

You're right, it was slightly odd. Fixed in the next version. Thanks for the review! Ryan On 5/29/14 1:17 PM, "Ethan Jackson" wrote: >It's kinda weird that route_table_get_ifindex() is a globally >accessible function which requires a mutex which is internal to the >module. I think the answer

Re: [ovs-dev] [PATCH] ofproto-dpif-xlate: Mark xcfgp and new_xcfg as static.

Acked-by: Ryan Wilson On 5/28/14 12:38 PM, "Ben Pfaff" wrote: >Found by sparse. > >Signed-off-by: Ben Pfaff >--- > ofproto/ofproto-dpif-xlate.c |4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > >diff --git a/ofproto/ofproto-dpif-xlate.c b/ofproto/ofproto-dpif-xlate.c >index c2302

Re: [ovs-dev] [PATCH v4] ofproto-dpif-xlate: Implement RCU locking in ofproto-dpif-xlate.

> >Acked-by: Ethan Jackson > > >On Thu, May 22, 2014 at 7:03 PM, Alex Wang wrote: >> Hey Ben, >> >> Could you review it? Should be all ready, >> >> >> Thanks, >> Alex Wang, >> >> >> On Tue, May 20, 2014 at 11:19 AM, Ben

Re: [ovs-dev] [PATCH v3] Rapid Spanning Protocol Implementation (IEEE 802.1D) + functional tests

http://patchwork.openvswitch.org/patch/4139/ With this patch, xlate_rwlock will be gone (as xlate will use RCU locking). So this will no longer be an issue. Still waiting on a review for this patch from Ben or Ethan, but hopefully it will be pushed next week. Ryan _

Re: [ovs-dev] [PATCH 1/4] ofproto: Remove ofproto_group_write_lookup()

This is a much cleaner improvement, thanks! Acked-by: Ryan Wilson On 5/22/14 11:04 AM, "Andy Zhou" wrote: >ofproto_group_write_lookup() slightly different from >ofproto_group_lookup() in handling reference counting. >Currently, it has only one caller: modify_group(). >It seems the abstraction

Re: [ovs-dev] [PATCH] ofproto: Add const to immutable members of ofgroup.

Thanks for this, Andy! Acked-by: Ryan Wilson From: dev on behalf of Andy Zhou Sent: Wednesday, May 21, 2014 9:47 PM To: d...@openvswitch.com Subject: [ovs-dev] [PATCH] ofproto: Add const to immutable members of ofgroup. Signed-off-by: Andy Zhou ---

Re: [ovs-dev] [PATCH v7] ofproto: Add reference count for Openflow groups.

Yup, modify_group looks much cleaner! And sorry for the lock annotations; I typically use GCC but its clear I need to use clang as well. You're welcome to push with your incremental, thanks! I'll also send out another version of my group and bucket stats patch, there are some rebase errors with th

Re: [ovs-dev] [PATCH v6] ofproto: Add reference count for Openflow groups.

Some inline comments below. The only thing I'm a bit concerned about it is the constant cast issue, especially since created / modified are actually altered in modify_group(). Also, the problem is the struct-defined variables are constant, so CONST_CAST won't work here. (CONST_CAST works for conve

Re: [ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for Openflow groups.

>>>How about we remove the rwlock and only use ref count? >> >> We need the ref count to determine when we can free the group / bucket. >> The rwlock is to serialize access to the ofgroup's properties. For >> example, 2 threads could be accessing ofgroup's properties, but a ref >> count doesn't p

Re: [ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for Openflow groups.

>> >> 1) Use refs for buckets (this seems unnecessarily heavyweight) >> 2) Always try bucket_lookup when adding stats to a bucket via the cache. >> If the bucket is not there in the list, we know its been removed via >> modify_group() and we simply don't update stats. >> 3) Use ofgroup->rw_lock to

Re: [ovs-dev] [PATCH] ofproto: Remove per-flow miss hash table from upcall handler.

I recently reproduced this today on servers109/110, using master branch with HEAD = 5a87054c2d832d0e10b30a1f223707acb8efbeb7. This commit is from yesterday, so it includes your fix (73a3c4757e596ff156d40f41496a0264373e5bc4). Ryan From: Joe Stringer mailto:joestrin...@nicira.com>> Date: Tuesday,

Re: [ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for Openflow groups.

>On Tue, May 20, 2014 at 3:54 PM, Ryan Wilson 76511 >wrote: >> >> Thanks for the review, Joe! I added a more clear description in 'struct >> ofgroup' and the commit message to explain why the refcount is needed. >That's good. Thanks for the improvement.

Re: [ovs-dev] [PATCH] ofproto: Remove per-flow miss hash table from upcall handler.

FYI, I just did a perf test on master and received the same error message as before. So this appears to be an issue unrelated to this patch. 2014-05-20T16:42:48.435Z|3|dpif(revalidator109)|WARN|system@ovs-system: failed to flow_del (No such file or directory) dp_hash(0),recirc_id(0),skb_pri

Re: [ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for Openflow groups.

ware.com>> Cc: Andy Zhou mailto:az...@nicira.com>>, Ryan Wilson mailto:wr...@nicira.com>>, "dev@openvswitch.org<mailto:dev@openvswitch.org>" mailto:dev@openvswitch.org>> Subject: Re: [ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for Openflow

Re: [ovs-dev] [PATCH v3 1/3] ofproto: Add reference count for Openflow groups.

Hey Andy, >At a high level, ofgroup struct current has rwlock that essentially >solving >the same problem as the ref count proposed in this patch. It would be >better >it seems confusing if we use both method together. > >Looking at the code, I'd think extending rwlock to cover xlate >fastpath is

Re: [ovs-dev] [PATCH v4] ofproto-dpif-xlate: Implement RCU locking in ofproto-dpif-xlate.

Per Alex's request, I ran a 10K internal port creation test (using batches of 500 ports at a time via ovs-vsctl) on my 8GB memory machine. Again RCU was slightly faster: master: real 3m28.301s with RCU: real 3m21.489s Also, the reason I don't simply batch all creation of ports together via — s

Re: [ovs-dev] [PATCHv2] ofproto: Add support for Openflow group and bucket stats.

We should use Andy's patch for the tests since I didn't write the tests: http://patchwork.openvswitch.org/patch/3626/ I'll split the group RCU and the crediting stats parts of this commit on my next version. Ryan - Original Message - From: "Alex Wang" To: "Ryan Wilson" Cc: dev

Re: [ovs-dev] [PATCH] netdev: Remove netdev from global shash when the user is changing interface configuration.

In netdev_remove, the netdev is unreffed after being removed from the global shash. If the ref count is 0, the netdev is destroyed. I'll re-submit with a more accurate comment for netdev_remove (it unrefs rather than destroys the netdev) and also adding a comment with netdev_remove in iface_des

Re: [ovs-dev] [PATCH] netdev: Add random tag to struct netdev.

be tied to netdevs to take account of the case when a tunnel is removed and re-added for whatever reason. Does that make sense? If so, an explanation of the sort should be added to the commit message. Ethan > > On Apr 22, 2014 4:09 PM, "Ryan Wilson 76511" < wr...@vmware.co

[ovs-dev] [PATCH] netdev: Add random tag to struct netdev.

netdev: Add random tag to struct netdev. Before, there would be no way to tell if ovs-vswitchd had been restarted or killed via ovsdb logging. Now, a random tag will be generated upon initialization of the netdev struct which happens when ovs-vswitchd is restarted. A change in tag value in