From patchwork Sat Oct 2 02:20:50 2010 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Simon Horman X-Patchwork-Id: 66539 X-Patchwork-Delegate: davem@davemloft.net Return-Path: X-Original-To: patchwork-incoming@ozlabs.org Delivered-To: patchwork-incoming@ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 11EC9B70CE for ; Sat, 2 Oct 2010 12:21:01 +1000 (EST) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752870Ab0JBCU4 (ORCPT ); Fri, 1 Oct 2010 22:20:56 -0400 Received: from kirsty.vergenet.net ([202.4.237.240]:53621 "EHLO kirsty.vergenet.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750706Ab0JBCUz (ORCPT ); Fri, 1 Oct 2010 22:20:55 -0400 Received: from ayumi.akashicho.tokyo.vergenet.net (219-109-213-121.bitcat.net [219.109.213.121]) by kirsty.vergenet.net (Postfix) with ESMTP id EBDFF24051; Sat, 2 Oct 2010 12:20:52 +1000 (EST) Received: by ayumi.akashicho.tokyo.vergenet.net (Postfix, from userid 7100) id 6474CEDE0E5; Sat, 2 Oct 2010 11:20:50 +0900 (JST) Date: Sat, 2 Oct 2010 11:20:50 +0900 From: Simon Horman To: Julian Anastasov Cc: lvs-devel@vger.kernel.org, netdev@vger.kernel.org, netfilter@vger.kernel.org, netfilter-devel@vger.kernel.org, Jan Engelhardt , Stephen Hemminger , Wensong Zhang , Patrick McHardy Subject: Re: [patch v2 03/12] [PATCH 03/12] IPVS: compact ip_vs_sched_persist() Message-ID: <20101002022050.GA16593@verge.net.au> References: <20101001143517.645421976@akiko.akashicho.tokyo.vergenet.net> <20101001143941.949669349@akiko.akashicho.tokyo.vergenet.net> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.20 (2009-06-14) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Sat, Oct 02, 2010 at 01:35:28AM +0300, Julian Anastasov wrote: > > Hello, > > On Fri, 1 Oct 2010, Simon Horman wrote: > > >Compact ip_vs_sched_persist() by setting up parameters > >and calling functions once. > > > >Signed-off-by: Simon Horman > >--- > > > >v2 > >* Make "union nf_inet_addr fwmark" const > >* Don't remove the comment next to the declaration of dport > >* Add a comment to the declaration of vport > > > >Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_core.c > >=================================================================== > >--- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_core.c 2010-10-01 21:56:39.000000000 +0900 > >+++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_core.c 2010-10-01 22:02:41.000000000 +0900 > >@@ -193,10 +193,14 @@ ip_vs_sched_persist(struct ip_vs_service > > struct ip_vs_iphdr iph; > > struct ip_vs_dest *dest; > > struct ip_vs_conn *ct; > >- __be16 dport; /* destination port to forward */ > >+ int protocol = iph.protocol; > >+ __be16 dport = 0; /* destination port to forward */ > >+ __be16 vport = 0; /* virtual service port */ > > unsigned int flags; > > union nf_inet_addr snet; /* source network of the client, > > after masking */ > >+ const union nf_inet_addr fwmark = { .ip = htonl(svc->fwmark) }; > >+ const union nf_inet_addr *vaddr = &iph.daddr; > > > > ip_vs_fill_iphdr(svc->af, skb_network_header(skb), &iph); > > > >@@ -227,119 +231,58 @@ ip_vs_sched_persist(struct ip_vs_service > > * service, and a template like > > * is created for other persistent services. > > */ > >- if (ports[1] == svc->port) { > >- /* Check if a template already exists */ > >- if (svc->port != FTPPORT) > >- ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0, > >- &iph.daddr, ports[1]); > >- else > >- ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0, > >- &iph.daddr, 0); > >- > >- if (!ct || !ip_vs_check_template(ct)) { > >- /* > >- * No template found or the dest of the connection > >- * template is not available. > >- */ > >- dest = svc->scheduler->schedule(svc, skb); > >- if (dest == NULL) { > >- IP_VS_DBG(1, "p-schedule: no dest found.\n"); > >- return NULL; > >- } > >- > >- /* > >- * Create a template like >- * vaddr,vport,daddr,dport> for non-ftp service, > >- * and > >- * for ftp service. > >+ { > >+ if (ports[1] == svc->port) { > >+ /* non-FTP template: > >+ * > >+ * FTP template: > >+ * > > */ > > if (svc->port != FTPPORT) > >- ct = ip_vs_conn_new(svc->af, iph.protocol, > >- &snet, 0, > >- &iph.daddr, > >- ports[1], > >- &dest->addr, dest->port, > >- IP_VS_CONN_F_TEMPLATE, > >- dest); > >- else > >- ct = ip_vs_conn_new(svc->af, iph.protocol, > >- &snet, 0, > >- &iph.daddr, 0, > >- &dest->addr, 0, > >- IP_VS_CONN_F_TEMPLATE, > >- dest); > >- if (ct == NULL) > >- return NULL; > >- > >- ct->timeout = svc->timeout; > >+ vport = ports[1]; > > } else { > >- /* set destination with the found template */ > >- dest = ct->dest; > >- } > >- dport = dest->port; > >- } else { > >- /* > >- * Note: persistent fwmark-based services and persistent > >- * port zero service are handled here. > >- * fwmark template: > >- * port zero template: > >- */ > >- if (svc->fwmark) { > >- union nf_inet_addr fwmark = { > >- .ip = htonl(svc->fwmark) > >- }; > >- > >- ct = ip_vs_ct_in_get(svc->af, IPPROTO_IP, &snet, 0, > >- &fwmark, 0); > >- } else > >- ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0, > >- &iph.daddr, 0); > >- > >- if (!ct || !ip_vs_check_template(ct)) { > >- /* > >- * If it is not persistent port zero, return NULL, > >- * otherwise create a connection template. > >+ /* Note: persistent fwmark-based services and > >+ * persistent port zero service are handled here. > >+ * fwmark template: > >+ * > >+ * port zero template: > >+ * > > */ > >- if (svc->port) > >- return NULL; > >- > >- dest = svc->scheduler->schedule(svc, skb); > >- if (dest == NULL) { > >- IP_VS_DBG(1, "p-schedule: no dest found.\n"); > >- return NULL; > >+ if (svc->fwmark) { > >+ protocol = IPPROTO_IP; > >+ vaddr = &fwmark; > > } > >+ } > >+ } > > > >- /* > >- * Create a template according to the service > >- */ > >- if (svc->fwmark) { > >- union nf_inet_addr fwmark = { > >- .ip = htonl(svc->fwmark) > >- }; > >- > >- ct = ip_vs_conn_new(svc->af, IPPROTO_IP, > >- &snet, 0, > >- &fwmark, 0, > >- &dest->addr, 0, > >- IP_VS_CONN_F_TEMPLATE, > >- dest); > >- } else > >- ct = ip_vs_conn_new(svc->af, iph.protocol, > >- &snet, 0, > >- &iph.daddr, 0, > >- &dest->addr, 0, > >- IP_VS_CONN_F_TEMPLATE, > >- dest); > >- if (ct == NULL) > >- return NULL; > >+ /* Check if a template already exists */ > >+ ct = ip_vs_ct_in_get(svc->af, protocol, &snet, 0, vaddr, vport); > > > >- ct->timeout = svc->timeout; > >- } else { > >- /* set destination with the found template */ > >- dest = ct->dest; > >+ if (!ct || !ip_vs_check_template(ct)) { > >+ /* No template found or the dest of the connection > >+ * template is not available. > >+ */ > >+ dest = svc->scheduler->schedule(svc, skb); > >+ if (!dest) { > >+ IP_VS_DBG(1, "p-schedule: no dest found.\n"); > >+ return NULL; > > } > >- dport = ports[1]; > >- } > >+ > >+ if (ports[1] == svc->port && svc->port != FTPPORT) > >+ dport = dest->port; > >+ > >+ /* Create a template */ > >+ ct = ip_vs_conn_new(svc->af, protocol, &snet, 0,vaddr, vport, > >+ &dest->addr, dport, > >+ IP_VS_CONN_F_TEMPLATE, dest); > >+ if (ct == NULL) > >+ return NULL; > >+ > >+ ct->timeout = svc->timeout; > >+ } else > >+ /* set destination with the found template */ > >+ dest = ct->dest; > > Here dport: > > >+ dport = dest->port; > > should be: > > dport = ports[1]; > if (dport == svc->port && dest->port) > dport = dest->port; Thanks, fixed. > > flags = (svc->flags & IP_VS_SVC_F_ONEPACKET > > && iph.protocol == IPPROTO_UDP)? I will repost the entire series a little later. For reference, here is the updated version of this patch. From a6310d1a8f21bdf15fa797ed748651679c0197e2 Mon Sep 17 00:00:00 2001 From: Simon Horman Date: Sun, 22 Aug 2010 21:37:51 +0900 Subject: [PATCH 03/12] IPVS: compact ip_vs_sched_persist() Compact ip_vs_sched_persist() by setting up parameters and calling functions once. Signed-off-by: Simon Horman --- v2 * Make "union nf_inet_addr fwmark" const * Don't remove the comment next to the declaration of dport * Add a comment to the declaration of vport v3 * As suggested by Julian Anastasov - Correct dport logic -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Index: lvs-test-2.6/net/netfilter/ipvs/ip_vs_core.c =================================================================== --- lvs-test-2.6.orig/net/netfilter/ipvs/ip_vs_core.c 2010-10-02 10:54:57.000000000 +0900 +++ lvs-test-2.6/net/netfilter/ipvs/ip_vs_core.c 2010-10-02 11:04:04.000000000 +0900 @@ -193,10 +193,14 @@ ip_vs_sched_persist(struct ip_vs_service struct ip_vs_iphdr iph; struct ip_vs_dest *dest; struct ip_vs_conn *ct; - __be16 dport; /* destination port to forward */ + int protocol = iph.protocol; + __be16 dport = 0; /* destination port to forward */ + __be16 vport = 0; /* virtual service port */ unsigned int flags; union nf_inet_addr snet; /* source network of the client, after masking */ + const union nf_inet_addr fwmark = { .ip = htonl(svc->fwmark) }; + const union nf_inet_addr *vaddr = &iph.daddr; ip_vs_fill_iphdr(svc->af, skb_network_header(skb), &iph); @@ -227,119 +231,61 @@ ip_vs_sched_persist(struct ip_vs_service * service, and a template like * is created for other persistent services. */ - if (ports[1] == svc->port) { - /* Check if a template already exists */ - if (svc->port != FTPPORT) - ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0, - &iph.daddr, ports[1]); - else - ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0, - &iph.daddr, 0); - - if (!ct || !ip_vs_check_template(ct)) { - /* - * No template found or the dest of the connection - * template is not available. - */ - dest = svc->scheduler->schedule(svc, skb); - if (dest == NULL) { - IP_VS_DBG(1, "p-schedule: no dest found.\n"); - return NULL; - } - - /* - * Create a template like for non-ftp service, - * and - * for ftp service. + { + if (ports[1] == svc->port) { + /* non-FTP template: + * + * FTP template: + * */ if (svc->port != FTPPORT) - ct = ip_vs_conn_new(svc->af, iph.protocol, - &snet, 0, - &iph.daddr, - ports[1], - &dest->addr, dest->port, - IP_VS_CONN_F_TEMPLATE, - dest); - else - ct = ip_vs_conn_new(svc->af, iph.protocol, - &snet, 0, - &iph.daddr, 0, - &dest->addr, 0, - IP_VS_CONN_F_TEMPLATE, - dest); - if (ct == NULL) - return NULL; - - ct->timeout = svc->timeout; + vport = ports[1]; } else { - /* set destination with the found template */ - dest = ct->dest; - } - dport = dest->port; - } else { - /* - * Note: persistent fwmark-based services and persistent - * port zero service are handled here. - * fwmark template: - * port zero template: - */ - if (svc->fwmark) { - union nf_inet_addr fwmark = { - .ip = htonl(svc->fwmark) - }; - - ct = ip_vs_ct_in_get(svc->af, IPPROTO_IP, &snet, 0, - &fwmark, 0); - } else - ct = ip_vs_ct_in_get(svc->af, iph.protocol, &snet, 0, - &iph.daddr, 0); - - if (!ct || !ip_vs_check_template(ct)) { - /* - * If it is not persistent port zero, return NULL, - * otherwise create a connection template. + /* Note: persistent fwmark-based services and + * persistent port zero service are handled here. + * fwmark template: + * + * port zero template: + * */ - if (svc->port) - return NULL; - - dest = svc->scheduler->schedule(svc, skb); - if (dest == NULL) { - IP_VS_DBG(1, "p-schedule: no dest found.\n"); - return NULL; + if (svc->fwmark) { + protocol = IPPROTO_IP; + vaddr = &fwmark; } + } + } - /* - * Create a template according to the service - */ - if (svc->fwmark) { - union nf_inet_addr fwmark = { - .ip = htonl(svc->fwmark) - }; - - ct = ip_vs_conn_new(svc->af, IPPROTO_IP, - &snet, 0, - &fwmark, 0, - &dest->addr, 0, - IP_VS_CONN_F_TEMPLATE, - dest); - } else - ct = ip_vs_conn_new(svc->af, iph.protocol, - &snet, 0, - &iph.daddr, 0, - &dest->addr, 0, - IP_VS_CONN_F_TEMPLATE, - dest); - if (ct == NULL) - return NULL; + /* Check if a template already exists */ + ct = ip_vs_ct_in_get(svc->af, protocol, &snet, 0, vaddr, vport); - ct->timeout = svc->timeout; - } else { - /* set destination with the found template */ - dest = ct->dest; + if (!ct || !ip_vs_check_template(ct)) { + /* No template found or the dest of the connection + * template is not available. + */ + dest = svc->scheduler->schedule(svc, skb); + if (!dest) { + IP_VS_DBG(1, "p-schedule: no dest found.\n"); + return NULL; } - dport = ports[1]; - } + + if (ports[1] == svc->port && svc->port != FTPPORT) + dport = dest->port; + + /* Create a template */ + ct = ip_vs_conn_new(svc->af, protocol, &snet, 0,vaddr, vport, + &dest->addr, dport, + IP_VS_CONN_F_TEMPLATE, dest); + if (ct == NULL) + return NULL; + + ct->timeout = svc->timeout; + } else + /* set destination with the found template */ + dest = ct->dest; + + dport = ports[1]; + if (dport == svc->port && dest->port) + dport = dest->port; flags = (svc->flags & IP_VS_SVC_F_ONEPACKET && iph.protocol == IPPROTO_UDP)?