Re: [PATCH V8 1/6] LIBIO: Introduce a generic PIO mapping method

[kbuild test robot]

Hi zhichang.yuan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc4 next-20170331]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/zhichang-yuan/LIBIO-Introduce-a-generic-PIO-mapping-method/20170401-104801
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=alpha 

All error/warnings (new ones prefixed by >>):

>> lib/logic_pio.c:32:50: error: 'PIO_MAX_SECT' undeclared here (not in a 
>> function)
static struct logic_pio_root logic_pio_root_list[PIO_MAX_SECT] = {
 ^~~~
>> lib/logic_pio.c:39:3: error: 'PIO_CPU_MMIO' undeclared here (not in a 
>> function)
 [PIO_CPU_MMIO ... PIO_INDIRECT - 1] = {
  ^~~~
>> lib/logic_pio.c:39:20: error: 'PIO_INDIRECT' undeclared here (not in a 
>> function)
 [PIO_CPU_MMIO ... PIO_INDIRECT - 1] = {
   ^~~~
>> lib/logic_pio.c:39:3: error: array index in initializer not of integer type
 [PIO_CPU_MMIO ... PIO_INDIRECT - 1] = {
  ^~~~
   lib/logic_pio.c:39:3: note: (near initialization for 'logic_pio_root_list')
>> lib/logic_pio.c:40:3: error: field name not in record or union initializer
  .sec_head = LIST_HEAD_INIT(logic_pio_root_list[PIO_CPU_MMIO].sec_head),
  ^
   lib/logic_pio.c:40:3: note: (near initialization for 'logic_pio_root_list')
   lib/logic_pio.c:41:3: error: field name not in record or union initializer
  .sec_min = PIO_SECT_MIN(PIO_CPU_MMIO),
  ^
   lib/logic_pio.c:41:3: note: (near initialization for 'logic_pio_root_list')
>> lib/logic_pio.c:41:14: error: implicit declaration of function 
>> 'PIO_SECT_MIN' [-Werror=implicit-function-declaration]
  .sec_min = PIO_SECT_MIN(PIO_CPU_MMIO),
 ^~~~
   lib/logic_pio.c:42:3: error: field name not in record or union initializer
  .sec_max = PIO_SECT_MAX(PIO_INDIRECT - 1),
  ^
   lib/logic_pio.c:42:3: note: (near initialization for 'logic_pio_root_list')
>> lib/logic_pio.c:42:14: error: implicit declaration of function 
>> 'PIO_SECT_MAX' [-Werror=implicit-function-declaration]
  .sec_max = PIO_SECT_MAX(PIO_INDIRECT - 1),
 ^~~~
   lib/logic_pio.c:46:3: error: array index in initializer not of integer type
 [PIO_INDIRECT] = {
  ^~~~
   lib/logic_pio.c:46:3: note: (near initialization for 'logic_pio_root_list')
   lib/logic_pio.c:47:3: error: field name not in record or union initializer
  .sec_head = LIST_HEAD_INIT(logic_pio_root_list[PIO_INDIRECT].sec_head),
  ^
   lib/logic_pio.c:47:3: note: (near initialization for 'logic_pio_root_list')
   lib/logic_pio.c:48:3: error: field name not in record or union initializer
  .sec_min = PIO_SECT_MIN(PIO_INDIRECT),
  ^
   lib/logic_pio.c:48:3: note: (near initialization for 'logic_pio_root_list')
   lib/logic_pio.c:49:3: error: field name not in record or union initializer
  .sec_max = PIO_SECT_MAX(PIO_INDIRECT),
  ^
   lib/logic_pio.c:49:3: note: (near initialization for 'logic_pio_root_list')
   In file included from include/linux/list.h:8:0,
from include/linux/kobject.h:20,
from include/linux/of.h:21,
from lib/logic_pio.c:18:
   lib/logic_pio.c: In function 'logic_pio_find_range_byaddr':
>> include/linux/rculist.h:351:49: error: dereferencing pointer to incomplete 
>> type 'struct logic_pio_hwaddr'
 for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
 
   include/linux/kernel.h:852:18: note: in definition of macro 'container_of'
 const typeof( ((type *)0)->member ) *__mptr = (ptr); \
 ^~~~
>> include/linux/rculist.h:351:13: note: in expansion of macro 'list_entry_rcu'
 for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
^~
>> lib/logic_pio.c:77:2: note: in expansion of macro 'list_for_each_entry_rcu'
 list_for_each_entry_rcu(range, _range_list, list) {
 ^~~
   include/linux/kernel.h:852:48: error: initialization from incompatible 
pointer type [-Werror=incompatible-pointer-types]
 const typeof( ((type *)0)->member ) *__mptr = (ptr); \
   ^
>> include/linux/rculist.h:277:2: note: in expansion of macro 'container_of'
 container_of(lockless_dereference(ptr), type, member)
 ^~~~
>> include/linux/rc

Re: [PATCH V8 1/6] LIBIO: Introduce a generic PIO mapping method

[kbuild test robot]

Hi zhichang.yuan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc4 next-20170331]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/zhichang-yuan/LIBIO-Introduce-a-generic-PIO-mapping-method/20170401-104801
config: alpha-allyesconfig (attached as .config)
compiler: alpha-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=alpha 

All error/warnings (new ones prefixed by >>):

>> lib/logic_pio.c:32:50: error: 'PIO_MAX_SECT' undeclared here (not in a 
>> function)
static struct logic_pio_root logic_pio_root_list[PIO_MAX_SECT] = {
 ^~~~
>> lib/logic_pio.c:39:3: error: 'PIO_CPU_MMIO' undeclared here (not in a 
>> function)
 [PIO_CPU_MMIO ... PIO_INDIRECT - 1] = {
  ^~~~
>> lib/logic_pio.c:39:20: error: 'PIO_INDIRECT' undeclared here (not in a 
>> function)
 [PIO_CPU_MMIO ... PIO_INDIRECT - 1] = {
   ^~~~
>> lib/logic_pio.c:39:3: error: array index in initializer not of integer type
 [PIO_CPU_MMIO ... PIO_INDIRECT - 1] = {
  ^~~~
   lib/logic_pio.c:39:3: note: (near initialization for 'logic_pio_root_list')
>> lib/logic_pio.c:40:3: error: field name not in record or union initializer
  .sec_head = LIST_HEAD_INIT(logic_pio_root_list[PIO_CPU_MMIO].sec_head),
  ^
   lib/logic_pio.c:40:3: note: (near initialization for 'logic_pio_root_list')
   lib/logic_pio.c:41:3: error: field name not in record or union initializer
  .sec_min = PIO_SECT_MIN(PIO_CPU_MMIO),
  ^
   lib/logic_pio.c:41:3: note: (near initialization for 'logic_pio_root_list')
>> lib/logic_pio.c:41:14: error: implicit declaration of function 
>> 'PIO_SECT_MIN' [-Werror=implicit-function-declaration]
  .sec_min = PIO_SECT_MIN(PIO_CPU_MMIO),
 ^~~~
   lib/logic_pio.c:42:3: error: field name not in record or union initializer
  .sec_max = PIO_SECT_MAX(PIO_INDIRECT - 1),
  ^
   lib/logic_pio.c:42:3: note: (near initialization for 'logic_pio_root_list')
>> lib/logic_pio.c:42:14: error: implicit declaration of function 
>> 'PIO_SECT_MAX' [-Werror=implicit-function-declaration]
  .sec_max = PIO_SECT_MAX(PIO_INDIRECT - 1),
 ^~~~
   lib/logic_pio.c:46:3: error: array index in initializer not of integer type
 [PIO_INDIRECT] = {
  ^~~~
   lib/logic_pio.c:46:3: note: (near initialization for 'logic_pio_root_list')
   lib/logic_pio.c:47:3: error: field name not in record or union initializer
  .sec_head = LIST_HEAD_INIT(logic_pio_root_list[PIO_INDIRECT].sec_head),
  ^
   lib/logic_pio.c:47:3: note: (near initialization for 'logic_pio_root_list')
   lib/logic_pio.c:48:3: error: field name not in record or union initializer
  .sec_min = PIO_SECT_MIN(PIO_INDIRECT),
  ^
   lib/logic_pio.c:48:3: note: (near initialization for 'logic_pio_root_list')
   lib/logic_pio.c:49:3: error: field name not in record or union initializer
  .sec_max = PIO_SECT_MAX(PIO_INDIRECT),
  ^
   lib/logic_pio.c:49:3: note: (near initialization for 'logic_pio_root_list')
   In file included from include/linux/list.h:8:0,
from include/linux/kobject.h:20,
from include/linux/of.h:21,
from lib/logic_pio.c:18:
   lib/logic_pio.c: In function 'logic_pio_find_range_byaddr':
>> include/linux/rculist.h:351:49: error: dereferencing pointer to incomplete 
>> type 'struct logic_pio_hwaddr'
 for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
 
   include/linux/kernel.h:852:18: note: in definition of macro 'container_of'
 const typeof( ((type *)0)->member ) *__mptr = (ptr); \
 ^~~~
>> include/linux/rculist.h:351:13: note: in expansion of macro 'list_entry_rcu'
 for (pos = list_entry_rcu((head)->next, typeof(*pos), member); \
^~
>> lib/logic_pio.c:77:2: note: in expansion of macro 'list_for_each_entry_rcu'
 list_for_each_entry_rcu(range, _range_list, list) {
 ^~~
   include/linux/kernel.h:852:48: error: initialization from incompatible 
pointer type [-Werror=incompatible-pointer-types]
 const typeof( ((type *)0)->member ) *__mptr = (ptr); \
   ^
>> include/linux/rculist.h:277:2: note: in expansion of macro 'container_of'
 container_of(lockless_dereference(ptr), type, member)
 ^~~~
>> include/linux/rc

[RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped

[Eric W. Biederman]

Take advantage of the situation when sighand->count == 1 to only wait
for threads to reach EXIT_ZOMBIE instead of EXIT_DEAD in de_thread.
Only old old linux threading libraries use CLONE_SIGHAND without
CLONE_THREAD.  So this situation should be present most of the time.

This allows ptracing through a multi-threaded exec without the danger
of stalling the exec.  As historically exec waits for the other
threads to be reaped in de_thread before completing.  This is
necessary as it is not safe to unshare the sighand_struct until all of
the other threads in this thread group are reaped, because the lock to
serialize threads in a thread group siglock lives in sighand_struct.

When oldsighand->count == 1 we know that there are no other
users and unsharing the sighand struct in exec is pointless.
This makes it safe to only wait for threads to become zombies
as the siglock won't change during exec and release_task
will use the samve siglock for the old threads as for
the new threads.

Cc: sta...@vger.kernel.org
Signed-off-by: "Eric W. Biederman" 
---
 fs/exec.c| 15 ++-
 include/linux/sched/signal.h |  2 +-
 kernel/exit.c| 13 +
 kernel/signal.c  |  8 ++--
 4 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 65145a3df065..0fd29342bbe4 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1052,6 +1052,7 @@ static int de_thread(struct task_struct *tsk)
struct signal_struct *sig = tsk->signal;
struct sighand_struct *oldsighand = tsk->sighand;
spinlock_t *lock = >siglock;
+   bool may_hang;
 
if (thread_group_empty(tsk))
goto no_thread_group;
@@ -1069,9 +1070,10 @@ static int de_thread(struct task_struct *tsk)
return -EAGAIN;
}
 
+   may_hang = atomic_read(>count) != 1;
sig->group_exit_task = tsk;
-   sig->notify_count = zap_other_threads(tsk);
-   if (!thread_group_leader(tsk))
+   sig->notify_count = zap_other_threads(tsk, may_hang ? 1 : -1);
+   if (may_hang && !thread_group_leader(tsk))
sig->notify_count--;
 
while (sig->notify_count) {
@@ -1092,9 +1094,10 @@ static int de_thread(struct task_struct *tsk)
if (!thread_group_leader(tsk)) {
struct task_struct *leader = tsk->group_leader;
 
-   for (;;) {
-   cgroup_threadgroup_change_begin(tsk);
-   write_lock_irq(_lock);
+   cgroup_threadgroup_change_begin(tsk);
+   write_lock_irq(_lock);
+
+   for (;may_hang;) {
/*
 * Do this under tasklist_lock to ensure that
 * exit_notify() can't miss ->group_exit_task
@@ -1108,6 +,8 @@ static int de_thread(struct task_struct *tsk)
schedule();
if (unlikely(__fatal_signal_pending(tsk)))
goto killed;
+   cgroup_threadgroup_change_begin(tsk);
+   write_lock_irq(_lock);
}
 
/*
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 2cf446704cd4..187a9e980d3a 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -298,7 +298,7 @@ extern __must_check bool do_notify_parent(struct 
task_struct *, int);
 extern void __wake_up_parent(struct task_struct *p, struct task_struct 
*parent);
 extern void force_sig(int, struct task_struct *);
 extern int send_sig(int, struct task_struct *, int);
-extern int zap_other_threads(struct task_struct *p);
+extern int zap_other_threads(struct task_struct *p, int do_count);
 extern struct sigqueue *sigqueue_alloc(void);
 extern void sigqueue_free(struct sigqueue *);
 extern int send_sigqueue(struct sigqueue *,  struct task_struct *, int group);
diff --git a/kernel/exit.c b/kernel/exit.c
index 8c5b3e106298..972df5ebf79f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -712,6 +712,8 @@ static void forget_original_parent(struct task_struct 
*father,
  */
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
+   struct sighand_struct *sighand = tsk->sighand;
+   struct signal_struct *signal = tsk->signal;
bool autoreap;
struct task_struct *p, *n;
LIST_HEAD(dead);
@@ -739,9 +741,12 @@ static void exit_notify(struct task_struct *tsk, int 
group_dead)
if (tsk->exit_state == EXIT_DEAD)
list_add(>ptrace_entry, );
 
-   /* mt-exec, de_thread() is waiting for group leader */
-   if (unlikely(tsk->signal->notify_count < 0))
-   wake_up_process(tsk->signal->group_exit_task);
+   spin_lock(>siglock);
+   /* mt-exec, de_thread is waiting for threads to exit */
+   if (signal->notify_count < 0 && !++signal->notify_count)
+   wake_up_process(signal->group_exit_task);
+
+ 

[RFC][PATCH 2/2] exec: If possible don't wait for ptraced threads to be reaped

[Eric W. Biederman]

Take advantage of the situation when sighand->count == 1 to only wait
for threads to reach EXIT_ZOMBIE instead of EXIT_DEAD in de_thread.
Only old old linux threading libraries use CLONE_SIGHAND without
CLONE_THREAD.  So this situation should be present most of the time.

This allows ptracing through a multi-threaded exec without the danger
of stalling the exec.  As historically exec waits for the other
threads to be reaped in de_thread before completing.  This is
necessary as it is not safe to unshare the sighand_struct until all of
the other threads in this thread group are reaped, because the lock to
serialize threads in a thread group siglock lives in sighand_struct.

When oldsighand->count == 1 we know that there are no other
users and unsharing the sighand struct in exec is pointless.
This makes it safe to only wait for threads to become zombies
as the siglock won't change during exec and release_task
will use the samve siglock for the old threads as for
the new threads.

Cc: sta...@vger.kernel.org
Signed-off-by: "Eric W. Biederman" 
---
 fs/exec.c| 15 ++-
 include/linux/sched/signal.h |  2 +-
 kernel/exit.c| 13 +
 kernel/signal.c  |  8 ++--
 4 files changed, 26 insertions(+), 12 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 65145a3df065..0fd29342bbe4 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1052,6 +1052,7 @@ static int de_thread(struct task_struct *tsk)
struct signal_struct *sig = tsk->signal;
struct sighand_struct *oldsighand = tsk->sighand;
spinlock_t *lock = >siglock;
+   bool may_hang;
 
if (thread_group_empty(tsk))
goto no_thread_group;
@@ -1069,9 +1070,10 @@ static int de_thread(struct task_struct *tsk)
return -EAGAIN;
}
 
+   may_hang = atomic_read(>count) != 1;
sig->group_exit_task = tsk;
-   sig->notify_count = zap_other_threads(tsk);
-   if (!thread_group_leader(tsk))
+   sig->notify_count = zap_other_threads(tsk, may_hang ? 1 : -1);
+   if (may_hang && !thread_group_leader(tsk))
sig->notify_count--;
 
while (sig->notify_count) {
@@ -1092,9 +1094,10 @@ static int de_thread(struct task_struct *tsk)
if (!thread_group_leader(tsk)) {
struct task_struct *leader = tsk->group_leader;
 
-   for (;;) {
-   cgroup_threadgroup_change_begin(tsk);
-   write_lock_irq(_lock);
+   cgroup_threadgroup_change_begin(tsk);
+   write_lock_irq(_lock);
+
+   for (;may_hang;) {
/*
 * Do this under tasklist_lock to ensure that
 * exit_notify() can't miss ->group_exit_task
@@ -1108,6 +,8 @@ static int de_thread(struct task_struct *tsk)
schedule();
if (unlikely(__fatal_signal_pending(tsk)))
goto killed;
+   cgroup_threadgroup_change_begin(tsk);
+   write_lock_irq(_lock);
}
 
/*
diff --git a/include/linux/sched/signal.h b/include/linux/sched/signal.h
index 2cf446704cd4..187a9e980d3a 100644
--- a/include/linux/sched/signal.h
+++ b/include/linux/sched/signal.h
@@ -298,7 +298,7 @@ extern __must_check bool do_notify_parent(struct 
task_struct *, int);
 extern void __wake_up_parent(struct task_struct *p, struct task_struct 
*parent);
 extern void force_sig(int, struct task_struct *);
 extern int send_sig(int, struct task_struct *, int);
-extern int zap_other_threads(struct task_struct *p);
+extern int zap_other_threads(struct task_struct *p, int do_count);
 extern struct sigqueue *sigqueue_alloc(void);
 extern void sigqueue_free(struct sigqueue *);
 extern int send_sigqueue(struct sigqueue *,  struct task_struct *, int group);
diff --git a/kernel/exit.c b/kernel/exit.c
index 8c5b3e106298..972df5ebf79f 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -712,6 +712,8 @@ static void forget_original_parent(struct task_struct 
*father,
  */
 static void exit_notify(struct task_struct *tsk, int group_dead)
 {
+   struct sighand_struct *sighand = tsk->sighand;
+   struct signal_struct *signal = tsk->signal;
bool autoreap;
struct task_struct *p, *n;
LIST_HEAD(dead);
@@ -739,9 +741,12 @@ static void exit_notify(struct task_struct *tsk, int 
group_dead)
if (tsk->exit_state == EXIT_DEAD)
list_add(>ptrace_entry, );
 
-   /* mt-exec, de_thread() is waiting for group leader */
-   if (unlikely(tsk->signal->notify_count < 0))
-   wake_up_process(tsk->signal->group_exit_task);
+   spin_lock(>siglock);
+   /* mt-exec, de_thread is waiting for threads to exit */
+   if (signal->notify_count < 0 && !++signal->notify_count)
+   wake_up_process(signal->group_exit_task);
+
+   

[RFC][PATCH 1/2] sighand: Count each thread group once in sighand_struct

[Eric W. Biederman]

In practice either a thread group is either using a sighand_struct or
it isn't.  Therefore simplify things a bit and only increment the
count in sighand_struct when a new thread group is created that uses
the existing sighand_struct, and only decrement the count in
sighand_struct when a thread group exits.

As well as standing on it's own merits this has the potential to simply
de_thread.

Signed-off-by: "Eric W. Biederman" 
---
 kernel/exit.c | 2 +-
 kernel/fork.c | 6 --
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index e126ebf2400c..8c5b3e106298 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -163,9 +163,9 @@ static void __exit_signal(struct task_struct *tsk)
tsk->sighand = NULL;
spin_unlock(>siglock);
 
-   __cleanup_sighand(sighand);
clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
if (group_dead) {
+   __cleanup_sighand(sighand);
flush_sigqueue(>shared_pending);
tty_kref_put(tty);
}
diff --git a/kernel/fork.c b/kernel/fork.c
index 6c463c80e93d..fe6f1bf32bb9 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1295,7 +1295,8 @@ static int copy_sighand(unsigned long clone_flags, struct 
task_struct *tsk)
struct sighand_struct *sig;
 
if (clone_flags & CLONE_SIGHAND) {
-   atomic_inc(>sighand->count);
+   if (!(clone_flags & CLONE_THREAD))
+   atomic_inc(>sighand->count);
return 0;
}
sig = kmem_cache_alloc(sighand_cachep, GFP_KERNEL);
@@ -1896,7 +1897,8 @@ static __latent_entropy struct task_struct *copy_process(
if (!(clone_flags & CLONE_THREAD))
free_signal_struct(p->signal);
 bad_fork_cleanup_sighand:
-   __cleanup_sighand(p->sighand);
+   if (!(clone_flags & CLONE_THREAD))
+   __cleanup_sighand(p->sighand);
 bad_fork_cleanup_fs:
exit_fs(p); /* blocking */
 bad_fork_cleanup_files:
-- 
2.10.1

[RFC][PATCH 1/2] sighand: Count each thread group once in sighand_struct

[Eric W. Biederman]

In practice either a thread group is either using a sighand_struct or
it isn't.  Therefore simplify things a bit and only increment the
count in sighand_struct when a new thread group is created that uses
the existing sighand_struct, and only decrement the count in
sighand_struct when a thread group exits.

As well as standing on it's own merits this has the potential to simply
de_thread.

Signed-off-by: "Eric W. Biederman" 
---
 kernel/exit.c | 2 +-
 kernel/fork.c | 6 --
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/kernel/exit.c b/kernel/exit.c
index e126ebf2400c..8c5b3e106298 100644
--- a/kernel/exit.c
+++ b/kernel/exit.c
@@ -163,9 +163,9 @@ static void __exit_signal(struct task_struct *tsk)
tsk->sighand = NULL;
spin_unlock(>siglock);
 
-   __cleanup_sighand(sighand);
clear_tsk_thread_flag(tsk, TIF_SIGPENDING);
if (group_dead) {
+   __cleanup_sighand(sighand);
flush_sigqueue(>shared_pending);
tty_kref_put(tty);
}
diff --git a/kernel/fork.c b/kernel/fork.c
index 6c463c80e93d..fe6f1bf32bb9 100644
--- a/kernel/fork.c
+++ b/kernel/fork.c
@@ -1295,7 +1295,8 @@ static int copy_sighand(unsigned long clone_flags, struct 
task_struct *tsk)
struct sighand_struct *sig;
 
if (clone_flags & CLONE_SIGHAND) {
-   atomic_inc(>sighand->count);
+   if (!(clone_flags & CLONE_THREAD))
+   atomic_inc(>sighand->count);
return 0;
}
sig = kmem_cache_alloc(sighand_cachep, GFP_KERNEL);
@@ -1896,7 +1897,8 @@ static __latent_entropy struct task_struct *copy_process(
if (!(clone_flags & CLONE_THREAD))
free_signal_struct(p->signal);
 bad_fork_cleanup_sighand:
-   __cleanup_sighand(p->sighand);
+   if (!(clone_flags & CLONE_THREAD))
+   __cleanup_sighand(p->sighand);
 bad_fork_cleanup_fs:
exit_fs(p); /* blocking */
 bad_fork_cleanup_files:
-- 
2.10.1

[RFC][PATCH 0/2] exec: Fixing ptrace'd mulit-threaded hang

[Eric W. Biederman]

I spent a little more time with this and only waiting until the killed
thread are zombies (and not reaped as we do today) really looks like
the right fix.

Oleg the following two patches work on top of your PTRACE_EVENT_EXIT
change and probably need a little more cleanup until they are ready
for serious posting.

That said I want to I want to post the code so I have a change at
some feedback before I prepare the final round of patches.

These patches only handle the case when sighand_struct is not
shared between different multi-threaded processes.  The general
case is solvable but that is a quite a bit more code.

Eric W. Biederman (2):
  sighand: Count each thread group once in sighand_struct
  exec: If possible don't wait for ptraced threads to be reaped

 fs/exec.c| 15 ++-
 include/linux/sched/signal.h |  2 +-
 kernel/exit.c| 15 ++-
 kernel/fork.c|  6 --
 kernel/signal.c  |  8 ++--
 5 files changed, 31 insertions(+), 15 deletions(-)

Eric

[RFC][PATCH 0/2] exec: Fixing ptrace'd mulit-threaded hang

[Eric W. Biederman]

I spent a little more time with this and only waiting until the killed
thread are zombies (and not reaped as we do today) really looks like
the right fix.

Oleg the following two patches work on top of your PTRACE_EVENT_EXIT
change and probably need a little more cleanup until they are ready
for serious posting.

That said I want to I want to post the code so I have a change at
some feedback before I prepare the final round of patches.

These patches only handle the case when sighand_struct is not
shared between different multi-threaded processes.  The general
case is solvable but that is a quite a bit more code.

Eric W. Biederman (2):
  sighand: Count each thread group once in sighand_struct
  exec: If possible don't wait for ptraced threads to be reaped

 fs/exec.c| 15 ++-
 include/linux/sched/signal.h |  2 +-
 kernel/exit.c| 15 ++-
 kernel/fork.c|  6 --
 kernel/signal.c  |  8 ++--
 5 files changed, 31 insertions(+), 15 deletions(-)

Eric

Re: [PATCH V2 net-next 1/7] ptr_ring: introduce batch dequeuing

[Jason Wang]

On 2017年03月31日 22:31, Michael S. Tsirkin wrote:

On Fri, Mar 31, 2017 at 11:52:24AM +0800, Jason Wang wrote:

On 2017年03月30日 21:53, Michael S. Tsirkin wrote:

On Thu, Mar 30, 2017 at 03:22:24PM +0800, Jason Wang wrote:

This patch introduce a batched version of consuming, consumer can
dequeue more than one pointers from the ring at a time. We don't care
about the reorder of reading here so no need for compiler barrier.

Signed-off-by: Jason Wang
---
   include/linux/ptr_ring.h | 65 

   1 file changed, 65 insertions(+)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 6c70444..2be0f350 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -247,6 +247,22 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
return ptr;
   }
+static inline int __ptr_ring_consume_batched(struct ptr_ring *r,
+void **array, int n)

Can we use a shorter name? ptr_ring_consume_batch?

Ok, but at least we need to keep the prefix since there's a locked version.




+{
+   void *ptr;
+   int i;
+
+   for (i = 0; i < n; i++) {
+   ptr = __ptr_ring_consume(r);
+   if (!ptr)
+   break;
+   array[i] = ptr;
+   }
+
+   return i;
+}
+
   /*
* Note: resize (below) nests producer lock within consumer lock, so if you
* call this in interrupt or BH context, you must disable interrupts/BH when

I'd like to add a code comment here explaining why we don't
care about cpu or compiler reordering. And I think the reason is
in the way you use this API: in vhost it does not matter
if you get less entries than present in the ring.
That's ok but needs to be noted
in a code comment so people use this function correctly.

Interesting, but I still think it's not necessary.

If consumer is doing a busy polling, it will eventually get the entries. If
the consumer need notification from producer, it should drain the queue
which means it need enable notification before last try of consuming call,
otherwise it was a bug. The batch consuming function in this patch can
guarantee return at least one pointer if there's many, this looks sufficient
for the correctness?

Thanks

You ask for N entries but get N-1. This seems to imply the
ring is now empty. Do we guarantee this?


I think consumer can not assume ring is empty consider producer can 
produce at the same time. It need enable notification and do another 
poll in this case.


Thanks

Re: [PATCH V2 net-next 1/7] ptr_ring: introduce batch dequeuing

[Jason Wang]

On 2017年03月31日 22:31, Michael S. Tsirkin wrote:

On Fri, Mar 31, 2017 at 11:52:24AM +0800, Jason Wang wrote:

On 2017年03月30日 21:53, Michael S. Tsirkin wrote:

On Thu, Mar 30, 2017 at 03:22:24PM +0800, Jason Wang wrote:

This patch introduce a batched version of consuming, consumer can
dequeue more than one pointers from the ring at a time. We don't care
about the reorder of reading here so no need for compiler barrier.

Signed-off-by: Jason Wang
---
   include/linux/ptr_ring.h | 65 

   1 file changed, 65 insertions(+)

diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
index 6c70444..2be0f350 100644
--- a/include/linux/ptr_ring.h
+++ b/include/linux/ptr_ring.h
@@ -247,6 +247,22 @@ static inline void *__ptr_ring_consume(struct ptr_ring *r)
return ptr;
   }
+static inline int __ptr_ring_consume_batched(struct ptr_ring *r,
+void **array, int n)

Can we use a shorter name? ptr_ring_consume_batch?

Ok, but at least we need to keep the prefix since there's a locked version.




+{
+   void *ptr;
+   int i;
+
+   for (i = 0; i < n; i++) {
+   ptr = __ptr_ring_consume(r);
+   if (!ptr)
+   break;
+   array[i] = ptr;
+   }
+
+   return i;
+}
+
   /*
* Note: resize (below) nests producer lock within consumer lock, so if you
* call this in interrupt or BH context, you must disable interrupts/BH when

I'd like to add a code comment here explaining why we don't
care about cpu or compiler reordering. And I think the reason is
in the way you use this API: in vhost it does not matter
if you get less entries than present in the ring.
That's ok but needs to be noted
in a code comment so people use this function correctly.

Interesting, but I still think it's not necessary.

If consumer is doing a busy polling, it will eventually get the entries. If
the consumer need notification from producer, it should drain the queue
which means it need enable notification before last try of consuming call,
otherwise it was a bug. The batch consuming function in this patch can
guarantee return at least one pointer if there's many, this looks sufficient
for the correctness?

Thanks

You ask for N entries but get N-1. This seems to imply the
ring is now empty. Do we guarantee this?


I think consumer can not assume ring is empty consider producer can 
produce at the same time. It need enable notification and do another 
poll in this case.


Thanks

Re: [PATCH -v2 1/2] mm, swap: Use kvzalloc to allocate some swap data structure

[Huang, Ying]

Hi, Michal,

Michal Hocko  writes:

> On Fri 24-03-17 06:56:10, Dave Hansen wrote:
>> On 03/24/2017 12:33 AM, John Hubbard wrote:
>> > There might be some additional information you are using to come up with
>> > that conclusion, that is not obvious to me. Any thoughts there? These
>> > calls use the same underlying page allocator (and I thought that both
>> > were subject to the same constraints on defragmentation, as a result of
>> > that). So I am not seeing any way that kmalloc could possibly be a
>> > less-fragmenting call than vmalloc.
>> 
>> You guys are having quite a discussion over a very small point.
>> 
>> But, Ying is right.
>> 
>> Let's say we have a two-page data structure.  vmalloc() takes two
>> effectively random order-0 pages, probably from two different 2M pages
>> and pins them.  That "kills" two 2M pages.
>> 
>> kmalloc(), allocating two *contiguous* pages, is very unlikely to cross
>> a 2M boundary (it theoretically could).  That means it will only "kill"
>> the possibility of a single 2M page.  More 2M pages == less fragmentation.
>
> Yes I agree with this. And the patch is no brainer. kvmalloc makes sure
> to not try too hard on the kmalloc side so I really didn't get the
> objection about direct compaction and reclaim which initially started
> this discussion. Besides that the swapon path usually happens early
> during the boot where we should have those larger blocks available.

Could I add your Acked-by for this patch?

Best Regards,
Huang, Ying

Re: [PATCH -v2 1/2] mm, swap: Use kvzalloc to allocate some swap data structure

[Huang, Ying]

Hi, Michal,

Michal Hocko  writes:

> On Fri 24-03-17 06:56:10, Dave Hansen wrote:
>> On 03/24/2017 12:33 AM, John Hubbard wrote:
>> > There might be some additional information you are using to come up with
>> > that conclusion, that is not obvious to me. Any thoughts there? These
>> > calls use the same underlying page allocator (and I thought that both
>> > were subject to the same constraints on defragmentation, as a result of
>> > that). So I am not seeing any way that kmalloc could possibly be a
>> > less-fragmenting call than vmalloc.
>> 
>> You guys are having quite a discussion over a very small point.
>> 
>> But, Ying is right.
>> 
>> Let's say we have a two-page data structure.  vmalloc() takes two
>> effectively random order-0 pages, probably from two different 2M pages
>> and pins them.  That "kills" two 2M pages.
>> 
>> kmalloc(), allocating two *contiguous* pages, is very unlikely to cross
>> a 2M boundary (it theoretically could).  That means it will only "kill"
>> the possibility of a single 2M page.  More 2M pages == less fragmentation.
>
> Yes I agree with this. And the patch is no brainer. kvmalloc makes sure
> to not try too hard on the kmalloc side so I really didn't get the
> objection about direct compaction and reclaim which initially started
> this discussion. Besides that the swapon path usually happens early
> during the boot where we should have those larger blocks available.

Could I add your Acked-by for this patch?

Best Regards,
Huang, Ying

Re: [PATCH v13 3/6] mmc: cavium: Add MMC PCI driver for ThunderX SOCs

[kbuild test robot]

Hi Jan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc4 next-20170331]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jan-Glauber/Cavium-MMC-driver/20170401-055302
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64 

All errors (new ones prefixed by >>):

>> ERROR: "of_platform_device_create" [drivers/mmc/host/thunderx-mmc.ko] 
>> undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v13 3/6] mmc: cavium: Add MMC PCI driver for ThunderX SOCs

[kbuild test robot]

Hi Jan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc4 next-20170331]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jan-Glauber/Cavium-MMC-driver/20170401-055302
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64 

All errors (new ones prefixed by >>):

>> ERROR: "of_platform_device_create" [drivers/mmc/host/thunderx-mmc.ko] 
>> undefined!

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Chewie Lin]

On Sat, Apr 01, 2017 at 04:32:39AM +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > WARNING: Prefer using "%s", __func__ to embedded function names
> > #417: FILE: main_usb.c:417:
> > +"usb_device_reset fail status=%d\n", status);
> > 
> > total: 0 errors, 1 warnings, 1058 lines checked
> > 
> > And after fix:
> > 
> > main_usb.c has no obvious style problems and is ready for submission.
> 
> 
> > -"usb_device_reset fail status=%d\n", status);
> > +"%s=%d\n", "usb_device_reset fail status", status);
> 
> Would you mind explaining the meaning of that warning?  Incidentally, why not
> "%se_reset fail status=%d\n", "usb_devic", status);
> - it would produce the identical output and silences checkpatch even more
> reliably.  Discuss.
> 
> Sarcasm aside, when you are proposing a change, there are several questions 
> you
> really must ask yourself and be able to answer.  First and foremost, of 
> course,
> is
>   * Why is it an improvement?
> If the answer to that was "it makes a warning go away", the next questions are
>   * What are we warned about?
>   * What is problematic in the original variant?
>   * Is the replacement free from the problem we had in the original?
> and if the answer to any of that is "I don't know", the next step is _not_
> "send it anyway".  "Try to figure out" might be a good idea, with usual
> heuristics regarding the reading comprehension ("if a sentence remains
> a mystery, see if there are any unfamiliar words skipped at the first reading
> and find what they mean", etc.) applying.
> 
> In this particular case the part you've apparently skipped was, indeed,
> critical - "__func__".  I am not trying to defend the quality of checkpatch -
> the warning is badly worded, the tool is badly written and more often than
> not its suggestions reflect nothing but authors' arbitrary preferences.
> 
> This one, however, does have some rationale behind it.  Namely, if some
> debugging output contains the name of a function that has produced it,
> having that name spelled out in format string is not a good idea.  If
> that code gets moved elsewhere one would have to replace the name in format
> string or face confusing messages refering to the function where that
> code used to be.  Since __func__ is interpreted as a constant string
> initialized with the name of the function it's used in, it is possible
> to avoid spelling the function name out - instead of
> "my_function: pink elephants; reduce the daily intake to %d glasses\n", n
> one could use
> "%s: pink elephants; reduce the daily intake to %d glasses\n", __func__, n
> producing the same output and avoiding the need to adjust anything when
> the whole thing is moved to another function.  It's not particularly big
> deal, but it's a more or less reasonable suggestion.
> 
> Your change, of course, has achieved only one thing - it has moved the
> explicitly spelled out function name out of format string.  It's still
> just as explicitly spelled out, still would need to be adjusted if moved
> to another function and the whole thing has become harder to read and
> understand since you've buried other parts of message in the place where
> it's harder to follow.
> 
> Again, checkpatch warning is badly written, but the main problem with
> your posting is not that you had been confused by checkpatch - it's
> that you have posted it based on an incomprehensible message with no better
> rationale than "it shuts checkpatch up, dunno why and what about".

Al, brilliant, that's exactly what I was trying to do on my first try. 
The checkpatch *is* confusing. It's fine with a simple string but doesn't 
like it when it's formatted string. From what you said, I 
think this may work better and more portable: 

"%s: fail status = %d\n", "usb_device_reset", status)

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Chewie Lin]

On Sat, Apr 01, 2017 at 04:32:39AM +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > WARNING: Prefer using "%s", __func__ to embedded function names
> > #417: FILE: main_usb.c:417:
> > +"usb_device_reset fail status=%d\n", status);
> > 
> > total: 0 errors, 1 warnings, 1058 lines checked
> > 
> > And after fix:
> > 
> > main_usb.c has no obvious style problems and is ready for submission.
> 
> 
> > -"usb_device_reset fail status=%d\n", status);
> > +"%s=%d\n", "usb_device_reset fail status", status);
> 
> Would you mind explaining the meaning of that warning?  Incidentally, why not
> "%se_reset fail status=%d\n", "usb_devic", status);
> - it would produce the identical output and silences checkpatch even more
> reliably.  Discuss.
> 
> Sarcasm aside, when you are proposing a change, there are several questions 
> you
> really must ask yourself and be able to answer.  First and foremost, of 
> course,
> is
>   * Why is it an improvement?
> If the answer to that was "it makes a warning go away", the next questions are
>   * What are we warned about?
>   * What is problematic in the original variant?
>   * Is the replacement free from the problem we had in the original?
> and if the answer to any of that is "I don't know", the next step is _not_
> "send it anyway".  "Try to figure out" might be a good idea, with usual
> heuristics regarding the reading comprehension ("if a sentence remains
> a mystery, see if there are any unfamiliar words skipped at the first reading
> and find what they mean", etc.) applying.
> 
> In this particular case the part you've apparently skipped was, indeed,
> critical - "__func__".  I am not trying to defend the quality of checkpatch -
> the warning is badly worded, the tool is badly written and more often than
> not its suggestions reflect nothing but authors' arbitrary preferences.
> 
> This one, however, does have some rationale behind it.  Namely, if some
> debugging output contains the name of a function that has produced it,
> having that name spelled out in format string is not a good idea.  If
> that code gets moved elsewhere one would have to replace the name in format
> string or face confusing messages refering to the function where that
> code used to be.  Since __func__ is interpreted as a constant string
> initialized with the name of the function it's used in, it is possible
> to avoid spelling the function name out - instead of
> "my_function: pink elephants; reduce the daily intake to %d glasses\n", n
> one could use
> "%s: pink elephants; reduce the daily intake to %d glasses\n", __func__, n
> producing the same output and avoiding the need to adjust anything when
> the whole thing is moved to another function.  It's not particularly big
> deal, but it's a more or less reasonable suggestion.
> 
> Your change, of course, has achieved only one thing - it has moved the
> explicitly spelled out function name out of format string.  It's still
> just as explicitly spelled out, still would need to be adjusted if moved
> to another function and the whole thing has become harder to read and
> understand since you've buried other parts of message in the place where
> it's harder to follow.
> 
> Again, checkpatch warning is badly written, but the main problem with
> your posting is not that you had been confused by checkpatch - it's
> that you have posted it based on an incomprehensible message with no better
> rationale than "it shuts checkpatch up, dunno why and what about".

Al, brilliant, that's exactly what I was trying to do on my first try. 
The checkpatch *is* confusing. It's fine with a simple string but doesn't 
like it when it's formatted string. From what you said, I 
think this may work better and more portable: 

"%s: fail status = %d\n", "usb_device_reset", status)

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Sat, 2017-04-01 at 05:08 +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 08:52:50PM -0700, Joe Perches wrote:
> 
> > > MILD SUGGESTION: don't spell the function name out in format strings;
> > >   "this_function: foo is %d", n
> > > might be better off as
> > >   "%s: foo is %d", __func__, n
> > > in case you ever move it to another function or rename your function.
> > 
> > Thank you sir, may I have another.
> > 
> > checkpatch messages are single line.
> 
> Too bad... Incidentally, being able to get more detailed explanation of
> a warning might be a serious improvement, especially if it contains
> the rationale.  Hell, something like TeX handling of errors might be
> a good idea - warning printed, offered actions include 'give more help',
> 'continue', 'exit', 'from now on suppress this kind of warning', 'from
> now on just dump this kind of warning into log and keep going', 'from
> now on dump all warnings into log and keep going'.

Well, there is the possibility to have longer messages.
It's just the --terse option has to be somewhat sensible.

> And yes, I'm serious about having something like "mild suggestion" as
> possible severity - people are using that thing to look for potential
> improvements to make and 'such and such change might be useful for such
> and such reasons' is a lot more useful than 'this needs to be thus because
> it must be thus or I'll keep warning'.

I agree about checkpatch and ERROR/WARNING/CHECK vs some other wording.

https://lkml.org/lkml/2016/8/27/180
https://lkml.org/lkml/2015/7/16/568

The other thing that might help is for people to take
the warnings the script produces less seriously.

Maybe convert:

ERROR -> defect
WARNING -> unstylish
CHECK -> nitpick

or some such.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Sat, 2017-04-01 at 05:08 +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 08:52:50PM -0700, Joe Perches wrote:
> 
> > > MILD SUGGESTION: don't spell the function name out in format strings;
> > >   "this_function: foo is %d", n
> > > might be better off as
> > >   "%s: foo is %d", __func__, n
> > > in case you ever move it to another function or rename your function.
> > 
> > Thank you sir, may I have another.
> > 
> > checkpatch messages are single line.
> 
> Too bad... Incidentally, being able to get more detailed explanation of
> a warning might be a serious improvement, especially if it contains
> the rationale.  Hell, something like TeX handling of errors might be
> a good idea - warning printed, offered actions include 'give more help',
> 'continue', 'exit', 'from now on suppress this kind of warning', 'from
> now on just dump this kind of warning into log and keep going', 'from
> now on dump all warnings into log and keep going'.

Well, there is the possibility to have longer messages.
It's just the --terse option has to be somewhat sensible.

> And yes, I'm serious about having something like "mild suggestion" as
> possible severity - people are using that thing to look for potential
> improvements to make and 'such and such change might be useful for such
> and such reasons' is a lot more useful than 'this needs to be thus because
> it must be thus or I'll keep warning'.

I agree about checkpatch and ERROR/WARNING/CHECK vs some other wording.

https://lkml.org/lkml/2016/8/27/180
https://lkml.org/lkml/2015/7/16/568

The other thing that might help is for people to take
the warnings the script produces less seriously.

Maybe convert:

ERROR -> defect
WARNING -> unstylish
CHECK -> nitpick

or some such.

Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

[Stefan Agner]

On 2017-03-31 20:03, Dong Aisheng wrote:
> On Wed, Mar 29, 2017 at 05:50:29PM -0700, Stefan Agner wrote:
>> The USDHC instances need the USDHC NAND clock in order to operate.
>> Add the clock as ahb bus clock.
>>
>> Signed-off-by: Stefan Agner 
>> ---
>>  arch/arm/boot/dts/imx7s.dtsi | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
>> index 5d3a43b8de20..5794febb19a4 100644
>> --- a/arch/arm/boot/dts/imx7s.dtsi
>> +++ b/arch/arm/boot/dts/imx7s.dtsi
>> @@ -936,7 +936,7 @@
>>  reg = <0x30b4 0x1>;
>>  interrupts = ;
>>  clocks = < IMX7D_CLK_DUMMY>,
> 
> Would you please change the left ipg dummy to IMX7D_IPG_ROOT_CLK as well?

IMX7D_IPG_ROOT_CLK is currently not a valid clock in upstream... So we
would have to add it to the clock driver first.

I guess we could/should add it anyway at one point? But probably also as
init on, just to make sure Linux does not disable it since it is
currently used by several IPs implicitly.

--
Stefan

> 
> Otherwise,
> 
> Acked-by: Dong Aisheng 
> 
> Regards
> Dong Aisheng
> 
>> -< IMX7D_CLK_DUMMY>,
>> +< IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>>  < IMX7D_USDHC1_ROOT_CLK>;
>>  clock-names = "ipg", "ahb", "per";
>>  bus-width = <4>;
>> @@ -948,7 +948,7 @@
>>  reg = <0x30b5 0x1>;
>>  interrupts = ;
>>  clocks = < IMX7D_CLK_DUMMY>,
>> -< IMX7D_CLK_DUMMY>,
>> +< IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>>  < IMX7D_USDHC2_ROOT_CLK>;
>>  clock-names = "ipg", "ahb", "per";
>>  bus-width = <4>;
>> @@ -960,7 +960,7 @@
>>  reg = <0x30b6 0x1>;
>>  interrupts = ;
>>  clocks = < IMX7D_CLK_DUMMY>,
>> -< IMX7D_CLK_DUMMY>,
>> +< IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>>  < IMX7D_USDHC3_ROOT_CLK>;
>>  clock-names = "ipg", "ahb", "per";
>>  bus-width = <4>;
>> --
>> 2.12.1
>>

Re: [PATCH 2/2] ARM: dts: imx7: add USDHC NAND clock to SDHC instances

[Stefan Agner]

On 2017-03-31 20:03, Dong Aisheng wrote:
> On Wed, Mar 29, 2017 at 05:50:29PM -0700, Stefan Agner wrote:
>> The USDHC instances need the USDHC NAND clock in order to operate.
>> Add the clock as ahb bus clock.
>>
>> Signed-off-by: Stefan Agner 
>> ---
>>  arch/arm/boot/dts/imx7s.dtsi | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/arch/arm/boot/dts/imx7s.dtsi b/arch/arm/boot/dts/imx7s.dtsi
>> index 5d3a43b8de20..5794febb19a4 100644
>> --- a/arch/arm/boot/dts/imx7s.dtsi
>> +++ b/arch/arm/boot/dts/imx7s.dtsi
>> @@ -936,7 +936,7 @@
>>  reg = <0x30b4 0x1>;
>>  interrupts = ;
>>  clocks = < IMX7D_CLK_DUMMY>,
> 
> Would you please change the left ipg dummy to IMX7D_IPG_ROOT_CLK as well?

IMX7D_IPG_ROOT_CLK is currently not a valid clock in upstream... So we
would have to add it to the clock driver first.

I guess we could/should add it anyway at one point? But probably also as
init on, just to make sure Linux does not disable it since it is
currently used by several IPs implicitly.

--
Stefan

> 
> Otherwise,
> 
> Acked-by: Dong Aisheng 
> 
> Regards
> Dong Aisheng
> 
>> -< IMX7D_CLK_DUMMY>,
>> +< IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>>  < IMX7D_USDHC1_ROOT_CLK>;
>>  clock-names = "ipg", "ahb", "per";
>>  bus-width = <4>;
>> @@ -948,7 +948,7 @@
>>  reg = <0x30b5 0x1>;
>>  interrupts = ;
>>  clocks = < IMX7D_CLK_DUMMY>,
>> -< IMX7D_CLK_DUMMY>,
>> +< IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>>  < IMX7D_USDHC2_ROOT_CLK>;
>>  clock-names = "ipg", "ahb", "per";
>>  bus-width = <4>;
>> @@ -960,7 +960,7 @@
>>  reg = <0x30b6 0x1>;
>>  interrupts = ;
>>  clocks = < IMX7D_CLK_DUMMY>,
>> -< IMX7D_CLK_DUMMY>,
>> +< IMX7D_NAND_USDHC_BUS_ROOT_CLK>,
>>  < IMX7D_USDHC3_ROOT_CLK>;
>>  clock-names = "ipg", "ahb", "per";
>>  bus-width = <4>;
>> --
>> 2.12.1
>>

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Al Viro]

On Fri, Mar 31, 2017 at 08:52:50PM -0700, Joe Perches wrote:

> > MILD SUGGESTION: don't spell the function name out in format strings;
> > "this_function: foo is %d", n
> > might be better off as
> > "%s: foo is %d", __func__, n
> > in case you ever move it to another function or rename your function.
> 
> Thank you sir, may I have another.
> 
> checkpatch messages are single line.

Too bad... Incidentally, being able to get more detailed explanation of
a warning might be a serious improvement, especially if it contains
the rationale.  Hell, something like TeX handling of errors might be
a good idea - warning printed, offered actions include 'give more help',
'continue', 'exit', 'from now on suppress this kind of warning', 'from
now on just dump this kind of warning into log and keep going', 'from
now on dump all warnings into log and keep going'.

And yes, I'm serious about having something like "mild suggestion" as
possible severity - people are using that thing to look for potential
improvements to make and 'such and such change might be useful for such
and such reasons' is a lot more useful than 'this needs to be thus because
it must be thus or I'll keep warning'.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Al Viro]

On Fri, Mar 31, 2017 at 08:52:50PM -0700, Joe Perches wrote:

> > MILD SUGGESTION: don't spell the function name out in format strings;
> > "this_function: foo is %d", n
> > might be better off as
> > "%s: foo is %d", __func__, n
> > in case you ever move it to another function or rename your function.
> 
> Thank you sir, may I have another.
> 
> checkpatch messages are single line.

Too bad... Incidentally, being able to get more detailed explanation of
a warning might be a serious improvement, especially if it contains
the rationale.  Hell, something like TeX handling of errors might be
a good idea - warning printed, offered actions include 'give more help',
'continue', 'exit', 'from now on suppress this kind of warning', 'from
now on just dump this kind of warning into log and keep going', 'from
now on dump all warnings into log and keep going'.

And yes, I'm serious about having something like "mild suggestion" as
possible severity - people are using that thing to look for potential
improvements to make and 'such and such change might be useful for such
and such reasons' is a lot more useful than 'this needs to be thus because
it must be thus or I'll keep warning'.

Re: [PATCH v13 3/6] mmc: cavium: Add MMC PCI driver for ThunderX SOCs

[kbuild test robot]

Hi Jan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc4 next-20170331]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jan-Glauber/Cavium-MMC-driver/20170401-055302
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `thunder_mmc_probe':
>> cavium-thunderx.c:(.text+0x2830fcc): undefined reference to 
>> `of_platform_device_create'
   `.exit.data' referenced in section `.exit.text' of drivers/built-in.o: 
defined in discarded section `.exit.data' of drivers/built-in.o
   `.exit.data' referenced in section `.exit.text' of drivers/built-in.o: 
defined in discarded section `.exit.data' of drivers/built-in.o
   `.exit.data' referenced in section `.exit.text' of drivers/built-in.o: 
defined in discarded section `.exit.data' of drivers/built-in.o
   `.exit.data' referenced in section `.exit.text' of drivers/built-in.o: 
defined in discarded section `.exit.data' of drivers/built-in.o

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH v13 3/6] mmc: cavium: Add MMC PCI driver for ThunderX SOCs

[kbuild test robot]

Hi Jan,

[auto build test ERROR on linus/master]
[also build test ERROR on v4.11-rc4 next-20170331]
[if your patch is applied to the wrong git tree, please drop us a note to help 
improve the system]

url:
https://github.com/0day-ci/linux/commits/Jan-Glauber/Cavium-MMC-driver/20170401-055302
config: sparc-allyesconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
wget 
https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc 

All errors (new ones prefixed by >>):

   drivers/built-in.o: In function `thunder_mmc_probe':
>> cavium-thunderx.c:(.text+0x2830fcc): undefined reference to 
>> `of_platform_device_create'
   `.exit.data' referenced in section `.exit.text' of drivers/built-in.o: 
defined in discarded section `.exit.data' of drivers/built-in.o
   `.exit.data' referenced in section `.exit.text' of drivers/built-in.o: 
defined in discarded section `.exit.data' of drivers/built-in.o
   `.exit.data' referenced in section `.exit.text' of drivers/built-in.o: 
defined in discarded section `.exit.data' of drivers/built-in.o
   `.exit.data' referenced in section `.exit.text' of drivers/built-in.o: 
defined in discarded section `.exit.data' of drivers/built-in.o

---
0-DAY kernel test infrastructureOpen Source Technology Center
https://lists.01.org/pipermail/kbuild-all   Intel Corporation


.config.gz
Description: application/gzip

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Sat, 2017-04-01 at 04:46 +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 08:36:22PM -0700, Joe Perches wrote:
> > On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote:
> > > On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > > > Replace string with formatted arguments in the dev_warn() call. It 
> > > > removes
> > > > the checkpatch warning:
> > > > 
> > > > WARNING: Prefer using "%s", __func__ to embedded function names
> > 
> > []
> > > Again, checkpatch warning is badly written
> > 
> > In your opinion, what wording would be better?
> 
> MILD SUGGESTION: don't spell the function name out in format strings;
>   "this_function: foo is %d", n
> might be better off as
>   "%s: foo is %d", __func__, n
> in case you ever move it to another function or rename your function.

Thank you sir, may I have another.

checkpatch messages are single line.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Sat, 2017-04-01 at 04:46 +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 08:36:22PM -0700, Joe Perches wrote:
> > On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote:
> > > On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > > > Replace string with formatted arguments in the dev_warn() call. It 
> > > > removes
> > > > the checkpatch warning:
> > > > 
> > > > WARNING: Prefer using "%s", __func__ to embedded function names
> > 
> > []
> > > Again, checkpatch warning is badly written
> > 
> > In your opinion, what wording would be better?
> 
> MILD SUGGESTION: don't spell the function name out in format strings;
>   "this_function: foo is %d", n
> might be better off as
>   "%s: foo is %d", __func__, n
> in case you ever move it to another function or rename your function.

Thank you sir, may I have another.

checkpatch messages are single line.

Re: [PATCH v23 00/11] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer

[Fu Wei]

Hi Xiongfeng Wang,

On 1 April 2017 at 10:14, Xiongfeng Wang  wrote:
>
>
> On 2017/4/1 1:50, fu@linaro.org wrote:
>> From: Fu Wei 
>>
>> This patchset:
>> (1)Preparation for adding GTDT support in arm_arch_timer:
>> 1. Introduce a MMIO CNTFRQ helper.
>> 2. separate out device-tree code from arch_timer_detect_rate
>> 3. remove arch_timer_detect_rate use arch_timer_*get_cntfrq directly
>> 4. Refactor arch_timer_needs_probing, and move it into DT init call
>> 5. Introduce some new structs and refactor the MMIO timer init code
>> for reusing some common code.
>>
>> (2)Introduce ACPI GTDT parser: drivers/acpi/arm64/acpi_gtdt.c
>> Parse all kinds of timer in GTDT table of ACPI:arch timer,
>> memory-mapped timer and SBSA Generic Watchdog timer.
>> This driver can help to simplify all the relevant timer drivers,
>> and separate all the ACPI GTDT knowledge from them.
>>
>> (3)Simplify ACPI code for arm_arch_timer
>>
>> (4)Add GTDT support for ARM memory-mapped timer.
>>
>> This patchset has been tested on the following platforms with ACPI enabled:
>> (1)ARM Foundation v8 model
>>
>
> for arm_arch_timer(not memory-mapped) and sbsa watchdog part,  Tested-by: 
> wangxiongfe...@huawei.com

Great thanks for your testing :-)

>
>
>
>
>
> Thanks,
>
> Wang Xiongfeng
> .
>
>> Changelog:
>> v23: https://lkml.org/lkml/2017/3/31/
>>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
>> arch-timer/cleanup
>>  Improve the data struct of arch_timer_mem and arch_timer_mem_frame to
>>  improve the parser of GT blocks and arch_timer_mem initualization.
>>  Improve arch_timer_rate detection: sysreg frequency is primary in DT 
>> boot
>>  Improve some comments in GTDT parser driver.
>>  Improve acpi_gtdt_init function, and make a comment for the multiple 
>> calls.
>>  Improve the unwinding for the irq of timers, when an error occurs.
>>  Handle the case of virtual timer GSIV is 0.
>>
>> v22: https://lkml.org/lkml/2017/3/21/523
>>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
>> arch-timer/cleanup
>>  Only Introduce arch_timer_mem_get_cntfrq to get the frequency from mmio.
>>  Merged patch 2,3(about arch_timer_detect_rate).
>>  Keep arch_timer_rate, do NOT split it for different types of timer.
>>  Improve  memory-mapped timer support by comments and variable name:
>>  data-->timer_mem
>>  frame-->gtdt_frame
>>  Delete zero check for SBSA watchdog irq.
>>  Skip secure SBSA watchdog in GTDT driver.
>>  Delete Kconfig modification for SBSA watchdog driver.
>>  Delete no_irq, using nr_res instead.
>>
>> v21: https://lkml.org/lkml/2017/2/6/734
>>  Introduce two functions to get the frequency from mmio and sysreg.
>>  Remove arch_timer_detect_rate use arch_timer_get_*_freq directly
>>  Split arch_timer_rate for different types of timer.
>>  Skip secure timer frame in GTDT driver.
>>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
>> arch-timer/cleanup
>>  (The first 6 patches in v20 have been merged into arch-timer/cleanup 
>> branch)
>>
>> v20: https://lkml.org/lkml/2017/1/18/534
>>  Reorder the first 4 patches and split the 4th patches.
>>  Leave CNTHCTL_* as they originally were.
>>  Fix the bug in arch_timer_select_ppi.
>>  Split "Rework counter frequency detection" patch.
>>  Rework the arch_timer_detect_rate function.
>>  Improve the commit message of "Refactor MMIO timer probing".
>>  Rebase to 4.10.0-rc4
>>
>> v19: https://lkml.org/lkml/2016/12/21/25
>>  Fix a '\n' missing in a error message in arch_timer_mem_init.
>>  Add "request_mem_region" for ioremapping cntbase, according to
>>  f947ee1 clocksource/drivers/arm_arch_timer: Map frame with 
>> of_io_request_and_map()
>>  Rebase to 4.9.0-gfb779ff
>>
>> v18: https://lkml.org/lkml/2016/12/8/446
>>  Fix 8/15 patch problem of "int ret;" in arch_timer_acpi_init.
>>  Rebase to 4.9.0-rc8-g9269898
>>
>> v17: https://lkml.org/lkml/2016/11/25/140
>>  Take out some cleanups from 4/15.
>>  Merge 5/15 and 6/15, improve PPI determination code,
>>  improve commit message.
>>  Rework counter frequency detection.
>>  Move arch_timer_needs_of_probing into DT init call.
>>  Move Platform Timer scan loop back to timer init call to avoid 
>> allocating
>>  and free memory.
>>  Improve all the exported functions' comment.
>>
>> v16: https://lkml.org/lkml/2016/11/16/268
>>  Fix patchset problem about static enum ppi_nr of 01/13 in v15.
>>  Refactor arch_timer_detect_rate.
>>  Refactor arch_timer_needs_probing.
>>
>> v15: https://lkml.org/lkml/2016/11/15/366
>>  Re-order patches
>>  Add arm_arch_timer refactoring patches to prepare for GTDT:
>>  1. rename some  enums and defines, 

Re: [PATCH v23 00/11] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer

[Fu Wei]

Hi Xiongfeng Wang,

On 1 April 2017 at 10:14, Xiongfeng Wang  wrote:
>
>
> On 2017/4/1 1:50, fu@linaro.org wrote:
>> From: Fu Wei 
>>
>> This patchset:
>> (1)Preparation for adding GTDT support in arm_arch_timer:
>> 1. Introduce a MMIO CNTFRQ helper.
>> 2. separate out device-tree code from arch_timer_detect_rate
>> 3. remove arch_timer_detect_rate use arch_timer_*get_cntfrq directly
>> 4. Refactor arch_timer_needs_probing, and move it into DT init call
>> 5. Introduce some new structs and refactor the MMIO timer init code
>> for reusing some common code.
>>
>> (2)Introduce ACPI GTDT parser: drivers/acpi/arm64/acpi_gtdt.c
>> Parse all kinds of timer in GTDT table of ACPI:arch timer,
>> memory-mapped timer and SBSA Generic Watchdog timer.
>> This driver can help to simplify all the relevant timer drivers,
>> and separate all the ACPI GTDT knowledge from them.
>>
>> (3)Simplify ACPI code for arm_arch_timer
>>
>> (4)Add GTDT support for ARM memory-mapped timer.
>>
>> This patchset has been tested on the following platforms with ACPI enabled:
>> (1)ARM Foundation v8 model
>>
>
> for arm_arch_timer(not memory-mapped) and sbsa watchdog part,  Tested-by: 
> wangxiongfe...@huawei.com

Great thanks for your testing :-)

>
>
>
>
>
> Thanks,
>
> Wang Xiongfeng
> .
>
>> Changelog:
>> v23: https://lkml.org/lkml/2017/3/31/
>>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
>> arch-timer/cleanup
>>  Improve the data struct of arch_timer_mem and arch_timer_mem_frame to
>>  improve the parser of GT blocks and arch_timer_mem initualization.
>>  Improve arch_timer_rate detection: sysreg frequency is primary in DT 
>> boot
>>  Improve some comments in GTDT parser driver.
>>  Improve acpi_gtdt_init function, and make a comment for the multiple 
>> calls.
>>  Improve the unwinding for the irq of timers, when an error occurs.
>>  Handle the case of virtual timer GSIV is 0.
>>
>> v22: https://lkml.org/lkml/2017/3/21/523
>>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
>> arch-timer/cleanup
>>  Only Introduce arch_timer_mem_get_cntfrq to get the frequency from mmio.
>>  Merged patch 2,3(about arch_timer_detect_rate).
>>  Keep arch_timer_rate, do NOT split it for different types of timer.
>>  Improve  memory-mapped timer support by comments and variable name:
>>  data-->timer_mem
>>  frame-->gtdt_frame
>>  Delete zero check for SBSA watchdog irq.
>>  Skip secure SBSA watchdog in GTDT driver.
>>  Delete Kconfig modification for SBSA watchdog driver.
>>  Delete no_irq, using nr_res instead.
>>
>> v21: https://lkml.org/lkml/2017/2/6/734
>>  Introduce two functions to get the frequency from mmio and sysreg.
>>  Remove arch_timer_detect_rate use arch_timer_get_*_freq directly
>>  Split arch_timer_rate for different types of timer.
>>  Skip secure timer frame in GTDT driver.
>>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
>> arch-timer/cleanup
>>  (The first 6 patches in v20 have been merged into arch-timer/cleanup 
>> branch)
>>
>> v20: https://lkml.org/lkml/2017/1/18/534
>>  Reorder the first 4 patches and split the 4th patches.
>>  Leave CNTHCTL_* as they originally were.
>>  Fix the bug in arch_timer_select_ppi.
>>  Split "Rework counter frequency detection" patch.
>>  Rework the arch_timer_detect_rate function.
>>  Improve the commit message of "Refactor MMIO timer probing".
>>  Rebase to 4.10.0-rc4
>>
>> v19: https://lkml.org/lkml/2016/12/21/25
>>  Fix a '\n' missing in a error message in arch_timer_mem_init.
>>  Add "request_mem_region" for ioremapping cntbase, according to
>>  f947ee1 clocksource/drivers/arm_arch_timer: Map frame with 
>> of_io_request_and_map()
>>  Rebase to 4.9.0-gfb779ff
>>
>> v18: https://lkml.org/lkml/2016/12/8/446
>>  Fix 8/15 patch problem of "int ret;" in arch_timer_acpi_init.
>>  Rebase to 4.9.0-rc8-g9269898
>>
>> v17: https://lkml.org/lkml/2016/11/25/140
>>  Take out some cleanups from 4/15.
>>  Merge 5/15 and 6/15, improve PPI determination code,
>>  improve commit message.
>>  Rework counter frequency detection.
>>  Move arch_timer_needs_of_probing into DT init call.
>>  Move Platform Timer scan loop back to timer init call to avoid 
>> allocating
>>  and free memory.
>>  Improve all the exported functions' comment.
>>
>> v16: https://lkml.org/lkml/2016/11/16/268
>>  Fix patchset problem about static enum ppi_nr of 01/13 in v15.
>>  Refactor arch_timer_detect_rate.
>>  Refactor arch_timer_needs_probing.
>>
>> v15: https://lkml.org/lkml/2016/11/15/366
>>  Re-order patches
>>  Add arm_arch_timer refactoring patches to prepare for GTDT:
>>  1. rename some  enums and defines, and some cleanups
>>  2. separate out 

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Al Viro]

On Fri, Mar 31, 2017 at 08:36:22PM -0700, Joe Perches wrote:
> On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote:
> > On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > > Replace string with formatted arguments in the dev_warn() call. It removes
> > > the checkpatch warning:
> > > 
> > >   WARNING: Prefer using "%s", __func__ to embedded function names
> []
> > Again, checkpatch warning is badly written
> 
> In your opinion, what wording would be better?

MILD SUGGESTION: don't spell the function name out in format strings;
"this_function: foo is %d", n
might be better off as
"%s: foo is %d", __func__, n
in case you ever move it to another function or rename your function.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Al Viro]

On Fri, Mar 31, 2017 at 08:36:22PM -0700, Joe Perches wrote:
> On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote:
> > On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > > Replace string with formatted arguments in the dev_warn() call. It removes
> > > the checkpatch warning:
> > > 
> > >   WARNING: Prefer using "%s", __func__ to embedded function names
> []
> > Again, checkpatch warning is badly written
> 
> In your opinion, what wording would be better?

MILD SUGGESTION: don't spell the function name out in format strings;
"this_function: foo is %d", n
might be better off as
"%s: foo is %d", __func__, n
in case you ever move it to another function or rename your function.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > WARNING: Prefer using "%s", __func__ to embedded function names
[]
> Again, checkpatch warning is badly written

In your opinion, what wording would be better?

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Sat, 2017-04-01 at 04:32 +0100, Al Viro wrote:
> On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > WARNING: Prefer using "%s", __func__ to embedded function names
[]
> Again, checkpatch warning is badly written

In your opinion, what wording would be better?

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Fri, 2017-03-31 at 20:18 -0700, Chewie Lin wrote:
> These are good points, but any feedback on the dev_warn() call itself?
> I was trying to fix the checkpatch warning on my first try.

Using "%s" with a long string is generally inefficient.

Compare the compiled object size of
printf("%s is %d\n", "Long string", 1);
to
printf("Long string is %d\n", 1); 

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Fri, 2017-03-31 at 20:18 -0700, Chewie Lin wrote:
> These are good points, but any feedback on the dev_warn() call itself?
> I was trying to fix the checkpatch warning on my first try.

Using "%s" with a long string is generally inefficient.

Compare the compiled object size of
printf("%s is %d\n", "Long string", 1);
to
printf("Long string is %d\n", 1); 

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Al Viro]

On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
> 
>   WARNING: Prefer using "%s", __func__ to embedded function names
>   #417: FILE: main_usb.c:417:
>   +"usb_device_reset fail status=%d\n", status);
> 
>   total: 0 errors, 1 warnings, 1058 lines checked
> 
> And after fix:
> 
>   main_usb.c has no obvious style problems and is ready for submission.


> -  "usb_device_reset fail status=%d\n", status);
> +  "%s=%d\n", "usb_device_reset fail status", status);

Would you mind explaining the meaning of that warning?  Incidentally, why not
  "%se_reset fail status=%d\n", "usb_devic", status);
- it would produce the identical output and silences checkpatch even more
reliably.  Discuss.

Sarcasm aside, when you are proposing a change, there are several questions you
really must ask yourself and be able to answer.  First and foremost, of course,
is
* Why is it an improvement?
If the answer to that was "it makes a warning go away", the next questions are
* What are we warned about?
* What is problematic in the original variant?
* Is the replacement free from the problem we had in the original?
and if the answer to any of that is "I don't know", the next step is _not_
"send it anyway".  "Try to figure out" might be a good idea, with usual
heuristics regarding the reading comprehension ("if a sentence remains
a mystery, see if there are any unfamiliar words skipped at the first reading
and find what they mean", etc.) applying.

In this particular case the part you've apparently skipped was, indeed,
critical - "__func__".  I am not trying to defend the quality of checkpatch -
the warning is badly worded, the tool is badly written and more often than
not its suggestions reflect nothing but authors' arbitrary preferences.

This one, however, does have some rationale behind it.  Namely, if some
debugging output contains the name of a function that has produced it,
having that name spelled out in format string is not a good idea.  If
that code gets moved elsewhere one would have to replace the name in format
string or face confusing messages refering to the function where that
code used to be.  Since __func__ is interpreted as a constant string
initialized with the name of the function it's used in, it is possible
to avoid spelling the function name out - instead of
"my_function: pink elephants; reduce the daily intake to %d glasses\n", n
one could use
"%s: pink elephants; reduce the daily intake to %d glasses\n", __func__, n
producing the same output and avoiding the need to adjust anything when
the whole thing is moved to another function.  It's not particularly big
deal, but it's a more or less reasonable suggestion.

Your change, of course, has achieved only one thing - it has moved the
explicitly spelled out function name out of format string.  It's still
just as explicitly spelled out, still would need to be adjusted if moved
to another function and the whole thing has become harder to read and
understand since you've buried other parts of message in the place where
it's harder to follow.

Again, checkpatch warning is badly written, but the main problem with
your posting is not that you had been confused by checkpatch - it's
that you have posted it based on an incomprehensible message with no better
rationale than "it shuts checkpatch up, dunno why and what about".

Re: [PATCH -mm -v7 4/9] mm, THP, swap: Add get_huge_swap_page()

[Huang, Ying]

Johannes Weiner  writes:

> On Thu, Mar 30, 2017 at 12:28:17PM +0800, Huang, Ying wrote:
>> Johannes Weiner  writes:
>> > On Tue, Mar 28, 2017 at 01:32:04PM +0800, Huang, Ying wrote:
>> >> @@ -527,6 +527,23 @@ static inline swp_entry_t get_swap_page(void)
>> >>  
>> >>  #endif /* CONFIG_SWAP */
>> >>  
>> >> +#ifdef CONFIG_THP_SWAP_CLUSTER
>> >> +static inline swp_entry_t get_huge_swap_page(void)
>> >> +{
>> >> + swp_entry_t entry;
>> >> +
>> >> + if (get_swap_pages(1, , true))
>> >> + return entry;
>> >> + else
>> >> + return (swp_entry_t) {0};
>> >> +}
>> >> +#else
>> >> +static inline swp_entry_t get_huge_swap_page(void)
>> >> +{
>> >> + return (swp_entry_t) {0};
>> >> +}
>> >> +#endif
>> >
>> > Your introducing a function without a user, making it very hard to
>> > judge whether the API is well-designed for the callers or not.
>> >
>> > I pointed this out as a systemic problem with this patch series in v3,
>> > along with other stuff, but with the way this series is structured I'm
>> > having a hard time seeing whether you implemented my other feedback or
>> > whether your counter arguments to them are justified.
>> >
>> > I cannot review and ack these patches this way.
>> 
>> Sorry for inconvenience, I will send a new version to combine the
>> function definition and usage into one patch at least for you to
>> review.
>
> We tried this before. I reviewed the self-contained patch and you
> incorporated the feedback into the split-out structure that made it
> impossible for me to verify the updates.
>
> I'm not sure why you insist on preserving this series format. It's not
> good for review, and it's not good for merging and git history.

I had thought some reviewers would prefer the original series format.
But I will use your suggested format in the future, unless more
reviewers prefer the original format.

Best Regards,
Huang, Ying

>> But I think we can continue our discussion in the comments your
>> raised so far firstly, what do you think about that?
>
> Yeah, let's finish the discussions before -v8.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Al Viro]

On Fri, Mar 31, 2017 at 06:59:19PM -0700, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
> 
>   WARNING: Prefer using "%s", __func__ to embedded function names
>   #417: FILE: main_usb.c:417:
>   +"usb_device_reset fail status=%d\n", status);
> 
>   total: 0 errors, 1 warnings, 1058 lines checked
> 
> And after fix:
> 
>   main_usb.c has no obvious style problems and is ready for submission.


> -  "usb_device_reset fail status=%d\n", status);
> +  "%s=%d\n", "usb_device_reset fail status", status);

Would you mind explaining the meaning of that warning?  Incidentally, why not
  "%se_reset fail status=%d\n", "usb_devic", status);
- it would produce the identical output and silences checkpatch even more
reliably.  Discuss.

Sarcasm aside, when you are proposing a change, there are several questions you
really must ask yourself and be able to answer.  First and foremost, of course,
is
* Why is it an improvement?
If the answer to that was "it makes a warning go away", the next questions are
* What are we warned about?
* What is problematic in the original variant?
* Is the replacement free from the problem we had in the original?
and if the answer to any of that is "I don't know", the next step is _not_
"send it anyway".  "Try to figure out" might be a good idea, with usual
heuristics regarding the reading comprehension ("if a sentence remains
a mystery, see if there are any unfamiliar words skipped at the first reading
and find what they mean", etc.) applying.

In this particular case the part you've apparently skipped was, indeed,
critical - "__func__".  I am not trying to defend the quality of checkpatch -
the warning is badly worded, the tool is badly written and more often than
not its suggestions reflect nothing but authors' arbitrary preferences.

This one, however, does have some rationale behind it.  Namely, if some
debugging output contains the name of a function that has produced it,
having that name spelled out in format string is not a good idea.  If
that code gets moved elsewhere one would have to replace the name in format
string or face confusing messages refering to the function where that
code used to be.  Since __func__ is interpreted as a constant string
initialized with the name of the function it's used in, it is possible
to avoid spelling the function name out - instead of
"my_function: pink elephants; reduce the daily intake to %d glasses\n", n
one could use
"%s: pink elephants; reduce the daily intake to %d glasses\n", __func__, n
producing the same output and avoiding the need to adjust anything when
the whole thing is moved to another function.  It's not particularly big
deal, but it's a more or less reasonable suggestion.

Your change, of course, has achieved only one thing - it has moved the
explicitly spelled out function name out of format string.  It's still
just as explicitly spelled out, still would need to be adjusted if moved
to another function and the whole thing has become harder to read and
understand since you've buried other parts of message in the place where
it's harder to follow.

Again, checkpatch warning is badly written, but the main problem with
your posting is not that you had been confused by checkpatch - it's
that you have posted it based on an incomprehensible message with no better
rationale than "it shuts checkpatch up, dunno why and what about".

Re: [PATCH -mm -v7 4/9] mm, THP, swap: Add get_huge_swap_page()

[Huang, Ying]

Johannes Weiner  writes:

> On Thu, Mar 30, 2017 at 12:28:17PM +0800, Huang, Ying wrote:
>> Johannes Weiner  writes:
>> > On Tue, Mar 28, 2017 at 01:32:04PM +0800, Huang, Ying wrote:
>> >> @@ -527,6 +527,23 @@ static inline swp_entry_t get_swap_page(void)
>> >>  
>> >>  #endif /* CONFIG_SWAP */
>> >>  
>> >> +#ifdef CONFIG_THP_SWAP_CLUSTER
>> >> +static inline swp_entry_t get_huge_swap_page(void)
>> >> +{
>> >> + swp_entry_t entry;
>> >> +
>> >> + if (get_swap_pages(1, , true))
>> >> + return entry;
>> >> + else
>> >> + return (swp_entry_t) {0};
>> >> +}
>> >> +#else
>> >> +static inline swp_entry_t get_huge_swap_page(void)
>> >> +{
>> >> + return (swp_entry_t) {0};
>> >> +}
>> >> +#endif
>> >
>> > Your introducing a function without a user, making it very hard to
>> > judge whether the API is well-designed for the callers or not.
>> >
>> > I pointed this out as a systemic problem with this patch series in v3,
>> > along with other stuff, but with the way this series is structured I'm
>> > having a hard time seeing whether you implemented my other feedback or
>> > whether your counter arguments to them are justified.
>> >
>> > I cannot review and ack these patches this way.
>> 
>> Sorry for inconvenience, I will send a new version to combine the
>> function definition and usage into one patch at least for you to
>> review.
>
> We tried this before. I reviewed the self-contained patch and you
> incorporated the feedback into the split-out structure that made it
> impossible for me to verify the updates.
>
> I'm not sure why you insist on preserving this series format. It's not
> good for review, and it's not good for merging and git history.

I had thought some reviewers would prefer the original series format.
But I will use your suggested format in the future, unless more
reviewers prefer the original format.

Best Regards,
Huang, Ying

>> But I think we can continue our discussion in the comments your
>> raised so far firstly, what do you think about that?
>
> Yeah, let's finish the discussions before -v8.

Re: [PATCH -mm -v7 1/9] mm, swap: Make swap cluster size same of THP size on x86_64

[Huang, Ying]

Johannes Weiner  writes:

> On Thu, Mar 30, 2017 at 08:45:56AM +0800, Huang, Ying wrote:
>> Johannes Weiner  writes:
>> 
>> > On Tue, Mar 28, 2017 at 01:32:01PM +0800, Huang, Ying wrote:
>> >> @@ -499,6 +499,19 @@ config FRONTSWAP
>> >>  
>> >> If unsure, say Y to enable frontswap.
>> >>  
>> >> +config ARCH_USES_THP_SWAP_CLUSTER
>> >> + bool
>> >> + default n
>> >
>> > This is fine.
>> >
>> >> +config THP_SWAP_CLUSTER
>> >> + bool
>> >> + depends on SWAP && TRANSPARENT_HUGEPAGE && ARCH_USES_THP_SWAP_CLUSTER
>> >> + default y
>> >> + help
>> >> +   Use one swap cluster to hold the contents of the THP
>> >> +   (Transparent Huge Page) swapped out.  The size of the swap
>> >> +   cluster will be same as that of THP.
>> >
>> > But this is a super weird thing to ask the user. How would they know
>> > what to say, if we don't know? I don't think this should be a config
>> > knob at all. Merge the two config items into a simple
>> 
>> The user will not see this, because there is no string after "bool" to
>> let user to select it.  The help here is for document only, so that
>> architecture developers could know what this is for.
>
> Oh, I missed that. My bad!
>
>> > config THP_SWAP_CLUSTER
>> >  bool
>> >  default n
>> >
>> > and let the archs with reasonable THP sizes select it.
>> 
>> This will have same effect as the original solution except the document
>> is removed.
>
> Then I still don't understand why we need two config symbols. Can't
> archs select the documented THP_SWAP_CLUSTER directly?
>
> The #ifdef in swapfile.c could check THP && THP_SWAP_CLUSTER.
>
> Am I missing something?

I use two config symbols just to save some typing, instead of

#ifdef CONFIG_THP_SWAP_CLUSTER

it will be,

#if defined(CONFIG_TRANSPARENT_HUGEPAGE) && defined(CONFIG_THP_SWAP_CLUSTER)

or

#if defined(CONFIG_SWAP) && defined(CONFIG_TRANSPARENT_HUGEPAGE) && 
defined(CONFIG_THP_SWAP_CLUSTER)

Best Regards,
Huang, Ying

Re: [PATCH -mm -v7 1/9] mm, swap: Make swap cluster size same of THP size on x86_64

[Huang, Ying]

Johannes Weiner  writes:

> On Thu, Mar 30, 2017 at 08:45:56AM +0800, Huang, Ying wrote:
>> Johannes Weiner  writes:
>> 
>> > On Tue, Mar 28, 2017 at 01:32:01PM +0800, Huang, Ying wrote:
>> >> @@ -499,6 +499,19 @@ config FRONTSWAP
>> >>  
>> >> If unsure, say Y to enable frontswap.
>> >>  
>> >> +config ARCH_USES_THP_SWAP_CLUSTER
>> >> + bool
>> >> + default n
>> >
>> > This is fine.
>> >
>> >> +config THP_SWAP_CLUSTER
>> >> + bool
>> >> + depends on SWAP && TRANSPARENT_HUGEPAGE && ARCH_USES_THP_SWAP_CLUSTER
>> >> + default y
>> >> + help
>> >> +   Use one swap cluster to hold the contents of the THP
>> >> +   (Transparent Huge Page) swapped out.  The size of the swap
>> >> +   cluster will be same as that of THP.
>> >
>> > But this is a super weird thing to ask the user. How would they know
>> > what to say, if we don't know? I don't think this should be a config
>> > knob at all. Merge the two config items into a simple
>> 
>> The user will not see this, because there is no string after "bool" to
>> let user to select it.  The help here is for document only, so that
>> architecture developers could know what this is for.
>
> Oh, I missed that. My bad!
>
>> > config THP_SWAP_CLUSTER
>> >  bool
>> >  default n
>> >
>> > and let the archs with reasonable THP sizes select it.
>> 
>> This will have same effect as the original solution except the document
>> is removed.
>
> Then I still don't understand why we need two config symbols. Can't
> archs select the documented THP_SWAP_CLUSTER directly?
>
> The #ifdef in swapfile.c could check THP && THP_SWAP_CLUSTER.
>
> Am I missing something?

I use two config symbols just to save some typing, instead of

#ifdef CONFIG_THP_SWAP_CLUSTER

it will be,

#if defined(CONFIG_TRANSPARENT_HUGEPAGE) && defined(CONFIG_THP_SWAP_CLUSTER)

or

#if defined(CONFIG_SWAP) && defined(CONFIG_TRANSPARENT_HUGEPAGE) && 
defined(CONFIG_THP_SWAP_CLUSTER)

Best Regards,
Huang, Ying

Re: [PATCH] drivers: fixed a checkpatch warning

[Chewie Lin]

On Fri, Mar 31, 2017 at 11:53:55AM -0700, Joe Perches wrote:
> On Fri, 2017-03-31 at 01:40 -0700, Chewie Lin wrote:
> > fixed a coding style error/warning.
> > 
> > Signed-off-by: Chewie Lin 
> > ---
> >  drivers/staging/vt6656/main_usb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/vt6656/main_usb.c 
> > b/drivers/staging/vt6656/main_usb.c
> > index 9e074e9..2d9e7af 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
> > status = usb_reset_device(priv->usb);
> > if (status)
> > dev_warn(>usb->dev,
> > -"usb_device_reset fail status=%d\n", status);
> > +"%s=%d\n", "usb_device_reset fail status", status);
> 
> Yeah, what Greg said.
> 
> Also most likely this should be
> 
>   dev_warn(>usb->dev,
>"usb_reset_device fail status=%d\n", status);
> 
> Note the function is usb_device_reset, but the
> function call that failed is usb_reset_device.

yep, I had the comic book guy from the Simpsons voice when I read that. :)
I submitted a separate patch, maybe I should have reply-to this instead?

> 

Re: [PATCH] drivers: fixed a checkpatch warning

[Chewie Lin]

On Fri, Mar 31, 2017 at 11:53:55AM -0700, Joe Perches wrote:
> On Fri, 2017-03-31 at 01:40 -0700, Chewie Lin wrote:
> > fixed a coding style error/warning.
> > 
> > Signed-off-by: Chewie Lin 
> > ---
> >  drivers/staging/vt6656/main_usb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/vt6656/main_usb.c 
> > b/drivers/staging/vt6656/main_usb.c
> > index 9e074e9..2d9e7af 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
> > status = usb_reset_device(priv->usb);
> > if (status)
> > dev_warn(>usb->dev,
> > -"usb_device_reset fail status=%d\n", status);
> > +"%s=%d\n", "usb_device_reset fail status", status);
> 
> Yeah, what Greg said.
> 
> Also most likely this should be
> 
>   dev_warn(>usb->dev,
>"usb_reset_device fail status=%d\n", status);
> 
> Note the function is usb_device_reset, but the
> function call that failed is usb_reset_device.

yep, I had the comic book guy from the Simpsons voice when I read that. :)
I submitted a separate patch, maybe I should have reply-to this instead?

> 

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Chewie Lin]

On Fri, Mar 31, 2017 at 07:45:09PM -0700, Joe Perches wrote:
> On Fri, 2017-03-31 at 19:15 -0700, Randy Dunlap wrote:
> > On 03/31/17 18:59, Chewie Lin wrote:
> > > Replace string with formatted arguments in the dev_warn() call. It removes
> > > the checkpatch warning:
> > > 
> > >   WARNING: Prefer using "%s", __func__ to embedded function names
> > >   #417: FILE: main_usb.c:417:
> > >   +"usb_device_reset fail status=%d\n", status);
> > > 
> > >   total: 0 errors, 1 warnings, 1058 lines checked
> > > 
> > > And after fix:
> > > 
> > >   main_usb.c has no obvious style problems and is ready for submission.
> > > 
> > > Signed-off-by: Chewie Lin 
> > > ---
> > >  drivers/staging/vt6656/main_usb.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/vt6656/main_usb.c 
> > > b/drivers/staging/vt6656/main_usb.c
> > > index 9e074e9..2d9e7af 100644
> > > --- a/drivers/staging/vt6656/main_usb.c
> > > +++ b/drivers/staging/vt6656/main_usb.c
> > > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
> > >   status = usb_reset_device(priv->usb);
> > >   if (status)
> > >   dev_warn(>usb->dev,
> > > -  "usb_device_reset fail status=%d\n", status);
> > > +  "%s=%d\n", "usb_device_reset fail status", status);
> > >  }
> > >  
> > >  static void vnt_free_int_bufs(struct vnt_private *priv)
> > > 
> > 
> > As other people have said:
> > 
> > This function is usb_device_reset().  If that function name string is to be
> > used in the message, it should be done so by using __func__.
> > See http://marc.info/?l=linux-driver-devel=149095639202492=2
> > 
> > Or is the called (failing) function name is to be kept in the message (as it
> > is currently), then the message should contain "usb_reset_device" instead of
> > "usb_device_reset".  See 
> > http://marc.info/?l=linux-driver-devel=149098680312723=2
> 
> If this is to be changed at all, I suggest
> just getting rid of the function.

These are good points, but any feedback on the dev_warn() call itself? 
I was trying to fix the checkpatch warning on my first try.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Chewie Lin]

On Fri, Mar 31, 2017 at 07:45:09PM -0700, Joe Perches wrote:
> On Fri, 2017-03-31 at 19:15 -0700, Randy Dunlap wrote:
> > On 03/31/17 18:59, Chewie Lin wrote:
> > > Replace string with formatted arguments in the dev_warn() call. It removes
> > > the checkpatch warning:
> > > 
> > >   WARNING: Prefer using "%s", __func__ to embedded function names
> > >   #417: FILE: main_usb.c:417:
> > >   +"usb_device_reset fail status=%d\n", status);
> > > 
> > >   total: 0 errors, 1 warnings, 1058 lines checked
> > > 
> > > And after fix:
> > > 
> > >   main_usb.c has no obvious style problems and is ready for submission.
> > > 
> > > Signed-off-by: Chewie Lin 
> > > ---
> > >  drivers/staging/vt6656/main_usb.c | 2 +-
> > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/staging/vt6656/main_usb.c 
> > > b/drivers/staging/vt6656/main_usb.c
> > > index 9e074e9..2d9e7af 100644
> > > --- a/drivers/staging/vt6656/main_usb.c
> > > +++ b/drivers/staging/vt6656/main_usb.c
> > > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
> > >   status = usb_reset_device(priv->usb);
> > >   if (status)
> > >   dev_warn(>usb->dev,
> > > -  "usb_device_reset fail status=%d\n", status);
> > > +  "%s=%d\n", "usb_device_reset fail status", status);
> > >  }
> > >  
> > >  static void vnt_free_int_bufs(struct vnt_private *priv)
> > > 
> > 
> > As other people have said:
> > 
> > This function is usb_device_reset().  If that function name string is to be
> > used in the message, it should be done so by using __func__.
> > See http://marc.info/?l=linux-driver-devel=149095639202492=2
> > 
> > Or is the called (failing) function name is to be kept in the message (as it
> > is currently), then the message should contain "usb_reset_device" instead of
> > "usb_device_reset".  See 
> > http://marc.info/?l=linux-driver-devel=149098680312723=2
> 
> If this is to be changed at all, I suggest
> just getting rid of the function.

These are good points, but any feedback on the dev_warn() call itself? 
I was trying to fix the checkpatch warning on my first try.

Re: [BUG nohz]: wrong user and system time accounting

[Luiz Capitulino]

On Sat, 1 Apr 2017 01:24:54 +0200
Frederic Weisbecker  wrote:

> On Fri, Mar 31, 2017 at 04:09:10PM -0400, Luiz Capitulino wrote:
> > On Thu, 30 Mar 2017 17:25:46 -0400
> > Luiz Capitulino  wrote:
> >   
> > > On Thu, 30 Mar 2017 16:18:17 +0200
> > > Frederic Weisbecker  wrote:
> > >   
> > > > On Thu, Mar 30, 2017 at 09:59:54PM +0800, Wanpeng Li wrote:
> > > > > 2017-03-30 21:38 GMT+08:00 Frederic Weisbecker :  
> > > > > 
> > > > > > If it works, we may want to take that solution, likely less 
> > > > > > performance sensitive
> > > > > > than using sched_clock(). In fact sched_clock() is fast, especially 
> > > > > > as we require it to
> > > > > > be stable for nohz_full, but using it involves costly conversion 
> > > > > > back and forth to jiffies.  
> > > > > 
> > > > > So both Rik and you agree with the skew tick solution, I will try it
> > > > > tomorrow. Btw, if we should just add random offset to the cpu in the
> > > > > nohz_full mode or add random offset to all cpus like the codes above? 
> > > > >  
> > > > 
> > > > Lets just keep it to all CPUs for simplicty.
> > > > Also please add a comment that explains why we need that skew_tick on 
> > > > nohz_full.
> > > 
> > > I've tried all the test-cases we discussed in this thread with skew_tick=1
> > > and it worked as expected in bare-metal and KVM guests.
> > > 
> > > However, I found a test-case that works in bare-metal but show problems
> > > in KVM guests. It could something that's KVM specific, or it could be
> > > something that's harder to reproduce in bare-metal.  
> > 
> > After discussing some findings on this issue with Rik, I realized that
> > we don't add the skew when restarting the tick in tick_nohz_restart().
> > Adding the offset there seems to solve this problem.  
> 
> Are you sure? tick_nohz_restart() doesn't seem to override the initial skew. 
> It
> always forwards the expiration time on top of the last tick.

OK, I'll double check. Without my change the bug triggers almost
instantly with the described reproducer. With my change it didn't
trig for several minutes (but it does look wrong looking at it now).

Re: [BUG nohz]: wrong user and system time accounting

[Luiz Capitulino]

On Sat, 1 Apr 2017 01:24:54 +0200
Frederic Weisbecker  wrote:

> On Fri, Mar 31, 2017 at 04:09:10PM -0400, Luiz Capitulino wrote:
> > On Thu, 30 Mar 2017 17:25:46 -0400
> > Luiz Capitulino  wrote:
> >   
> > > On Thu, 30 Mar 2017 16:18:17 +0200
> > > Frederic Weisbecker  wrote:
> > >   
> > > > On Thu, Mar 30, 2017 at 09:59:54PM +0800, Wanpeng Li wrote:
> > > > > 2017-03-30 21:38 GMT+08:00 Frederic Weisbecker :  
> > > > > 
> > > > > > If it works, we may want to take that solution, likely less 
> > > > > > performance sensitive
> > > > > > than using sched_clock(). In fact sched_clock() is fast, especially 
> > > > > > as we require it to
> > > > > > be stable for nohz_full, but using it involves costly conversion 
> > > > > > back and forth to jiffies.  
> > > > > 
> > > > > So both Rik and you agree with the skew tick solution, I will try it
> > > > > tomorrow. Btw, if we should just add random offset to the cpu in the
> > > > > nohz_full mode or add random offset to all cpus like the codes above? 
> > > > >  
> > > > 
> > > > Lets just keep it to all CPUs for simplicty.
> > > > Also please add a comment that explains why we need that skew_tick on 
> > > > nohz_full.
> > > 
> > > I've tried all the test-cases we discussed in this thread with skew_tick=1
> > > and it worked as expected in bare-metal and KVM guests.
> > > 
> > > However, I found a test-case that works in bare-metal but show problems
> > > in KVM guests. It could something that's KVM specific, or it could be
> > > something that's harder to reproduce in bare-metal.  
> > 
> > After discussing some findings on this issue with Rik, I realized that
> > we don't add the skew when restarting the tick in tick_nohz_restart().
> > Adding the offset there seems to solve this problem.  
> 
> Are you sure? tick_nohz_restart() doesn't seem to override the initial skew. 
> It
> always forwards the expiration time on top of the last tick.

OK, I'll double check. Without my change the bug triggers almost
instantly with the described reproducer. With my change it didn't
trig for several minutes (but it does look wrong looking at it now).

Re: [PATCH -mm -v7 9/9] mm, THP, swap: Delay splitting THP during swap out

[Huang, Ying]

Johannes Weiner  writes:

> On Thu, Mar 30, 2017 at 12:15:13PM +0800, Huang, Ying wrote:
>> Johannes Weiner  writes:
>> > On Tue, Mar 28, 2017 at 01:32:09PM +0800, Huang, Ying wrote:
>> >> @@ -198,6 +240,18 @@ int add_to_swap(struct page *page, struct list_head 
>> >> *list)
>> >>   VM_BUG_ON_PAGE(!PageLocked(page), page);
>> >>   VM_BUG_ON_PAGE(!PageUptodate(page), page);
>> >>  
>> >> + if (unlikely(PageTransHuge(page))) {
>> >> + err = add_to_swap_trans_huge(page, list);
>> >> + switch (err) {
>> >> + case 1:
>> >> + return 1;
>> >> + case 0:
>> >> + /* fallback to split firstly if return 0 */
>> >> + break;
>> >> + default:
>> >> + return 0;
>> >> + }
>> >> + }
>> >>   entry = get_swap_page();
>> >>   if (!entry.val)
>> >>   return 0;
>> >
>> > add_to_swap_trans_huge() is too close a copy of add_to_swap(), which
>> > makes the code error prone for future modifications to the swap slot
>> > allocation protocol.
>> >
>> > This should read:
>> >
>> > retry:
>> >entry = get_swap_page(page);
>> >if (!entry.val) {
>> >if (PageTransHuge(page)) {
>> >split_huge_page_to_list(page, list);
>> >goto retry;
>> >}
>> >return 0;
>> >}
>> 
>> If the swap space is used up, that is, get_swap_page() cannot allocate
>> even 1 swap entry for a normal page.  We will split THP unnecessarily
>> with the change, but in the original code, we just skip the THP.  There
>> may be a performance regression here.  Similar problem exists for
>> mem_cgroup_try_charge_swap() too.  If the mem cgroup exceeds the swap
>> limit, the THP will be split unnecessary with the change too.
>
> If we skip the page, we're swapping out another page hotter than this
> one. Giving THP preservation priority over LRU order is an issue best
> kept for a separate patch set;

In my original patch, if we failed to allocate the swap space for a THP,
and we can allocate the swap space for a normal page, we will split the
THP.  We skip the page only if we cannot allocate the swap space for a
normal page, that is, nr_swap_pages is 0.  So we will not give THP
preservation priority over LRU order in the patch.

> this one is supposed to be a mechanical
> implementation of THP swapping. Let's nail down the basics first.

Yes.  So I tried to keep the original behavior to deal with THP if we
cannot allocate the swap space (a swap cluster) for a whole THP.

Per my understanding, the difference between what you suggested and the
original behavior is that, when nr_swap_pages is 0, whether to split the
THP.

> Such a decision would need proof that splitting THPs on full swap
> devices is a concern for real applications. I would assume that we're
> pretty close to OOM anyway; it's much more likely that a single slot
> frees up than a full cluster, at which point we'll be splitting THPs
> anyway; etc. I have my doubts that this would be measurable.
>
> But even if so, I don't think we'd have to duplicate the main code
> flow to handle this corner case. You can extend get_swap_page() to
> return an error code that tells add_to_swap() whether to split and
> retry, or to fail and move on. So this way should be future proof.

Yes.  I will try to merge add_to_swap_trans_huge() into add_to_swap() in
the next version.  But if we want to keep the original behavior, we will
need an extra "nr_entries" parameter for mem_cgroup_try_charge_swap().

Best Regards,
Huang, Ying

Re: [PATCH -mm -v7 9/9] mm, THP, swap: Delay splitting THP during swap out

[Huang, Ying]

Johannes Weiner  writes:

> On Thu, Mar 30, 2017 at 12:15:13PM +0800, Huang, Ying wrote:
>> Johannes Weiner  writes:
>> > On Tue, Mar 28, 2017 at 01:32:09PM +0800, Huang, Ying wrote:
>> >> @@ -198,6 +240,18 @@ int add_to_swap(struct page *page, struct list_head 
>> >> *list)
>> >>   VM_BUG_ON_PAGE(!PageLocked(page), page);
>> >>   VM_BUG_ON_PAGE(!PageUptodate(page), page);
>> >>  
>> >> + if (unlikely(PageTransHuge(page))) {
>> >> + err = add_to_swap_trans_huge(page, list);
>> >> + switch (err) {
>> >> + case 1:
>> >> + return 1;
>> >> + case 0:
>> >> + /* fallback to split firstly if return 0 */
>> >> + break;
>> >> + default:
>> >> + return 0;
>> >> + }
>> >> + }
>> >>   entry = get_swap_page();
>> >>   if (!entry.val)
>> >>   return 0;
>> >
>> > add_to_swap_trans_huge() is too close a copy of add_to_swap(), which
>> > makes the code error prone for future modifications to the swap slot
>> > allocation protocol.
>> >
>> > This should read:
>> >
>> > retry:
>> >entry = get_swap_page(page);
>> >if (!entry.val) {
>> >if (PageTransHuge(page)) {
>> >split_huge_page_to_list(page, list);
>> >goto retry;
>> >}
>> >return 0;
>> >}
>> 
>> If the swap space is used up, that is, get_swap_page() cannot allocate
>> even 1 swap entry for a normal page.  We will split THP unnecessarily
>> with the change, but in the original code, we just skip the THP.  There
>> may be a performance regression here.  Similar problem exists for
>> mem_cgroup_try_charge_swap() too.  If the mem cgroup exceeds the swap
>> limit, the THP will be split unnecessary with the change too.
>
> If we skip the page, we're swapping out another page hotter than this
> one. Giving THP preservation priority over LRU order is an issue best
> kept for a separate patch set;

In my original patch, if we failed to allocate the swap space for a THP,
and we can allocate the swap space for a normal page, we will split the
THP.  We skip the page only if we cannot allocate the swap space for a
normal page, that is, nr_swap_pages is 0.  So we will not give THP
preservation priority over LRU order in the patch.

> this one is supposed to be a mechanical
> implementation of THP swapping. Let's nail down the basics first.

Yes.  So I tried to keep the original behavior to deal with THP if we
cannot allocate the swap space (a swap cluster) for a whole THP.

Per my understanding, the difference between what you suggested and the
original behavior is that, when nr_swap_pages is 0, whether to split the
THP.

> Such a decision would need proof that splitting THPs on full swap
> devices is a concern for real applications. I would assume that we're
> pretty close to OOM anyway; it's much more likely that a single slot
> frees up than a full cluster, at which point we'll be splitting THPs
> anyway; etc. I have my doubts that this would be measurable.
>
> But even if so, I don't think we'd have to duplicate the main code
> flow to handle this corner case. You can extend get_swap_page() to
> return an error code that tells add_to_swap() whether to split and
> retry, or to fail and move on. So this way should be future proof.

Yes.  I will try to merge add_to_swap_trans_huge() into add_to_swap() in
the next version.  But if we want to keep the original behavior, we will
need an extra "nr_entries" parameter for mem_cgroup_try_charge_swap().

Best Regards,
Huang, Ying

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Fri, 2017-03-31 at 19:15 -0700, Randy Dunlap wrote:
> On 03/31/17 18:59, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > WARNING: Prefer using "%s", __func__ to embedded function names
> > #417: FILE: main_usb.c:417:
> > +"usb_device_reset fail status=%d\n", status);
> > 
> > total: 0 errors, 1 warnings, 1058 lines checked
> > 
> > And after fix:
> > 
> > main_usb.c has no obvious style problems and is ready for submission.
> > 
> > Signed-off-by: Chewie Lin 
> > ---
> >  drivers/staging/vt6656/main_usb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/vt6656/main_usb.c 
> > b/drivers/staging/vt6656/main_usb.c
> > index 9e074e9..2d9e7af 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
> > status = usb_reset_device(priv->usb);
> > if (status)
> > dev_warn(>usb->dev,
> > -"usb_device_reset fail status=%d\n", status);
> > +"%s=%d\n", "usb_device_reset fail status", status);
> >  }
> >  
> >  static void vnt_free_int_bufs(struct vnt_private *priv)
> > 
> 
> As other people have said:
> 
> This function is usb_device_reset().  If that function name string is to be
> used in the message, it should be done so by using __func__.
> See http://marc.info/?l=linux-driver-devel=149095639202492=2
> 
> Or is the called (failing) function name is to be kept in the message (as it
> is currently), then the message should contain "usb_reset_device" instead of
> "usb_device_reset".  See 
> http://marc.info/?l=linux-driver-devel=149098680312723=2

If this is to be changed at all, I suggest
just getting rid of the function.
---
 drivers/staging/vt6656/main_usb.c | 14 +++---
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c 
b/drivers/staging/vt6656/main_usb.c
index 9e074e9daf4e..799aeeb6812d 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -407,16 +407,6 @@ static void vnt_free_rx_bufs(struct vnt_private *priv)
}
 }
 
-static void usb_device_reset(struct vnt_private *priv)
-{
-   int status;
-
-   status = usb_reset_device(priv->usb);
-   if (status)
-   dev_warn(>usb->dev,
-"usb_device_reset fail status=%d\n", status);
-}
-
 static void vnt_free_int_bufs(struct vnt_private *priv)
 {
kfree(priv->int_buf.data_buf);
@@ -995,7 +985,9 @@ vt6656_probe(struct usb_interface *intf, const struct 
usb_device_id *id)
 
SET_IEEE80211_DEV(priv->hw, >dev);
 
-   usb_device_reset(priv);
+   rc = usb_reset_device(priv->usb);
+   if (rc)
+   dev_warn(>usb->dev, "usb_reset_device failed: %d\n", rc);
 
clear_bit(DEVICE_FLAGS_DISCONNECTED, >flags);
vnt_reset_command_timer(priv);

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Fri, 2017-03-31 at 19:15 -0700, Randy Dunlap wrote:
> On 03/31/17 18:59, Chewie Lin wrote:
> > Replace string with formatted arguments in the dev_warn() call. It removes
> > the checkpatch warning:
> > 
> > WARNING: Prefer using "%s", __func__ to embedded function names
> > #417: FILE: main_usb.c:417:
> > +"usb_device_reset fail status=%d\n", status);
> > 
> > total: 0 errors, 1 warnings, 1058 lines checked
> > 
> > And after fix:
> > 
> > main_usb.c has no obvious style problems and is ready for submission.
> > 
> > Signed-off-by: Chewie Lin 
> > ---
> >  drivers/staging/vt6656/main_usb.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/drivers/staging/vt6656/main_usb.c 
> > b/drivers/staging/vt6656/main_usb.c
> > index 9e074e9..2d9e7af 100644
> > --- a/drivers/staging/vt6656/main_usb.c
> > +++ b/drivers/staging/vt6656/main_usb.c
> > @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
> > status = usb_reset_device(priv->usb);
> > if (status)
> > dev_warn(>usb->dev,
> > -"usb_device_reset fail status=%d\n", status);
> > +"%s=%d\n", "usb_device_reset fail status", status);
> >  }
> >  
> >  static void vnt_free_int_bufs(struct vnt_private *priv)
> > 
> 
> As other people have said:
> 
> This function is usb_device_reset().  If that function name string is to be
> used in the message, it should be done so by using __func__.
> See http://marc.info/?l=linux-driver-devel=149095639202492=2
> 
> Or is the called (failing) function name is to be kept in the message (as it
> is currently), then the message should contain "usb_reset_device" instead of
> "usb_device_reset".  See 
> http://marc.info/?l=linux-driver-devel=149098680312723=2

