Re: [RFC] ipvs: Cleanup sync daemon code

2008-02-10 Thread Sven Wegener 

On Sat, 9 Feb 2008, Christoph Hellwig wrote:


On Sun, Feb 10, 2008 at 12:38:11AM +0100, Sven Wegener wrote:

 struct ip_vs_sync_thread_data {
-   struct completion *startup;
+   struct completion *startup; /* set to NULL once completed */


This is not needed anmore.  kthread_run guarantees that the newly
creates thread is run before returning to the caller.


The completion is currently used to return an error code for errors that 
happen during initialization in the threads (open socket, allocate 
memory). We could move the setup code out of the threads and have them 
only run an error-safe loop.



+/* wait queue for master sync daemon */
+static DECLARE_WAIT_QUEUE_HEAD(sync_master_wait);


I don't think you need this one either.  You can use wake_up_process
on the task_struct pointer instead.


Thanks, now using schedule_timeout with wake_up_process.


spin_lock(ip_vs_sync_lock);
list_add_tail(sb-list, ip_vs_sync_queue);
+   if (++ip_vs_sync_count == 10)
+   wake_up_interruptible(sync_master_wait);
spin_unlock(ip_vs_sync_lock);
 }



-static int sync_thread(void *startup)
+static int sync_thread(void *data)


Btw, it might make sense to remove sync_thread and just call the
master and backup threads directly.


When the setup code has been moved out of the threads, the code gets much 
simpler.



+void __init ip_vs_sync_init(void)
+{
+   /* set up multicast address */
+   mcast_addr.sin_family = AF_INET;
+   mcast_addr.sin_port = htons(IP_VS_SYNC_PORT);
+   mcast_addr.sin_addr.s_addr = htonl(IP_VS_SYNC_GROUP);
 }


Why can't this be initialized at compile time by:

static struct sockaddr_in mcast_addr = {
.sin_family = AF_INET,
.sin_port   = htons(IP_VS_SYNC_PORT),
.sin_addr.s_addr= htonl(IP_VS_SYNC_GROUP),
}

(the hton* might need __constant_hton* also I'm not sure without trying)


Thanks.
--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[RFC] ipvs: Cleanup sync daemon code

2008-02-09 Thread Sven Wegener 

Hi all,

I'd like to get your feedback on this:

- Use kthread_run instead of doing a double-fork via kernel_thread()

- Return proper error codes to user-space on failures

Currently ipvsadm --start-daemon with an invalid --mcast-interface will 
silently suceed. With these changes we get an appropriate No such device 
error.


- Use wait queues for both master and backup thread

Instead of doing an endless loop with sleeping for one second, we now use 
wait queues. The master sync daemon has its own wait queue and gets woken 
up when we have enough data to sent and also at a regular interval. The 
backup sync daemon sits on the wait queue of the mcast socket and gets 
woken up as soon as we have data to process.


diff --git a/include/net/ip_vs.h b/include/net/ip_vs.h
index 56f3c94..519bd96 100644
--- a/include/net/ip_vs.h
+++ b/include/net/ip_vs.h
@@ -890,6 +890,7 @@ extern char ip_vs_backup_mcast_ifn[IP_VS_IFNAME_MAXLEN];
 extern int start_sync_thread(int state, char *mcast_ifn, __u8 syncid);
 extern int stop_sync_thread(int state);
 extern void ip_vs_sync_conn(struct ip_vs_conn *cp);
+extern void ip_vs_sync_init(void);


 /*
diff --git a/net/ipv4/ipvs/ip_vs_core.c b/net/ipv4/ipvs/ip_vs_core.c
index 963981a..0ccee4b 100644
--- a/net/ipv4/ipvs/ip_vs_core.c
+++ b/net/ipv4/ipvs/ip_vs_core.c
@@ -1071,6 +1071,8 @@ static int __init ip_vs_init(void)
 {
int ret;

+   ip_vs_sync_init();
+
ret = ip_vs_control_init();
if (ret  0) {
IP_VS_ERR(can't setup control.\n);
diff --git a/net/ipv4/ipvs/ip_vs_sync.c b/net/ipv4/ipvs/ip_vs_sync.c
index 948378d..36063d3 100644
--- a/net/ipv4/ipvs/ip_vs_sync.c
+++ b/net/ipv4/ipvs/ip_vs_sync.c
@@ -29,6 +29,9 @@
 #include linux/in.h
 #include linux/igmp.h /* for ip_mc_join_group */
 #include linux/udp.h
+#include linux/kthread.h
+#include linux/wait.h
+#include linux/err.h

 #include net/ip.h
 #include net/sock.h
@@ -68,7 +71,8 @@ struct ip_vs_sync_conn_options {
 };

 struct ip_vs_sync_thread_data {
-   struct completion *startup;
+   struct completion *startup; /* set to NULL once completed */
+   int *retval; /* only valid until startup is completed */
int state;
 };

@@ -123,9 +127,10 @@ struct ip_vs_sync_buff {
 };


-/* the sync_buff list head and the lock */
+/* the sync_buff list head, the lock and the counter */
 static LIST_HEAD(ip_vs_sync_queue);
 static DEFINE_SPINLOCK(ip_vs_sync_lock);
+static unsigned int ip_vs_sync_count;

 /* current sync_buff for accepting new conn entries */
 static struct ip_vs_sync_buff   *curr_sb = NULL;
@@ -140,6 +145,13 @@ volatile int ip_vs_backup_syncid = 0;
 char ip_vs_master_mcast_ifn[IP_VS_IFNAME_MAXLEN];
 char ip_vs_backup_mcast_ifn[IP_VS_IFNAME_MAXLEN];

+/* sync daemon tasks */
+static struct task_struct *sync_master_thread;
+static struct task_struct *sync_backup_thread;
+
+/* wait queue for master sync daemon */
+static DECLARE_WAIT_QUEUE_HEAD(sync_master_wait);
+
 /* multicast addr */
 static struct sockaddr_in mcast_addr;

@@ -148,6 +160,8 @@ static inline void sb_queue_tail(struct ip_vs_sync_buff *sb)
 {
spin_lock(ip_vs_sync_lock);
list_add_tail(sb-list, ip_vs_sync_queue);
+   if (++ip_vs_sync_count == 10)
+   wake_up_interruptible(sync_master_wait);
spin_unlock(ip_vs_sync_lock);
 }

@@ -163,6 +177,7 @@ static inline struct ip_vs_sync_buff * sb_dequeue(void)
struct ip_vs_sync_buff,
list);
list_del(sb-list);
+   ip_vs_sync_count--;
}
spin_unlock_bh(ip_vs_sync_lock);

