| Message ID | 1366852353-32561-1-git-send-email-horms@verge.net.au |
|---|---|
| State | Not Applicable |
| Headers | show |
Hello, On Thu, 25 Apr 2013, Simon Horman wrote: > It is unclear to me that there is any utility in the following: > > rcu_read_unlock(); > rcu_read_lock(); I thought it is a good idea for fixed hash table of IP_VS_TAB_BITS=20. May be if guarded by if (!((++idx) & 4095)) to reduce its rate to 256 (with idx++ removed from the for loop) ? Netfilter has no such logic for nf_conntrack because it has limit of 16384 rows. Not sure how fatal is to try 1048576 empty rows under RCU lock for such rare operations as connection listing. OTOH, ip_vs_conn_array() needs to seek at some initial position, so it can skip many entries if reading table with many conns, for example, 1048576 rows * 16 conns per row, we will need to touch 16777216 conns under lock. Not sure what is the best practice for such cases. > So this patch removes the two instances of the above from ip_vs_conn.c. > > This was introduced in 7cf2eb7bccbe0d7a8ab1d1382c4faa2b3abf817f ("ipvs: fix > sparse warnings for ip_vs_conn listing") as part of cleaning up warnings > flagged by sparse. > > Compile and sparse tested only. > > Cc: Julian Anastasov <ja@ssi.bg> > Reported-by: Pablo Neira Ayuso <pablo@netfilter.org> > Signed-off-by: Simon Horman <horms@verge.net.au> > --- > net/netfilter/ipvs/ip_vs_conn.c | 4 ---- > 1 file changed, 4 deletions(-) > > diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c > index a083bda..458dc68 100644 > --- a/net/netfilter/ipvs/ip_vs_conn.c > +++ b/net/netfilter/ipvs/ip_vs_conn.c > @@ -975,8 +975,6 @@ static void *ip_vs_conn_array(struct seq_file *seq, loff_t pos) > return cp; > } > } > - rcu_read_unlock(); > - rcu_read_lock(); > } > > return NULL; > @@ -1015,8 +1013,6 @@ static void *ip_vs_conn_seq_next(struct seq_file *seq, void *v, loff_t *pos) > iter->l = &ip_vs_conn_tab[idx]; > return cp; > } > - rcu_read_unlock(); > - rcu_read_lock(); > } > iter->l = NULL; > return NULL; > -- > 1.8.2.1 Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello On Thu, 2013-04-25 at 11:15 +0300, Julian Anastasov wrote: > Hello, > > On Thu, 25 Apr 2013, Simon Horman wrote: > > > It is unclear to me that there is any utility in the following: > > > > rcu_read_unlock(); > > rcu_read_lock(); > > I thought it is a good idea for fixed hash table > of IP_VS_TAB_BITS=20. May be if guarded by > > if (!((++idx) & 4095)) > > to reduce its rate to 256 (with idx++ removed from the for loop) ? > > Netfilter has no such logic for nf_conntrack because > it has limit of 16384 rows. Not sure how fatal is to try 1048576 > empty rows under RCU lock for such rare operations as > connection listing. OTOH, ip_vs_conn_array() needs to > seek at some initial position, so it can skip many > entries if reading table with many conns, for example, > 1048576 rows * 16 conns per row, we will need to > touch 16777216 conns under lock. Not sure what is the > best practice for such cases. My opinion is to keep it, people tends to do such "rare" things. It's not unusual with 256k - 1M rows... Regards Hans
On Thu, Apr 25, 2013 at 11:05:26AM +0200, Hans Schillstrom wrote: > Hello > On Thu, 2013-04-25 at 11:15 +0300, Julian Anastasov wrote: > > Hello, > > > > On Thu, 25 Apr 2013, Simon Horman wrote: > > > > > It is unclear to me that there is any utility in the following: > > > > > > rcu_read_unlock(); > > > rcu_read_lock(); > > > > I thought it is a good idea for fixed hash table > > of IP_VS_TAB_BITS=20. May be if guarded by > > > > if (!((++idx) & 4095)) > > > > to reduce its rate to 256 (with idx++ removed from the for loop) ? > > > > Netfilter has no such logic for nf_conntrack because > > it has limit of 16384 rows. Not sure how fatal is to try 1048576 > > empty rows under RCU lock for such rare operations as > > connection listing. OTOH, ip_vs_conn_array() needs to > > seek at some initial position, so it can skip many > > entries if reading table with many conns, for example, > > 1048576 rows * 16 conns per row, we will need to > > touch 16777216 conns under lock. Not sure what is the > > best practice for such cases. > > My opinion is to keep it, people tends to do such "rare" things. > It's not unusual with 256k - 1M rows... Ok, leaving it seems reasonable. Pablo, do you have any objections? -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, 2013-04-25 at 22:36 +0900, Simon Horman wrote: > Ok, leaving it seems reasonable. > Pablo, do you have any objections? I have objections. I would _add_ a cond_resched() there to explicitly do what we want Maybe a macro/inline doing this already exists. static void inline cond_resched_rcu_lock(void) { if (need_resched()) { rcu_read_unlock(); cond_resched(); rcu_read_lock(); } } -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Thu, 25 Apr 2013, Eric Dumazet wrote: > On Thu, 2013-04-25 at 22:36 +0900, Simon Horman wrote: > > > Ok, leaving it seems reasonable. > > Pablo, do you have any objections? > > I have objections. > > I would _add_ a cond_resched() there to explicitly do what we want > > Maybe a macro/inline doing this already exists. > > static void inline cond_resched_rcu_lock(void) > { > if (need_resched()) { > rcu_read_unlock(); > cond_resched(); > rcu_read_lock(); > } > } Thanks, looks like a good idea to me, I guess its place is include/linux/sched.h. Simon, can you prepare 2 patches instead, one for cond_resched_rcu_lock and second for ipvs? Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thu, Apr 25, 2013 at 10:46:55PM +0300, Julian Anastasov wrote: > > Hello, > > On Thu, 25 Apr 2013, Eric Dumazet wrote: > > > On Thu, 2013-04-25 at 22:36 +0900, Simon Horman wrote: > > > > > Ok, leaving it seems reasonable. > > > Pablo, do you have any objections? > > > > I have objections. > > > > I would _add_ a cond_resched() there to explicitly do what we want > > > > Maybe a macro/inline doing this already exists. > > > > static void inline cond_resched_rcu_lock(void) > > { > > if (need_resched()) { > > rcu_read_unlock(); > > cond_resched(); > > rcu_read_lock(); > > } > > } > > Thanks, looks like a good idea to me, I guess > its place is include/linux/sched.h. Simon, can you prepare > 2 patches instead, one for cond_resched_rcu_lock and second > for ipvs? Sure, will do. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Julian, On Thu, Apr 25, 2013 at 11:15:25AM +0300, Julian Anastasov wrote: > > Hello, > > On Thu, 25 Apr 2013, Simon Horman wrote: > > > It is unclear to me that there is any utility in the following: > > > > rcu_read_unlock(); > > rcu_read_lock(); > > I thought it is a good idea for fixed hash table > of IP_VS_TAB_BITS=20. May be if guarded by > > if (!((++idx) & 4095)) > > to reduce its rate to 256 (with idx++ removed from the for loop) ? > > Netfilter has no such logic for nf_conntrack because > it has limit of 16384 rows. We seem to be supporting over that limit via module_param and sysfs: /sys/module/nf_conntrack/parameters/hashsize Regards. -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Fri, 26 Apr 2013, Pablo Neira Ayuso wrote: > Hi Julian, > > On Thu, Apr 25, 2013 at 11:15:25AM +0300, Julian Anastasov wrote: > > > > Hello, > > > > On Thu, 25 Apr 2013, Simon Horman wrote: > > > > > It is unclear to me that there is any utility in the following: > > > > > > rcu_read_unlock(); > > > rcu_read_lock(); > > > > I thought it is a good idea for fixed hash table > > of IP_VS_TAB_BITS=20. May be if guarded by > > > > if (!((++idx) & 4095)) > > > > to reduce its rate to 256 (with idx++ removed from the for loop) ? > > > > Netfilter has no such logic for nf_conntrack because > > it has limit of 16384 rows. > > We seem to be supporting over that limit via module_param and sysfs: > > /sys/module/nf_conntrack/parameters/hashsize Thanks for the note, I overlooked this parameter. Regards -- Julian Anastasov <ja@ssi.bg> -- To unsubscribe from this list: send the line "unsubscribe netfilter-devel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/net/netfilter/ipvs/ip_vs_conn.c b/net/netfilter/ipvs/ip_vs_conn.c index a083bda..458dc68 100644 --- a/net/netfilter/ipvs/ip_vs_conn.c +++ b/net/netfilter/ipvs/ip_vs_conn.c @@ -975,8 +975,6 @@ static void *ip_vs_conn_array(struct seq_file *seq, loff_t pos) return cp; } } - rcu_read_unlock(); - rcu_read_lock(); } return NULL; @@ -1015,8 +1013,6 @@ static void *ip_vs_conn_seq_next(struct seq_file *seq, void *v, loff_t *pos) iter->l = &ip_vs_conn_tab[idx]; return cp; } - rcu_read_unlock(); - rcu_read_lock(); } iter->l = NULL; return NULL;
It is unclear to me that there is any utility in the following: rcu_read_unlock(); rcu_read_lock(); So this patch removes the two instances of the above from ip_vs_conn.c. This was introduced in 7cf2eb7bccbe0d7a8ab1d1382c4faa2b3abf817f ("ipvs: fix sparse warnings for ip_vs_conn listing") as part of cleaning up warnings flagged by sparse. Compile and sparse tested only. Cc: Julian Anastasov <ja@ssi.bg> Reported-by: Pablo Neira Ayuso <pablo@netfilter.org> Signed-off-by: Simon Horman <horms@verge.net.au> --- net/netfilter/ipvs/ip_vs_conn.c | 4 ---- 1 file changed, 4 deletions(-)