If this is to be changed at all, I suggest
just getting rid of the function.
---
 drivers/staging/vt6656/main_usb.c | 14 +++---
 1 file changed, 3 insertions(+), 11 deletions(-)

diff --git a/drivers/staging/vt6656/main_usb.c 
b/drivers/staging/vt6656/main_usb.c
index 9e074e9daf4e..799aeeb6812d 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -407,16 +407,6 @@ static void vnt_free_rx_bufs(struct vnt_private *priv)
}
 }
 
-static void usb_device_reset(struct vnt_private *priv)
-{
-   int status;
-
-   status = usb_reset_device(priv->usb);
-   if (status)
-   dev_warn(>usb->dev,
-"usb_device_reset fail status=%d\n", status);
-}
-
 static void vnt_free_int_bufs(struct vnt_private *priv)
 {
kfree(priv->int_buf.data_buf);
@@ -995,7 +985,9 @@ vt6656_probe(struct usb_interface *intf, const struct 
usb_device_id *id)
 
SET_IEEE80211_DEV(priv->hw, >dev);
 
-   usb_device_reset(priv);
+   rc = usb_reset_device(priv->usb);
+   if (rc)
+   dev_warn(>usb->dev, "usb_reset_device failed: %d\n", rc);
 
clear_bit(DEVICE_FLAGS_DISCONNECTED, >flags);
vnt_reset_command_timer(priv);

[PATCH] f2fs: remove the redundant variable definition

[Kaixu Xia]

The variable 'i' has been defined before, so here we can
use it directly.

Signed-off-by: Kaixu Xia 
---
 fs/f2fs/checkpoint.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 0339daf..2e42684 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1143,7 +1143,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct 
cp_control *cpc)
/* write nat bits */
if (enabled_nat_bits(sbi, cpc)) {
__u64 cp_ver = cur_cp_version(ckpt);
-   unsigned int i;
block_t blk;
 
cp_ver |= ((__u64)crc32 << 32);
-- 
1.7.10.4

[PATCH] f2fs: remove the redundant variable definition

[Kaixu Xia]

The variable 'i' has been defined before, so here we can
use it directly.

Signed-off-by: Kaixu Xia 
---
 fs/f2fs/checkpoint.c |1 -
 1 file changed, 1 deletion(-)

diff --git a/fs/f2fs/checkpoint.c b/fs/f2fs/checkpoint.c
index 0339daf..2e42684 100644
--- a/fs/f2fs/checkpoint.c
+++ b/fs/f2fs/checkpoint.c
@@ -1143,7 +1143,6 @@ static int do_checkpoint(struct f2fs_sb_info *sbi, struct 
cp_control *cpc)
/* write nat bits */
if (enabled_nat_bits(sbi, cpc)) {
__u64 cp_ver = cur_cp_version(ckpt);
-   unsigned int i;
block_t blk;
 
cp_ver |= ((__u64)crc32 << 32);
-- 
1.7.10.4

[PATCH] staging: iio: light: constify attribute_group structures

[simran singhal]

As the event_attrs field of iio_info structures is constant, so these
attribute_group structures can also be declared constant.

File size before:
   textdata bss dec hex filename
  150641528   0   1659240d0
drivers/staging/iio/light/tsl2x7x_core.o

File size after:
   textdata bss dec hex filename
  151921400   0   1659240d0
drivers/staging/iio/light/tsl2x7x_core.o

Signed-off-by: simran singhal 
---
 drivers/staging/iio/light/tsl2x7x_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/light/tsl2x7x_core.c 
b/drivers/staging/iio/light/tsl2x7x_core.c
index 0490c1d..9a0046a 100644
--- a/drivers/staging/iio/light/tsl2x7x_core.c
+++ b/drivers/staging/iio/light/tsl2x7x_core.c
@@ -1676,7 +1676,7 @@ static const struct attribute_group 
tsl2X7X_device_attr_group_tbl[] = {
},
 };
 
-static struct attribute_group tsl2X7X_event_attr_group_tbl[] = {
+static const struct attribute_group tsl2X7X_event_attr_group_tbl[] = {
[ALS] = {
.attrs = tsl2X7X_ALS_event_attrs,
.name = "events",
-- 
2.7.4

[PATCH] staging: iio: light: constify attribute_group structures

[simran singhal]

As the event_attrs field of iio_info structures is constant, so these
attribute_group structures can also be declared constant.

File size before:
   textdata bss dec hex filename
  150641528   0   1659240d0
drivers/staging/iio/light/tsl2x7x_core.o

File size after:
   textdata bss dec hex filename
  151921400   0   1659240d0
drivers/staging/iio/light/tsl2x7x_core.o

Signed-off-by: simran singhal 
---
 drivers/staging/iio/light/tsl2x7x_core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/iio/light/tsl2x7x_core.c 
b/drivers/staging/iio/light/tsl2x7x_core.c
index 0490c1d..9a0046a 100644
--- a/drivers/staging/iio/light/tsl2x7x_core.c
+++ b/drivers/staging/iio/light/tsl2x7x_core.c
@@ -1676,7 +1676,7 @@ static const struct attribute_group 
tsl2X7X_device_attr_group_tbl[] = {
},
 };
 
-static struct attribute_group tsl2X7X_event_attr_group_tbl[] = {
+static const struct attribute_group tsl2X7X_event_attr_group_tbl[] = {
[ALS] = {
.attrs = tsl2X7X_ALS_event_attrs,
.name = "events",
-- 
2.7.4

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

[Andy Lutomirski]

On Fri, Mar 31, 2017 at 2:26 PM, Stas Sergeev  wrote:
> 31.03.2017 17:11, Alexandre Julliard пишет:
>>
>> In fact it would be nice to be able to make sidt/sgdt/etc. segfault
>> too. I know a new syscall is a pain,
>
> Maybe arch_prctl() then?

I still like my idea of a generic mechanism to turn off
backwards-compatibility things.  After all, hardened programs should
turn off UMIP fixups entirely.  They should also turn off vsyscall
emulation entirely, and I see no reason that these mechanisms should
be different.

--Andy

Re: [v6 PATCH 00/21] x86: Enable User-Mode Instruction Prevention

[Andy Lutomirski]

On Fri, Mar 31, 2017 at 2:26 PM, Stas Sergeev  wrote:
> 31.03.2017 17:11, Alexandre Julliard пишет:
>>
>> In fact it would be nice to be able to make sidt/sgdt/etc. segfault
>> too. I know a new syscall is a pain,
>
> Maybe arch_prctl() then?

I still like my idea of a generic mechanism to turn off
backwards-compatibility things.  After all, hardened programs should
turn off UMIP fixups entirely.  They should also turn off vsyscall
emulation entirely, and I see no reason that these mechanisms should
be different.

--Andy

Re: [RFC 1/8] Introduce Peer-to-Peer memory (p2pmem) device

[okaya]

On 2017-03-31 21:57, Logan Gunthorpe wrote:

On 31/03/17 05:51 PM, Sinan Kaya wrote:
You can put a restriction with DMI/SMBIOS such that all devices from 
2016

work else they belong to blacklist.


How do you get a manufacturing date for a given device within the
kernel? Is this actually something generically available?

Logan


Smbios calls are used all over the place in kernel for introducing new 
functionality while maintaining backwards compatibility.


See drivers/pci and drivers/acpi directory.

Re: [RFC 1/8] Introduce Peer-to-Peer memory (p2pmem) device

[okaya]

On 2017-03-31 21:57, Logan Gunthorpe wrote:

On 31/03/17 05:51 PM, Sinan Kaya wrote:
You can put a restriction with DMI/SMBIOS such that all devices from 
2016

work else they belong to blacklist.


How do you get a manufacturing date for a given device within the
kernel? Is this actually something generically available?

Logan


Smbios calls are used all over the place in kernel for introducing new 
functionality while maintaining backwards compatibility.


See drivers/pci and drivers/acpi directory.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Fri, 2017-03-31 at 18:59 -0700, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
> 
>   WARNING: Prefer using "%s", __func__ to embedded function names
>   #417: FILE: main_usb.c:417:
>   +"usb_device_reset fail status=%d\n", status);
> 
>   total: 0 errors, 1 warnings, 1058 lines checked
> 
> And after fix:
> 
>   main_usb.c has no obvious style problems and is ready for submission.
> 
> Signed-off-by: Chewie Lin 
> ---
>  drivers/staging/vt6656/main_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/main_usb.c 
> b/drivers/staging/vt6656/main_usb.c
> index 9e074e9..2d9e7af 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
>   status = usb_reset_device(priv->usb);
>   if (status)
>   dev_warn(>usb->dev,
> -  "usb_device_reset fail status=%d\n", status);
> +  "%s=%d\n", "usb_device_reset fail status", status);

So, what is failing?  usb_reset_device or usb_device_reset?

And this function is used exactly once, is trivial, and
would likely be better as direct code in the one place
it's used.

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Joe Perches]

On Fri, 2017-03-31 at 18:59 -0700, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
> 
>   WARNING: Prefer using "%s", __func__ to embedded function names
>   #417: FILE: main_usb.c:417:
>   +"usb_device_reset fail status=%d\n", status);
> 
>   total: 0 errors, 1 warnings, 1058 lines checked
> 
> And after fix:
> 
>   main_usb.c has no obvious style problems and is ready for submission.
> 
> Signed-off-by: Chewie Lin 
> ---
>  drivers/staging/vt6656/main_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/main_usb.c 
> b/drivers/staging/vt6656/main_usb.c
> index 9e074e9..2d9e7af 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
>   status = usb_reset_device(priv->usb);
>   if (status)
>   dev_warn(>usb->dev,
> -  "usb_device_reset fail status=%d\n", status);
> +  "%s=%d\n", "usb_device_reset fail status", status);

So, what is failing?  usb_reset_device or usb_device_reset?

And this function is used exactly once, is trivial, and
would likely be better as direct code in the one place
it's used.

Re: [PATCH V8 5/6] ACPI: Support the probing on the devices which apply indirect-IO

[zhichang.yuan]

On 04/01/2017 07:02 AM, Rafael J. Wysocki wrote:
> On Fri, Mar 31, 2017 at 8:52 AM, zhichang.yuan
>  wrote:
>> Hi, Rafael,
>>
>> Thanks for reviewing this!
>>
>> On 2017/3/31 4:31, Rafael J. Wysocki wrote:
>>> On Thursday, March 30, 2017 11:26:58 PM zhichang.yuan wrote:
 On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access 
 I/O
 with some special host-local I/O ports known on x86. To access the I/O
 peripherals, an indirect-IO mechanism is introduced to mapped the 
 host-local
 I/O to system logical/fake PIO similar the PCI MMIO on architectures where 
 no
 separate I/O space exists. Just as PCI MMIO, the host I/O range should be
 registered before probing the downstream devices and set up the I/O 
 mapping.
 But current ACPI bus probing doesn't support these indirect-IO 
 hosts/devices.

 This patch introdueces a new ACPI handler for this device category. 
 Through the
 handler attach callback, the indirect-IO hosts I/O registration is done and
 all peripherals' I/O resources are translated into logic/fake PIO before
 starting the enumeration.
>>>
>>> Can you explain to me briefly what exactly this code is expected to be 
>>> doing?
>>
>> As you know currently for ARM architecture IO space is memory mapped and
>> is only used by pci devices. The port number is dynamically allocated
>> converting the device IO address into a PIO token: i.e.
>> http://lxr.free-electrons.com/source/drivers/acpi/pci_root.c#L745
>> This patch is meant to support a new class of IO host controller
>> that are not PCI based and that still require to have the IO addresses
>> be translated in the same PIO token space as the PCI controller
> 
> IOW, this is ARM-specific, right?

Yes. The current host added in this patch with _HID "HISI0191" is on ARM64.
But, I think the handler driver is architecture dependent.

Thanks,
Zhichang

> 
> Thanks,
> Rafael
> 

Re: [PATCH V8 5/6] ACPI: Support the probing on the devices which apply indirect-IO

[zhichang.yuan]

On 04/01/2017 07:02 AM, Rafael J. Wysocki wrote:
> On Fri, Mar 31, 2017 at 8:52 AM, zhichang.yuan
>  wrote:
>> Hi, Rafael,
>>
>> Thanks for reviewing this!
>>
>> On 2017/3/31 4:31, Rafael J. Wysocki wrote:
>>> On Thursday, March 30, 2017 11:26:58 PM zhichang.yuan wrote:
 On some platforms(such as Hip06/Hip07), the legacy ISA/LPC devices access 
 I/O
 with some special host-local I/O ports known on x86. To access the I/O
 peripherals, an indirect-IO mechanism is introduced to mapped the 
 host-local
 I/O to system logical/fake PIO similar the PCI MMIO on architectures where 
 no
 separate I/O space exists. Just as PCI MMIO, the host I/O range should be
 registered before probing the downstream devices and set up the I/O 
 mapping.
 But current ACPI bus probing doesn't support these indirect-IO 
 hosts/devices.

 This patch introdueces a new ACPI handler for this device category. 
 Through the
 handler attach callback, the indirect-IO hosts I/O registration is done and
 all peripherals' I/O resources are translated into logic/fake PIO before
 starting the enumeration.
>>>
>>> Can you explain to me briefly what exactly this code is expected to be 
>>> doing?
>>
>> As you know currently for ARM architecture IO space is memory mapped and
>> is only used by pci devices. The port number is dynamically allocated
>> converting the device IO address into a PIO token: i.e.
>> http://lxr.free-electrons.com/source/drivers/acpi/pci_root.c#L745
>> This patch is meant to support a new class of IO host controller
>> that are not PCI based and that still require to have the IO addresses
>> be translated in the same PIO token space as the PCI controller
> 
> IOW, this is ARM-specific, right?

Yes. The current host added in this patch with _HID "HISI0191" is on ARM64.
But, I think the handler driver is architecture dependent.

Thanks,
Zhichang

> 
> Thanks,
> Rafael
> 

Re: [PATCH v23 00/11] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer

[Xiongfeng Wang]

On 2017/4/1 1:50, fu@linaro.org wrote:
> From: Fu Wei 
> 
> This patchset:
> (1)Preparation for adding GTDT support in arm_arch_timer:
> 1. Introduce a MMIO CNTFRQ helper.
> 2. separate out device-tree code from arch_timer_detect_rate
> 3. remove arch_timer_detect_rate use arch_timer_*get_cntfrq directly
> 4. Refactor arch_timer_needs_probing, and move it into DT init call
> 5. Introduce some new structs and refactor the MMIO timer init code
> for reusing some common code.
> 
> (2)Introduce ACPI GTDT parser: drivers/acpi/arm64/acpi_gtdt.c
> Parse all kinds of timer in GTDT table of ACPI:arch timer,
> memory-mapped timer and SBSA Generic Watchdog timer.
> This driver can help to simplify all the relevant timer drivers,
> and separate all the ACPI GTDT knowledge from them.
> 
> (3)Simplify ACPI code for arm_arch_timer
> 
> (4)Add GTDT support for ARM memory-mapped timer.
> 
> This patchset has been tested on the following platforms with ACPI enabled:
> (1)ARM Foundation v8 model
> 

for arm_arch_timer(not memory-mapped) and sbsa watchdog part,  Tested-by: 
wangxiongfe...@huawei.com





Thanks,

Wang Xiongfeng
.

> Changelog:
> v23: https://lkml.org/lkml/2017/3/31/
>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
> arch-timer/cleanup
>  Improve the data struct of arch_timer_mem and arch_timer_mem_frame to
>  improve the parser of GT blocks and arch_timer_mem initualization.
>  Improve arch_timer_rate detection: sysreg frequency is primary in DT boot
>  Improve some comments in GTDT parser driver.
>  Improve acpi_gtdt_init function, and make a comment for the multiple 
> calls.
>  Improve the unwinding for the irq of timers, when an error occurs.
>  Handle the case of virtual timer GSIV is 0.
> 
> v22: https://lkml.org/lkml/2017/3/21/523
>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
> arch-timer/cleanup
>  Only Introduce arch_timer_mem_get_cntfrq to get the frequency from mmio.
>  Merged patch 2,3(about arch_timer_detect_rate).
>  Keep arch_timer_rate, do NOT split it for different types of timer.
>  Improve  memory-mapped timer support by comments and variable name:
>  data-->timer_mem
>  frame-->gtdt_frame
>  Delete zero check for SBSA watchdog irq.
>  Skip secure SBSA watchdog in GTDT driver.
>  Delete Kconfig modification for SBSA watchdog driver.
>  Delete no_irq, using nr_res instead.
> 
> v21: https://lkml.org/lkml/2017/2/6/734
>  Introduce two functions to get the frequency from mmio and sysreg.
>  Remove arch_timer_detect_rate use arch_timer_get_*_freq directly
>  Split arch_timer_rate for different types of timer.
>  Skip secure timer frame in GTDT driver.
>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
> arch-timer/cleanup
>  (The first 6 patches in v20 have been merged into arch-timer/cleanup 
> branch)
> 
> v20: https://lkml.org/lkml/2017/1/18/534
>  Reorder the first 4 patches and split the 4th patches.
>  Leave CNTHCTL_* as they originally were.
>  Fix the bug in arch_timer_select_ppi.
>  Split "Rework counter frequency detection" patch.
>  Rework the arch_timer_detect_rate function.
>  Improve the commit message of "Refactor MMIO timer probing".
>  Rebase to 4.10.0-rc4
> 
> v19: https://lkml.org/lkml/2016/12/21/25
>  Fix a '\n' missing in a error message in arch_timer_mem_init.
>  Add "request_mem_region" for ioremapping cntbase, according to
>  f947ee1 clocksource/drivers/arm_arch_timer: Map frame with 
> of_io_request_and_map()
>  Rebase to 4.9.0-gfb779ff
> 
> v18: https://lkml.org/lkml/2016/12/8/446
>  Fix 8/15 patch problem of "int ret;" in arch_timer_acpi_init.
>  Rebase to 4.9.0-rc8-g9269898
> 
> v17: https://lkml.org/lkml/2016/11/25/140
>  Take out some cleanups from 4/15.
>  Merge 5/15 and 6/15, improve PPI determination code,
>  improve commit message.
>  Rework counter frequency detection.
>  Move arch_timer_needs_of_probing into DT init call.
>  Move Platform Timer scan loop back to timer init call to avoid allocating
>  and free memory.
>  Improve all the exported functions' comment.
> 
> v16: https://lkml.org/lkml/2016/11/16/268
>  Fix patchset problem about static enum ppi_nr of 01/13 in v15.
>  Refactor arch_timer_detect_rate.
>  Refactor arch_timer_needs_probing.
> 
> v15: https://lkml.org/lkml/2016/11/15/366
>  Re-order patches
>  Add arm_arch_timer refactoring patches to prepare for GTDT:
>  1. rename some  enums and defines, and some cleanups
>  2. separate out arch_timer_uses_ppi init code and fix a potential bug
>  3. Improve some new structs, refactor the timer init code.
>  Since the some structs have been changed, GTDT parser for memory-mapped
>  

Re: [PATCH v23 00/11] acpi, clocksource: add GTDT driver and GTDT support in arm_arch_timer

[Xiongfeng Wang]

On 2017/4/1 1:50, fu@linaro.org wrote:
> From: Fu Wei 
> 
> This patchset:
> (1)Preparation for adding GTDT support in arm_arch_timer:
> 1. Introduce a MMIO CNTFRQ helper.
> 2. separate out device-tree code from arch_timer_detect_rate
> 3. remove arch_timer_detect_rate use arch_timer_*get_cntfrq directly
> 4. Refactor arch_timer_needs_probing, and move it into DT init call
> 5. Introduce some new structs and refactor the MMIO timer init code
> for reusing some common code.
> 
> (2)Introduce ACPI GTDT parser: drivers/acpi/arm64/acpi_gtdt.c
> Parse all kinds of timer in GTDT table of ACPI:arch timer,
> memory-mapped timer and SBSA Generic Watchdog timer.
> This driver can help to simplify all the relevant timer drivers,
> and separate all the ACPI GTDT knowledge from them.
> 
> (3)Simplify ACPI code for arm_arch_timer
> 
> (4)Add GTDT support for ARM memory-mapped timer.
> 
> This patchset has been tested on the following platforms with ACPI enabled:
> (1)ARM Foundation v8 model
> 

for arm_arch_timer(not memory-mapped) and sbsa watchdog part,  Tested-by: 
wangxiongfe...@huawei.com





Thanks,

Wang Xiongfeng
.

> Changelog:
> v23: https://lkml.org/lkml/2017/3/31/
>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
> arch-timer/cleanup
>  Improve the data struct of arch_timer_mem and arch_timer_mem_frame to
>  improve the parser of GT blocks and arch_timer_mem initualization.
>  Improve arch_timer_rate detection: sysreg frequency is primary in DT boot
>  Improve some comments in GTDT parser driver.
>  Improve acpi_gtdt_init function, and make a comment for the multiple 
> calls.
>  Improve the unwinding for the irq of timers, when an error occurs.
>  Handle the case of virtual timer GSIV is 0.
> 
> v22: https://lkml.org/lkml/2017/3/21/523
>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
> arch-timer/cleanup
>  Only Introduce arch_timer_mem_get_cntfrq to get the frequency from mmio.
>  Merged patch 2,3(about arch_timer_detect_rate).
>  Keep arch_timer_rate, do NOT split it for different types of timer.
>  Improve  memory-mapped timer support by comments and variable name:
>  data-->timer_mem
>  frame-->gtdt_frame
>  Delete zero check for SBSA watchdog irq.
>  Skip secure SBSA watchdog in GTDT driver.
>  Delete Kconfig modification for SBSA watchdog driver.
>  Delete no_irq, using nr_res instead.
> 
> v21: https://lkml.org/lkml/2017/2/6/734
>  Introduce two functions to get the frequency from mmio and sysreg.
>  Remove arch_timer_detect_rate use arch_timer_get_*_freq directly
>  Split arch_timer_rate for different types of timer.
>  Skip secure timer frame in GTDT driver.
>  Rebase to git://git.kernel.org/pub/scm/linux/kernel/git/mark/linux.git 
> arch-timer/cleanup
>  (The first 6 patches in v20 have been merged into arch-timer/cleanup 
> branch)
> 
> v20: https://lkml.org/lkml/2017/1/18/534
>  Reorder the first 4 patches and split the 4th patches.
>  Leave CNTHCTL_* as they originally were.
>  Fix the bug in arch_timer_select_ppi.
>  Split "Rework counter frequency detection" patch.
>  Rework the arch_timer_detect_rate function.
>  Improve the commit message of "Refactor MMIO timer probing".
>  Rebase to 4.10.0-rc4
> 
> v19: https://lkml.org/lkml/2016/12/21/25
>  Fix a '\n' missing in a error message in arch_timer_mem_init.
>  Add "request_mem_region" for ioremapping cntbase, according to
>  f947ee1 clocksource/drivers/arm_arch_timer: Map frame with 
> of_io_request_and_map()
>  Rebase to 4.9.0-gfb779ff
> 
> v18: https://lkml.org/lkml/2016/12/8/446
>  Fix 8/15 patch problem of "int ret;" in arch_timer_acpi_init.
>  Rebase to 4.9.0-rc8-g9269898
> 
> v17: https://lkml.org/lkml/2016/11/25/140
>  Take out some cleanups from 4/15.
>  Merge 5/15 and 6/15, improve PPI determination code,
>  improve commit message.
>  Rework counter frequency detection.
>  Move arch_timer_needs_of_probing into DT init call.
>  Move Platform Timer scan loop back to timer init call to avoid allocating
>  and free memory.
>  Improve all the exported functions' comment.
> 
> v16: https://lkml.org/lkml/2016/11/16/268
>  Fix patchset problem about static enum ppi_nr of 01/13 in v15.
>  Refactor arch_timer_detect_rate.
>  Refactor arch_timer_needs_probing.
> 
> v15: https://lkml.org/lkml/2016/11/15/366
>  Re-order patches
>  Add arm_arch_timer refactoring patches to prepare for GTDT:
>  1. rename some  enums and defines, and some cleanups
>  2. separate out arch_timer_uses_ppi init code and fix a potential bug
>  3. Improve some new structs, refactor the timer init code.
>  Since the some structs have been changed, GTDT parser for memory-mapped
>  timer and SBSA 

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Randy Dunlap]

