* [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly.
@ 2003-09-16 21:21 Stephen Hemminger
2003-09-17 16:44 ` Wensong Zhang
0 siblings, 1 reply; 5+ messages in thread
From: Stephen Hemminger @ 2003-09-16 21:21 UTC (permalink / raw)
To: Wensong Zhang; +Cc: lvs-users, netdev
The code to get the service table was not taking a reader lock
because it needed to copy_to_user.
Do it right:
- hold refcnt when copying out, and use a read lock
- make lock local to ip_svc
- get rid of IP_VS_WAIT_WHILE, use proper read locks and refcounts.
- move write_lock_bh in flush to outside of loop
- tag user pointers (for sparse).
diff -Nru a/include/net/ip_vs.h b/include/net/ip_vs.h
--- a/include/net/ip_vs.h Tue Sep 16 14:08:55 2003
+++ b/include/net/ip_vs.h Tue Sep 16 14:08:55 2003
@@ -319,8 +319,6 @@
#define LeaveFunction(level) do {} while (0)
#endif
-#define IP_VS_WAIT_WHILE(expr) while (expr) { cpu_relax(); }
-
/*
* The port number of FTP service (in network order).
diff -Nru a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c
--- a/net/ipv4/ipvs/ip_vs_ctl.c Tue Sep 16 14:08:55 2003
+++ b/net/ipv4/ipvs/ip_vs_ctl.c Tue Sep 16 14:08:55 2003
@@ -48,7 +48,7 @@
static DECLARE_MUTEX(__ip_vs_mutex);
/* lock for service table */
-rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
+static rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED;
/* lock for table with the real services */
static rwlock_t __ip_vs_rs_lock = RW_LOCK_UNLOCKED;
@@ -808,11 +808,6 @@
write_lock_bh(&__ip_vs_svc_lock);
- /*
- * Wait until all other svc users go away.
- */
- IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
list_add(&dest->n_list, &svc->destinations);
svc->num_dests++;
@@ -838,11 +833,6 @@
write_lock_bh(&__ip_vs_svc_lock);
- /*
- * Wait until all other svc users go away.
- */
- IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
list_add(&dest->n_list, &svc->destinations);
svc->num_dests++;
@@ -980,12 +970,6 @@
}
write_lock_bh(&__ip_vs_svc_lock);
-
- /*
- * Wait until all other svc users go away.
- */
- IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
/*
* Unlink dest from the service
*/
@@ -1118,11 +1102,6 @@
write_lock_bh(&__ip_vs_svc_lock);
/*
- * Wait until all other svc users go away.
- */
- IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
- /*
* Set the flags and timeout value
*/
svc->flags = u->flags | IP_VS_SVC_F_HASHED;
@@ -1235,11 +1214,6 @@
ip_vs_svc_unhash(svc);
- /*
- * Wait until all the svc users go away.
- */
- IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1);
-
__ip_vs_del_service(svc);
write_unlock_bh(&__ip_vs_svc_lock);
@@ -1259,16 +1233,11 @@
/*
* Flush the service table hashed by <protocol,addr,port>
*/
+ write_lock_bh(&__ip_vs_svc_lock);
for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
list_for_each_entry_safe(svc, nxt, &ip_vs_svc_table[idx], s_list) {
- write_lock_bh(&__ip_vs_svc_lock);
ip_vs_svc_unhash(svc);
- /*
- * Wait until all the svc users go away.
- */
- IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 0);
__ip_vs_del_service(svc);
- write_unlock_bh(&__ip_vs_svc_lock);
}
}
@@ -1278,16 +1247,11 @@
for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) {
list_for_each_entry_safe(svc, nxt,
&ip_vs_svc_fwm_table[idx], f_list) {
- write_lock_bh(&__ip_vs_svc_lock);
ip_vs_svc_unhash(svc);
- /*
- * Wait until all the svc users go away.
- */
- IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 0);
__ip_vs_del_service(svc);
- write_unlock_bh(&__ip_vs_svc_lock);
}
}
+ write_unlock_bh(&__ip_vs_svc_lock);
return 0;
}
@@ -1926,7 +1890,7 @@
static inline int
__ip_vs_get_service_entries(const struct ip_vs_get_services *get,
- struct ip_vs_get_services *uptr)
+ struct ip_vs_get_services __user *uptr)
{
int idx, count=0;
struct ip_vs_service *svc;
@@ -1966,7 +1930,7 @@
static inline int
__ip_vs_get_dest_entries(const struct ip_vs_get_dests *get,
- struct ip_vs_get_dests *uptr)
+ struct ip_vs_get_dests __user *uptr)
{
struct ip_vs_service *svc;
int ret = 0;
@@ -1981,6 +1945,7 @@
struct ip_vs_dest *dest;
struct ip_vs_dest_entry entry;
+ read_lock_bh(&__ip_vs_svc_lock);
list_for_each_entry(dest, &svc->destinations, n_list) {
if (count >= get->num_dests)
break;
@@ -1994,15 +1959,25 @@
entry.activeconns = atomic_read(&dest->activeconns);
entry.inactconns = atomic_read(&dest->inactconns);
entry.persistconns = atomic_read(&dest->persistconns);
+
ip_vs_copy_stats(&entry.stats, &dest->stats);
+
+ atomic_inc(&dest->refcnt);
+ read_unlock_bh(&__ip_vs_svc_lock);
if (copy_to_user(&uptr->entrytable[count],
&entry, sizeof(entry))) {
ret = -EFAULT;
break;
}
count++;
+
+ read_lock_bh(&__ip_vs_svc_lock);
+ if (atomic_dec_and_test(&dest->refcnt))
+ kfree(dest);
+
}
ip_vs_service_put(svc);
+ read_unlock_bh(&__ip_vs_svc_lock);
} else
ret = -ESRCH;
return ret;
@@ -2043,7 +2018,7 @@
};
static int
-do_ip_vs_get_ctl(struct sock *sk, int cmd, void *user, int *len)
+do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len)
{
unsigned char arg[128];
int ret = 0;
^ permalink raw reply [flat|nested] 5+ messages in thread* Re: [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly. 2003-09-16 21:21 [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly Stephen Hemminger @ 2003-09-17 16:44 ` Wensong Zhang 2003-09-20 8:00 ` David S. Miller 0 siblings, 1 reply; 5+ messages in thread From: Wensong Zhang @ 2003-09-17 16:44 UTC (permalink / raw) To: Stephen Hemminger; +Cc: lvs-users, netdev [-- Attachment #1: Type: TEXT/PLAIN, Size: 6219 bytes --] Hi, The locking change may be not very correct. On Tue, 16 Sep 2003, Stephen Hemminger wrote: > The code to get the service table was not taking a reader lock > because it needed to copy_to_user. > > Do it right: > - hold refcnt when copying out, and use a read lock There is no need to hold lock, because thread has already gained __ip_vs_mutex, there is no writer of service table, although net softirq may read service table, there is no conflict in reading the service table and copying out. > - make lock local to ip_svc Good. > - get rid of IP_VS_WAIT_WHILE, use proper read locks and refcounts. We have to use IP_VS_WAIT_WHILE to wait that all service users put back the service, because the user doesn't hold read locks all the time while using the service, it just increase the usecnt to indicate that the service is used and release the read lock immediately. Before the user put back the service, we cannot remove it. > - move write_lock_bh in flush to outside of loop Not necessary, because it has already hold __ip_vs_mutex, there is only one writer. > - tag user pointers (for sparse). > OK. So, maybe we just apply the simple patch with making the lock local and tagging user pointers. Thanks, Wensong > diff -Nru a/include/net/ip_vs.h b/include/net/ip_vs.h > --- a/include/net/ip_vs.h Tue Sep 16 14:08:55 2003 > +++ b/include/net/ip_vs.h Tue Sep 16 14:08:55 2003 > @@ -319,8 +319,6 @@ > #define LeaveFunction(level) do {} while (0) > #endif > > -#define IP_VS_WAIT_WHILE(expr) while (expr) { cpu_relax(); } > - > > /* > * The port number of FTP service (in network order). > diff -Nru a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c > --- a/net/ipv4/ipvs/ip_vs_ctl.c Tue Sep 16 14:08:55 2003 > +++ b/net/ipv4/ipvs/ip_vs_ctl.c Tue Sep 16 14:08:55 2003 > @@ -48,7 +48,7 @@ > static DECLARE_MUTEX(__ip_vs_mutex); > > /* lock for service table */ > -rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED; > +static rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED; > > /* lock for table with the real services */ > static rwlock_t __ip_vs_rs_lock = RW_LOCK_UNLOCKED; > @@ -808,11 +808,6 @@ > > write_lock_bh(&__ip_vs_svc_lock); > > - /* > - * Wait until all other svc users go away. > - */ > - IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1); > - > list_add(&dest->n_list, &svc->destinations); > svc->num_dests++; > > @@ -838,11 +833,6 @@ > > write_lock_bh(&__ip_vs_svc_lock); > > - /* > - * Wait until all other svc users go away. > - */ > - IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1); > - > list_add(&dest->n_list, &svc->destinations); > svc->num_dests++; > > @@ -980,12 +970,6 @@ > } > > write_lock_bh(&__ip_vs_svc_lock); > - > - /* > - * Wait until all other svc users go away. > - */ > - IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1); > - > /* > * Unlink dest from the service > */ > @@ -1118,11 +1102,6 @@ > write_lock_bh(&__ip_vs_svc_lock); > > /* > - * Wait until all other svc users go away. > - */ > - IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1); > - > - /* > * Set the flags and timeout value > */ > svc->flags = u->flags | IP_VS_SVC_F_HASHED; > @@ -1235,11 +1214,6 @@ > > ip_vs_svc_unhash(svc); > > - /* > - * Wait until all the svc users go away. > - */ > - IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 1); > - > __ip_vs_del_service(svc); > > write_unlock_bh(&__ip_vs_svc_lock); > @@ -1259,16 +1233,11 @@ > /* > * Flush the service table hashed by <protocol,addr,port> > */ > + write_lock_bh(&__ip_vs_svc_lock); > for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) { > list_for_each_entry_safe(svc, nxt, &ip_vs_svc_table[idx], s_list) { > - write_lock_bh(&__ip_vs_svc_lock); > ip_vs_svc_unhash(svc); > - /* > - * Wait until all the svc users go away. > - */ > - IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 0); > __ip_vs_del_service(svc); > - write_unlock_bh(&__ip_vs_svc_lock); > } > } > > @@ -1278,16 +1247,11 @@ > for(idx = 0; idx < IP_VS_SVC_TAB_SIZE; idx++) { > list_for_each_entry_safe(svc, nxt, > &ip_vs_svc_fwm_table[idx], f_list) { > - write_lock_bh(&__ip_vs_svc_lock); > ip_vs_svc_unhash(svc); > - /* > - * Wait until all the svc users go away. > - */ > - IP_VS_WAIT_WHILE(atomic_read(&svc->usecnt) > 0); > __ip_vs_del_service(svc); > - write_unlock_bh(&__ip_vs_svc_lock); > } > } > + write_unlock_bh(&__ip_vs_svc_lock); > > return 0; > } > @@ -1926,7 +1890,7 @@ > > static inline int > __ip_vs_get_service_entries(const struct ip_vs_get_services *get, > - struct ip_vs_get_services *uptr) > + struct ip_vs_get_services __user *uptr) > { > int idx, count=0; > struct ip_vs_service *svc; > @@ -1966,7 +1930,7 @@ > > static inline int > __ip_vs_get_dest_entries(const struct ip_vs_get_dests *get, > - struct ip_vs_get_dests *uptr) > + struct ip_vs_get_dests __user *uptr) > { > struct ip_vs_service *svc; > int ret = 0; > @@ -1981,6 +1945,7 @@ > struct ip_vs_dest *dest; > struct ip_vs_dest_entry entry; > > + read_lock_bh(&__ip_vs_svc_lock); > list_for_each_entry(dest, &svc->destinations, n_list) { > if (count >= get->num_dests) > break; > @@ -1994,15 +1959,25 @@ > entry.activeconns = atomic_read(&dest->activeconns); > entry.inactconns = atomic_read(&dest->inactconns); > entry.persistconns = atomic_read(&dest->persistconns); > + > ip_vs_copy_stats(&entry.stats, &dest->stats); > + > + atomic_inc(&dest->refcnt); > + read_unlock_bh(&__ip_vs_svc_lock); > if (copy_to_user(&uptr->entrytable[count], > &entry, sizeof(entry))) { > ret = -EFAULT; > break; > } > count++; > + > + read_lock_bh(&__ip_vs_svc_lock); > + if (atomic_dec_and_test(&dest->refcnt)) > + kfree(dest); > + > } > ip_vs_service_put(svc); > + read_unlock_bh(&__ip_vs_svc_lock); > } else > ret = -ESRCH; > return ret; > @@ -2043,7 +2018,7 @@ > }; > > static int > -do_ip_vs_get_ctl(struct sock *sk, int cmd, void *user, int *len) > +do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) > { > unsigned char arg[128]; > int ret = 0; > [-- Attachment #2: Type: TEXT/PLAIN, Size: 1282 bytes --] diff -Nru a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c --- a/net/ipv4/ipvs/ip_vs_ctl.c Thu Sep 18 00:43:00 2003 +++ b/net/ipv4/ipvs/ip_vs_ctl.c Thu Sep 18 00:43:00 2003 @@ -46,7 +46,7 @@ static DECLARE_MUTEX(__ip_vs_mutex); /* lock for service table */ -rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED; +static rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED; /* lock for table with the real services */ static rwlock_t __ip_vs_rs_lock = RW_LOCK_UNLOCKED; @@ -1912,7 +1912,7 @@ static inline int __ip_vs_get_service_entries(const struct ip_vs_get_services *get, - struct ip_vs_get_services *uptr) + struct ip_vs_get_services __user *uptr) { int idx, count=0; struct ip_vs_service *svc; @@ -1955,7 +1955,7 @@ static inline int __ip_vs_get_dest_entries(const struct ip_vs_get_dests *get, - struct ip_vs_get_dests *uptr) + struct ip_vs_get_dests __user *uptr) { struct ip_vs_service *svc; int ret = 0; @@ -2034,7 +2034,7 @@ }; static int -do_ip_vs_get_ctl(struct sock *sk, int cmd, void *user, int *len) +do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) { unsigned char arg[128]; int ret = 0; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly. 2003-09-17 16:44 ` Wensong Zhang @ 2003-09-20 8:00 ` David S. Miller 2003-09-20 15:39 ` Wensong Zhang 0 siblings, 1 reply; 5+ messages in thread From: David S. Miller @ 2003-09-20 8:00 UTC (permalink / raw) To: Wensong Zhang; +Cc: shemminger, lvs-users, netdev On Thu, 18 Sep 2003 00:44:16 +0800 (CST) Wensong Zhang <wensong@linux-vs.org> wrote: > So, maybe we just apply the simple patch with making the lock local and > tagging user pointers. Can someone put such a patch together for me? Thanks. ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly. 2003-09-20 8:00 ` David S. Miller @ 2003-09-20 15:39 ` Wensong Zhang 2003-09-22 0:53 ` David S. Miller 0 siblings, 1 reply; 5+ messages in thread From: Wensong Zhang @ 2003-09-20 15:39 UTC (permalink / raw) To: David S. Miller; +Cc: shemminger, lvs-users, netdev [-- Attachment #1: Type: TEXT/PLAIN, Size: 347 bytes --] On Sat, 20 Sep 2003, David S. Miller wrote: > On Thu, 18 Sep 2003 00:44:16 +0800 (CST) > Wensong Zhang <wensong@linux-vs.org> wrote: > > > So, maybe we just apply the simple patch with making the lock local and > > tagging user pointers. > > Can someone put such a patch together for me? > Please see the attached patch. Thanks, Wensong [-- Attachment #2: Type: TEXT/PLAIN, Size: 1831 bytes --] # This is a BitKeeper generated patch for the following project: # Project Name: Linux kernel tree # This patch format is intended for GNU patch command version 2.5 or higher. # This patch includes the following deltas: # ChangeSet 1.1315 -> 1.1316 # net/ipv4/ipvs/ip_vs_ctl.c 1.4 -> 1.5 # # The following is the BitKeeper ChangeSet Log # -------------------------------------------- # 03/09/18 shemminger@osdl.org 1.1316 # [IPV4] IPVS: some type declaration tidy up # # - make the __ip_vs_svc_lock local # - tag user pointers __user # -------------------------------------------- # diff -Nru a/net/ipv4/ipvs/ip_vs_ctl.c b/net/ipv4/ipvs/ip_vs_ctl.c --- a/net/ipv4/ipvs/ip_vs_ctl.c Sat Sep 20 22:55:39 2003 +++ b/net/ipv4/ipvs/ip_vs_ctl.c Sat Sep 20 22:55:39 2003 @@ -46,7 +46,7 @@ static DECLARE_MUTEX(__ip_vs_mutex); /* lock for service table */ -rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED; +static rwlock_t __ip_vs_svc_lock = RW_LOCK_UNLOCKED; /* lock for table with the real services */ static rwlock_t __ip_vs_rs_lock = RW_LOCK_UNLOCKED; @@ -1912,7 +1912,7 @@ static inline int __ip_vs_get_service_entries(const struct ip_vs_get_services *get, - struct ip_vs_get_services *uptr) + struct ip_vs_get_services __user *uptr) { int idx, count=0; struct ip_vs_service *svc; @@ -1955,7 +1955,7 @@ static inline int __ip_vs_get_dest_entries(const struct ip_vs_get_dests *get, - struct ip_vs_get_dests *uptr) + struct ip_vs_get_dests __user *uptr) { struct ip_vs_service *svc; int ret = 0; @@ -2034,7 +2034,7 @@ }; static int -do_ip_vs_get_ctl(struct sock *sk, int cmd, void *user, int *len) +do_ip_vs_get_ctl(struct sock *sk, int cmd, void __user *user, int *len) { unsigned char arg[128]; int ret = 0; ^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly. 2003-09-20 15:39 ` Wensong Zhang @ 2003-09-22 0:53 ` David S. Miller 0 siblings, 0 replies; 5+ messages in thread From: David S. Miller @ 2003-09-22 0:53 UTC (permalink / raw) To: Wensong Zhang; +Cc: shemminger, lvs-users, netdev On Sat, 20 Sep 2003 23:39:10 +0800 (CST) Wensong Zhang <wensong@linux-vs.org> wrote: > On Sat, 20 Sep 2003, David S. Miller wrote: > > > On Thu, 18 Sep 2003 00:44:16 +0800 (CST) > > Wensong Zhang <wensong@linux-vs.org> wrote: > > > > > So, maybe we just apply the simple patch with making the lock local and > > > tagging user pointers. > > > > Can someone put such a patch together for me? > > > > Please see the attached patch. Applied, thanks Wensong. ^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2003-09-22 0:53 UTC | newest] Thread overview: 5+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2003-09-16 21:21 [PATCH] (4/6) ipvs -- use reader locks and refcounts correctly Stephen Hemminger 2003-09-17 16:44 ` Wensong Zhang 2003-09-20 8:00 ` David S. Miller 2003-09-20 15:39 ` Wensong Zhang 2003-09-22 0:53 ` David S. Miller
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).