Message ID | 20091104202716.GE14821@hostway.ca |
---|---|
State | Not Applicable, archived |
Delegated to: | David Miller |
Headers | show |
Hello, On Wed, 4 Nov 2009, Simon Kirby wrote: > Hello! > > I was noticing a significant amount of what seems/seemed to be > destination lists with multiple entries with the lblcr LVS algorithm. > While tracking it down, I think I stumbled over a mistake. In > ip_vs_lblcr_full_check(), it appears the time check logic is reversed: > > for (i=0, j=tbl->rover; i<IP_VS_LBLCR_TAB_SIZE; i++) { > j = (j + 1) & IP_VS_LBLCR_TAB_MASK; > > write_lock(&svc->sched_lock); > list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) { If 'time to expire' is after current time then continue, i.e. current time didn't reached the limit, seems correct, no need to patch. For better reading and to match ip_vs_lblcr_check_expire() it can be converted to: if (time_before(now, en->lastuse+sysctl_ip_vs_lblcr_expiration)) continue; > if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration, > now)) > continue; > > ip_vs_lblcr_free(en); > atomic_dec(&tbl->entries); > } > write_unlock(&svc->sched_lock); > } > > Shouldn't this be "time_before"? It seems that it currently nukes all > recently-used entries every time this function is called, which seems to > be every 30 minutes, rather than removing the not-recently-used ones. > > If my reading is correct, this patch should fix it. Am I missing > something? include/linux/jiffies.h: time_after(a,b) returns true if the time a is after time b. #define time_before(a,b) time_after(b,a) Regards -- Julian Anastasov <ja@ssi.bg> -- 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
On Thu, Nov 05, 2009 at 11:26:27AM +0200, Julian Anastasov wrote: > If 'time to expire' is after current time then continue, > i.e. current time didn't reached the limit, seems correct, > no need to patch. For better reading and to match > ip_vs_lblcr_check_expire() it can be converted to: > > if (time_before(now, en->lastuse+sysctl_ip_vs_lblcr_expiration)) > continue; D'oh. I noticed the use of time_before() further down in ip_vs_lblcr_check_expire(), but not the reversed arguments, hence my confusion. I still suspect there may be something not quite right, or which could perhaps do with some tuning. It's difficult to see exactly how it's working internally, since there's currently nothing to get a summary of the dest_sets to userspace. I'll follow up if I find anything. Simon- -- 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
diff --git a/net/netfilter/ipvs/ip_vs_lblcr.c b/net/netfilter/ipvs/ip_vs_lblcr.c index 715b57f..937743f 100644 --- a/net/netfilter/ipvs/ip_vs_lblcr.c +++ b/net/netfilter/ipvs/ip_vs_lblcr.c @@ -432,7 +432,7 @@ static inline void ip_vs_lblcr_full_check(struct ip_vs_service *svc) write_lock(&svc->sched_lock); list_for_each_entry_safe(en, nxt, &tbl->bucket[j], list) { - if (time_after(en->lastuse+sysctl_ip_vs_lblcr_expiration, + if (time_before(en->lastuse+sysctl_ip_vs_lblcr_expiration, now)) continue;