@@ -536,14 +551,17 @@ static int bind_mcastif_addr(struct socket *sock, char 
*ifname)
 static struct socket * make_send_sock(void)
 {
struct socket *sock;
+   int result;

/* First create a socket */
-   if (sock_create_kern(PF_INET, SOCK_DGRAM, IPPROTO_UDP, sock)  0) {
+   result = sock_create_kern(PF_INET, SOCK_DGRAM, IPPROTO_UDP, sock);
+   if (result  0) {
IP_VS_ERR(Error during creation of socket; terminating\n);
-   return NULL;
+   return ERR_PTR(result);
}

-   if (set_mcast_if(sock-sk, ip_vs_master_mcast_ifn)  0) {
+   result = set_mcast_if(sock-sk, ip_vs_master_mcast_ifn);
+   if (result  0) {
IP_VS_ERR(Error setting outbound mcast interface\n);
goto error;
}
@@ -551,14 +569,16 @@ static struct socket * make_send_sock(void)
set_mcast_loop(sock-sk, 0);
set_mcast_ttl(sock-sk, 1);

-   if (bind_mcastif_addr(sock, ip_vs_master_mcast_ifn)  0) {
+   result = bind_mcastif_addr(sock, ip_vs_master_mcast_ifn);
+   if (result  0) {
IP_VS_ERR(Error binding address of the mcast interface\n);
goto error;
}

-   if (sock-ops-connect(sock,
-  (struct sockaddr*)mcast_addr,
-  sizeof(struct

Re: [RFC] ipvs: Cleanup sync daemon code

2008-02-09 Thread Simon Horman 
On Sun, Feb 10, 2008 at 12:38:11AM +0100, Sven Wegener wrote:
 Hi all,

 I'd like to get your feedback on this:

 - Use kthread_run instead of doing a double-fork via kernel_thread()

 - Return proper error codes to user-space on failures

 Currently ipvsadm --start-daemon with an invalid --mcast-interface will  
 silently suceed. With these changes we get an appropriate No such 
 device error.

 - Use wait queues for both master and backup thread

 Instead of doing an endless loop with sleeping for one second, we now use 
 wait queues. The master sync daemon has its own wait queue and gets woken 
 up when we have enough data to sent and also at a regular interval. The  
 backup sync daemon sits on the wait queue of the mcast socket and gets  
 woken up as soon as we have data to process.

Hi Sven,

This looks good to me, assuming that its tested and works.

A few minor things:

In sb_queue_tail() master loop is woken up if
the ip_vs_sync_count reaches 10, which seems a bit arbitary.

Perhaps its just my mail reader, but the patch seemed a bit screwy when
I saved it to a file. I this fixed the problem I was seeing using s/^  / /

Unfortuantely/Fortunately I am about to leave for a few days skiing,
so if I am quiet you will know why.

Acked-by: Simon Horman [EMAIL PROTECTED]

-- 
Horms

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC] ipvs: Cleanup sync daemon code

2008-02-09 Thread Christoph Hellwig 
On Sun, Feb 10, 2008 at 12:38:11AM +0100, Sven Wegener wrote:
  struct ip_vs_sync_thread_data {
 - struct completion *startup;
 + struct completion *startup; /* set to NULL once completed */

This is not needed anmore.  kthread_run guarantees that the newly
creates thread is run before returning to the caller.

 +/* wait queue for master sync daemon */
 +static DECLARE_WAIT_QUEUE_HEAD(sync_master_wait);

I don't think you need this one either.  You can use wake_up_process
on the task_struct pointer instead.

   spin_lock(ip_vs_sync_lock);
   list_add_tail(sb-list, ip_vs_sync_queue);
 + if (++ip_vs_sync_count == 10)
 + wake_up_interruptible(sync_master_wait);
   spin_unlock(ip_vs_sync_lock);
  }

 -static int sync_thread(void *startup)
 +static int sync_thread(void *data)

Btw, it might make sense to remove sync_thread and just call the
master and backup threads directly.
 +void __init ip_vs_sync_init(void)
 +{
 + /* set up multicast address */
 + mcast_addr.sin_family = AF_INET;
 + mcast_addr.sin_port = htons(IP_VS_SYNC_PORT);
 + mcast_addr.sin_addr.s_addr = htonl(IP_VS_SYNC_GROUP);
  }

Why can't this be initialized at compile time by:

static struct sockaddr_in mcast_addr = {
.sin_family = AF_INET,
.sin_port   = htons(IP_VS_SYNC_PORT),
.sin_addr.s_addr= htonl(IP_VS_SYNC_GROUP),
}

(the hton* might need __constant_hton* also I'm not sure without trying)

--
To unsubscribe from this list: send the line unsubscribe netdev in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html