On 03/31/17 18:59, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
> 
>   WARNING: Prefer using "%s", __func__ to embedded function names
>   #417: FILE: main_usb.c:417:
>   +"usb_device_reset fail status=%d\n", status);
> 
>   total: 0 errors, 1 warnings, 1058 lines checked
> 
> And after fix:
> 
>   main_usb.c has no obvious style problems and is ready for submission.
> 
> Signed-off-by: Chewie Lin 
> ---
>  drivers/staging/vt6656/main_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/main_usb.c 
> b/drivers/staging/vt6656/main_usb.c
> index 9e074e9..2d9e7af 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
>   status = usb_reset_device(priv->usb);
>   if (status)
>   dev_warn(>usb->dev,
> -  "usb_device_reset fail status=%d\n", status);
> +  "%s=%d\n", "usb_device_reset fail status", status);
>  }
>  
>  static void vnt_free_int_bufs(struct vnt_private *priv)
> 

As other people have said:

This function is usb_device_reset().  If that function name string is to be
used in the message, it should be done so by using __func__.
See http://marc.info/?l=linux-driver-devel=149095639202492=2

Or is the called (failing) function name is to be kept in the message (as it
is currently), then the message should contain "usb_reset_device" instead of
"usb_device_reset".  See 
http://marc.info/?l=linux-driver-devel=149098680312723=2



