Re: [dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations

2017-01-03 Thread tang . junhui
Hello Ben,

I know the issue of socket's buffer fills up, 
but I think uevent_can_discard() totally process in memory,
it is light-weight and low cpu consumption, and it reduce the 
iteration count in merging, the test result is also good in
massive multipath devices scene. 

But if you still stick to it, I will revert it to uevents 
processing thread, and call it in uevent_dispatch() before 
calling merge_uevq(), actually, I'm going to do that.

Regards
Tang Junhui





发件人: "Benjamin Marzinski" 
收件人: tang.jun...@zte.com.cn, 
抄送:   tang.wenj...@zte.com.cn, zhang.ka...@zte.com.cn, 
dm-devel@redhat.com, bart.vanass...@sandisk.com, mwi...@suse.com
日期:   2017/01/04 06:38
主题:   Re: [dm-devel] [PATCH 08/12] libmultipath: wait one seconds for 
more uevents in uevent_listen() in uevents burst situations
发件人: dm-devel-boun...@redhat.com



On Tue, Dec 27, 2016 at 04:03:25PM +0800, tang.jun...@zte.com.cn wrote:
> From: tang.junhui 
> 
> The more uevents are merged, the higher efficiency program will 
performs.
> So, do not process uevents after receiving immediately in uevents burst
> situations, but continue wait 1 seconds for more uevents except that too
> much uevents (2048) have already been received or too much time eclipse
> (60 seconds).

The issue I have with doing this in uevent_listen is that we seperated
receiving the uevents from servicing the uevents specificially to make
sure what we received them as fast as possible. The udev monitor code
is all based on a NETLINK socket. If our socket's receive buffer fills
up, there is no flow control. Events just start getting dropped, which
does cut down on processing time, but not in a way we would like.

This issue applies to a lesser extent to you previous two patches. I
don't really see the benefit of making sure that we drop the uevents
as soon as possible.  As long as we don't process them, that should
be good enough, right?

Now, maybe you put a lot of thought into your timeouts, and feel
confident that we will start processing well before the receive buffer
fills up. But if so, some comments on that would be reassuring, because
from the commit message, they seem fairly arbitrary to me.

-Ben

> 
> Change-Id: I763d491540e8114a81d12d603281540a81502742
> Signed-off-by: tang.junhui 
> ---
>  libmultipath/uevent.c | 35 +--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index cc10d65..b0b05e9 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -39,6 +39,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
> 
> @@ -490,11 +491,37 @@ struct uevent *uevent_from_udev_device(struct 
udev_device *dev)
>return uev;
>  }
> 
> +bool uevent_burst(struct timeval *start_time, int events)
> +{
> +  struct timeval diff_time, end_time;
> +  unsigned long speed;
> +  unsigned long eclipse_ms;
> +
> +  gettimeofday(_time, NULL);
> +  timersub(_time, start_time, _time);
> +
> +  eclipse_ms = diff_time.tv_sec * 1000 + diff_time.tv_usec 
/ 1000;
> +  if (eclipse_ms == 0)
> +  return true;
> +  /* max wait 60s */
> +  if (eclipse_ms > 60*1000) {
> +  condlog(1, "burst continued =%lu ms, 
stoped", eclipse_ms);
> +  return false;
> +  }
> +
> +  speed = (events * 1000) / eclipse_ms;
> +  if (speed > 10)
> +  return true;
> +
> +  return false;
> +}
> +
>  int uevent_listen(struct udev *udev)
>  {
>int err = 2;
>struct udev_monitor *monitor = NULL;
>int fd, socket_flags, events;
> +  struct timeval start_time;
>int need_failback = 1;
>int timeout = 30;
>LIST_HEAD(uevlisten_tmp);
> @@ -548,6 +575,7 @@ int uevent_listen(struct udev *udev)
>}
> 
>events = 0;
> +  gettimeofday(_time, NULL);
>while (1) {
>struct uevent *uev;
>struct udev_device *dev;
> @@ -562,7 +590,8 @@ int uevent_listen(struct udev *udev)
>errno = 0;
>fdcount = poll(_poll, 1, 
poll_timeout);
>if (fdcount && ev_poll.revents & POLLIN) 
{
> -  timeout = 0;
> +  timeout = 
uevent_burst(_time, events + 1) ? 1:0;
> +
>dev = 
udev_monitor_receive_device(monitor);
>if (!dev) {
> condlog(0, "failed getting udev 

Re: [dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations

2017-01-03 Thread Benjamin Marzinski
On Tue, Dec 27, 2016 at 04:03:25PM +0800, tang.jun...@zte.com.cn wrote:
> From: tang.junhui 
> 
> The more uevents are merged, the higher efficiency program will performs.
> So, do not process uevents after receiving immediately in uevents burst
> situations, but continue wait 1 seconds for more uevents except that too
> much uevents (2048) have already been received or too much time eclipse
> (60 seconds).

The issue I have with doing this in uevent_listen is that we seperated
receiving the uevents from servicing the uevents specificially to make
sure what we received them as fast as possible. The udev monitor code
is all based on a NETLINK socket. If our socket's receive buffer fills
up, there is no flow control. Events just start getting dropped, which
does cut down on processing time, but not in a way we would like.

This issue applies to a lesser extent to you previous two patches. I
don't really see the benefit of making sure that we drop the uevents
as soon as possible.  As long as we don't process them, that should
be good enough, right?

Now, maybe you put a lot of thought into your timeouts, and feel
confident that we will start processing well before the receive buffer
fills up. But if so, some comments on that would be reassuring, because
from the commit message, they seem fairly arbitrary to me.

-Ben

> 
> Change-Id: I763d491540e8114a81d12d603281540a81502742
> Signed-off-by: tang.junhui 
> ---
>  libmultipath/uevent.c | 35 +--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index cc10d65..b0b05e9 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -39,6 +39,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -490,11 +491,37 @@ struct uevent *uevent_from_udev_device(struct 
> udev_device *dev)
>   return uev;
>  }
>  
> +bool uevent_burst(struct timeval *start_time, int events)
> +{
> + struct timeval diff_time, end_time;
> + unsigned long speed;
> + unsigned long eclipse_ms;
> +
> + gettimeofday(_time, NULL);
> + timersub(_time, start_time, _time);
> +
> + eclipse_ms = diff_time.tv_sec * 1000 + diff_time.tv_usec / 1000;
> + if (eclipse_ms == 0)
> + return true;
> + /* max wait 60s */
> + if (eclipse_ms > 60*1000) {
> + condlog(1, "burst continued =%lu ms, stoped", eclipse_ms);
> + return false;
> + }
> +
> + speed = (events * 1000) / eclipse_ms;
> + if (speed > 10)
> + return true;
> +
> + return false;
> +}
> +
>  int uevent_listen(struct udev *udev)
>  {
>   int err = 2;
>   struct udev_monitor *monitor = NULL;
>   int fd, socket_flags, events;
> + struct timeval start_time;
>   int need_failback = 1;
>   int timeout = 30;
>   LIST_HEAD(uevlisten_tmp);
> @@ -548,6 +575,7 @@ int uevent_listen(struct udev *udev)
>   }
>  
>   events = 0;
> + gettimeofday(_time, NULL);
>   while (1) {
>   struct uevent *uev;
>   struct udev_device *dev;
> @@ -562,7 +590,8 @@ int uevent_listen(struct udev *udev)
>   errno = 0;
>   fdcount = poll(_poll, 1, poll_timeout);
>   if (fdcount && ev_poll.revents & POLLIN) {
> - timeout = 0;
> + timeout = uevent_burst(_time, events + 1) ? 1:0;
> +
>   dev = udev_monitor_receive_device(monitor);
>   if (!dev) {
>   condlog(0, "failed getting udev device");
> @@ -578,7 +607,8 @@ int uevent_listen(struct udev *udev)
>   }
>   list_add_tail(>node, _tmp);
>   events++;
> - continue;
> + if(events < 2048)
> + continue;
>   }
>   if (fdcount < 0) {
>   if (errno == EINTR)
> @@ -600,6 +630,7 @@ int uevent_listen(struct udev *udev)
>   pthread_mutex_unlock(uevq_lockp);
>   events = 0;
>   }
> + gettimeofday(_time, NULL);
>   timeout = 30;
>   }
>   need_failback = 0;
> -- 
> 2.8.1.windows.1
> 

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


[dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations

2016-12-28 Thread tang . junhui
From: tang.junhui 

The more uevents are merged, the higher efficiency program will performs.
So, do not process uevents after receiving immediately in uevents burst
situations, but continue wait one seconds for more uevents except that
too much uevents (2048) have already been received or too much time
eclipse (30 seconds).

Change-Id: I763d491540e8114a81d12d603281540a81502742
Signed-off-by: tang.junhui 
---
 libmultipath/uevent.c | 42 +-
 1 file changed, 41 insertions(+), 1 deletion(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index cc10d65..0345e6e 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -39,6 +39,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -51,6 +52,10 @@
 #include "config.h"
 #include "blacklist.h"
 
+#define MAX_ACCUMULATION_COUNT 2048
+#define MAX_ACCUMULATION_TIME 30*1000
+#define MIN_BURST_SPEED 10
+
 typedef int (uev_trigger)(struct uevent *, void * trigger_data);
 
 LIST_HEAD(uevq);
@@ -490,11 +495,43 @@ struct uevent *uevent_from_udev_device(struct udev_device 
*dev)
return uev;
 }
 
+bool uevent_burst(struct timeval *start_time, int events)
+{
+   struct timeval diff_time, end_time;
+   unsigned long speed;
+   unsigned long eclipse_ms;
+
+   if(events > MAX_ACCUMULATION_COUNT) {
+   condlog(2, "burst got %u uevents, too much uevents, stopped", 
events);
+   return false;
+   }
+
+   gettimeofday(_time, NULL);
+   timersub(_time, start_time, _time);
+
+   eclipse_ms = diff_time.tv_sec * 1000 + diff_time.tv_usec / 1000;
+
+   if (eclipse_ms == 0)
+   return true;
+
+   if (eclipse_ms > MAX_ACCUMULATION_TIME) {
+   condlog(2, "burst continued %lu ms, too long time, stopped", 
eclipse_ms);
+   return false;
+   }
+
+   speed = (events * 1000) / eclipse_ms;
+   if (speed > MIN_BURST_SPEED)
+   return true;
+
+   return false;
+}
+
 int uevent_listen(struct udev *udev)
 {
int err = 2;
struct udev_monitor *monitor = NULL;
int fd, socket_flags, events;
+   struct timeval start_time;
int need_failback = 1;
int timeout = 30;
LIST_HEAD(uevlisten_tmp);
@@ -548,6 +585,7 @@ int uevent_listen(struct udev *udev)
}
 
events = 0;
+   gettimeofday(_time, NULL);
while (1) {
struct uevent *uev;
struct udev_device *dev;
@@ -562,7 +600,8 @@ int uevent_listen(struct udev *udev)
errno = 0;
fdcount = poll(_poll, 1, poll_timeout);
if (fdcount && ev_poll.revents & POLLIN) {
-   timeout = 0;
+   timeout = uevent_burst(_time, events + 1) ? 1 : 0;
+
dev = udev_monitor_receive_device(monitor);
if (!dev) {
condlog(0, "failed getting udev device");
@@ -600,6 +639,7 @@ int uevent_listen(struct udev *udev)
pthread_mutex_unlock(uevq_lockp);
events = 0;
}
+   gettimeofday(_time, NULL);
timeout = 30;
}
need_failback = 0;
-- 
2.8.1.windows.1


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel


Re: [dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations

2016-12-28 Thread Martin Wilck
On Tue, 2016-12-27 at 16:03 +0800, tang.jun...@zte.com.cn wrote:
> From: tang.junhui 
> 
> The more uevents are merged, the higher efficiency program will
> performs.
> So, do not process uevents after receiving immediately in uevents
> burst
> situations, but continue wait 1 seconds for more uevents except that
> too
> much uevents (2048) have already been received or too much time
> eclipse
> (60 seconds).

Hi Tang,

thanks for this impressive patch set. While I haven't had time to read
through the more complex parts yet, one small nitpick on this patch:
please use named constants rather than explicit numbers in the code.

Regards,
Martin


> 
> Change-Id: I763d491540e8114a81d12d603281540a81502742
> Signed-off-by: tang.junhui 
> ---
>  libmultipath/uevent.c | 35 +--
>  1 file changed, 33 insertions(+), 2 deletions(-)
> 
> diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
> index cc10d65..b0b05e9 100644
> --- a/libmultipath/uevent.c
> +++ b/libmultipath/uevent.c
> @@ -39,6 +39,7 @@
>  #include 
>  #include 
>  #include 
> +#include 
>  #include 
>  #include 
>  
> @@ -490,11 +491,37 @@ struct uevent *uevent_from_udev_device(struct
> udev_device *dev)
>   return uev;
>  }
>  
> +bool uevent_burst(struct timeval *start_time, int events)
> +{
> + struct timeval diff_time, end_time;
> + unsigned long speed;
> + unsigned long eclipse_ms;
> +
> + gettimeofday(_time, NULL);
> + timersub(_time, start_time, _time);
> +
> + eclipse_ms = diff_time.tv_sec * 1000 + diff_time.tv_usec /
> 1000;
> + if (eclipse_ms == 0)
> + return true;
> + /* max wait 60s */
> + if (eclipse_ms > 60*1000) {
> + condlog(1, "burst continued =%lu ms, stoped",
> eclipse_ms);
> + return false;
> + }
> +
> + speed = (events * 1000) / eclipse_ms;
> + if (speed > 10)
> + return true;
> +
> + return false;
> +}
> +
>  int uevent_listen(struct udev *udev)
>  {
>   int err = 2;
>   struct udev_monitor *monitor = NULL;
>   int fd, socket_flags, events;
> + struct timeval start_time;
>   int need_failback = 1;
>   int timeout = 30;
>   LIST_HEAD(uevlisten_tmp);
> @@ -548,6 +575,7 @@ int uevent_listen(struct udev *udev)
>   }
>  
>   events = 0;
> + gettimeofday(_time, NULL);
>   while (1) {
>   struct uevent *uev;
>   struct udev_device *dev;
> @@ -562,7 +590,8 @@ int uevent_listen(struct udev *udev)
>   errno = 0;
>   fdcount = poll(_poll, 1, poll_timeout);
>   if (fdcount && ev_poll.revents & POLLIN) {
> - timeout = 0;
> + timeout = uevent_burst(_time, events +
> 1) ? 1:0;
> +
>   dev = udev_monitor_receive_device(monitor);
>   if (!dev) {
>   condlog(0, "failed getting udev
> device");
> @@ -578,7 +607,8 @@ int uevent_listen(struct udev *udev)
>   }
>   list_add_tail(>node, _tmp);
>   events++;
> - continue;
> + if(events < 2048)
> + continue;
>   }
>   if (fdcount < 0) {
>   if (errno == EINTR)
> @@ -600,6 +630,7 @@ int uevent_listen(struct udev *udev)
>   pthread_mutex_unlock(uevq_lockp);
>   events = 0;
>   }
> + gettimeofday(_time, NULL);
>   timeout = 30;
>   }
>   need_failback = 0;

-- 
Dr. Martin Wilck , Tel. +49 (0)911 74053 2107
SUSE Linux GmbH, GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)

--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel

[dm-devel] [PATCH 08/12] libmultipath: wait one seconds for more uevents in uevent_listen() in uevents burst situations

2016-12-27 Thread tang . junhui
From: tang.junhui 

The more uevents are merged, the higher efficiency program will performs.
So, do not process uevents after receiving immediately in uevents burst
situations, but continue wait 1 seconds for more uevents except that too
much uevents (2048) have already been received or too much time eclipse
(60 seconds).

Change-Id: I763d491540e8114a81d12d603281540a81502742
Signed-off-by: tang.junhui 
---
 libmultipath/uevent.c | 35 +--
 1 file changed, 33 insertions(+), 2 deletions(-)

diff --git a/libmultipath/uevent.c b/libmultipath/uevent.c
index cc10d65..b0b05e9 100644
--- a/libmultipath/uevent.c
+++ b/libmultipath/uevent.c
@@ -39,6 +39,7 @@
 #include 
 #include 
 #include 
+#include 
 #include 
 #include 
 
@@ -490,11 +491,37 @@ struct uevent *uevent_from_udev_device(struct udev_device 
*dev)
return uev;
 }
 
+bool uevent_burst(struct timeval *start_time, int events)
+{
+   struct timeval diff_time, end_time;
+   unsigned long speed;
+   unsigned long eclipse_ms;
+
+   gettimeofday(_time, NULL);
+   timersub(_time, start_time, _time);
+
+   eclipse_ms = diff_time.tv_sec * 1000 + diff_time.tv_usec / 1000;
+   if (eclipse_ms == 0)
+   return true;
+   /* max wait 60s */
+   if (eclipse_ms > 60*1000) {
+   condlog(1, "burst continued =%lu ms, stoped", eclipse_ms);
+   return false;
+   }
+
+   speed = (events * 1000) / eclipse_ms;
+   if (speed > 10)
+   return true;
+
+   return false;
+}
+
 int uevent_listen(struct udev *udev)
 {
int err = 2;
struct udev_monitor *monitor = NULL;
int fd, socket_flags, events;
+   struct timeval start_time;
int need_failback = 1;
int timeout = 30;
LIST_HEAD(uevlisten_tmp);
@@ -548,6 +575,7 @@ int uevent_listen(struct udev *udev)
}
 
events = 0;
+   gettimeofday(_time, NULL);
while (1) {
struct uevent *uev;
struct udev_device *dev;
@@ -562,7 +590,8 @@ int uevent_listen(struct udev *udev)
errno = 0;
fdcount = poll(_poll, 1, poll_timeout);
if (fdcount && ev_poll.revents & POLLIN) {
-   timeout = 0;
+   timeout = uevent_burst(_time, events + 1) ? 1:0;
+
dev = udev_monitor_receive_device(monitor);
if (!dev) {
condlog(0, "failed getting udev device");
@@ -578,7 +607,8 @@ int uevent_listen(struct udev *udev)
}
list_add_tail(>node, _tmp);
events++;
-   continue;
+   if(events < 2048)
+   continue;
}
if (fdcount < 0) {
if (errno == EINTR)
@@ -600,6 +630,7 @@ int uevent_listen(struct udev *udev)
pthread_mutex_unlock(uevq_lockp);
events = 0;
}
+   gettimeofday(_time, NULL);
timeout = 30;
}
need_failback = 0;
-- 
2.8.1.windows.1


--
dm-devel mailing list
dm-devel@redhat.com
https://www.redhat.com/mailman/listinfo/dm-devel