From patchwork Mon Apr 4 21:50:32 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Justin Pettit X-Patchwork-Id: 605994 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from archives.nicira.com (unknown [96.126.127.54]) by ozlabs.org (Postfix) with ESMTP id 3qf5Jw5sKvz9s5w for ; Tue, 5 Apr 2016 07:50:55 +1000 (AEST) Received: from archives.nicira.com (localhost [127.0.0.1]) by archives.nicira.com (Postfix) with ESMTP id B9F8D102A8; Mon, 4 Apr 2016 14:50:44 -0700 (PDT) X-Original-To: dev@openvswitch.org Delivered-To: dev@openvswitch.org Received: from mx1e3.cudamail.com (mx1.cudamail.com [69.90.118.67]) by archives.nicira.com (Postfix) with ESMTPS id A92A6102A7 for ; Mon, 4 Apr 2016 14:50:43 -0700 (PDT) Received: from bar5.cudamail.com (localhost [127.0.0.1]) by mx1e3.cudamail.com (Postfix) with ESMTPS id 0B8AF42033E for ; Mon, 4 Apr 2016 15:50:43 -0600 (MDT) X-ASG-Debug-ID: 1459806641-09eadd287b55f160001-byXFYA Received: from mx1-pf2.cudamail.com ([192.168.24.2]) by bar5.cudamail.com with ESMTP id 2T83dgUDqQgWZjq8 (version=TLSv1 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO) for ; Mon, 04 Apr 2016 15:50:41 -0600 (MDT) X-Barracuda-Envelope-From: jpettit@ovn.org X-Barracuda-RBL-Trusted-Forwarder: 192.168.24.2 Received: from unknown (HELO relay4-d.mail.gandi.net) (217.70.183.196) by mx1-pf2.cudamail.com with ESMTPS (DHE-RSA-AES256-SHA encrypted); 4 Apr 2016 21:50:41 -0000 Received-SPF: pass (mx1-pf2.cudamail.com: SPF record at ovn.org designates 217.70.183.196 as permitted sender) X-Barracuda-Apparent-Source-IP: 217.70.183.196 X-Barracuda-RBL-IP: 217.70.183.196 Received: from mfilter13-d.gandi.net (mfilter13-d.gandi.net [217.70.178.141]) by relay4-d.mail.gandi.net (Postfix) with ESMTP id ADCBB1720A3; Mon, 4 Apr 2016 23:50:39 +0200 (CEST) X-Virus-Scanned: Debian amavisd-new at mfilter13-d.gandi.net Received: from relay4-d.mail.gandi.net ([IPv6:::ffff:217.70.183.196]) by mfilter13-d.gandi.net (mfilter13-d.gandi.net [::ffff:10.0.15.180]) (amavisd-new, port 10024) with ESMTP id OHqYPt9r0_5Y; Mon, 4 Apr 2016 23:50:37 +0200 (CEST) X-Originating-IP: 208.91.2.4 Received: from [10.1.179.159] (unknown [208.91.2.4]) (Authenticated sender: jpettit@ovn.org) by relay4-d.mail.gandi.net (Postfix) with ESMTPSA id B0ED917209A; Mon, 4 Apr 2016 23:50:36 +0200 (CEST) Mime-Version: 1.0 (Mac OS X Mail 9.3 \(3124\)) X-CudaMail-Whitelist-To: dev@openvswitch.org X-CudaMail-MID: CM-E2-403079593 X-CudaMail-DTE: 040416 X-CudaMail-Originating-IP: 217.70.183.196 X-CudaMail-Envelope-Sender: jpettit@ovn.org X-ASG-Orig-Subj: [##CM-E2-403079593##]Re: [ovs-dev] [PATCH] Patch v2: OVN: Support BUM traffic in the VTEP schema From: Justin Pettit In-Reply-To: <1458889476-1329-2-git-send-email-dball@vmware.com> Date: Mon, 4 Apr 2016 14:50:32 -0700 Message-Id: <5339F544-24AF-4AB0-AB14-9599458FB71D@ovn.org> References: <1458889476-1329-1-git-send-email-dball@vmware.com> <1458889476-1329-2-git-send-email-dball@vmware.com> To: Darrell Ball X-Mailer: Apple Mail (2.3124) X-Barracuda-Connect: UNKNOWN[192.168.24.2] X-Barracuda-Start-Time: 1459806641 X-Barracuda-Encrypted: DHE-RSA-AES256-SHA X-Barracuda-URL: https://web.cudamail.com:443/cgi-mod/mark.cgi X-ASG-Whitelist: Header =?UTF-8?B?eFwtY3VkYW1haWxcLXdoaXRlbGlzdFwtdG8=?= X-Virus-Scanned: by bsmtpd at cudamail.com X-Barracuda-BRTS-Status: 1 Cc: dev@openvswitch.org Subject: Re: [ovs-dev] [PATCH] Patch v2: OVN: Support BUM traffic in the VTEP schema X-BeenThere: dev@openvswitch.org X-Mailman-Version: 2.1.16 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@openvswitch.org Sender: "dev" > On Mar 25, 2016, at 12:04 AM, Darrell Ball wrote: Thanks for the patch. I did a quick review of the functionality, but didn't do a full review yet. There are some style issues that I'd like to see addressed before I do that. Also, you'll need to fold in a patch like the one at the end of this message to get this to compile due to recent changes upstream. Can you look at the current feedback and respin a new patch? I flagged most of the instances where I saw them, but can you take a pass through and look for any - Declare variables in the block that needs them. - End full sentence comments with a period. - In general, try to get as many full words into a line as will fit in 79 characters. The way the patch was sent, "Patch v2: " would be part of the commit message. Also, can you use "ovn-controller-vtep:" instead of "OVN:" in the title? > This patch implements BUM support in the VTEP schema. > This relates to BUM traffic flowing from a gateway towards > HVs. This code would be relevant to HW gateways and > the ovs-vtep simulator. > In order to do this, the mcast macs remote table in the VTEP > schema is populated based on the OVN SB port binding. > For each logical switch, the SB port bindings are queried > to find all the physical locators to send BUM traffic to > and the VTEP DB is updated. Can you rejustify these to be closer to 79 characters? If you want to indicate paragraphs, please insert a blank line. > These code changes are contained in vtep.c. > > An OVN test for the ovs-vtep based gateway was > enabled for relevant packet types to test this functionality. > This test is contained in ovn.at > > The AUTHORS file was updated as well. You don't need to mention the source changes in the description, since they'll be in the commit. > Signed-off-by: Darrell Ball Your signed-off-by should match the address used to submit the patch. > diff --git a/AUTHORS b/AUTHORS > index 9e44e4c..dd0fc85 100644 > --- a/AUTHORS > +++ b/AUTHORS > @@ -50,6 +50,7 @@ Daniel Roman droman@nicira.com > Daniele Di Proietto daniele.di.proietto@gmail.com > Daniele Venturino daniele.venturino@m3s.it > Danny Kukawka danny.kukawka@bisect.de > +Darrell Ball dlu998@gmail.com Do you want to use this address or your VMware one? > @@ -84,6 +94,96 @@ create_pl(struct ovsdb_idl_txn *vtep_idl_txn, const char *chassis_ip) > } > > > +/* Creates a new 'Mcast_Macs_Remote'. entry */ Can you move that period to the end of the sentence? > +static void > +vtep_create_mmr(struct ovsdb_idl_txn *vtep_idl_txn, > + const char *mac, I think this line will fit on the previous one. > +/* Compares prev and new mmr locator sets and return true if they > + * differ; false otherwise; also preps new locator set > + * for database write */ > +static bool > +vtep_process_pls(const struct ovs_list *locators_list, > + const struct mmr_hash_node_data *mmr_ext, > + struct vteprec_physical_locator **locators) It's not immediately obvious what these arguments mean and which is previous and which is new. Can you describe them in the function description? Also, can you end full sentences with a period? > +{ > + int i; > + size_t n_locators_prev = 0; > + size_t n_locators_new = list_size(locators_list); > + struct vtep_rec_physical_locator_list_entry *ploc_entry; > + const struct vteprec_physical_locator *pl = NULL; The variables "i" and "pl" don't seem to be used until later code blocks. Can you move them there? > + bool prev_and_new_locator_lists_differ = false; It might be nice to use a shorter name. Maybe "lists_differ"? > +/* Creates a new 'Mcast_Macs_Remote' entry if needed. > + * Also cleans prev remote mcast mac entries as needed */ Same comment about ending the sentence with a period. > /* Maps from ovn logical datapath tunnel key (which is also the vtep > * logical switch tunnel key) to the corresponding vtep logical switch > * instance. Also, the shash map 'added_macs' is used for checking > - * duplicated MAC addresses in the same ovn logical datapath. */ > + * duplicated MAC addresses in the same ovn logical datapath. > + * mmr_ext is used to track mmr info per LS that needs creation/update > + * and locators_list collects the physical locators to be bound > + * for an mmr; physical_locators is used to track existing locators > + * and filter duplicates. */ Can you use single quotes around the field names? > > @@ -222,18 +336,40 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct shash *ucast_macs_rmts, > continue; > } > > + pl = shash_find_data(physical_locators, chassis_ip); > + if (!pl) { > + pl = create_pl(vtep_idl_txn, chassis_ip); > + shash_add(physical_locators, chassis_ip, pl); > + } > + ls_pl = shash_find_data(&ls_node->physical_locators, chassis_ip); > + if (!ls_pl) { > + ploc_entry = xmalloc(sizeof *ploc_entry); > + ploc_entry->vteprec_ploc = pl; > + list_push_back(&ls_node->locators_list, > + &ploc_entry->locators_node); The first character of this line should be right under the ampersand of "&ls_node". > + shash_add(&ls_node->physical_locators, chassis_ip, pl); > + } > + > + char *mac_tnlkey = > + xasprintf("%s_%"PRId64, "unknown-dst", tnl_key); > + ls_node->mmr_ext = > + shash_find_data(mcast_macs_rmts, mac_tnlkey); The assignment of these two variables will each fit on one line. > + > + if (ls_node->mmr_ext && > + ls_node->mmr_ext->mmr->logical_switch == ls_node->vtep_ls) { > + > + /* Delete the entry from the hash table so the MMR does > + * not get removed from the DB later on during stale > + * checking. */ This comment will fit on two lines. > @@ -305,17 +443,23 @@ vtep_lswitch_cleanup(struct ovsdb_idl *vtep_idl) > return done; > } > > -/* Removes all entries in the 'Ucast_Macs_Remote' table in vtep database. > +/* Removes all entries in the 'Ucast_Macs_Remote' > + * and 'Mcast_Macs_Remote' table in vtep database. > * Returns true when all done (no entry to remove). */ > static bool > vtep_macs_cleanup(struct ovsdb_idl *vtep_idl) > { > const struct vteprec_ucast_macs_remote *umr; > + const struct vteprec_mcast_macs_remote *mmr; > > VTEPREC_UCAST_MACS_REMOTE_FOR_EACH (umr, vtep_idl) { > vteprec_ucast_macs_remote_delete(umr); > return false; > } > + VTEPREC_MCAST_MACS_REMOTE_FOR_EACH (mmr, vtep_idl) { > + vteprec_mcast_macs_remote_delete(mmr); > + return false; > + } This block won't execute until there are no Ucast_Macs_Remote. Do you think it's worth going through both loops and if either one has an entry and then return false? > @@ -338,6 +483,13 @@ vtep_run(struct controller_vtep_ctx *ctx) > const struct vteprec_ucast_macs_remote *umr; > const struct vteprec_physical_locator *pl; > const struct sbrec_port_binding *port_binding_rec; > + const struct vteprec_mcast_macs_remote *mmr; > + struct mmr_hash_node_data *mmr_ext; > + const struct vteprec_physical_locator_set *locator_set; > + struct vteprec_physical_locator **locators_list; > + size_t n_locators; > + size_t i; > + struct shash_node *node; Many of these variable appear to only be used in a later nested block. Can you scope the declarations to where they're needed? > @@ -360,6 +512,29 @@ vtep_run(struct controller_vtep_ctx *ctx) > shash_add(&ucast_macs_rmts, mac_ip_tnlkey, umr); > free(mac_ip_tnlkey); > } > + /* Collects 'Mcast_Macs_Remote's. */ > + VTEPREC_MCAST_MACS_REMOTE_FOR_EACH (mmr, ctx->vtep_idl) { > ... > + locator_set = mmr_ext->mmr->locator_set; > + n_locators = locator_set->n_locators; > + locators_list = locator_set->locators; It looks like these were only introduced to shorten a couple of references later. I think it would be clearer to drop them and just use "mmr->locator_set" in the accesses below. > + > + shash_init(&mmr_ext->physical_locators); > + for (i = 0; i < n_locators; i++) { > + shash_add(&mmr_ext->physical_locators, > + locators_list[i]->dst_ip, > + locators_list[i]); > + } > + > + free(mac_tnlkey); > + } I'd put a blank line before and after this code block to separate from the other "Collects..." blocks. Thanks again. --Justin -=-=-=-=-=-=-=-=-=-=- diff --git a/ovn/controller-vtep/vtep.c b/ovn/controller-vtep/vtep.c index 4c7d77e..22eafc5 100644 --- a/ovn/controller-vtep/vtep.c +++ b/ovn/controller-vtep/vtep.c @@ -119,7 +119,7 @@ vtep_process_pls(const struct ovs_list *locators_list, { int i; size_t n_locators_prev = 0; - size_t n_locators_new = list_size(locators_list); + size_t n_locators_new = ovs_list_size(locators_list); struct vtep_rec_physical_locator_list_entry *ploc_entry; const struct vteprec_physical_locator *pl = NULL; bool prev_and_new_locator_lists_differ = false; @@ -159,7 +159,7 @@ vtep_update_mmr(struct ovsdb_idl_txn *vtep_idl_txn, const struct mmr_hash_node_data *mmr_ext) { struct vteprec_physical_locator **locators = NULL; - size_t n_locators_new = list_size(locators_list); + size_t n_locators_new = ovs_list_size(locators_list); bool mmr_changed = false; const struct vteprec_physical_locator_set *ploc_set; @@ -293,7 +293,7 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct sha ls_node->vtep_ls = vtep_ls; shash_init(&ls_node->added_macs); shash_init(&ls_node->physical_locators); - list_init(&ls_node->locators_list); + ovs_list_init(&ls_node->locators_list); ls_node->mmr_ext = NULL; hmap_insert(&ls_map, &ls_node->hmap_node, hash_uint64((uint64_t) vtep_ls->tunnel_key[0])); @@ -345,7 +345,7 @@ vtep_macs_run(struct ovsdb_idl_txn *vtep_idl_txn, struct sha if (!ls_pl) { ploc_entry = xmalloc(sizeof *ploc_entry); ploc_entry->vteprec_ploc = pl; - list_push_back(&ls_node->locators_list, + ovs_list_push_back(&ls_node->locators_list, &ploc_entry->locators_node); shash_add(&ls_node->physical_locators, chassis_ip, pl); }