-- 
~Randy

Re: [PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Randy Dunlap]

On 03/31/17 18:59, Chewie Lin wrote:
> Replace string with formatted arguments in the dev_warn() call. It removes
> the checkpatch warning:
> 
>   WARNING: Prefer using "%s", __func__ to embedded function names
>   #417: FILE: main_usb.c:417:
>   +"usb_device_reset fail status=%d\n", status);
> 
>   total: 0 errors, 1 warnings, 1058 lines checked
> 
> And after fix:
> 
>   main_usb.c has no obvious style problems and is ready for submission.
> 
> Signed-off-by: Chewie Lin 
> ---
>  drivers/staging/vt6656/main_usb.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/staging/vt6656/main_usb.c 
> b/drivers/staging/vt6656/main_usb.c
> index 9e074e9..2d9e7af 100644
> --- a/drivers/staging/vt6656/main_usb.c
> +++ b/drivers/staging/vt6656/main_usb.c
> @@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
>   status = usb_reset_device(priv->usb);
>   if (status)
>   dev_warn(>usb->dev,
> -  "usb_device_reset fail status=%d\n", status);
> +  "%s=%d\n", "usb_device_reset fail status", status);
>  }
>  
>  static void vnt_free_int_bufs(struct vnt_private *priv)
> 

As other people have said:

This function is usb_device_reset().  If that function name string is to be
used in the message, it should be done so by using __func__.
See http://marc.info/?l=linux-driver-devel=149095639202492=2

Or is the called (failing) function name is to be kept in the message (as it
is currently), then the message should contain "usb_reset_device" instead of
"usb_device_reset".  See 
http://marc.info/?l=linux-driver-devel=149098680312723=2



-- 
~Randy

[PATCH 5/9] perf trace: Handle unpaired raw_syscalls:sys_exit event

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo 

Which may happen when we start a tracing session and a thread is waiting
for something like "poll" to return, in which case we better print "?"
both for the syscall entry timestamp and for the duration.

E.g.:

Tracing existing mutt session:

  # perf trace -p `pidof mutt`
  ? ( ?   ): mutt/17135  ... [continued]: poll()) = 1
  0.027 ( 0.013 ms): mutt/17135 read(buf: 0x7ffcb3c42cef, count: 1) = 1
  0.047 ( 0.008 ms): mutt/17135 poll(ufds: 0x7ffcb3c42c50, nfds: 1, 
timeout_msecs: 1000) = 1
  0.059 ( 0.008 ms): mutt/17135 read(buf: 0x7ffcb3c42cef, count: 1) = 1
  

Before it would print a large number because we'd do:

  ttrace->entry_time - trace->base_time

And entry_time would be 0, while base_time would be the timestamp for
the first event 'perf trace' reads, oops.

Cc: Adrian Hunter 
Cc: David Ahern 
Cc: Jiri Olsa 
Cc: Luis Claudio Gonçalves 
Cc: Namhyung Kim 
Cc: Wang Nan 
Link: http://lkml.kernel.org/n/tip-wbcb93ofva2qdjd5ltn5e...@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/builtin-trace.c | 43 ++-
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index c88f9f215e6f..7379792a6504 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -821,12 +821,21 @@ struct syscall {
void**arg_parm;
 };
 
-static size_t fprintf_duration(unsigned long t, FILE *fp)
+/*
+ * We need to have this 'calculated' boolean because in some cases we really
+ * don't know what is the duration of a syscall, for instance, when we start
+ * a session and some threads are waiting for a syscall to finish, say 'poll',
+ * in which case all we can do is to print "( ? ) for duration and for the
+ * start timestamp.
+ */
+static size_t fprintf_duration(unsigned long t, bool calculated, FILE *fp)
 {
double duration = (double)t / NSEC_PER_MSEC;
size_t printed = fprintf(fp, "(");
 
-   if (duration >= 1.0)
+   if (!calculated)
+   printed += fprintf(fp, " ?   ");
+   else if (duration >= 1.0)
printed += color_fprintf(fp, PERF_COLOR_RED, "%6.3f ms", 
duration);
else if (duration >= 0.01)
printed += color_fprintf(fp, PERF_COLOR_YELLOW, "%6.3f ms", 
duration);
@@ -1028,13 +1037,27 @@ static bool trace__filter_duration(struct trace *trace, 
double t)
return t < (trace->duration_filter * NSEC_PER_MSEC);
 }
 
-static size_t trace__fprintf_tstamp(struct trace *trace, u64 tstamp, FILE *fp)
+static size_t __trace__fprintf_tstamp(struct trace *trace, u64 tstamp, FILE 
*fp)
 {
double ts = (double)(tstamp - trace->base_time) / NSEC_PER_MSEC;
 
return fprintf(fp, "%10.3f ", ts);
 }
 
+/*
+ * We're handling tstamp=0 as an undefined tstamp, i.e. like when we are
+ * using ttrace->entry_time for a thread that receives a sys_exit without
+ * first having received a sys_enter ("poll" issued before tracing session
+ * starts, lost sys_enter exit due to ring buffer overflow).
+ */
+static size_t trace__fprintf_tstamp(struct trace *trace, u64 tstamp, FILE *fp)
+{
+   if (tstamp > 0)
+   return __trace__fprintf_tstamp(trace, tstamp, fp);
+
+   return fprintf(fp, " ? ");
+}
+
 static bool done = false;
 static bool interrupted = false;
 
@@ -1045,10 +1068,10 @@ static void sig_handler(int sig)
 }
 
 static size_t trace__fprintf_entry_head(struct trace *trace, struct thread 
*thread,
-   u64 duration, u64 tstamp, FILE *fp)
+   u64 duration, bool duration_calculated, 
u64 tstamp, FILE *fp)
 {
size_t printed = trace__fprintf_tstamp(trace, tstamp, fp);
-   printed += fprintf_duration(duration, fp);
+   printed += fprintf_duration(duration, duration_calculated, fp);
 
if (trace->multiple_threads) {
if (trace->show_comm)
@@ -1450,7 +1473,7 @@ static int trace__printf_interrupted_entry(struct trace 
*trace, struct perf_samp
 
duration = sample->time - ttrace->entry_time;
 
-   printed  = trace__fprintf_entry_head(trace, trace->current, duration, 
ttrace->entry_time, trace->output);
+   printed  = trace__fprintf_entry_head(trace, trace->current, duration, 
true, ttrace->entry_time, trace->output);
printed += fprintf(trace->output, "%-70s) ...\n", ttrace->entry_str);
ttrace->entry_pending = false;
 
@@ -1497,7 +1520,7 @@ static int trace__sys_enter(struct trace *trace, struct 
perf_evsel *evsel,
 
if (sc->is_exit) {
if (!(trace->duration_filter || trace->summary_only || 
trace->min_stack)) {
-   trace__fprintf_entry_head(trace, thread, 1, 
ttrace->entry_time, 

[PATCH 5/9] perf trace: Handle unpaired raw_syscalls:sys_exit event

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo 

Which may happen when we start a tracing session and a thread is waiting
for something like "poll" to return, in which case we better print "?"
both for the syscall entry timestamp and for the duration.

E.g.:

Tracing existing mutt session:

  # perf trace -p `pidof mutt`
  ? ( ?   ): mutt/17135  ... [continued]: poll()) = 1
  0.027 ( 0.013 ms): mutt/17135 read(buf: 0x7ffcb3c42cef, count: 1) = 1
  0.047 ( 0.008 ms): mutt/17135 poll(ufds: 0x7ffcb3c42c50, nfds: 1, 
timeout_msecs: 1000) = 1
  0.059 ( 0.008 ms): mutt/17135 read(buf: 0x7ffcb3c42cef, count: 1) = 1
  

Before it would print a large number because we'd do:

  ttrace->entry_time - trace->base_time

And entry_time would be 0, while base_time would be the timestamp for
the first event 'perf trace' reads, oops.

Cc: Adrian Hunter 
Cc: David Ahern 
Cc: Jiri Olsa 
Cc: Luis Claudio Gonçalves 
Cc: Namhyung Kim 
Cc: Wang Nan 
Link: http://lkml.kernel.org/n/tip-wbcb93ofva2qdjd5ltn5e...@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/builtin-trace.c | 43 ++-
 1 file changed, 34 insertions(+), 9 deletions(-)

diff --git a/tools/perf/builtin-trace.c b/tools/perf/builtin-trace.c
index c88f9f215e6f..7379792a6504 100644
--- a/tools/perf/builtin-trace.c
+++ b/tools/perf/builtin-trace.c
@@ -821,12 +821,21 @@ struct syscall {
void**arg_parm;
 };
 
-static size_t fprintf_duration(unsigned long t, FILE *fp)
+/*
+ * We need to have this 'calculated' boolean because in some cases we really
+ * don't know what is the duration of a syscall, for instance, when we start
+ * a session and some threads are waiting for a syscall to finish, say 'poll',
+ * in which case all we can do is to print "( ? ) for duration and for the
+ * start timestamp.
+ */
+static size_t fprintf_duration(unsigned long t, bool calculated, FILE *fp)
 {
double duration = (double)t / NSEC_PER_MSEC;
size_t printed = fprintf(fp, "(");
 
-   if (duration >= 1.0)
+   if (!calculated)
+   printed += fprintf(fp, " ?   ");
+   else if (duration >= 1.0)
printed += color_fprintf(fp, PERF_COLOR_RED, "%6.3f ms", 
duration);
else if (duration >= 0.01)
printed += color_fprintf(fp, PERF_COLOR_YELLOW, "%6.3f ms", 
duration);
@@ -1028,13 +1037,27 @@ static bool trace__filter_duration(struct trace *trace, 
double t)
return t < (trace->duration_filter * NSEC_PER_MSEC);
 }
 
-static size_t trace__fprintf_tstamp(struct trace *trace, u64 tstamp, FILE *fp)
+static size_t __trace__fprintf_tstamp(struct trace *trace, u64 tstamp, FILE 
*fp)
 {
double ts = (double)(tstamp - trace->base_time) / NSEC_PER_MSEC;
 
return fprintf(fp, "%10.3f ", ts);
 }
 
+/*
+ * We're handling tstamp=0 as an undefined tstamp, i.e. like when we are
+ * using ttrace->entry_time for a thread that receives a sys_exit without
+ * first having received a sys_enter ("poll" issued before tracing session
+ * starts, lost sys_enter exit due to ring buffer overflow).
+ */
+static size_t trace__fprintf_tstamp(struct trace *trace, u64 tstamp, FILE *fp)
+{
+   if (tstamp > 0)
+   return __trace__fprintf_tstamp(trace, tstamp, fp);
+
+   return fprintf(fp, " ? ");
+}
+
 static bool done = false;
 static bool interrupted = false;
 
@@ -1045,10 +1068,10 @@ static void sig_handler(int sig)
 }
 
 static size_t trace__fprintf_entry_head(struct trace *trace, struct thread 
*thread,
-   u64 duration, u64 tstamp, FILE *fp)
+   u64 duration, bool duration_calculated, 
u64 tstamp, FILE *fp)
 {
size_t printed = trace__fprintf_tstamp(trace, tstamp, fp);
-   printed += fprintf_duration(duration, fp);
+   printed += fprintf_duration(duration, duration_calculated, fp);
 
if (trace->multiple_threads) {
if (trace->show_comm)
@@ -1450,7 +1473,7 @@ static int trace__printf_interrupted_entry(struct trace 
*trace, struct perf_samp
 
duration = sample->time - ttrace->entry_time;
 
-   printed  = trace__fprintf_entry_head(trace, trace->current, duration, 
ttrace->entry_time, trace->output);
+   printed  = trace__fprintf_entry_head(trace, trace->current, duration, 
true, ttrace->entry_time, trace->output);
printed += fprintf(trace->output, "%-70s) ...\n", ttrace->entry_str);
ttrace->entry_pending = false;
 
@@ -1497,7 +1520,7 @@ static int trace__sys_enter(struct trace *trace, struct 
perf_evsel *evsel,
 
if (sc->is_exit) {
if (!(trace->duration_filter || trace->summary_only || 
trace->min_stack)) {
-   trace__fprintf_entry_head(trace, thread, 1, 
ttrace->entry_time, trace->output);
+   trace__fprintf_entry_head(trace, thread, 0, false, 
ttrace->entry_time, trace->output);

[PATCH 8/9] perf tools: Do not fail in case of empty HOME env variable

[Arnaldo Carvalho de Melo]

From: Jiri Olsa 

Currently we fail in the following case:

  $ unset HOME
  $ ./perf record ls
  $ echo $?
  255

It's because the config code init fails due to a missing HOME variable
value. Fix this by skipping the user config init if there's no HOME
variable value.

Reported-by: Jan Stancek 
Signed-off-by: Jiri Olsa 
Cc: David Ahern 
Cc: Namhyung Kim 
Cc: Peter Zijlstra 
Link: http://lkml.kernel.org/r/20170330144637.7468-1-jo...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/util/config.c | 54 +++-
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 0c7d5a4975cd..7b01d59076d3 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -627,6 +627,8 @@ static int perf_config_set__init(struct perf_config_set 
*set)
 {
int ret = -1;
const char *home = NULL;
+   char *user_config;
+   struct stat st;
 
/* Setting $PERF_CONFIG makes perf read _only_ the given config file. */
if (config_exclusive_filename)
@@ -637,35 +639,41 @@ static int perf_config_set__init(struct perf_config_set 
*set)
}
 
home = getenv("HOME");
-   if (perf_config_global() && home) {
-   char *user_config = strdup(mkpath("%s/.perfconfig", home));
-   struct stat st;
 
-   if (user_config == NULL) {
-   warning("Not enough memory to process %s/.perfconfig, "
-   "ignoring it.", home);
-   goto out;
-   }
+   /*
+* Skip reading user config if:
+*   - there is no place to read it from (HOME)
+*   - we are asked not to (PERF_CONFIG_NOGLOBAL=1)
+*/
+   if (!home || !*home || !perf_config_global())
+   return 0;
 
-   if (stat(user_config, ) < 0) {
-   if (errno == ENOENT)
-   ret = 0;
-   goto out_free;
-   }
+   user_config = strdup(mkpath("%s/.perfconfig", home));
+   if (user_config == NULL) {
+   warning("Not enough memory to process %s/.perfconfig, "
+   "ignoring it.", home);
+   goto out;
+   }
+
+   if (stat(user_config, ) < 0) {
+   if (errno == ENOENT)
+   ret = 0;
+   goto out_free;
+   }
 
-   ret = 0;
+   ret = 0;
 
-   if (st.st_uid && (st.st_uid != geteuid())) {
-   warning("File %s not owned by current user or root, "
-   "ignoring it.", user_config);
-   goto out_free;
-   }
+   if (st.st_uid && (st.st_uid != geteuid())) {
+   warning("File %s not owned by current user or root, "
+   "ignoring it.", user_config);
+   goto out_free;
+   }
+
+   if (st.st_size)
+   ret = perf_config_from_file(collect_config, user_config, set);
 
-   if (st.st_size)
-   ret = perf_config_from_file(collect_config, 
user_config, set);
 out_free:
-   free(user_config);
-   }
+   free(user_config);
 out:
return ret;
 }
-- 
2.9.3

[PATCH 7/9] tools include uapi: Grab copies of stat.h and fcntl.h

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo 

We will need it to build tools/perf/trace/beauty/statx.h.

Cc: Adrian Hunter 
Cc: David Ahern 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Wang Nan 
Link: http://lkml.kernel.org/n/tip-nin41ve2fa63lrfbdr6x5...@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/include/linux/types.h  |   1 +
 tools/include/uapi/linux/fcntl.h |  72 
 tools/include/uapi/linux/stat.h  | 176 +++
 tools/perf/MANIFEST  |   2 +
 tools/perf/check-headers.sh  |   2 +
 5 files changed, 253 insertions(+)
 create mode 100644 tools/include/uapi/linux/fcntl.h
 create mode 100644 tools/include/uapi/linux/stat.h

diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h
index c24b3e3ae296..77a28a26a670 100644
--- a/tools/include/linux/types.h
+++ b/tools/include/linux/types.h
@@ -7,6 +7,7 @@
 
 #define __SANE_USERSPACE_TYPES__   /* For PPC64, to get LL64 types */
 #include 
+#include 
 
 struct page;
 struct kmem_cache;
diff --git a/tools/include/uapi/linux/fcntl.h b/tools/include/uapi/linux/fcntl.h
new file mode 100644
index ..813afd6eee71
--- /dev/null
+++ b/tools/include/uapi/linux/fcntl.h
@@ -0,0 +1,72 @@
+#ifndef _UAPI_LINUX_FCNTL_H
+#define _UAPI_LINUX_FCNTL_H
+
+#include 
+
+#define F_SETLEASE (F_LINUX_SPECIFIC_BASE + 0)
+#define F_GETLEASE (F_LINUX_SPECIFIC_BASE + 1)
+
+/*
+ * Cancel a blocking posix lock; internal use only until we expose an
+ * asynchronous lock api to userspace:
+ */
+#define F_CANCELLK (F_LINUX_SPECIFIC_BASE + 5)
+
+/* Create a file descriptor with FD_CLOEXEC set. */
+#define F_DUPFD_CLOEXEC(F_LINUX_SPECIFIC_BASE + 6)
+
+/*
+ * Request nofications on a directory.
+ * See below for events that may be notified.
+ */
+#define F_NOTIFY   (F_LINUX_SPECIFIC_BASE+2)
+
+/*
+ * Set and get of pipe page size array
+ */
+#define F_SETPIPE_SZ   (F_LINUX_SPECIFIC_BASE + 7)
+#define F_GETPIPE_SZ   (F_LINUX_SPECIFIC_BASE + 8)
+
+/*
+ * Set/Get seals
+ */
+#define F_ADD_SEALS(F_LINUX_SPECIFIC_BASE + 9)
+#define F_GET_SEALS(F_LINUX_SPECIFIC_BASE + 10)
+
+/*
+ * Types of seals
+ */
+#define F_SEAL_SEAL0x0001  /* prevent further seals from being set */
+#define F_SEAL_SHRINK  0x0002  /* prevent file from shrinking */
+#define F_SEAL_GROW0x0004  /* prevent file from growing */
+#define F_SEAL_WRITE   0x0008  /* prevent writes */
+/* (1U << 31) is reserved for signed error codes */
+
+/*
+ * Types of directory notifications that may be requested.
+ */
+#define DN_ACCESS  0x0001  /* File accessed */
+#define DN_MODIFY  0x0002  /* File modified */
+#define DN_CREATE  0x0004  /* File created */
+#define DN_DELETE  0x0008  /* File removed */
+#define DN_RENAME  0x0010  /* File renamed */
+#define DN_ATTRIB  0x0020  /* File changed attibutes */
+#define DN_MULTISHOT   0x8000  /* Don't remove notifier */
+
+#define AT_FDCWD   -100/* Special value used to indicate
+   openat should use the current
+   working directory. */
+#define AT_SYMLINK_NOFOLLOW0x100   /* Do not follow symbolic links.  */
+#define AT_REMOVEDIR   0x200   /* Remove directory instead of
+   unlinking file.  */
+#define AT_SYMLINK_FOLLOW  0x400   /* Follow symbolic links.  */
+#define AT_NO_AUTOMOUNT0x800   /* Suppress terminal automount 
traversal */
+#define AT_EMPTY_PATH  0x1000  /* Allow empty relative pathname */
+
+#define AT_STATX_SYNC_TYPE 0x6000  /* Type of synchronisation required 
from statx() */
+#define AT_STATX_SYNC_AS_STAT  0x  /* - Do whatever stat() does */
+#define AT_STATX_FORCE_SYNC0x2000  /* - Force the attributes to be sync'd 
with the server */
+#define AT_STATX_DONT_SYNC 0x4000  /* - Don't sync attributes with the 
server */
+
+
+#endif /* _UAPI_LINUX_FCNTL_H */
diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h
new file mode 100644
index ..51a6b86e3700
--- /dev/null
+++ b/tools/include/uapi/linux/stat.h
@@ -0,0 +1,176 @@
+#ifndef _UAPI_LINUX_STAT_H
+#define _UAPI_LINUX_STAT_H
+
+#include 
+
+#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
+
+#define S_IFMT  0017
+#define S_IFSOCK 014
+#define S_IFLNK 012
+#define S_IFREG  010
+#define S_IFBLK  006
+#define S_IFDIR  004
+#define S_IFCHR  002
+#define S_IFIFO  001
+#define S_ISUID  0004000
+#define S_ISGID  0002000
+#define S_ISVTX  0001000
+
+#define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
+#define S_ISREG(m) (((m) & S_IFMT) == S_IFREG)
+#define S_ISDIR(m) (((m) & S_IFMT) == S_IFDIR)
+#define S_ISCHR(m) 

[PATCH 8/9] perf tools: Do not fail in case of empty HOME env variable

[Arnaldo Carvalho de Melo]

From: Jiri Olsa 

Currently we fail in the following case:

  $ unset HOME
  $ ./perf record ls
  $ echo $?
  255

It's because the config code init fails due to a missing HOME variable
value. Fix this by skipping the user config init if there's no HOME
variable value.

Reported-by: Jan Stancek 
Signed-off-by: Jiri Olsa 
Cc: David Ahern 
Cc: Namhyung Kim 
Cc: Peter Zijlstra 
Link: http://lkml.kernel.org/r/20170330144637.7468-1-jo...@kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/util/config.c | 54 +++-
 1 file changed, 31 insertions(+), 23 deletions(-)

diff --git a/tools/perf/util/config.c b/tools/perf/util/config.c
index 0c7d5a4975cd..7b01d59076d3 100644
--- a/tools/perf/util/config.c
+++ b/tools/perf/util/config.c
@@ -627,6 +627,8 @@ static int perf_config_set__init(struct perf_config_set 
*set)
 {
int ret = -1;
const char *home = NULL;
+   char *user_config;
+   struct stat st;
 
/* Setting $PERF_CONFIG makes perf read _only_ the given config file. */
if (config_exclusive_filename)
@@ -637,35 +639,41 @@ static int perf_config_set__init(struct perf_config_set 
*set)
}
 
home = getenv("HOME");
-   if (perf_config_global() && home) {
-   char *user_config = strdup(mkpath("%s/.perfconfig", home));
-   struct stat st;
 
-   if (user_config == NULL) {
-   warning("Not enough memory to process %s/.perfconfig, "
-   "ignoring it.", home);
-   goto out;
-   }
+   /*
+* Skip reading user config if:
+*   - there is no place to read it from (HOME)
+*   - we are asked not to (PERF_CONFIG_NOGLOBAL=1)
+*/
+   if (!home || !*home || !perf_config_global())
+   return 0;
 
-   if (stat(user_config, ) < 0) {
-   if (errno == ENOENT)
-   ret = 0;
-   goto out_free;
-   }
+   user_config = strdup(mkpath("%s/.perfconfig", home));
+   if (user_config == NULL) {
+   warning("Not enough memory to process %s/.perfconfig, "
+   "ignoring it.", home);
+   goto out;
+   }
+
+   if (stat(user_config, ) < 0) {
+   if (errno == ENOENT)
+   ret = 0;
+   goto out_free;
+   }
 
-   ret = 0;
+   ret = 0;
 
-   if (st.st_uid && (st.st_uid != geteuid())) {
-   warning("File %s not owned by current user or root, "
-   "ignoring it.", user_config);
-   goto out_free;
-   }
+   if (st.st_uid && (st.st_uid != geteuid())) {
+   warning("File %s not owned by current user or root, "
+   "ignoring it.", user_config);
+   goto out_free;
+   }
+
+   if (st.st_size)
+   ret = perf_config_from_file(collect_config, user_config, set);
 
-   if (st.st_size)
-   ret = perf_config_from_file(collect_config, 
user_config, set);
 out_free:
-   free(user_config);
-   }
+   free(user_config);
 out:
return ret;
 }
-- 
2.9.3

[PATCH 7/9] tools include uapi: Grab copies of stat.h and fcntl.h

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo 

We will need it to build tools/perf/trace/beauty/statx.h.

Cc: Adrian Hunter 
Cc: David Ahern 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Wang Nan 
Link: http://lkml.kernel.org/n/tip-nin41ve2fa63lrfbdr6x5...@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/include/linux/types.h  |   1 +
 tools/include/uapi/linux/fcntl.h |  72 
 tools/include/uapi/linux/stat.h  | 176 +++
 tools/perf/MANIFEST  |   2 +
 tools/perf/check-headers.sh  |   2 +
 5 files changed, 253 insertions(+)
 create mode 100644 tools/include/uapi/linux/fcntl.h
 create mode 100644 tools/include/uapi/linux/stat.h

diff --git a/tools/include/linux/types.h b/tools/include/linux/types.h
index c24b3e3ae296..77a28a26a670 100644
--- a/tools/include/linux/types.h
+++ b/tools/include/linux/types.h
@@ -7,6 +7,7 @@
 
 #define __SANE_USERSPACE_TYPES__   /* For PPC64, to get LL64 types */
 #include 
+#include 
 
 struct page;
 struct kmem_cache;
diff --git a/tools/include/uapi/linux/fcntl.h b/tools/include/uapi/linux/fcntl.h
new file mode 100644
index ..813afd6eee71
--- /dev/null
+++ b/tools/include/uapi/linux/fcntl.h
@@ -0,0 +1,72 @@
+#ifndef _UAPI_LINUX_FCNTL_H
+#define _UAPI_LINUX_FCNTL_H
+
+#include 
+
+#define F_SETLEASE (F_LINUX_SPECIFIC_BASE + 0)
+#define F_GETLEASE (F_LINUX_SPECIFIC_BASE + 1)
+
+/*
+ * Cancel a blocking posix lock; internal use only until we expose an
+ * asynchronous lock api to userspace:
+ */
+#define F_CANCELLK (F_LINUX_SPECIFIC_BASE + 5)
+
+/* Create a file descriptor with FD_CLOEXEC set. */
+#define F_DUPFD_CLOEXEC(F_LINUX_SPECIFIC_BASE + 6)
+
+/*
+ * Request nofications on a directory.
+ * See below for events that may be notified.
+ */
+#define F_NOTIFY   (F_LINUX_SPECIFIC_BASE+2)
+
+/*
+ * Set and get of pipe page size array
+ */
+#define F_SETPIPE_SZ   (F_LINUX_SPECIFIC_BASE + 7)
+#define F_GETPIPE_SZ   (F_LINUX_SPECIFIC_BASE + 8)
+
+/*
+ * Set/Get seals
+ */
+#define F_ADD_SEALS(F_LINUX_SPECIFIC_BASE + 9)
+#define F_GET_SEALS(F_LINUX_SPECIFIC_BASE + 10)
+
+/*
+ * Types of seals
+ */
+#define F_SEAL_SEAL0x0001  /* prevent further seals from being set */
+#define F_SEAL_SHRINK  0x0002  /* prevent file from shrinking */
+#define F_SEAL_GROW0x0004  /* prevent file from growing */
+#define F_SEAL_WRITE   0x0008  /* prevent writes */
+/* (1U << 31) is reserved for signed error codes */
+
+/*
+ * Types of directory notifications that may be requested.
+ */
+#define DN_ACCESS  0x0001  /* File accessed */
+#define DN_MODIFY  0x0002  /* File modified */
+#define DN_CREATE  0x0004  /* File created */
+#define DN_DELETE  0x0008  /* File removed */
+#define DN_RENAME  0x0010  /* File renamed */
+#define DN_ATTRIB  0x0020  /* File changed attibutes */
+#define DN_MULTISHOT   0x8000  /* Don't remove notifier */
+
+#define AT_FDCWD   -100/* Special value used to indicate
+   openat should use the current
+   working directory. */
+#define AT_SYMLINK_NOFOLLOW0x100   /* Do not follow symbolic links.  */
+#define AT_REMOVEDIR   0x200   /* Remove directory instead of
+   unlinking file.  */
+#define AT_SYMLINK_FOLLOW  0x400   /* Follow symbolic links.  */
+#define AT_NO_AUTOMOUNT0x800   /* Suppress terminal automount 
traversal */
+#define AT_EMPTY_PATH  0x1000  /* Allow empty relative pathname */
+
+#define AT_STATX_SYNC_TYPE 0x6000  /* Type of synchronisation required 
from statx() */
+#define AT_STATX_SYNC_AS_STAT  0x  /* - Do whatever stat() does */
+#define AT_STATX_FORCE_SYNC0x2000  /* - Force the attributes to be sync'd 
with the server */
+#define AT_STATX_DONT_SYNC 0x4000  /* - Don't sync attributes with the 
server */
+
+
+#endif /* _UAPI_LINUX_FCNTL_H */
diff --git a/tools/include/uapi/linux/stat.h b/tools/include/uapi/linux/stat.h
new file mode 100644
index ..51a6b86e3700
--- /dev/null
+++ b/tools/include/uapi/linux/stat.h
@@ -0,0 +1,176 @@
+#ifndef _UAPI_LINUX_STAT_H
+#define _UAPI_LINUX_STAT_H
+
+#include 
+
+#if defined(__KERNEL__) || !defined(__GLIBC__) || (__GLIBC__ < 2)
+
+#define S_IFMT  0017
+#define S_IFSOCK 014
+#define S_IFLNK 012
+#define S_IFREG  010
+#define S_IFBLK  006
+#define S_IFDIR  004
+#define S_IFCHR  002
+#define S_IFIFO  001
+#define S_ISUID  0004000
+#define S_ISGID  0002000
+#define S_ISVTX  0001000
+
+#define S_ISLNK(m) (((m) & S_IFMT) == S_IFLNK)
+#define S_ISREG(m) (((m) & S_IFMT) == S_IFREG)
+#define S_ISDIR(m) (((m) & S_IFMT) == S_IFDIR)
+#define S_ISCHR(m) (((m) & S_IFMT) == S_IFCHR)
+#define S_ISBLK(m) (((m) & S_IFMT) == S_IFBLK)
+#define S_ISFIFO(m)(((m) & S_IFMT) == S_IFIFO)

[GIT PULL 0/9] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

Hi Ingo,

Please consider pulling,

- Arnaldo

Test results at the end of this message, as usual.

The following changes since commit 3906a13a6b4e78fbc0def03a808f091f0dff1b44:

  Merge tag 'perf-core-for-mingo-4.12-20170327' of 
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core 
(2017-03-28 07:44:43 +0200)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git 
tags/perf-core-for-mingo-4.12-20170331

for you to fetch changes up to fd5cead23f54697310bd565aa2a23ae5128080a0:

  perf trace: Beautify statx syscall 'flag' and 'mask' arguments (2017-03-31 
14:42:31 -0300)


perf/core improvements and fixes:

New features:

- Beautify the statx syscall arguments in 'perf trace' (Arnaldo Carvalho de 
Melo)

e.g.:

  System wide strace like session:

  # trace -e statx
   16612.967 ( 0.028 ms): statx/4562 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffef195d660) = 0
   36050.891 ( 0.007 ms): statx/4576 statx(dfd: CWD, filename: /etc/passwd, 
flags: SYMLINK_NOFOLLOW|STATX_DONT_SYNC, mask: BTIME, buffer: 0x7ffda9bf50f0) = 0
  ^C#

User visible:

- Handle unpaired raw_syscalls:sys_exit events in 'perf trace', i.e. we
  shouldn't try to calculate duration or print the timestamp for a missing
  matching raw_syscalls:sys_enter (Arnaldo Carvalho de Melo)

- Do not print "cycles: 0" in perf report LBR lines in platforms not
  supporting 'cycles', such as Intel's Broadwell (Jin Yao)

- Handle missing $HOME env var (Jiri Olsa)

- Map 8-bit registers (al, bl, etc), not supported in uprobes_events, to
  the next best thing (ax, bx, etc) supported (Ravi Bangoria)

Signed-off-by: Arnaldo Carvalho de Melo <a...@redhat.com>


Arnaldo Carvalho de Melo (4):
  perf tools: Remove support for command aliases
  perf trace: Handle unpaired raw_syscalls:sys_exit event
  tools include uapi: Grab copies of stat.h and fcntl.h
  perf trace: Beautify statx syscall 'flag' and 'mask' arguments

Colin Ian King (1):
  perf utils: Fix spelling mistake: "Invalud" -> "Invalid"

Jin Yao (1):
  perf report: Drop cycles 0 for LBR print

Jiri Olsa (1):
  perf tools: Do not fail in case of empty HOME env variable

Ravi Bangoria (2):
  perf/sdt/x86: Add renaming logic for (missing) 8 bit registers
  perf/sdt/x86: Move OP parser to tools/perf/arch/x86/

 tools/include/linux/types.h   |   1 +
 tools/include/uapi/linux/fcntl.h  |  72 +
 tools/include/uapi/linux/stat.h   | 176 
 tools/perf/Build  |   1 +
 tools/perf/MANIFEST   |   2 +
 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl |   1 +
 tools/perf/arch/x86/util/perf_regs.c  | 187 ++
 tools/perf/builtin-help.c |  13 --
 tools/perf/builtin-trace.c|  57 ---
 tools/perf/check-headers.sh   |   2 +
 tools/perf/perf.c |  97 +--
 tools/perf/trace/beauty/Build |   1 +
 tools/perf/trace/beauty/beauty.h  |  24 +++
 tools/perf/trace/beauty/statx.c   |  72 +
 tools/perf/util/Build |   1 -
 tools/perf/util/alias.c   |  78 -
 tools/perf/util/cache.h   |   1 -
 tools/perf/util/callchain.c   | 111 -
 tools/perf/util/config.c  |  54 ---
 tools/perf/util/help-unknown-cmd.c|   8 +-
 tools/perf/util/hist.c|   2 +-
 tools/perf/util/perf_regs.c   |   6 +-
 tools/perf/util/perf_regs.h   |  11 +-
 tools/perf/util/probe-file.c  | 132 +--
 24 files changed, 707 insertions(+), 403 deletions(-)
 create mode 100644 tools/include/uapi/linux/fcntl.h
 create mode 100644 tools/include/uapi/linux/stat.h
 create mode 100644 tools/perf/trace/beauty/Build
 create mode 100644 tools/perf/trace/beauty/beauty.h
 create mode 100644 tools/perf/trace/beauty/statx.c
 delete mode 100644 tools/perf/util/alias.c

Test results:

The first ones are container (docker) based builds of tools/perf with and
without libelf support, objtool where it is supported and samples/bpf/, ditto.
Where clang is available, it is also used to build perf with/without libelf.

For this specific pull request the samples/bpf/ was disabled, as 'make 
headers_install'
is failing with the following error, in this case in fedora:rawhide:

INSTALL usr/include/uapi/ (0 file)
  /git/linux/scripts/Makefil

[PATCH 6/9] perf utils: Fix spelling mistake: "Invalud" -> "Invalid"

[Arnaldo Carvalho de Melo]

From: Colin Ian King 

Trivial fix to spelling mistake in pr_debug message.

Signed-off-by: Colin King 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Krister Johansen 
Cc: Peter Zijlstra 
Cc: kernel-janit...@vger.kernel.org
Link: http://lkml.kernel.org/r/20170330095440.19444-1-colin.k...@canonical.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/util/hist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 3c4d4d00cb2c..61bf304206fd 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2459,7 +2459,7 @@ int parse_filter_percentage(const struct option *opt 
__maybe_unused,
else if (!strcmp(arg, "absolute"))
symbol_conf.filter_relative = false;
else {
-   pr_debug("Invalud percentage: %s\n", arg);
+   pr_debug("Invalid percentage: %s\n", arg);
return -1;
}
 
-- 
2.9.3

[GIT PULL 0/9] perf/core improvements and fixes

[Arnaldo Carvalho de Melo]

Hi Ingo,

Please consider pulling,

- Arnaldo

Test results at the end of this message, as usual.

The following changes since commit 3906a13a6b4e78fbc0def03a808f091f0dff1b44:

  Merge tag 'perf-core-for-mingo-4.12-20170327' of 
git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux into perf/core 
(2017-03-28 07:44:43 +0200)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/acme/linux.git 
tags/perf-core-for-mingo-4.12-20170331

for you to fetch changes up to fd5cead23f54697310bd565aa2a23ae5128080a0:

  perf trace: Beautify statx syscall 'flag' and 'mask' arguments (2017-03-31 
14:42:31 -0300)


perf/core improvements and fixes:

New features:

- Beautify the statx syscall arguments in 'perf trace' (Arnaldo Carvalho de 
Melo)

e.g.:

  System wide strace like session:

  # trace -e statx
   16612.967 ( 0.028 ms): statx/4562 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffef195d660) = 0
   36050.891 ( 0.007 ms): statx/4576 statx(dfd: CWD, filename: /etc/passwd, 
flags: SYMLINK_NOFOLLOW|STATX_DONT_SYNC, mask: BTIME, buffer: 0x7ffda9bf50f0) = 0
  ^C#

User visible:

- Handle unpaired raw_syscalls:sys_exit events in 'perf trace', i.e. we
  shouldn't try to calculate duration or print the timestamp for a missing
  matching raw_syscalls:sys_enter (Arnaldo Carvalho de Melo)

- Do not print "cycles: 0" in perf report LBR lines in platforms not
  supporting 'cycles', such as Intel's Broadwell (Jin Yao)

- Handle missing $HOME env var (Jiri Olsa)

- Map 8-bit registers (al, bl, etc), not supported in uprobes_events, to
  the next best thing (ax, bx, etc) supported (Ravi Bangoria)

Signed-off-by: Arnaldo Carvalho de Melo 


Arnaldo Carvalho de Melo (4):
  perf tools: Remove support for command aliases
  perf trace: Handle unpaired raw_syscalls:sys_exit event
  tools include uapi: Grab copies of stat.h and fcntl.h
  perf trace: Beautify statx syscall 'flag' and 'mask' arguments

Colin Ian King (1):
  perf utils: Fix spelling mistake: "Invalud" -> "Invalid"

Jin Yao (1):
  perf report: Drop cycles 0 for LBR print

Jiri Olsa (1):
  perf tools: Do not fail in case of empty HOME env variable

Ravi Bangoria (2):
  perf/sdt/x86: Add renaming logic for (missing) 8 bit registers
  perf/sdt/x86: Move OP parser to tools/perf/arch/x86/

 tools/include/linux/types.h   |   1 +
 tools/include/uapi/linux/fcntl.h  |  72 +
 tools/include/uapi/linux/stat.h   | 176 
 tools/perf/Build  |   1 +
 tools/perf/MANIFEST   |   2 +
 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl |   1 +
 tools/perf/arch/x86/util/perf_regs.c  | 187 ++
 tools/perf/builtin-help.c |  13 --
 tools/perf/builtin-trace.c|  57 ---
 tools/perf/check-headers.sh   |   2 +
 tools/perf/perf.c |  97 +--
 tools/perf/trace/beauty/Build |   1 +
 tools/perf/trace/beauty/beauty.h  |  24 +++
 tools/perf/trace/beauty/statx.c   |  72 +
 tools/perf/util/Build |   1 -
 tools/perf/util/alias.c   |  78 -
 tools/perf/util/cache.h   |   1 -
 tools/perf/util/callchain.c   | 111 -
 tools/perf/util/config.c  |  54 ---
 tools/perf/util/help-unknown-cmd.c|   8 +-
 tools/perf/util/hist.c|   2 +-
 tools/perf/util/perf_regs.c   |   6 +-
 tools/perf/util/perf_regs.h   |  11 +-
 tools/perf/util/probe-file.c  | 132 +--
 24 files changed, 707 insertions(+), 403 deletions(-)
 create mode 100644 tools/include/uapi/linux/fcntl.h
 create mode 100644 tools/include/uapi/linux/stat.h
 create mode 100644 tools/perf/trace/beauty/Build
 create mode 100644 tools/perf/trace/beauty/beauty.h
 create mode 100644 tools/perf/trace/beauty/statx.c
 delete mode 100644 tools/perf/util/alias.c

Test results:

The first ones are container (docker) based builds of tools/perf with and
without libelf support, objtool where it is supported and samples/bpf/, ditto.
Where clang is available, it is also used to build perf with/without libelf.

For this specific pull request the samples/bpf/ was disabled, as 'make 
headers_install'
is failing with the following error, in this case in fedora:rawhide:

INSTALL usr/include/uapi/ (0 file)
  /git/linux/scripts/Makefile.headersinst:62

[PATCH 6/9] perf utils: Fix spelling mistake: "Invalud" -> "Invalid"

[Arnaldo Carvalho de Melo]

From: Colin Ian King 

Trivial fix to spelling mistake in pr_debug message.

Signed-off-by: Colin King 
Cc: Alexander Shishkin 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Krister Johansen 
Cc: Peter Zijlstra 
Cc: kernel-janit...@vger.kernel.org
Link: http://lkml.kernel.org/r/20170330095440.19444-1-colin.k...@canonical.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/util/hist.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tools/perf/util/hist.c b/tools/perf/util/hist.c
index 3c4d4d00cb2c..61bf304206fd 100644
--- a/tools/perf/util/hist.c
+++ b/tools/perf/util/hist.c
@@ -2459,7 +2459,7 @@ int parse_filter_percentage(const struct option *opt 
__maybe_unused,
else if (!strcmp(arg, "absolute"))
symbol_conf.filter_relative = false;
else {
-   pr_debug("Invalud percentage: %s\n", arg);
+   pr_debug("Invalid percentage: %s\n", arg);
return -1;
}
 
-- 
2.9.3

[PATCH 3/9] perf/sdt/x86: Move OP parser to tools/perf/arch/x86/

[Arnaldo Carvalho de Melo]

From: Ravi Bangoria 

SDT marker argument is in N@OP format. N is the size of argument and OP
is the actual assembly operand. OP is arch dependent component and hence
it's parsing logic also should be placed under tools/perf/arch/.

Signed-off-by: Ravi Bangoria 
Acked-by: Masami Hiramatsu 
Cc: Alexander Shishkin 
Cc: Alexis Berlemont 
Cc: Hemant Kumar 
Cc: Michael Ellerman 
Cc: Naveen N. Rao 
Cc: Peter Zijlstra 
Link: 
http://lkml.kernel.org/r/20170328094754.3156-3-ravi.bango...@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/arch/x86/util/perf_regs.c | 179 ---
 tools/perf/util/perf_regs.c  |   6 +-
 tools/perf/util/perf_regs.h  |  11 ++-
 tools/perf/util/probe-file.c | 132 --
 4 files changed, 194 insertions(+), 134 deletions(-)

diff --git a/tools/perf/arch/x86/util/perf_regs.c 
b/tools/perf/arch/x86/util/perf_regs.c
index fa1fd196837d..3bf3548c5e2d 100644
--- a/tools/perf/arch/x86/util/perf_regs.c
+++ b/tools/perf/arch/x86/util/perf_regs.c
@@ -1,8 +1,10 @@
 #include 
+#include 
 
 #include "../../perf.h"
 #include "../../util/util.h"
 #include "../../util/perf_regs.h"
+#include "../../util/debug.h"
 
 const struct sample_reg sample_reg_masks[] = {
SMPL_REG(AX, PERF_REG_X86_AX),
@@ -37,7 +39,7 @@ struct sdt_name_reg {
 #define SDT_NAME_REG(n, m) {.sdt_name = "%" #n, .uprobe_name = "%" #m}
 #define SDT_NAME_REG_END {.sdt_name = NULL, .uprobe_name = NULL}
 
-static const struct sdt_name_reg sdt_reg_renamings[] = {
+static const struct sdt_name_reg sdt_reg_tbl[] = {
SDT_NAME_REG(eax, ax),
SDT_NAME_REG(rax, ax),
SDT_NAME_REG(al,  ax),
@@ -95,45 +97,158 @@ static const struct sdt_name_reg sdt_reg_renamings[] = {
SDT_NAME_REG_END,
 };
 
-int sdt_rename_register(char **pdesc, char *old_name)
+/*
+ * Perf only supports OP which is in  +/-NUM(REG)  form.
+ * Here plus-minus sign, NUM and parenthesis are optional,
+ * only REG is mandatory.
+ *
+ * SDT events also supports indirect addressing mode with a
+ * symbol as offset, scaled mode and constants in OP. But
+ * perf does not support them yet. Below are few examples.
+ *
+ * OP with scaled mode:
+ * (%rax,%rsi,8)
+ * 10(%ras,%rsi,8)
+ *
+ * OP with indirect addressing mode:
+ * check_action(%rip)
+ * mp_+52(%rip)
+ * 44+mp_(%rip)
+ *
+ * OP with constant values:
+ * $0
+ * $123
+ * $-1
+ */
+#define SDT_OP_REGEX  "^([+\\-]?)([0-9]*)(\\(?)(%[a-z][a-z0-9]+)(\\)?)$"
+
+static regex_t sdt_op_regex;
+
+static int sdt_init_op_regex(void)
 {
-   const struct sdt_name_reg *rnames = sdt_reg_renamings;
-   char *new_desc, *old_desc = *pdesc;
-   size_t prefix_len, sdt_len, uprobe_len, old_desc_len, offset;
-   int ret = -1;
-
-   while (ret != 0 && rnames->sdt_name != NULL) {
-   sdt_len = strlen(rnames->sdt_name);
-   ret = strncmp(old_name, rnames->sdt_name, sdt_len);
-   rnames += !!ret;
-   }
+   static int initialized;
+   int ret = 0;
 
-   if (rnames->sdt_name == NULL)
+   if (initialized)
return 0;
 
-   sdt_len = strlen(rnames->sdt_name);
-   uprobe_len = strlen(rnames->uprobe_name);
-   old_desc_len = strlen(old_desc) + 1;
+   ret = regcomp(_op_regex, SDT_OP_REGEX, REG_EXTENDED);
+   if (ret < 0) {
+   pr_debug4("Regex compilation error.\n");
+   return ret;
+   }
 
-   new_desc = zalloc(old_desc_len + uprobe_len - sdt_len);
-   if (new_desc == NULL)
-   return -1;
+   initialized = 1;
+   return 0;
+}
 
-   /* Copy the chars before the register name (at least '%') */
-   prefix_len = old_name - old_desc;
-   memcpy(new_desc, old_desc, prefix_len);
+/*
+ * Max x86 register name length is 5(ex: %r15d). So, 6th char
+ * should always contain NULL. This helps to find register name
+ * length using strlen, insted of maintaing one more variable.
+ */
+#define SDT_REG_NAME_SIZE  6
 
-   /* Copy the new register name */
-   memcpy(new_desc + prefix_len, rnames->uprobe_name, uprobe_len);
+/*
+ * The uprobe parser does not support all gas register names;
+ * so, we have to replace them (ex. for x86_64: %rax -> %ax).
+ * Note: If register does not require renaming, just copy
+ * paste as it is, but don't leave it empty.
+ */
+static void sdt_rename_register(char *sdt_reg, int sdt_len, char *uprobe_reg)
+{
+   int i = 0;
 
-   /* Copy the chars after the register name (if need be) */
-   offset = prefix_len + sdt_len;
-   if (offset < old_desc_len)
-   memcpy(new_desc + prefix_len + uprobe_len,
-   

[PATCH 3/9] perf/sdt/x86: Move OP parser to tools/perf/arch/x86/

[Arnaldo Carvalho de Melo]

From: Ravi Bangoria 

SDT marker argument is in N@OP format. N is the size of argument and OP
is the actual assembly operand. OP is arch dependent component and hence
it's parsing logic also should be placed under tools/perf/arch/.

Signed-off-by: Ravi Bangoria 
Acked-by: Masami Hiramatsu 
Cc: Alexander Shishkin 
Cc: Alexis Berlemont 
Cc: Hemant Kumar 
Cc: Michael Ellerman 
Cc: Naveen N. Rao 
Cc: Peter Zijlstra 
Link: 
http://lkml.kernel.org/r/20170328094754.3156-3-ravi.bango...@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/arch/x86/util/perf_regs.c | 179 ---
 tools/perf/util/perf_regs.c  |   6 +-
 tools/perf/util/perf_regs.h  |  11 ++-
 tools/perf/util/probe-file.c | 132 --
 4 files changed, 194 insertions(+), 134 deletions(-)

diff --git a/tools/perf/arch/x86/util/perf_regs.c 
b/tools/perf/arch/x86/util/perf_regs.c
index fa1fd196837d..3bf3548c5e2d 100644
--- a/tools/perf/arch/x86/util/perf_regs.c
+++ b/tools/perf/arch/x86/util/perf_regs.c
@@ -1,8 +1,10 @@
 #include 
+#include 
 
 #include "../../perf.h"
 #include "../../util/util.h"
 #include "../../util/perf_regs.h"
+#include "../../util/debug.h"
 
 const struct sample_reg sample_reg_masks[] = {
SMPL_REG(AX, PERF_REG_X86_AX),
@@ -37,7 +39,7 @@ struct sdt_name_reg {
 #define SDT_NAME_REG(n, m) {.sdt_name = "%" #n, .uprobe_name = "%" #m}
 #define SDT_NAME_REG_END {.sdt_name = NULL, .uprobe_name = NULL}
 
-static const struct sdt_name_reg sdt_reg_renamings[] = {
+static const struct sdt_name_reg sdt_reg_tbl[] = {
SDT_NAME_REG(eax, ax),
SDT_NAME_REG(rax, ax),
SDT_NAME_REG(al,  ax),
@@ -95,45 +97,158 @@ static const struct sdt_name_reg sdt_reg_renamings[] = {
SDT_NAME_REG_END,
 };
 
-int sdt_rename_register(char **pdesc, char *old_name)
+/*
+ * Perf only supports OP which is in  +/-NUM(REG)  form.
+ * Here plus-minus sign, NUM and parenthesis are optional,
+ * only REG is mandatory.
+ *
+ * SDT events also supports indirect addressing mode with a
+ * symbol as offset, scaled mode and constants in OP. But
+ * perf does not support them yet. Below are few examples.
+ *
+ * OP with scaled mode:
+ * (%rax,%rsi,8)
+ * 10(%ras,%rsi,8)
+ *
+ * OP with indirect addressing mode:
+ * check_action(%rip)
+ * mp_+52(%rip)
+ * 44+mp_(%rip)
+ *
+ * OP with constant values:
+ * $0
+ * $123
+ * $-1
+ */
+#define SDT_OP_REGEX  "^([+\\-]?)([0-9]*)(\\(?)(%[a-z][a-z0-9]+)(\\)?)$"
+
+static regex_t sdt_op_regex;
+
+static int sdt_init_op_regex(void)
 {
-   const struct sdt_name_reg *rnames = sdt_reg_renamings;
-   char *new_desc, *old_desc = *pdesc;
-   size_t prefix_len, sdt_len, uprobe_len, old_desc_len, offset;
-   int ret = -1;
-
-   while (ret != 0 && rnames->sdt_name != NULL) {
-   sdt_len = strlen(rnames->sdt_name);
-   ret = strncmp(old_name, rnames->sdt_name, sdt_len);
-   rnames += !!ret;
-   }
+   static int initialized;
+   int ret = 0;
 
-   if (rnames->sdt_name == NULL)
+   if (initialized)
return 0;
 
-   sdt_len = strlen(rnames->sdt_name);
-   uprobe_len = strlen(rnames->uprobe_name);
-   old_desc_len = strlen(old_desc) + 1;
+   ret = regcomp(_op_regex, SDT_OP_REGEX, REG_EXTENDED);
+   if (ret < 0) {
+   pr_debug4("Regex compilation error.\n");
+   return ret;
+   }
 
-   new_desc = zalloc(old_desc_len + uprobe_len - sdt_len);
-   if (new_desc == NULL)
-   return -1;
+   initialized = 1;
+   return 0;
+}
 
-   /* Copy the chars before the register name (at least '%') */
-   prefix_len = old_name - old_desc;
-   memcpy(new_desc, old_desc, prefix_len);
+/*
+ * Max x86 register name length is 5(ex: %r15d). So, 6th char
+ * should always contain NULL. This helps to find register name
+ * length using strlen, insted of maintaing one more variable.
+ */
+#define SDT_REG_NAME_SIZE  6
 
-   /* Copy the new register name */
-   memcpy(new_desc + prefix_len, rnames->uprobe_name, uprobe_len);
+/*
+ * The uprobe parser does not support all gas register names;
+ * so, we have to replace them (ex. for x86_64: %rax -> %ax).
+ * Note: If register does not require renaming, just copy
+ * paste as it is, but don't leave it empty.
+ */
+static void sdt_rename_register(char *sdt_reg, int sdt_len, char *uprobe_reg)
+{
+   int i = 0;
 
-   /* Copy the chars after the register name (if need be) */
-   offset = prefix_len + sdt_len;
-   if (offset < old_desc_len)
-   memcpy(new_desc + prefix_len + uprobe_len,
-   old_desc + offset, old_desc_len - offset);
+   for (i = 0; sdt_reg_tbl[i].sdt_name != NULL; i++) {
+   if (!strncmp(sdt_reg_tbl[i].sdt_name, sdt_reg, sdt_len)) {
+   strcpy(uprobe_reg, sdt_reg_tbl[i].uprobe_name);
+  

[PATCH 4/9] perf report: Drop cycles 0 for LBR print

[Arnaldo Carvalho de Melo]

From: Jin Yao 

For some platforms, for example Broadwell, it doesn't support cycles
for LBR. But the perf always prints cycles:0, it's not necessary.

The patch refactors the LBR info print code and drops the cycles:0.

For example: perf report --branch-history --no-children --stdio

On Broadwell:
--0.91%--__random_r random_r.c:394 (iterations:2)
  __random_r random_r.c:360 (predicted:0.0%)
  __random_r random_r.c:380 (predicted:0.0%)
  __random_r random_r.c:357

On Skylake:
--1.07%--main div.c:39 (predicted:52.4% cycles:1 iterations:17)
  main div.c:44 (predicted:52.4% cycles:1)
  main div.c:42 (cycles:2)
  compute_flag div.c:28 (cycles:2)
  compute_flag div.c:27 (cycles:1)
  rand rand.c:28 (cycles:1)
  rand rand.c:28 (cycles:1)
  __random random.c:298 (cycles:1)
  __random random.c:297 (cycles:1)
  __random random.c:295 (cycles:1)
  __random random.c:295 (cycles:1)
  __random random.c:295 (cycles:1)

Signed-off-by: Yao Jin 
Reviewed-by: Andi Kleen 
Cc: Jiri Olsa 
Cc: Kan Liang 
Link: 
http://lkml.kernel.org/r/1489046786-10061-1-git-send-email-yao@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo 

Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/util/callchain.c | 111 +---
 1 file changed, 74 insertions(+), 37 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index d78776a20e80..3cea1fb5404b 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -1105,63 +1105,100 @@ int callchain_branch_counts(struct callchain_root 
*root,
  cycles_count);
 }
 
-static int callchain_counts_printf(FILE *fp, char *bf, int bfsize,
-  u64 branch_count, u64 predicted_count,
-  u64 abort_count, u64 cycles_count,
-  u64 iter_count, u64 samples_count)
+static int counts_str_build(char *bf, int bfsize,
+u64 branch_count, u64 predicted_count,
+u64 abort_count, u64 cycles_count,
+u64 iter_count, u64 samples_count)
 {
double predicted_percent = 0.0;
const char *null_str = "";
char iter_str[32];
-   char *str;
-   u64 cycles = 0;
-
-   if (branch_count == 0) {
-   if (fp)
-   return fprintf(fp, " (calltrace)");
+   char cycle_str[32];
+   char *istr, *cstr;
+   u64 cycles;
 
+   if (branch_count == 0)
return scnprintf(bf, bfsize, " (calltrace)");
-   }
+
+   cycles = cycles_count / branch_count;
 
if (iter_count && samples_count) {
-   scnprintf(iter_str, sizeof(iter_str),
-", iterations:%" PRId64 "",
-iter_count / samples_count);
-   str = iter_str;
+   if (cycles > 0)
+   scnprintf(iter_str, sizeof(iter_str),
+" iterations:%" PRId64 "",
+iter_count / samples_count);
+   else
+   scnprintf(iter_str, sizeof(iter_str),
+"iterations:%" PRId64 "",
+iter_count / samples_count);
+   istr = iter_str;
+   } else
+   istr = (char *)null_str;
+
+   if (cycles > 0) {
+   scnprintf(cycle_str, sizeof(cycle_str),
+ "cycles:%" PRId64 "", cycles);
+   cstr = cycle_str;
} else
-   str = (char *)null_str;
+   cstr = (char *)null_str;
 
predicted_percent = predicted_count * 100.0 / branch_count;
-   cycles = cycles_count / branch_count;
 
-   if ((predicted_percent >= 100.0) && (abort_count == 0)) {
-   if (fp)
-   return fprintf(fp, " (cycles:%" PRId64 "%s)",
-  cycles, str);
+   if ((predicted_count == branch_count) && (abort_count == 0)) {
+   if ((cycles > 0) || (istr != (char *)null_str))
+   return scnprintf(bf, bfsize, " (%s%s)", cstr, istr);
+   else
+   return scnprintf(bf, bfsize, "%s", (char *)null_str);
+   }
 
-   return scnprintf(bf, bfsize, " (cycles:%" PRId64 "%s)",
-cycles, str);
+   if ((predicted_count < branch_count) && (abort_count == 0)) {
+   if ((cycles > 0) || (istr != (char *)null_str))
+   return scnprintf(bf, bfsize,
+   " (predicted:%.1f%% %s%s)",
+   

[PATCH 2/9] perf/sdt/x86: Add renaming logic for (missing) 8 bit registers

[Arnaldo Carvalho de Melo]

From: Ravi Bangoria 

I found couple of events using al, bl, cl and dl registers for argument.
These are not directly accepted by uprobe_events and thus needs to be
mapped to ax, bx, cx and dx respectively.

Few ex,

  /usr/bin/qemu-system-s390x
css_adapter_interrupt: 1@%bl
css_chpid_add: 1@%cl 1@%sil 1@%dl
dma_bdrv_io: 8@%rbx 8@%rbp -8@%r14 1@%al

  /usr/bin/postgres
buffer__read__done: ... -1@-bash -1@%al
buffer__read__start: ... -1@%al

I don't find any sdt events using ah, bh,... registers. But I also don't
see any reason to not use them, so there might be rare events using
these registers, and if so, perf should have a renaming logic for them
too.

Signed-off-by: Ravi Bangoria 
Acked-by: Masami Hiramatsu 
Cc: Alexander Shishkin 
Cc: Alexis Berlemont 
Cc: Hemant Kumar 
Cc: Michael Ellerman 
Cc: Naveen N. Rao 
Cc: Peter Zijlstra 
Link: 
http://lkml.kernel.org/r/20170328094754.3156-2-ravi.bango...@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/arch/x86/util/perf_regs.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/arch/x86/util/perf_regs.c 
b/tools/perf/arch/x86/util/perf_regs.c
index d8a8dcf761f7..fa1fd196837d 100644
--- a/tools/perf/arch/x86/util/perf_regs.c
+++ b/tools/perf/arch/x86/util/perf_regs.c
@@ -40,12 +40,20 @@ struct sdt_name_reg {
 static const struct sdt_name_reg sdt_reg_renamings[] = {
SDT_NAME_REG(eax, ax),
SDT_NAME_REG(rax, ax),
+   SDT_NAME_REG(al,  ax),
+   SDT_NAME_REG(ah,  ax),
SDT_NAME_REG(ebx, bx),
SDT_NAME_REG(rbx, bx),
+   SDT_NAME_REG(bl,  bx),
+   SDT_NAME_REG(bh,  bx),
SDT_NAME_REG(ecx, cx),
SDT_NAME_REG(rcx, cx),
+   SDT_NAME_REG(cl,  cx),
+   SDT_NAME_REG(ch,  cx),
SDT_NAME_REG(edx, dx),
SDT_NAME_REG(rdx, dx),
+   SDT_NAME_REG(dl,  dx),
+   SDT_NAME_REG(dh,  dx),
SDT_NAME_REG(esi, si),
SDT_NAME_REG(rsi, si),
SDT_NAME_REG(sil, si),
-- 
2.9.3

[PATCH 1/9] perf tools: Remove support for command aliases

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo 

This came from 'git', but isn't documented anywhere in
tools/perf/Documentation/, looks like baggage we can do without, ditch
it.

Cc: Adrian Hunter 
Cc: David Ahern 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Wang Nan 
Link: http://lkml.kernel.org/n/tip-e7uwkn60t4hmlnwj99ba4...@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/builtin-help.c  | 13 -
 tools/perf/perf.c  | 97 +++---
 tools/perf/util/Build  |  1 -
 tools/perf/util/alias.c| 78 --
 tools/perf/util/cache.h|  1 -
 tools/perf/util/help-unknown-cmd.c |  8 +---
 6 files changed, 8 insertions(+), 190 deletions(-)
 delete mode 100644 tools/perf/util/alias.c

diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c
index 7ae238929e95..1eec96a0fa67 100644
--- a/tools/perf/builtin-help.c
+++ b/tools/perf/builtin-help.c
@@ -301,12 +301,6 @@ void list_common_cmds_help(void)
}
 }
 
-static int is_perf_command(const char *s)
-{
-   return is_in_cmdlist(_cmds, s) ||
-   is_in_cmdlist(_cmds, s);
-}
-
 static const char *cmd_to_page(const char *perf_cmd)
 {
char *s;
@@ -446,7 +440,6 @@ int cmd_help(int argc, const char **argv)
"perf help [--all] [--man|--web|--info] [command]",
NULL
};
-   const char *alias;
int rc;
 
load_command_list("perf-", _cmds, _cmds);
@@ -472,12 +465,6 @@ int cmd_help(int argc, const char **argv)
return 0;
}
 
-   alias = alias_lookup(argv[0]);
-   if (alias && !is_perf_command(argv[0])) {
-   printf("`perf %s' is aliased to `%s'\n", argv[0], alias);
-   return 0;
-   }
-
switch (help_format) {
case HELP_FORMAT_MAN:
rc = show_man_page(argv[0]);
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 4b283d18e158..9217f2227f3d 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -267,71 +267,6 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
return handled;
 }
 
-static int handle_alias(int *argcp, const char ***argv)
-{
-   int envchanged = 0, ret = 0, saved_errno = errno;
-   int count, option_count;
-   const char **new_argv;
-   const char *alias_command;
-   char *alias_string;
-
-   alias_command = (*argv)[0];
-   alias_string = alias_lookup(alias_command);
-   if (alias_string) {
-   if (alias_string[0] == '!') {
-   if (*argcp > 1) {
-   struct strbuf buf;
-
-   if (strbuf_init(, PATH_MAX) < 0 ||
-   strbuf_addstr(, alias_string) < 0 ||
-   sq_quote_argv(, (*argv) + 1,
- PATH_MAX) < 0)
-   die("Failed to allocate memory.");
-   free(alias_string);
-   alias_string = buf.buf;
-   }
-   ret = system(alias_string + 1);
-   if (ret >= 0 && WIFEXITED(ret) &&
-   WEXITSTATUS(ret) != 127)
-   exit(WEXITSTATUS(ret));
-   die("Failed to run '%s' when expanding alias '%s'",
-   alias_string + 1, alias_command);
-   }
-   count = split_cmdline(alias_string, _argv);
-   if (count < 0)
-   die("Bad alias.%s string", alias_command);
-   option_count = handle_options(_argv, , );
-   if (envchanged)
-   die("alias '%s' changes environment variables\n"
-"You can use '!perf' in the alias to do this.",
-alias_command);
-   memmove(new_argv - option_count, new_argv,
-   count * sizeof(char *));
-   new_argv -= option_count;
-
-   if (count < 1)
-   die("empty alias for %s", alias_command);
-
-   if (!strcmp(alias_command, new_argv[0]))
-   die("recursive alias: %s", alias_command);
-
-   new_argv = realloc(new_argv, sizeof(char *) *
-   (count + *argcp + 1));
-   /* insert after command name */
-   memcpy(new_argv + count, *argv + 1, sizeof(char *) * *argcp);
-   new_argv[count + *argcp] = NULL;
-
-   *argv = new_argv;
-   *argcp += count - 1;
-
-   ret = 1;
-   }
-
-   errno = saved_errno;
-
-   return ret;
-}
-
 #define RUN_SETUP  

[PATCH 2/9] perf/sdt/x86: Add renaming logic for (missing) 8 bit registers

[Arnaldo Carvalho de Melo]

From: Ravi Bangoria 

I found couple of events using al, bl, cl and dl registers for argument.
These are not directly accepted by uprobe_events and thus needs to be
mapped to ax, bx, cx and dx respectively.

Few ex,

  /usr/bin/qemu-system-s390x
css_adapter_interrupt: 1@%bl
css_chpid_add: 1@%cl 1@%sil 1@%dl
dma_bdrv_io: 8@%rbx 8@%rbp -8@%r14 1@%al

  /usr/bin/postgres
buffer__read__done: ... -1@-bash -1@%al
buffer__read__start: ... -1@%al

I don't find any sdt events using ah, bh,... registers. But I also don't
see any reason to not use them, so there might be rare events using
these registers, and if so, perf should have a renaming logic for them
too.

Signed-off-by: Ravi Bangoria 
Acked-by: Masami Hiramatsu 
Cc: Alexander Shishkin 
Cc: Alexis Berlemont 
Cc: Hemant Kumar 
Cc: Michael Ellerman 
Cc: Naveen N. Rao 
Cc: Peter Zijlstra 
Link: 
http://lkml.kernel.org/r/20170328094754.3156-2-ravi.bango...@linux.vnet.ibm.com
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/arch/x86/util/perf_regs.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/tools/perf/arch/x86/util/perf_regs.c 
b/tools/perf/arch/x86/util/perf_regs.c
index d8a8dcf761f7..fa1fd196837d 100644
--- a/tools/perf/arch/x86/util/perf_regs.c
+++ b/tools/perf/arch/x86/util/perf_regs.c
@@ -40,12 +40,20 @@ struct sdt_name_reg {
 static const struct sdt_name_reg sdt_reg_renamings[] = {
SDT_NAME_REG(eax, ax),
SDT_NAME_REG(rax, ax),
+   SDT_NAME_REG(al,  ax),
+   SDT_NAME_REG(ah,  ax),
SDT_NAME_REG(ebx, bx),
SDT_NAME_REG(rbx, bx),
+   SDT_NAME_REG(bl,  bx),
+   SDT_NAME_REG(bh,  bx),
SDT_NAME_REG(ecx, cx),
SDT_NAME_REG(rcx, cx),
+   SDT_NAME_REG(cl,  cx),
+   SDT_NAME_REG(ch,  cx),
SDT_NAME_REG(edx, dx),
SDT_NAME_REG(rdx, dx),
+   SDT_NAME_REG(dl,  dx),
+   SDT_NAME_REG(dh,  dx),
SDT_NAME_REG(esi, si),
SDT_NAME_REG(rsi, si),
SDT_NAME_REG(sil, si),
-- 
2.9.3

[PATCH 1/9] perf tools: Remove support for command aliases

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo 

This came from 'git', but isn't documented anywhere in
tools/perf/Documentation/, looks like baggage we can do without, ditch
it.

Cc: Adrian Hunter 
Cc: David Ahern 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Wang Nan 
Link: http://lkml.kernel.org/n/tip-e7uwkn60t4hmlnwj99ba4...@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/builtin-help.c  | 13 -
 tools/perf/perf.c  | 97 +++---
 tools/perf/util/Build  |  1 -
 tools/perf/util/alias.c| 78 --
 tools/perf/util/cache.h|  1 -
 tools/perf/util/help-unknown-cmd.c |  8 +---
 6 files changed, 8 insertions(+), 190 deletions(-)
 delete mode 100644 tools/perf/util/alias.c

diff --git a/tools/perf/builtin-help.c b/tools/perf/builtin-help.c
index 7ae238929e95..1eec96a0fa67 100644
--- a/tools/perf/builtin-help.c
+++ b/tools/perf/builtin-help.c
@@ -301,12 +301,6 @@ void list_common_cmds_help(void)
}
 }
 
-static int is_perf_command(const char *s)
-{
-   return is_in_cmdlist(_cmds, s) ||
-   is_in_cmdlist(_cmds, s);
-}
-
 static const char *cmd_to_page(const char *perf_cmd)
 {
char *s;
@@ -446,7 +440,6 @@ int cmd_help(int argc, const char **argv)
"perf help [--all] [--man|--web|--info] [command]",
NULL
};
-   const char *alias;
int rc;
 
load_command_list("perf-", _cmds, _cmds);
@@ -472,12 +465,6 @@ int cmd_help(int argc, const char **argv)
return 0;
}
 
-   alias = alias_lookup(argv[0]);
-   if (alias && !is_perf_command(argv[0])) {
-   printf("`perf %s' is aliased to `%s'\n", argv[0], alias);
-   return 0;
-   }
-
switch (help_format) {
case HELP_FORMAT_MAN:
rc = show_man_page(argv[0]);
diff --git a/tools/perf/perf.c b/tools/perf/perf.c
index 4b283d18e158..9217f2227f3d 100644
--- a/tools/perf/perf.c
+++ b/tools/perf/perf.c
@@ -267,71 +267,6 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
return handled;
 }
 
-static int handle_alias(int *argcp, const char ***argv)
-{
-   int envchanged = 0, ret = 0, saved_errno = errno;
-   int count, option_count;
-   const char **new_argv;
-   const char *alias_command;
-   char *alias_string;
-
-   alias_command = (*argv)[0];
-   alias_string = alias_lookup(alias_command);
-   if (alias_string) {
-   if (alias_string[0] == '!') {
-   if (*argcp > 1) {
-   struct strbuf buf;
-
-   if (strbuf_init(, PATH_MAX) < 0 ||
-   strbuf_addstr(, alias_string) < 0 ||
-   sq_quote_argv(, (*argv) + 1,
- PATH_MAX) < 0)
-   die("Failed to allocate memory.");
-   free(alias_string);
-   alias_string = buf.buf;
-   }
-   ret = system(alias_string + 1);
-   if (ret >= 0 && WIFEXITED(ret) &&
-   WEXITSTATUS(ret) != 127)
-   exit(WEXITSTATUS(ret));
-   die("Failed to run '%s' when expanding alias '%s'",
-   alias_string + 1, alias_command);
-   }
-   count = split_cmdline(alias_string, _argv);
-   if (count < 0)
-   die("Bad alias.%s string", alias_command);
-   option_count = handle_options(_argv, , );
-   if (envchanged)
-   die("alias '%s' changes environment variables\n"
-"You can use '!perf' in the alias to do this.",
-alias_command);
-   memmove(new_argv - option_count, new_argv,
-   count * sizeof(char *));
-   new_argv -= option_count;
-
-   if (count < 1)
-   die("empty alias for %s", alias_command);
-
-   if (!strcmp(alias_command, new_argv[0]))
-   die("recursive alias: %s", alias_command);
-
-   new_argv = realloc(new_argv, sizeof(char *) *
-   (count + *argcp + 1));
-   /* insert after command name */
-   memcpy(new_argv + count, *argv + 1, sizeof(char *) * *argcp);
-   new_argv[count + *argcp] = NULL;
-
-   *argv = new_argv;
-   *argcp += count - 1;
-
-   ret = 1;
-   }
-
-   errno = saved_errno;
-
-   return ret;
-}
-
 #define RUN_SETUP  (1<<0)
 #define USE_PAGER  (1<<1)
 
@@ -455,25 +390,12 @@ static void execv_dashed_external(const char **argv)
 
 static int 

[PATCH 4/9] perf report: Drop cycles 0 for LBR print

[Arnaldo Carvalho de Melo]

From: Jin Yao 

For some platforms, for example Broadwell, it doesn't support cycles
for LBR. But the perf always prints cycles:0, it's not necessary.

The patch refactors the LBR info print code and drops the cycles:0.

For example: perf report --branch-history --no-children --stdio

On Broadwell:
--0.91%--__random_r random_r.c:394 (iterations:2)
  __random_r random_r.c:360 (predicted:0.0%)
  __random_r random_r.c:380 (predicted:0.0%)
  __random_r random_r.c:357

On Skylake:
--1.07%--main div.c:39 (predicted:52.4% cycles:1 iterations:17)
  main div.c:44 (predicted:52.4% cycles:1)
  main div.c:42 (cycles:2)
  compute_flag div.c:28 (cycles:2)
  compute_flag div.c:27 (cycles:1)
  rand rand.c:28 (cycles:1)
  rand rand.c:28 (cycles:1)
  __random random.c:298 (cycles:1)
  __random random.c:297 (cycles:1)
  __random random.c:295 (cycles:1)
  __random random.c:295 (cycles:1)
  __random random.c:295 (cycles:1)

Signed-off-by: Yao Jin 
Reviewed-by: Andi Kleen 
Cc: Jiri Olsa 
Cc: Kan Liang 
Link: 
http://lkml.kernel.org/r/1489046786-10061-1-git-send-email-yao@linux.intel.com
Signed-off-by: Arnaldo Carvalho de Melo 

Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/util/callchain.c | 111 +---
 1 file changed, 74 insertions(+), 37 deletions(-)

diff --git a/tools/perf/util/callchain.c b/tools/perf/util/callchain.c
index d78776a20e80..3cea1fb5404b 100644
--- a/tools/perf/util/callchain.c
+++ b/tools/perf/util/callchain.c
@@ -1105,63 +1105,100 @@ int callchain_branch_counts(struct callchain_root 
*root,
  cycles_count);
 }
 
-static int callchain_counts_printf(FILE *fp, char *bf, int bfsize,
-  u64 branch_count, u64 predicted_count,
-  u64 abort_count, u64 cycles_count,
-  u64 iter_count, u64 samples_count)
+static int counts_str_build(char *bf, int bfsize,
+u64 branch_count, u64 predicted_count,
+u64 abort_count, u64 cycles_count,
+u64 iter_count, u64 samples_count)
 {
double predicted_percent = 0.0;
const char *null_str = "";
char iter_str[32];
-   char *str;
-   u64 cycles = 0;
-
-   if (branch_count == 0) {
-   if (fp)
-   return fprintf(fp, " (calltrace)");
+   char cycle_str[32];
+   char *istr, *cstr;
+   u64 cycles;
 
+   if (branch_count == 0)
return scnprintf(bf, bfsize, " (calltrace)");
-   }
+
+   cycles = cycles_count / branch_count;
 
if (iter_count && samples_count) {
-   scnprintf(iter_str, sizeof(iter_str),
-", iterations:%" PRId64 "",
-iter_count / samples_count);
-   str = iter_str;
+   if (cycles > 0)
+   scnprintf(iter_str, sizeof(iter_str),
+" iterations:%" PRId64 "",
+iter_count / samples_count);
+   else
+   scnprintf(iter_str, sizeof(iter_str),
+"iterations:%" PRId64 "",
+iter_count / samples_count);
+   istr = iter_str;
+   } else
+   istr = (char *)null_str;
+
+   if (cycles > 0) {
+   scnprintf(cycle_str, sizeof(cycle_str),
+ "cycles:%" PRId64 "", cycles);
+   cstr = cycle_str;
} else
-   str = (char *)null_str;
+   cstr = (char *)null_str;
 
predicted_percent = predicted_count * 100.0 / branch_count;
-   cycles = cycles_count / branch_count;
 
-   if ((predicted_percent >= 100.0) && (abort_count == 0)) {
-   if (fp)
-   return fprintf(fp, " (cycles:%" PRId64 "%s)",
-  cycles, str);
+   if ((predicted_count == branch_count) && (abort_count == 0)) {
+   if ((cycles > 0) || (istr != (char *)null_str))
+   return scnprintf(bf, bfsize, " (%s%s)", cstr, istr);
+   else
+   return scnprintf(bf, bfsize, "%s", (char *)null_str);
+   }
 
-   return scnprintf(bf, bfsize, " (cycles:%" PRId64 "%s)",
-cycles, str);
+   if ((predicted_count < branch_count) && (abort_count == 0)) {
+   if ((cycles > 0) || (istr != (char *)null_str))
+   return scnprintf(bf, bfsize,
+   " (predicted:%.1f%% %s%s)",
+   predicted_percent, cstr, istr);
+   else {
+   return scnprintf(bf, bfsize,
+   " 

[PATCH 9/9] perf trace: Beautify statx syscall 'flag' and 'mask' arguments

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo 

To test it, build samples/statx/test_statx, which I did as:

  $ make headers_install
  $ cc -I ~/git/linux/usr/include samples/statx/test-statx.c -o /tmp/statx

And then use perf trace on it:

  # perf trace -e statx /tmp/statx /etc/passwd
  statx(/etc/passwd) = 0
  results=7ff
Size: 3496Blocks: 8  IO Block: 4096regular file
  Device: fd:00   Inode: 280156  Links: 1
  Access: (0644/-rw-r--r--)  Uid: 0   Gid: 0
  Access: 2017-03-29 16:01:01.650073438-0300
  Modify: 2017-03-10 16:25:14.156479354-0300
  Change: 2017-03-10 16:25:14.171479328-0300
 0.000 ( 0.007 ms): statx/30648 statx(dfd: CWD, filename: 0x7ef503f4, 
flags: SYMLINK_NOFOLLOW, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7fff7ef4eb10) = 0
  #

Using the test-stat.c options to change the mask:

  # perf trace -e statx /tmp/statx -O /etc/passwd > /dev/null
 0.000 ( 0.008 ms): statx/30745 statx(dfd: CWD, filename: 0x3a0753f4, 
flags: SYMLINK_NOFOLLOW, mask: BTIME, buffer: 0x7ffd3a0735c0) = 0
  #
  # perf trace -e statx /tmp/statx -A /etc/passwd > /dev/null
 0.000 ( 0.010 ms): statx/30757 statx(dfd: CWD, filename: 0xa94e63f4, 
flags: SYMLINK_NOFOLLOW|NO_AUTOMOUNT, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffea94e49d0) = 0
  #
  # trace --no-inherit -e statx /tmp/statx -F /etc/passwd > /dev/null
 0.000 ( 0.011 ms): statx(dfd: CWD, filename: 0x3b02d3f3, flags: 
SYMLINK_NOFOLLOW|STATX_FORCE_SYNC, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffd3b02c850) = 0
  #
  # trace --no-inherit -e statx /tmp/statx -F -L /etc/passwd > /dev/null
 0.000 ( 0.008 ms): statx(dfd: CWD, filename: 0x15cff3f3, flags: 
STATX_FORCE_SYNC, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7fff15cfdda0) = 0
  #
  # trace --no-inherit -e statx /tmp/statx -D -O /etc/passwd > /dev/null
 0.000 ( 0.009 ms): statx(dfd: CWD, filename: 0xfa37f3f3, flags: 
SYMLINK_NOFOLLOW|STATX_DONT_SYNC, mask: BTIME, buffer: 0x7a37da20) = 0
  #

Adding a probe to get the filename collected as well:

  # perf probe 'vfs_getname=getname_flags:72 pathname=result->name:string'
  Added new event:
probe:vfs_getname(on getname_flags:72 with pathname=result->name:string)

  You can now use it in all perf tools, such as:

  perf record -e probe:vfs_getname -aR sleep 1

  # trace --no-inherit -e statx /tmp/statx -D -O /etc/passwd > /dev/null
 0.169 ( 0.007 ms): statx(dfd: CWD, filename: /etc/passwd, flags: 
SYMLINK_NOFOLLOW|STATX_DONT_SYNC, mask: BTIME, buffer: 0x7ffda9bf50f0) = 0
  #

Same technique could be used to collect and beautify the result put in
the 'buffer' argument.

Finally do a system wide 'perf trace' session looking for any use of statx,
then run the test proggie with various flags:

  # trace -e statx
   16612.967 ( 0.028 ms): statx/4562 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffef195d660) = 0
   33064.447 ( 0.011 ms): statx/4569 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW|STATX_FORCE_SYNC, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffc5484c790) = 0
   36050.891 ( 0.023 ms): statx/4576 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW, mask: BTIME, buffer: 0x7ffeb18b66e0) = 0
   38039.889 ( 0.023 ms): statx/4584 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7fff1db0ea90) = 0
  ^C#

This one also starts moving the beautifiers from files directly included
in builtin-trace.c to separate objects + a beauty.h header with
prototypes, so that we can add test cases in tools/perf/tests/ to fire
syscalls with various arguments and then get them intercepted as
syscalls:sys_enter_foo or raw_syscalls:sys_enter + sys_exit to then
format and check that the formatted output is the one we expect.

Cc: Adrian Hunter 
Cc: Al Viro 
Cc: David Ahern 
Cc: David Howells 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Wang Nan 
Link: http://lkml.kernel.org/n/tip-xvzw8eynffvez5czyzidh...@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/Build  |  1 +
 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl |  1 +
 tools/perf/builtin-trace.c| 14 ++---
 tools/perf/trace/beauty/Build |  1 +
 tools/perf/trace/beauty/beauty.h  | 24 
 tools/perf/trace/beauty/statx.c   | 72 +++
 6 files changed, 104 insertions(+), 9 deletions(-)
 create mode 100644 

[PATCH 9/9] perf trace: Beautify statx syscall 'flag' and 'mask' arguments

[Arnaldo Carvalho de Melo]

From: Arnaldo Carvalho de Melo 

To test it, build samples/statx/test_statx, which I did as:

  $ make headers_install
  $ cc -I ~/git/linux/usr/include samples/statx/test-statx.c -o /tmp/statx

And then use perf trace on it:

  # perf trace -e statx /tmp/statx /etc/passwd
  statx(/etc/passwd) = 0
  results=7ff
Size: 3496Blocks: 8  IO Block: 4096regular file
  Device: fd:00   Inode: 280156  Links: 1
  Access: (0644/-rw-r--r--)  Uid: 0   Gid: 0
  Access: 2017-03-29 16:01:01.650073438-0300
  Modify: 2017-03-10 16:25:14.156479354-0300
  Change: 2017-03-10 16:25:14.171479328-0300
 0.000 ( 0.007 ms): statx/30648 statx(dfd: CWD, filename: 0x7ef503f4, 
flags: SYMLINK_NOFOLLOW, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7fff7ef4eb10) = 0
  #

Using the test-stat.c options to change the mask:

  # perf trace -e statx /tmp/statx -O /etc/passwd > /dev/null
 0.000 ( 0.008 ms): statx/30745 statx(dfd: CWD, filename: 0x3a0753f4, 
flags: SYMLINK_NOFOLLOW, mask: BTIME, buffer: 0x7ffd3a0735c0) = 0
  #
  # perf trace -e statx /tmp/statx -A /etc/passwd > /dev/null
 0.000 ( 0.010 ms): statx/30757 statx(dfd: CWD, filename: 0xa94e63f4, 
flags: SYMLINK_NOFOLLOW|NO_AUTOMOUNT, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffea94e49d0) = 0
  #
  # trace --no-inherit -e statx /tmp/statx -F /etc/passwd > /dev/null
 0.000 ( 0.011 ms): statx(dfd: CWD, filename: 0x3b02d3f3, flags: 
SYMLINK_NOFOLLOW|STATX_FORCE_SYNC, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffd3b02c850) = 0
  #
  # trace --no-inherit -e statx /tmp/statx -F -L /etc/passwd > /dev/null
 0.000 ( 0.008 ms): statx(dfd: CWD, filename: 0x15cff3f3, flags: 
STATX_FORCE_SYNC, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7fff15cfdda0) = 0
  #
  # trace --no-inherit -e statx /tmp/statx -D -O /etc/passwd > /dev/null
 0.000 ( 0.009 ms): statx(dfd: CWD, filename: 0xfa37f3f3, flags: 
SYMLINK_NOFOLLOW|STATX_DONT_SYNC, mask: BTIME, buffer: 0x7a37da20) = 0
  #

Adding a probe to get the filename collected as well:

  # perf probe 'vfs_getname=getname_flags:72 pathname=result->name:string'
  Added new event:
probe:vfs_getname(on getname_flags:72 with pathname=result->name:string)

  You can now use it in all perf tools, such as:

  perf record -e probe:vfs_getname -aR sleep 1

  # trace --no-inherit -e statx /tmp/statx -D -O /etc/passwd > /dev/null
 0.169 ( 0.007 ms): statx(dfd: CWD, filename: /etc/passwd, flags: 
SYMLINK_NOFOLLOW|STATX_DONT_SYNC, mask: BTIME, buffer: 0x7ffda9bf50f0) = 0
  #

Same technique could be used to collect and beautify the result put in
the 'buffer' argument.

Finally do a system wide 'perf trace' session looking for any use of statx,
then run the test proggie with various flags:

  # trace -e statx
   16612.967 ( 0.028 ms): statx/4562 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffef195d660) = 0
   33064.447 ( 0.011 ms): statx/4569 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW|STATX_FORCE_SYNC, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7ffc5484c790) = 0
   36050.891 ( 0.023 ms): statx/4576 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW, mask: BTIME, buffer: 0x7ffeb18b66e0) = 0
   38039.889 ( 0.023 ms): statx/4584 statx(dfd: CWD, filename: /tmp/statx, 
flags: SYMLINK_NOFOLLOW, mask: 
TYPE|MODE|NLINK|UID|GID|ATIME|MTIME|CTIME|INO|SIZE|BLOCKS|BTIME, buffer: 
0x7fff1db0ea90) = 0
  ^C#

This one also starts moving the beautifiers from files directly included
in builtin-trace.c to separate objects + a beauty.h header with
prototypes, so that we can add test cases in tools/perf/tests/ to fire
syscalls with various arguments and then get them intercepted as
syscalls:sys_enter_foo or raw_syscalls:sys_enter + sys_exit to then
format and check that the formatted output is the one we expect.

Cc: Adrian Hunter 
Cc: Al Viro 
Cc: David Ahern 
Cc: David Howells 
Cc: Jiri Olsa 
Cc: Namhyung Kim 
Cc: Wang Nan 
Link: http://lkml.kernel.org/n/tip-xvzw8eynffvez5czyzidh...@git.kernel.org
Signed-off-by: Arnaldo Carvalho de Melo 
---
 tools/perf/Build  |  1 +
 tools/perf/arch/x86/entry/syscalls/syscall_64.tbl |  1 +
 tools/perf/builtin-trace.c| 14 ++---
 tools/perf/trace/beauty/Build |  1 +
 tools/perf/trace/beauty/beauty.h  | 24 
 tools/perf/trace/beauty/statx.c   | 72 +++
 6 files changed, 104 insertions(+), 9 deletions(-)
 create mode 100644 tools/perf/trace/beauty/Build
 create mode 100644 tools/perf/trace/beauty/beauty.h
 create mode 100644 tools/perf/trace/beauty/statx.c

diff --git a/tools/perf/Build b/tools/perf/Build

Re: [PATCH] dm ioctl: Remove double parentheses

[Joe Perches]

On Fri, 2017-03-31 at 18:50 -0700, Matthias Kaehlcke wrote:
> El Thu, Mar 16, 2017 at 09:48:30AM -0700 Matthias Kaehlcke ha dit:
> 
> > The extra pair of parantheses is not needed and causes clang to generate
> > the following warning:
> > 
> > drivers/md/dm-ioctl.c:1776:11: error: equality comparison with extraneous 
> > parentheses [-Werror,-Wparentheses-equality]
> > if ((cmd == DM_DEV_CREATE_CMD)) {
> >  ^~~~
> > drivers/md/dm-ioctl.c:1776:11: note: remove extraneous parentheses around 
> > the comparison to silence this warning
> > if ((cmd == DM_DEV_CREATE_CMD)) {
> > ~^   ~
> > drivers/md/dm-ioctl.c:1776:11: note: use '=' to turn this equality 
> > comparison into an assignment
> > if ((cmd == DM_DEV_CREATE_CMD)) {

There are dozens of these comparisons in the kernel.
Are you fixing them all or just this one?

Re: [PATCH] dm ioctl: Remove double parentheses

[Joe Perches]

On Fri, 2017-03-31 at 18:50 -0700, Matthias Kaehlcke wrote:
> El Thu, Mar 16, 2017 at 09:48:30AM -0700 Matthias Kaehlcke ha dit:
> 
> > The extra pair of parantheses is not needed and causes clang to generate
> > the following warning:
> > 
> > drivers/md/dm-ioctl.c:1776:11: error: equality comparison with extraneous 
> > parentheses [-Werror,-Wparentheses-equality]
> > if ((cmd == DM_DEV_CREATE_CMD)) {
> >  ^~~~
> > drivers/md/dm-ioctl.c:1776:11: note: remove extraneous parentheses around 
> > the comparison to silence this warning
> > if ((cmd == DM_DEV_CREATE_CMD)) {
> > ~^   ~
> > drivers/md/dm-ioctl.c:1776:11: note: use '=' to turn this equality 
> > comparison into an assignment
> > if ((cmd == DM_DEV_CREATE_CMD)) {

There are dozens of these comparisons in the kernel.
Are you fixing them all or just this one?

[PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Chewie Lin]

Replace string with formatted arguments in the dev_warn() call. It removes
the checkpatch warning:

WARNING: Prefer using "%s", __func__ to embedded function names
#417: FILE: main_usb.c:417:
+"usb_device_reset fail status=%d\n", status);

total: 0 errors, 1 warnings, 1058 lines checked

And after fix:

main_usb.c has no obvious style problems and is ready for submission.

Signed-off-by: Chewie Lin 
---
 drivers/staging/vt6656/main_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6656/main_usb.c 
b/drivers/staging/vt6656/main_usb.c
index 9e074e9..2d9e7af 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
status = usb_reset_device(priv->usb);
if (status)
dev_warn(>usb->dev,
-"usb_device_reset fail status=%d\n", status);
+"%s=%d\n", "usb_device_reset fail status", status);
 }
 
 static void vnt_free_int_bufs(struct vnt_private *priv)
-- 
2.10.0

[PATCH 001/001] drivers/staging/vt6656/main_usb.c: checkpatch warning

[Chewie Lin]

Replace string with formatted arguments in the dev_warn() call. It removes
the checkpatch warning:

WARNING: Prefer using "%s", __func__ to embedded function names
#417: FILE: main_usb.c:417:
+"usb_device_reset fail status=%d\n", status);

total: 0 errors, 1 warnings, 1058 lines checked

And after fix:

main_usb.c has no obvious style problems and is ready for submission.

Signed-off-by: Chewie Lin 
---
 drivers/staging/vt6656/main_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/vt6656/main_usb.c 
b/drivers/staging/vt6656/main_usb.c
index 9e074e9..2d9e7af 100644
--- a/drivers/staging/vt6656/main_usb.c
+++ b/drivers/staging/vt6656/main_usb.c
@@ -414,7 +414,7 @@ static void usb_device_reset(struct vnt_private *priv)
status = usb_reset_device(priv->usb);
if (status)
dev_warn(>usb->dev,
-"usb_device_reset fail status=%d\n", status);
+"%s=%d\n", "usb_device_reset fail status", status);
 }
 
 static void vnt_free_int_bufs(struct vnt_private *priv)
-- 
2.10.0

Re: [PATCH] x86/intel_rdt: Add cpus_list rdtgroup file

[Fenghua Yu]

On Thu, Mar 30, 2017 at 04:59:47PM +0200, Jiri Olsa wrote:
> On Wed, Mar 29, 2017 at 06:16:00PM +0200, Jiri Olsa wrote:
> > On Wed, Mar 29, 2017 at 09:08:26AM -0700, Fenghua Yu wrote:
> > > On Wed, Mar 29, 2017 at 05:09:48PM +0200, Jiri Olsa wrote:
> > > > While playing with the resctrl interface I found it much
> > > > easier to deal with cpumask list rather than just regular
> > > > cpumask.
> > > 
> > > Could you please explain specifically why and when it's easier
> > > to deal with cpumask list? In programming cases, cpumask
> > > and cpumask list are almost same. And people are working
> > > on higher level tools to control resctrl. The tools can
> > > hide detailed regular cpumask or cpumask list and user
> > > doesn't need to care lower level format of cpumask. So
> > > is it really useful to add cpus_list?
> > 
> > 
> > well I'm not aware about any such tool so I used resctrl
> > interface directly, and in that case it was much simpler
> 
> is there any tool available for this actualy?

Here is a tool created by Marcelo:
https://lwn.net/Articles/710514/

Thanks.

-Fenghua

[PATCH] eudyptula challenge

[Chewie Lin]

Hi all:

Better and improved changelog version 3. 

I'm submitting this patch as part of Eudyptula challenge to fix a
coding style problem.  Thanks for taking time on this trivial patch.

sl424

Chewie Lin (1):
drivers/staging/vt6656/main_usb.c: checkpatch warning

 drivers/staging/vt6656/main_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.10.0

Re: [PATCH] x86/intel_rdt: Add cpus_list rdtgroup file

[Fenghua Yu]

On Thu, Mar 30, 2017 at 04:59:47PM +0200, Jiri Olsa wrote:
> On Wed, Mar 29, 2017 at 06:16:00PM +0200, Jiri Olsa wrote:
> > On Wed, Mar 29, 2017 at 09:08:26AM -0700, Fenghua Yu wrote:
> > > On Wed, Mar 29, 2017 at 05:09:48PM +0200, Jiri Olsa wrote:
> > > > While playing with the resctrl interface I found it much
> > > > easier to deal with cpumask list rather than just regular
> > > > cpumask.
> > > 
> > > Could you please explain specifically why and when it's easier
> > > to deal with cpumask list? In programming cases, cpumask
> > > and cpumask list are almost same. And people are working
> > > on higher level tools to control resctrl. The tools can
> > > hide detailed regular cpumask or cpumask list and user
> > > doesn't need to care lower level format of cpumask. So
> > > is it really useful to add cpus_list?
> > 
> > 
> > well I'm not aware about any such tool so I used resctrl
> > interface directly, and in that case it was much simpler
> 
> is there any tool available for this actualy?

Here is a tool created by Marcelo:
https://lwn.net/Articles/710514/

Thanks.

-Fenghua

[PATCH] eudyptula challenge

[Chewie Lin]

Hi all:

Better and improved changelog version 3. 

I'm submitting this patch as part of Eudyptula challenge to fix a
coding style problem.  Thanks for taking time on this trivial patch.

sl424

Chewie Lin (1):
drivers/staging/vt6656/main_usb.c: checkpatch warning

 drivers/staging/vt6656/main_usb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

-- 
2.10.0

Re: [RFC 1/8] Introduce Peer-to-Peer memory (p2pmem) device

[Logan Gunthorpe]

On 31/03/17 05:51 PM, Sinan Kaya wrote:
> You can put a restriction with DMI/SMBIOS such that all devices from 2016
> work else they belong to blacklist.

How do you get a manufacturing date for a given device within the
kernel? Is this actually something generically available?

Logan

Re: [RFC 1/8] Introduce Peer-to-Peer memory (p2pmem) device

[Logan Gunthorpe]

On 31/03/17 05:51 PM, Sinan Kaya wrote:
> You can put a restriction with DMI/SMBIOS such that all devices from 2016
> work else they belong to blacklist.

How do you get a manufacturing date for a given device within the
kernel? Is this actually something generically available?

Logan

[GIT PULL] nfsd bugfixes for 4.11

[J. Bruce Fields]

Please pull nfsd bugfixes from

  git://linux-nfs.org/~bfields/linux.git tags/nfsd-4.11-1

The restriction of NFSv4 to TCP went overboard and also broke the
backchannel; fix.  Also some minor refinements to the nfsd
version-setting interface that we'd like to get fixed before release.

--b.

Chuck Lever (1):
  svcrdma: set XPT_CONG_CTRL flag for bc xprt

Kinglong Mee (2):
  SUNRPC/backchanel: set XPT_CONG_CTRL flag for bc xprt
  nfsd: map the ENOKEY to nfserr_perm for avoiding warning

NeilBrown (3):
  NFSD: further refinement of content of /proc/fs/nfsd/versions
  NFSD: fix nfsd_minorversion(.., NFSD_AVAIL)
  NFSD: fix nfsd_reset_versions for NFSv4.

 fs/nfsd/nfsctl.c | 43 
 fs/nfsd/nfsproc.c|  1 +
 fs/nfsd/nfssvc.c | 28 ++---
 net/sunrpc/svcsock.c |  1 +
 net/sunrpc/xprtrdma/svc_rdma_transport.c |  1 +
 5 files changed, 49 insertions(+), 25 deletions(-)

[GIT PULL] nfsd bugfixes for 4.11

[J. Bruce Fields]

Please pull nfsd bugfixes from

  git://linux-nfs.org/~bfields/linux.git tags/nfsd-4.11-1

The restriction of NFSv4 to TCP went overboard and also broke the
backchannel; fix.  Also some minor refinements to the nfsd
version-setting interface that we'd like to get fixed before release.

--b.

Chuck Lever (1):
  svcrdma: set XPT_CONG_CTRL flag for bc xprt

Kinglong Mee (2):
  SUNRPC/backchanel: set XPT_CONG_CTRL flag for bc xprt
  nfsd: map the ENOKEY to nfserr_perm for avoiding warning

NeilBrown (3):
  NFSD: further refinement of content of /proc/fs/nfsd/versions
  NFSD: fix nfsd_minorversion(.., NFSD_AVAIL)
  NFSD: fix nfsd_reset_versions for NFSv4.

 fs/nfsd/nfsctl.c | 43 
 fs/nfsd/nfsproc.c|  1 +
 fs/nfsd/nfssvc.c | 28 ++---
 net/sunrpc/svcsock.c |  1 +
 net/sunrpc/xprtrdma/svc_rdma_transport.c |  1 +
 5 files changed, 49 insertions(+), 25 deletions(-)

Re: [PATCH] dm ioctl: Remove double parentheses

[Matthias Kaehlcke]

El Thu, Mar 16, 2017 at 09:48:30AM -0700 Matthias Kaehlcke ha dit:

> The extra pair of parantheses is not needed and causes clang to generate
> the following warning:
> 
> drivers/md/dm-ioctl.c:1776:11: error: equality comparison with extraneous 
> parentheses [-Werror,-Wparentheses-equality]
> if ((cmd == DM_DEV_CREATE_CMD)) {
>  ^~~~
> drivers/md/dm-ioctl.c:1776:11: note: remove extraneous parentheses around the 
> comparison to silence this warning
> if ((cmd == DM_DEV_CREATE_CMD)) {
> ~^   ~
> drivers/md/dm-ioctl.c:1776:11: note: use '=' to turn this equality comparison 
> into an assignment
> if ((cmd == DM_DEV_CREATE_CMD)) {
>  ^~
>  =
> 
> Also remove another double parentheses that don't cause a warning.
> 
> Signed-off-by: Matthias Kaehlcke 
> ---
>  drivers/md/dm-ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index a5a9b17f0f7f..9360036bf6d3 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1777,12 +1777,12 @@ static int validate_params(uint cmd, struct dm_ioctl 
> *param)
>   cmd == DM_LIST_VERSIONS_CMD)
>   return 0;
>  
> - if ((cmd == DM_DEV_CREATE_CMD)) {
> + if (cmd == DM_DEV_CREATE_CMD) {
>   if (!*param->name) {
>   DMWARN("name not supplied when creating device");
>   return -EINVAL;
>   }
> - } else if ((*param->uuid && *param->name)) {
> + } else if (*param->uuid && *param->name) {
>   DMWARN("only supply one of name or uuid, cmd(%u)", cmd);
>   return -EINVAL;
>   }

Ping? Any feedback on this patch?

Cheers

Matthias

Re: [PATCH] dm ioctl: Remove double parentheses

[Matthias Kaehlcke]

El Thu, Mar 16, 2017 at 09:48:30AM -0700 Matthias Kaehlcke ha dit:

> The extra pair of parantheses is not needed and causes clang to generate
> the following warning:
> 
> drivers/md/dm-ioctl.c:1776:11: error: equality comparison with extraneous 
> parentheses [-Werror,-Wparentheses-equality]
> if ((cmd == DM_DEV_CREATE_CMD)) {
>  ^~~~
> drivers/md/dm-ioctl.c:1776:11: note: remove extraneous parentheses around the 
> comparison to silence this warning
> if ((cmd == DM_DEV_CREATE_CMD)) {
> ~^   ~
> drivers/md/dm-ioctl.c:1776:11: note: use '=' to turn this equality comparison 
> into an assignment
> if ((cmd == DM_DEV_CREATE_CMD)) {
>  ^~
>  =
> 
> Also remove another double parentheses that don't cause a warning.
> 
> Signed-off-by: Matthias Kaehlcke 
> ---
>  drivers/md/dm-ioctl.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/md/dm-ioctl.c b/drivers/md/dm-ioctl.c
> index a5a9b17f0f7f..9360036bf6d3 100644
> --- a/drivers/md/dm-ioctl.c
> +++ b/drivers/md/dm-ioctl.c
> @@ -1777,12 +1777,12 @@ static int validate_params(uint cmd, struct dm_ioctl 
> *param)
>   cmd == DM_LIST_VERSIONS_CMD)
>   return 0;
>  
> - if ((cmd == DM_DEV_CREATE_CMD)) {
> + if (cmd == DM_DEV_CREATE_CMD) {
>   if (!*param->name) {
>   DMWARN("name not supplied when creating device");
>   return -EINVAL;
>   }
> - } else if ((*param->uuid && *param->name)) {
> + } else if (*param->uuid && *param->name) {
>   DMWARN("only supply one of name or uuid, cmd(%u)", cmd);
>   return -EINVAL;
>   }

Ping? Any feedback on this patch?

Cheers

Matthias