Re: [PATCH] uapi: linux: acct: Remove redundant type comp2_t from kernel
On 10/22/16 06:53, Chen Gang wrote: > > On 10/21/16 11:41, Andrew Morton wrote: >> On Wed, 5 Oct 2016 21:40:10 +0800 cheng...@emindsoft.com.cn wrote: >> >>> In api itself, kernel does not use it -- it is divided into ac_etime_hi >>> and ac_etime_lo. So kernel side only need generate the correct >>> ac_etime_hi and ac_etime_lo, but need not know about comp2_t. >>> >>> At present, kernel use normal u64 type for it, when kernel provdes it to >>> outside, kernel can translate it into ac_etime_hi and ac_etime_lo, >>> directly, but need not notice about comp2_t, in fact. >> >> hm. Why is this an improvement? >> > > For me, it will let code a little more understanding, a little simpler, > and let the code a little more extendable (when kernel members really > needs comp2_t in future, they need not have to treat it as __u32). > > Only when comp2_t is really used in api header in future, kernel has to > know about it, but kernel still can keep original code no touch. So for > me, our changing is harmless. > Oh sorry, for "Only when comp2_t is really used in api header in future", we may need encode_comp2_t, but in kernel wide, this changing is very small. At present, only encode_comp2_t uses comp2_t, and it is only called by fill_ac in an area, and the goal of fill_ac is to encode etime to ac ( comp2_t is the intermediate generation). And I guess, we have very small chance to use comp2_t in uapi header in future, so now, encode_comp2_t can be removed, when we really need it, we can revert to encode_comp2_t and let encode_ac_etime_hilo call it. Thanks -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] uapi: linux: acct: Remove redundant type comp2_t from kernel
On 10/22/16 06:53, Chen Gang wrote: > > On 10/21/16 11:41, Andrew Morton wrote: >> On Wed, 5 Oct 2016 21:40:10 +0800 cheng...@emindsoft.com.cn wrote: >> >>> In api itself, kernel does not use it -- it is divided into ac_etime_hi >>> and ac_etime_lo. So kernel side only need generate the correct >>> ac_etime_hi and ac_etime_lo, but need not know about comp2_t. >>> >>> At present, kernel use normal u64 type for it, when kernel provdes it to >>> outside, kernel can translate it into ac_etime_hi and ac_etime_lo, >>> directly, but need not notice about comp2_t, in fact. >> >> hm. Why is this an improvement? >> > > For me, it will let code a little more understanding, a little simpler, > and let the code a little more extendable (when kernel members really > needs comp2_t in future, they need not have to treat it as __u32). > > Only when comp2_t is really used in api header in future, kernel has to > know about it, but kernel still can keep original code no touch. So for > me, our changing is harmless. > Oh sorry, for "Only when comp2_t is really used in api header in future", we may need encode_comp2_t, but in kernel wide, this changing is very small. At present, only encode_comp2_t uses comp2_t, and it is only called by fill_ac in an area, and the goal of fill_ac is to encode etime to ac ( comp2_t is the intermediate generation). And I guess, we have very small chance to use comp2_t in uapi header in future, so now, encode_comp2_t can be removed, when we really need it, we can revert to encode_comp2_t and let encode_ac_etime_hilo call it. Thanks -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] uapi: linux: acct: Remove redundant type comp2_t from kernel
On 10/21/16 11:41, Andrew Morton wrote: > On Wed, 5 Oct 2016 21:40:10 +0800 cheng...@emindsoft.com.cn wrote: > >> In api itself, kernel does not use it -- it is divided into ac_etime_hi >> and ac_etime_lo. So kernel side only need generate the correct >> ac_etime_hi and ac_etime_lo, but need not know about comp2_t. >> >> At present, kernel use normal u64 type for it, when kernel provdes it to >> outside, kernel can translate it into ac_etime_hi and ac_etime_lo, >> directly, but need not notice about comp2_t, in fact. > > hm. Why is this an improvement? > For me, it will let code a little more understanding, a little simpler, and let the code a little more extendable (when kernel members really needs comp2_t in future, they need not have to treat it as __u32). Only when comp2_t is really used in api header in future, kernel has to know about it, but kernel still can keep original code no touch. So for me, our changing is harmless. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] uapi: linux: acct: Remove redundant type comp2_t from kernel
On 10/21/16 11:41, Andrew Morton wrote: > On Wed, 5 Oct 2016 21:40:10 +0800 cheng...@emindsoft.com.cn wrote: > >> In api itself, kernel does not use it -- it is divided into ac_etime_hi >> and ac_etime_lo. So kernel side only need generate the correct >> ac_etime_hi and ac_etime_lo, but need not know about comp2_t. >> >> At present, kernel use normal u64 type for it, when kernel provdes it to >> outside, kernel can translate it into ac_etime_hi and ac_etime_lo, >> directly, but need not notice about comp2_t, in fact. > > hm. Why is this an improvement? > For me, it will let code a little more understanding, a little simpler, and let the code a little more extendable (when kernel members really needs comp2_t in future, they need not have to treat it as __u32). Only when comp2_t is really used in api header in future, kernel has to know about it, but kernel still can keep original code no touch. So for me, our changing is harmless. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] uapi: linux: acct: Remove redundant type comp2_t from kernel
Hello:
For me if necessary, can add additional comments for it:
If kernel will really need comp2_t in the future (I guess not), kernel
can define it as a union with structures (need not must be the same as
the user mode), which will be easier for kernel using. e.g.
#pragma push(1)
typedef union {
#if ACCT_BYTEORDER == 0x80 /* big endian */
struct {
__u8 pad0;
__u8 ac_etime_hi;
__u16 ac_etime_lo;
};
struct {
__u32 pad1 : 8;
__u32 exp : 5;
__u32 mant : 19;
};
#else
struct {
__u16 ac_etime_lo;
__u8 ac_etime_hi;
};
struct {
__u32 mant : 19;
__u32 exp : 5;
};
#endif
__u32 v;
} comp2_t;
#pragma pop()
Some members may be not interested in bits definition (mant, exp), I
guess, it is because of performance reason (but I am not quite sure).
But ac_etime_lo and ac_etime_hi are useful.
Although this definition may be better than u32, we can not change the
uapi -- which may cause user mode application generate building break,
e.g. some informal usage -- use u32 directly for shift operations.
Thanks.
On 10/5/16 21:40, cheng...@emindsoft.com.cn wrote:
> From: Chen Gang <cheng...@emindsoft.com.cn>
>
> In api itself, kernel does not use it -- it is divided into ac_etime_hi
> and ac_etime_lo. So kernel side only need generate the correct
> ac_etime_hi and ac_etime_lo, but need not know about comp2_t.
>
> At present, kernel use normal u64 type for it, when kernel provdes it to
> outside, kernel can translate it into ac_etime_hi and ac_etime_lo,
> directly, but need not notice about comp2_t, in fact.
>
> The patch notices about coding styles:
>
> - Use 1 instead of 1ul, since type int is enough.
>
> - Use white space between operator and constant or macro.
>
> - Initialize variables directly, when declare it, since they need be
>initialized and can be initialized by constant (include constant
>macros).
>
> - Remove redundant empty line.
>
> Signed-off-by: Chen Gang <gang.chen.5...@gmail.com>
> ---
> include/uapi/linux/acct.h | 6 --
> kernel/acct.c | 31 ++-
> 2 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/include/uapi/linux/acct.h b/include/uapi/linux/acct.h
> index df2f9a0..97acdd4 100644
> --- a/include/uapi/linux/acct.h
> +++ b/include/uapi/linux/acct.h
> @@ -24,12 +24,15 @@
> * comp_t is a 16-bit "floating" point number with a 3-bit base 8
> * exponent and a 13-bit fraction.
> * comp2_t is 24-bit with 5-bit base 2 exponent and 20 bit fraction
> - * (leading 1 not stored).
> + * (leading 1 not stored). And it is described as ac_etime_hi and
> + * ac_etime_lo in kernel.
> * See linux/kernel/acct.c for the specific encoding systems used.
> */
>
> typedef __u16comp_t;
> +#ifndef __KERNEL__
> typedef __u32comp2_t;
> +#endif /* __KERNEL */
>
> /*
> * accounting file record
> @@ -120,5 +123,4 @@ struct acct_v3
> #define AHZ (HZ)
> #endif /* __KERNEL */
>
> -
> #endif /* _UAPI_LINUX_ACCT_H */
> diff --git a/kernel/acct.c b/kernel/acct.c
> index 74963d1..f707a10 100644
> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -338,7 +338,7 @@ static comp_t encode_comp_t(unsigned long value)
>
> #if ACCT_VERSION == 1 || ACCT_VERSION == 2
> /*
> - * encode an u64 into a comp2_t (24 bits)
> + * encode an u64 into ac_etime_hi and ac_etime_lo (24 bits)
> *
> * Format: 5 bit base 2 exponent, 20 bits mantissa.
> * The leading bit of the mantissa is not stored, but implied for
> @@ -348,15 +348,15 @@ static comp_t encode_comp_t(unsigned long value)
>
> #define MANTSIZE2 20 /* 20 bit mantissa. */
> #define EXPSIZE25 /* 5 bit base 2 exponent. */
> -#define MAXFRACT2 ((1ul << MANTSIZE2) - 1) /* Maximum fractional
> value. */
> -#define MAXEXP2 ((1 << EXPSIZE2) - 1)/* Maximum exponent. */
> +#define MAXFRACT2 ((1 << MANTSIZE2) - 1) /* Maximum fractional value.
> */
> +#define MAXEXP2 ((1 << EXPSIZE2) - 1) /* Maximum exponent. */
>
> -static comp2_t encode_comp2_t(u64 value)
> +static void encode_ac_etime_hilo(u64 value, acct_t *ac)
> {
> - int exp, rnd;
> + int exp = (value > (MAXFRACT2 >> 1));
> + int rn
Re: [PATCH] uapi: linux: acct: Remove redundant type comp2_t from kernel
Hello:
For me if necessary, can add additional comments for it:
If kernel will really need comp2_t in the future (I guess not), kernel
can define it as a union with structures (need not must be the same as
the user mode), which will be easier for kernel using. e.g.
#pragma push(1)
typedef union {
#if ACCT_BYTEORDER == 0x80 /* big endian */
struct {
__u8 pad0;
__u8 ac_etime_hi;
__u16 ac_etime_lo;
};
struct {
__u32 pad1 : 8;
__u32 exp : 5;
__u32 mant : 19;
};
#else
struct {
__u16 ac_etime_lo;
__u8 ac_etime_hi;
};
struct {
__u32 mant : 19;
__u32 exp : 5;
};
#endif
__u32 v;
} comp2_t;
#pragma pop()
Some members may be not interested in bits definition (mant, exp), I
guess, it is because of performance reason (but I am not quite sure).
But ac_etime_lo and ac_etime_hi are useful.
Although this definition may be better than u32, we can not change the
uapi -- which may cause user mode application generate building break,
e.g. some informal usage -- use u32 directly for shift operations.
Thanks.
On 10/5/16 21:40, cheng...@emindsoft.com.cn wrote:
> From: Chen Gang
>
> In api itself, kernel does not use it -- it is divided into ac_etime_hi
> and ac_etime_lo. So kernel side only need generate the correct
> ac_etime_hi and ac_etime_lo, but need not know about comp2_t.
>
> At present, kernel use normal u64 type for it, when kernel provdes it to
> outside, kernel can translate it into ac_etime_hi and ac_etime_lo,
> directly, but need not notice about comp2_t, in fact.
>
> The patch notices about coding styles:
>
> - Use 1 instead of 1ul, since type int is enough.
>
> - Use white space between operator and constant or macro.
>
> - Initialize variables directly, when declare it, since they need be
>initialized and can be initialized by constant (include constant
>macros).
>
> - Remove redundant empty line.
>
> Signed-off-by: Chen Gang
> ---
> include/uapi/linux/acct.h | 6 --
> kernel/acct.c | 31 ++-
> 2 files changed, 18 insertions(+), 19 deletions(-)
>
> diff --git a/include/uapi/linux/acct.h b/include/uapi/linux/acct.h
> index df2f9a0..97acdd4 100644
> --- a/include/uapi/linux/acct.h
> +++ b/include/uapi/linux/acct.h
> @@ -24,12 +24,15 @@
> * comp_t is a 16-bit "floating" point number with a 3-bit base 8
> * exponent and a 13-bit fraction.
> * comp2_t is 24-bit with 5-bit base 2 exponent and 20 bit fraction
> - * (leading 1 not stored).
> + * (leading 1 not stored). And it is described as ac_etime_hi and
> + * ac_etime_lo in kernel.
> * See linux/kernel/acct.c for the specific encoding systems used.
> */
>
> typedef __u16comp_t;
> +#ifndef __KERNEL__
> typedef __u32comp2_t;
> +#endif /* __KERNEL */
>
> /*
> * accounting file record
> @@ -120,5 +123,4 @@ struct acct_v3
> #define AHZ (HZ)
> #endif /* __KERNEL */
>
> -
> #endif /* _UAPI_LINUX_ACCT_H */
> diff --git a/kernel/acct.c b/kernel/acct.c
> index 74963d1..f707a10 100644
> --- a/kernel/acct.c
> +++ b/kernel/acct.c
> @@ -338,7 +338,7 @@ static comp_t encode_comp_t(unsigned long value)
>
> #if ACCT_VERSION == 1 || ACCT_VERSION == 2
> /*
> - * encode an u64 into a comp2_t (24 bits)
> + * encode an u64 into ac_etime_hi and ac_etime_lo (24 bits)
> *
> * Format: 5 bit base 2 exponent, 20 bits mantissa.
> * The leading bit of the mantissa is not stored, but implied for
> @@ -348,15 +348,15 @@ static comp_t encode_comp_t(unsigned long value)
>
> #define MANTSIZE2 20 /* 20 bit mantissa. */
> #define EXPSIZE25 /* 5 bit base 2 exponent. */
> -#define MAXFRACT2 ((1ul << MANTSIZE2) - 1) /* Maximum fractional
> value. */
> -#define MAXEXP2 ((1 << EXPSIZE2) - 1)/* Maximum exponent. */
> +#define MAXFRACT2 ((1 << MANTSIZE2) - 1) /* Maximum fractional value.
> */
> +#define MAXEXP2 ((1 << EXPSIZE2) - 1) /* Maximum exponent. */
>
> -static comp2_t encode_comp2_t(u64 value)
> +static void encode_ac_etime_hilo(u64 value, acct_t *ac)
> {
> - int exp, rnd;
> + int exp = (value > (MAXFRACT2 >> 1));
> + int rnd = 0;
> + u32 etime;
>
> - exp = (val
Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions
Firstly, excuse me for replying late -- since I also agree, this patch is not urgent ;-) On 9/21/16 16:11, Michal Hocko wrote: > On Wed 21-09-16 06:06:44, Chen Gang wrote: >> On 9/20/16 16:09, Michal Hocko wrote: > [...] > > skipping the large part of the email because I do not have a spare time > to discuss this. > I agree, they are not urgent, so if we have no time on it, just leave it. But for me, they are still important (not urgent != not important), so every member can continue to discuss about it, when he/she have time, e.g. Do we have another better solving way for this issue? >>> So what is the point of this whole exercise? Do not take me wrong, this >>> area could see some improvements but I believe that doing int->bool >>> change is not just the right thing to do and worth spending both your >>> and reviewers time. >>> >> >> I am not quite sure about that. > > Maybe you should listen to the feedback your are getting. I do not think > I am not the first one here. > OK, for me, normally, when a mailing list contents 100+ members, every feedback has not only one member (especially, we have about 10K members). > Look, MM surely needs some man power. There are issues to be solved, > patches to review. Doing the cleanups is really nice but there are more > serious problems to solve first. OK, we really need a task management, for me, we need notice about the urgent and important. If the patch or issue is either urgent nor important, we can just drop it. If they are not urgent, but still important, just discuss about it when have time, but do not forget it (I guess, quite a few of volunteers can not for urgent things -- their time resources are not stable, e.g. me). > If you want to help then starting > with review would be much much more helpful and hugely appreciated. We > are really lacking people there a _lot_. I guess, I can try (at least, I want to try). But excuse me, in honest, I am not quite familiar with mm, and my time resources are not stable enough, either. So I am not quite sure I can do. > Just generating more work for > reviewers with something that doesn't make any real difference in the > runtime is far less helpful IMHO. > For urgent things, really it is less helpful (in fact, it will generate negative effect). But if it is related with important things, we need discuss about it when we have time (do not treat it as urgent thing). For me, all issues in public header files are important, at least. When a developer want to put or modify something in public header files, they need think more -- since the members outside of mm may see them. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions
Firstly, excuse me for replying late -- since I also agree, this patch is not urgent ;-) On 9/21/16 16:11, Michal Hocko wrote: > On Wed 21-09-16 06:06:44, Chen Gang wrote: >> On 9/20/16 16:09, Michal Hocko wrote: > [...] > > skipping the large part of the email because I do not have a spare time > to discuss this. > I agree, they are not urgent, so if we have no time on it, just leave it. But for me, they are still important (not urgent != not important), so every member can continue to discuss about it, when he/she have time, e.g. Do we have another better solving way for this issue? >>> So what is the point of this whole exercise? Do not take me wrong, this >>> area could see some improvements but I believe that doing int->bool >>> change is not just the right thing to do and worth spending both your >>> and reviewers time. >>> >> >> I am not quite sure about that. > > Maybe you should listen to the feedback your are getting. I do not think > I am not the first one here. > OK, for me, normally, when a mailing list contents 100+ members, every feedback has not only one member (especially, we have about 10K members). > Look, MM surely needs some man power. There are issues to be solved, > patches to review. Doing the cleanups is really nice but there are more > serious problems to solve first. OK, we really need a task management, for me, we need notice about the urgent and important. If the patch or issue is either urgent nor important, we can just drop it. If they are not urgent, but still important, just discuss about it when have time, but do not forget it (I guess, quite a few of volunteers can not for urgent things -- their time resources are not stable, e.g. me). > If you want to help then starting > with review would be much much more helpful and hugely appreciated. We > are really lacking people there a _lot_. I guess, I can try (at least, I want to try). But excuse me, in honest, I am not quite familiar with mm, and my time resources are not stable enough, either. So I am not quite sure I can do. > Just generating more work for > reviewers with something that doesn't make any real difference in the > runtime is far less helpful IMHO. > For urgent things, really it is less helpful (in fact, it will generate negative effect). But if it is related with important things, we need discuss about it when we have time (do not treat it as urgent thing). For me, all issues in public header files are important, at least. When a developer want to put or modify something in public header files, they need think more -- since the members outside of mm may see them. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions
On 9/20/16 16:09, Michal Hocko wrote: > On Tue 20-09-16 05:46:58, Chen Gang wrote: >> >> For me, it really need return false: >> >> - For real implementation, when do nothing, it will return false. >> >> - I assume that the input page already is in a node (although maybe my >>assumption incorrect), and migrate to the same node. When the real >>implementation fails (e.g. -EAGAIN 10 times), it still returns false. >> >> - Original dummy implementation always return -EAGAIN, And -EAGAIN in >>real implementation will trigger returning false, after 10 times. >> >> - After grep TNF_MIGRATE_FAIL and TNF_MIGRATED, we only use them in >>task_numa_fault in kernel/sched/fair.c for numa_pages_migrated and >>numa_faults_locality, I guess they are only used for statistics. >> >> So for me the dummy implementation need return false instead of -EAGAIN. > > I see that the return value semantic might be really confusing. But I am > not sure why bool would make it all of the sudden any less confusing. > migrate_page returns -EAGAIN on failure and 0 on success, migrate_pages > returns -EAGAIN or number of not migrated pages on failure and 0 on > success. So migrate_misplaced_page doesn't fit into this mode with the > bool return value. So I would argue that the code is not any better. > I guess, numamigrate_isolate_page can be bool, at least. And yes, commonly, bool functions are for asking something, and int functions are for doing something, but not must be. When the caller care about success, but never care about every failure details, bool is OK. In our case, for me, numa balancing is for performance. When return failure, the system has no any negative effect -- only lose a chance for improving performance. - For user, the failure times statistics is enough, they need not care about every failure details. - For tracer, the failure details statistics are meaningfulness, but focusing on each failure details is meaningless. Now, it finishes a part of failure details statistics -- which can be improved next. - For debugger (or printing log), focusing on each failure details is useful. But debugger already can check every details, returning every failure details is still a little helpful, but not necessary. >> >> If our original implementation already used bool, our this issue (return >> -EAGAIN) would be avoided (compiler would help us to find this issue). > > OK, so you pushed me to look into it deeper and the fact is that > migrate_misplaced_page return value doesn't matter at all for > CONFIG_NUMA_BALANCING=n because task_numa_fault is noop for that > configuration. Moreover the whole do_numa_page should never execute with > that configuration because we will not have numa pte_protnone() ptes in > that path. do_huge_pmd_numa_page seems be in a similar case. So this > doesn't have any real impact on the runtime AFAICS. > OK, thanks. > So what is the point of this whole exercise? Do not take me wrong, this > area could see some improvements but I believe that doing int->bool > change is not just the right thing to do and worth spending both your > and reviewers time. > I am not quite sure about that. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions
On 9/20/16 16:09, Michal Hocko wrote: > On Tue 20-09-16 05:46:58, Chen Gang wrote: >> >> For me, it really need return false: >> >> - For real implementation, when do nothing, it will return false. >> >> - I assume that the input page already is in a node (although maybe my >>assumption incorrect), and migrate to the same node. When the real >>implementation fails (e.g. -EAGAIN 10 times), it still returns false. >> >> - Original dummy implementation always return -EAGAIN, And -EAGAIN in >>real implementation will trigger returning false, after 10 times. >> >> - After grep TNF_MIGRATE_FAIL and TNF_MIGRATED, we only use them in >>task_numa_fault in kernel/sched/fair.c for numa_pages_migrated and >>numa_faults_locality, I guess they are only used for statistics. >> >> So for me the dummy implementation need return false instead of -EAGAIN. > > I see that the return value semantic might be really confusing. But I am > not sure why bool would make it all of the sudden any less confusing. > migrate_page returns -EAGAIN on failure and 0 on success, migrate_pages > returns -EAGAIN or number of not migrated pages on failure and 0 on > success. So migrate_misplaced_page doesn't fit into this mode with the > bool return value. So I would argue that the code is not any better. > I guess, numamigrate_isolate_page can be bool, at least. And yes, commonly, bool functions are for asking something, and int functions are for doing something, but not must be. When the caller care about success, but never care about every failure details, bool is OK. In our case, for me, numa balancing is for performance. When return failure, the system has no any negative effect -- only lose a chance for improving performance. - For user, the failure times statistics is enough, they need not care about every failure details. - For tracer, the failure details statistics are meaningfulness, but focusing on each failure details is meaningless. Now, it finishes a part of failure details statistics -- which can be improved next. - For debugger (or printing log), focusing on each failure details is useful. But debugger already can check every details, returning every failure details is still a little helpful, but not necessary. >> >> If our original implementation already used bool, our this issue (return >> -EAGAIN) would be avoided (compiler would help us to find this issue). > > OK, so you pushed me to look into it deeper and the fact is that > migrate_misplaced_page return value doesn't matter at all for > CONFIG_NUMA_BALANCING=n because task_numa_fault is noop for that > configuration. Moreover the whole do_numa_page should never execute with > that configuration because we will not have numa pte_protnone() ptes in > that path. do_huge_pmd_numa_page seems be in a similar case. So this > doesn't have any real impact on the runtime AFAICS. > OK, thanks. > So what is the point of this whole exercise? Do not take me wrong, this > area could see some improvements but I believe that doing int->bool > change is not just the right thing to do and worth spending both your > and reviewers time. > I am not quite sure about that. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions
On 9/17/16 23:46, Michal Hocko wrote:
> On Sat 17-09-16 15:20:36, cheng...@emindsoft.com.cn wrote:
>
>> Also change their related pure Boolean function numamigrate_isolate_page.
>
> this is not true. Just look at the current usage
>
> migrated = migrate_misplaced_page(page, vma, target_nid);
> if (migrated) {
> page_nid = target_nid;
> flags |= TNF_MIGRATED;
> } else
> flags |= TNF_MIGRATE_FAIL;
>
> and now take your change which changes -EAGAIN into false. See the
> difference? Now I didn't even try to understand why
> CONFIG_NUMA_BALANCING=n pretends a success but then in order to keep the
> current semantic your patch should return true in that path. So NAK from
> me until you either explain why this is OK or change it.
>
For me, it really need return false:
- For real implementation, when do nothing, it will return false.
- I assume that the input page already is in a node (although maybe my
assumption incorrect), and migrate to the same node. When the real
implementation fails (e.g. -EAGAIN 10 times), it still returns false.
- Original dummy implementation always return -EAGAIN, And -EAGAIN in
real implementation will trigger returning false, after 10 times.
- After grep TNF_MIGRATE_FAIL and TNF_MIGRATED, we only use them in
task_numa_fault in kernel/sched/fair.c for numa_pages_migrated and
numa_faults_locality, I guess they are only used for statistics.
So for me the dummy implementation need return false instead of -EAGAIN.
> But to be honest I am not keen of this int -> bool changes much.
> Especially if they are bringing a risk of subtle behavior change like
> this patch. And without a good changelog explaining why this makes
> sense.
>
If our original implementation already used bool, our this issue (return
-EAGAIN) would be avoided (compiler would help us to find this issue).
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Return false instead of -EAGAIN for dummy functions
On 9/17/16 23:46, Michal Hocko wrote:
> On Sat 17-09-16 15:20:36, cheng...@emindsoft.com.cn wrote:
>
>> Also change their related pure Boolean function numamigrate_isolate_page.
>
> this is not true. Just look at the current usage
>
> migrated = migrate_misplaced_page(page, vma, target_nid);
> if (migrated) {
> page_nid = target_nid;
> flags |= TNF_MIGRATED;
> } else
> flags |= TNF_MIGRATE_FAIL;
>
> and now take your change which changes -EAGAIN into false. See the
> difference? Now I didn't even try to understand why
> CONFIG_NUMA_BALANCING=n pretends a success but then in order to keep the
> current semantic your patch should return true in that path. So NAK from
> me until you either explain why this is OK or change it.
>
For me, it really need return false:
- For real implementation, when do nothing, it will return false.
- I assume that the input page already is in a node (although maybe my
assumption incorrect), and migrate to the same node. When the real
implementation fails (e.g. -EAGAIN 10 times), it still returns false.
- Original dummy implementation always return -EAGAIN, And -EAGAIN in
real implementation will trigger returning false, after 10 times.
- After grep TNF_MIGRATE_FAIL and TNF_MIGRATED, we only use them in
task_numa_fault in kernel/sched/fair.c for numa_pages_migrated and
numa_faults_locality, I guess they are only used for statistics.
So for me the dummy implementation need return false instead of -EAGAIN.
> But to be honest I am not keen of this int -> bool changes much.
> Especially if they are bringing a risk of subtle behavior change like
> this patch. And without a good changelog explaining why this makes
> sense.
>
If our original implementation already used bool, our this issue (return
-EAGAIN) would be avoided (compiler would help us to find this issue).
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH v2] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
If necessary to send patch v3 for it, please let me know (I guess not). Next, I shall try to find and make another patches for our kernel. Thanks. On 9/7/16 23:39, Chen Gang wrote: > > Thank you all for your work. Commonly, I should send patch v3 for it. > > And very sorry for replying too late. During these days I have no > enough time on it (working, buying house, and catching a cold, but I am > lucky enough that my father's health is OK). > > Thanks. > > On 9/7/16 09:52, Fengguang Wu wrote: >> Hi Andrew, >> >> On Tue, Sep 06, 2016 at 12:27:32PM -0700, Andrew Morton wrote: >>> On Sun, 4 Sep 2016 08:27:36 +0800 kbuild test robot <l...@intel.com> wrote: >>> >>>> [auto build test ERROR on linus/master] >>>> [also build test ERROR on v4.8-rc4 next-20160825] >>>> [if your patch is applied to the wrong git tree, please drop us a note to >>>> help improve the system] >>>> [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto >>>> for convenience) to record what (public, well-known) commit your patch >>>> series was built on] >>>> [Check https://git-scm.com/docs/git-format-patch for more information] >>>> >>>> url: >>>> https://github.com/0day-ci/linux/commits/chengang-emindsoft-com-cn/arch-all-include-asm-bitops-Use-bool-instead-of-int-for-all-bit-test-functions/20160828-230301 >>>> config: s390-default_defconfig (attached as .config) >>>> compiler: s390x-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609 >>>> reproduce: >>>> wget >>>> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross >>>> -O ~/bin/make.cross >>>> chmod +x ~/bin/make.cross >>>> # save the attached .config to linux build tree >>>> make.cross ARCH=s390 >>>> >>>> All errors (new ones prefixed by >>): >>>> >>>>In file included from include/linux/bitops.h:36:0, >>>> from fs/btrfs/extent_io.c:1: >>>>>> arch/s390/include/asm/bitops.h:176:15: error: unknown type name 'bool' >>>> static inline bool >>>> ^ >>>>arch/s390/include/asm/bitops.h:187:15: error: unknown type name 'bool' >>>> static inline bool >>> >>> My s390 cross compiler doesn't like that config. Can someone test this? >> >> Tested-by: Fengguang Wu <fengguang...@intel.com> >> >> It works fine with Debian's gcc-6-s390x-linux-gnu and crosstool gcc: >> >> https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/4.9.0/x86_64-gcc-4.9.0-nolibc_s390x-linux.tar.xz >> >>> --- a/arch/s390/include/asm/bitops.h~a >>> +++ a/arch/s390/include/asm/bitops.h >>> @@ -40,6 +40,7 @@ >>> #error only can be included directly >>> #endif >>> >>> +#include >>> #include >>> #include >>> #include >> >> Regards, >> Fengguang > -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH v2] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
If necessary to send patch v3 for it, please let me know (I guess not). Next, I shall try to find and make another patches for our kernel. Thanks. On 9/7/16 23:39, Chen Gang wrote: > > Thank you all for your work. Commonly, I should send patch v3 for it. > > And very sorry for replying too late. During these days I have no > enough time on it (working, buying house, and catching a cold, but I am > lucky enough that my father's health is OK). > > Thanks. > > On 9/7/16 09:52, Fengguang Wu wrote: >> Hi Andrew, >> >> On Tue, Sep 06, 2016 at 12:27:32PM -0700, Andrew Morton wrote: >>> On Sun, 4 Sep 2016 08:27:36 +0800 kbuild test robot wrote: >>> >>>> [auto build test ERROR on linus/master] >>>> [also build test ERROR on v4.8-rc4 next-20160825] >>>> [if your patch is applied to the wrong git tree, please drop us a note to >>>> help improve the system] >>>> [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto >>>> for convenience) to record what (public, well-known) commit your patch >>>> series was built on] >>>> [Check https://git-scm.com/docs/git-format-patch for more information] >>>> >>>> url: >>>> https://github.com/0day-ci/linux/commits/chengang-emindsoft-com-cn/arch-all-include-asm-bitops-Use-bool-instead-of-int-for-all-bit-test-functions/20160828-230301 >>>> config: s390-default_defconfig (attached as .config) >>>> compiler: s390x-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609 >>>> reproduce: >>>> wget >>>> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross >>>> -O ~/bin/make.cross >>>> chmod +x ~/bin/make.cross >>>> # save the attached .config to linux build tree >>>> make.cross ARCH=s390 >>>> >>>> All errors (new ones prefixed by >>): >>>> >>>>In file included from include/linux/bitops.h:36:0, >>>> from fs/btrfs/extent_io.c:1: >>>>>> arch/s390/include/asm/bitops.h:176:15: error: unknown type name 'bool' >>>> static inline bool >>>> ^ >>>>arch/s390/include/asm/bitops.h:187:15: error: unknown type name 'bool' >>>> static inline bool >>> >>> My s390 cross compiler doesn't like that config. Can someone test this? >> >> Tested-by: Fengguang Wu >> >> It works fine with Debian's gcc-6-s390x-linux-gnu and crosstool gcc: >> >> https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/4.9.0/x86_64-gcc-4.9.0-nolibc_s390x-linux.tar.xz >> >>> --- a/arch/s390/include/asm/bitops.h~a >>> +++ a/arch/s390/include/asm/bitops.h >>> @@ -40,6 +40,7 @@ >>> #error only can be included directly >>> #endif >>> >>> +#include >>> #include >>> #include >>> #include >> >> Regards, >> Fengguang > -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)
On 9/4/16 09:01, Al Viro wrote: > On Sun, Sep 04, 2016 at 06:36:56AM +0800, Chen Gang wrote: > >> And for all: shall I provide the proof for another archs? >> >> For me, Boolean gives additional chance to compiler to improve the code. > > Whereas for compiler it gives nothing. Not in those cases. > >> If the compiler can not improve the code, it can treat it as int simply. >> So theoretically, at least, Boolean should not be worse than int. > > Except for pointless code churn and pandering to irrational beliefs, that > is... For me, it is not quite suitable to get conclusion during discussing. > Please, RTFISO9899 and learn the semantics of _Bool; it's not that > complicated. > Start with 6.2.5[2,6] and 6.3.1.2, then look through 6.8.4 and 6.8.5 to > figure out the semantics of conditions in if/while/for. Note also 6.5.8, > 6.5.9, 6.5.13 and 6.5.14 and observe that type of (x > 5 && y < 1) is *NOT* > _Bool; it's int. > Yes, what you said above is true to me. But for (x > 5 && y < 1) will return 1 for true and 0 for false, although its' type is still int. So for compiler, it can simply treat it as Boolean type internally. For my original saying, I assume 2 things (excuse me, I did not mention originally): - I assume what I said is for pure Boolean functions, in our case, all functions intend to return 'Boolean' value (0 or 1) in most of archs, although they use int type as return value. - What I said is for compiler's optimization at middle language level and at instruction level (internal processing), not for language definition (interface for outside -- for C developers). For me, one way is: in middle language level, bool can be treated as int or long to be processed firstly, then in instruction level, the compiler performs additional optimization and qualification for bool. - So the compiler has additional chance for optimizing in instruction level. - Since for pure Boolean function, it is already sure that related int value must be 0 or 1, and the compiler should be smart enough to know that, so the output code need not additional qualifications. - So, for me, theoretically, bool is equal or better than int for pure bool functions, unless the compiler has performance bugs. > If you can show any improvement or loss in code generation in this case > (static inline int converted to static inline bool), I would really like to > see the details. As in .config/file/function/gcc version/target architecture. > Optimizer bugs happens, but they should be reported when found, and I would > expect _Bool handling to be _less_ exercised than that of normal logical > expressions, so loss is probably more likely. And yes, it also should be > reported. > At least for x86_64 arch, as far as I know, I can find the case which bool is better than int, but I can not find the case which worse than int. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)
On 9/4/16 09:01, Al Viro wrote: > On Sun, Sep 04, 2016 at 06:36:56AM +0800, Chen Gang wrote: > >> And for all: shall I provide the proof for another archs? >> >> For me, Boolean gives additional chance to compiler to improve the code. > > Whereas for compiler it gives nothing. Not in those cases. > >> If the compiler can not improve the code, it can treat it as int simply. >> So theoretically, at least, Boolean should not be worse than int. > > Except for pointless code churn and pandering to irrational beliefs, that > is... For me, it is not quite suitable to get conclusion during discussing. > Please, RTFISO9899 and learn the semantics of _Bool; it's not that > complicated. > Start with 6.2.5[2,6] and 6.3.1.2, then look through 6.8.4 and 6.8.5 to > figure out the semantics of conditions in if/while/for. Note also 6.5.8, > 6.5.9, 6.5.13 and 6.5.14 and observe that type of (x > 5 && y < 1) is *NOT* > _Bool; it's int. > Yes, what you said above is true to me. But for (x > 5 && y < 1) will return 1 for true and 0 for false, although its' type is still int. So for compiler, it can simply treat it as Boolean type internally. For my original saying, I assume 2 things (excuse me, I did not mention originally): - I assume what I said is for pure Boolean functions, in our case, all functions intend to return 'Boolean' value (0 or 1) in most of archs, although they use int type as return value. - What I said is for compiler's optimization at middle language level and at instruction level (internal processing), not for language definition (interface for outside -- for C developers). For me, one way is: in middle language level, bool can be treated as int or long to be processed firstly, then in instruction level, the compiler performs additional optimization and qualification for bool. - So the compiler has additional chance for optimizing in instruction level. - Since for pure Boolean function, it is already sure that related int value must be 0 or 1, and the compiler should be smart enough to know that, so the output code need not additional qualifications. - So, for me, theoretically, bool is equal or better than int for pure bool functions, unless the compiler has performance bugs. > If you can show any improvement or loss in code generation in this case > (static inline int converted to static inline bool), I would really like to > see the details. As in .config/file/function/gcc version/target architecture. > Optimizer bugs happens, but they should be reported when found, and I would > expect _Bool handling to be _less_ exercised than that of normal logical > expressions, so loss is probably more likely. And yes, it also should be > reported. > At least for x86_64 arch, as far as I know, I can find the case which bool is better than int, but I can not find the case which worse than int. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)
On 9/4/16 09:01, Al Viro wrote: > On Sun, Sep 04, 2016 at 06:36:56AM +0800, Chen Gang wrote: > >> And for all: shall I provide the proof for another archs? >> >> For me, Boolean gives additional chance to compiler to improve the code. > > Whereas for compiler it gives nothing. Not in those cases. > >> If the compiler can not improve the code, it can treat it as int simply. >> So theoretically, at least, Boolean should not be worse than int. > > Except for pointless code churn and pandering to irrational beliefs, that > is... > Please, RTFISO9899 and learn the semantics of _Bool; it's not that > complicated. > Start with 6.2.5[2,6] and 6.3.1.2, then look through 6.8.4 and 6.8.5 to > figure out the semantics of conditions in if/while/for. Note also 6.5.8, > 6.5.9, 6.5.13 and 6.5.14 and observe that type of (x > 5 && y < 1) is *NOT* > _Bool; it's int. > > If you can show any improvement or loss in code generation in this case > (static inline int converted to static inline bool), I would really like to > see the details. As in .config/file/function/gcc version/target architecture. > Optimizer bugs happens, but they should be reported when found, and I would > expect _Bool handling to be _less_ exercised than that of normal logical > expressions, so loss is probably more likely. And yes, it also should be > reported. > Sorry for replying late, and excuse me, I did not read the details more. During these days I have no enough time on it (working, buying house, and catching a cold, but lucky enough that my father's health is OK). I shall try to read the details and analyze it within next weekend (I guess I can not finish within this week end, sorry again for I really have no time during these days). But all together, for me, I guess our discussion can not 'prevent' that bool return value instead of int return value for pure bool function in our kernel. :-) Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)
On 9/4/16 09:01, Al Viro wrote: > On Sun, Sep 04, 2016 at 06:36:56AM +0800, Chen Gang wrote: > >> And for all: shall I provide the proof for another archs? >> >> For me, Boolean gives additional chance to compiler to improve the code. > > Whereas for compiler it gives nothing. Not in those cases. > >> If the compiler can not improve the code, it can treat it as int simply. >> So theoretically, at least, Boolean should not be worse than int. > > Except for pointless code churn and pandering to irrational beliefs, that > is... > Please, RTFISO9899 and learn the semantics of _Bool; it's not that > complicated. > Start with 6.2.5[2,6] and 6.3.1.2, then look through 6.8.4 and 6.8.5 to > figure out the semantics of conditions in if/while/for. Note also 6.5.8, > 6.5.9, 6.5.13 and 6.5.14 and observe that type of (x > 5 && y < 1) is *NOT* > _Bool; it's int. > > If you can show any improvement or loss in code generation in this case > (static inline int converted to static inline bool), I would really like to > see the details. As in .config/file/function/gcc version/target architecture. > Optimizer bugs happens, but they should be reported when found, and I would > expect _Bool handling to be _less_ exercised than that of normal logical > expressions, so loss is probably more likely. And yes, it also should be > reported. > Sorry for replying late, and excuse me, I did not read the details more. During these days I have no enough time on it (working, buying house, and catching a cold, but lucky enough that my father's health is OK). I shall try to read the details and analyze it within next weekend (I guess I can not finish within this week end, sorry again for I really have no time during these days). But all together, for me, I guess our discussion can not 'prevent' that bool return value instead of int return value for pure bool function in our kernel. :-) Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH v2] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
Thank you all for your work. Commonly, I should send patch v3 for it. And very sorry for replying too late. During these days I have no enough time on it (working, buying house, and catching a cold, but I am lucky enough that my father's health is OK). Thanks. On 9/7/16 09:52, Fengguang Wu wrote: > Hi Andrew, > > On Tue, Sep 06, 2016 at 12:27:32PM -0700, Andrew Morton wrote: >> On Sun, 4 Sep 2016 08:27:36 +0800 kbuild test robot <l...@intel.com> wrote: >> >>> [auto build test ERROR on linus/master] >>> [also build test ERROR on v4.8-rc4 next-20160825] >>> [if your patch is applied to the wrong git tree, please drop us a note to >>> help improve the system] >>> [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto >>> for convenience) to record what (public, well-known) commit your patch >>> series was built on] >>> [Check https://git-scm.com/docs/git-format-patch for more information] >>> >>> url: >>> https://github.com/0day-ci/linux/commits/chengang-emindsoft-com-cn/arch-all-include-asm-bitops-Use-bool-instead-of-int-for-all-bit-test-functions/20160828-230301 >>> config: s390-default_defconfig (attached as .config) >>> compiler: s390x-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609 >>> reproduce: >>> wget >>> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross >>> -O ~/bin/make.cross >>> chmod +x ~/bin/make.cross >>> # save the attached .config to linux build tree >>> make.cross ARCH=s390 >>> >>> All errors (new ones prefixed by >>): >>> >>>In file included from include/linux/bitops.h:36:0, >>> from fs/btrfs/extent_io.c:1: >>> >> arch/s390/include/asm/bitops.h:176:15: error: unknown type name 'bool' >>> static inline bool >>> ^ >>>arch/s390/include/asm/bitops.h:187:15: error: unknown type name 'bool' >>> static inline bool >> >> My s390 cross compiler doesn't like that config. Can someone test this? > > Tested-by: Fengguang Wu <fengguang...@intel.com> > > It works fine with Debian's gcc-6-s390x-linux-gnu and crosstool gcc: > > https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/4.9.0/x86_64-gcc-4.9.0-nolibc_s390x-linux.tar.xz > >> --- a/arch/s390/include/asm/bitops.h~a >> +++ a/arch/s390/include/asm/bitops.h >> @@ -40,6 +40,7 @@ >> #error only can be included directly >> #endif >> >> +#include >> #include >> #include >> #include > > Regards, > Fengguang -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH v2] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
Thank you all for your work. Commonly, I should send patch v3 for it. And very sorry for replying too late. During these days I have no enough time on it (working, buying house, and catching a cold, but I am lucky enough that my father's health is OK). Thanks. On 9/7/16 09:52, Fengguang Wu wrote: > Hi Andrew, > > On Tue, Sep 06, 2016 at 12:27:32PM -0700, Andrew Morton wrote: >> On Sun, 4 Sep 2016 08:27:36 +0800 kbuild test robot wrote: >> >>> [auto build test ERROR on linus/master] >>> [also build test ERROR on v4.8-rc4 next-20160825] >>> [if your patch is applied to the wrong git tree, please drop us a note to >>> help improve the system] >>> [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto >>> for convenience) to record what (public, well-known) commit your patch >>> series was built on] >>> [Check https://git-scm.com/docs/git-format-patch for more information] >>> >>> url: >>> https://github.com/0day-ci/linux/commits/chengang-emindsoft-com-cn/arch-all-include-asm-bitops-Use-bool-instead-of-int-for-all-bit-test-functions/20160828-230301 >>> config: s390-default_defconfig (attached as .config) >>> compiler: s390x-linux-gnu-gcc (Debian 5.4.0-6) 5.4.0 20160609 >>> reproduce: >>> wget >>> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross >>> -O ~/bin/make.cross >>> chmod +x ~/bin/make.cross >>> # save the attached .config to linux build tree >>> make.cross ARCH=s390 >>> >>> All errors (new ones prefixed by >>): >>> >>>In file included from include/linux/bitops.h:36:0, >>> from fs/btrfs/extent_io.c:1: >>> >> arch/s390/include/asm/bitops.h:176:15: error: unknown type name 'bool' >>> static inline bool >>> ^ >>>arch/s390/include/asm/bitops.h:187:15: error: unknown type name 'bool' >>> static inline bool >> >> My s390 cross compiler doesn't like that config. Can someone test this? > > Tested-by: Fengguang Wu > > It works fine with Debian's gcc-6-s390x-linux-gnu and crosstool gcc: > > https://www.kernel.org/pub/tools/crosstool/files/bin/x86_64/4.9.0/x86_64-gcc-4.9.0-nolibc_s390x-linux.tar.xz > >> --- a/arch/s390/include/asm/bitops.h~a >> +++ a/arch/s390/include/asm/bitops.h >> @@ -40,6 +40,7 @@ >> #error only can be included directly >> #endif >> >> +#include >> #include >> #include >> #include > > Regards, > Fengguang -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)
On 9/3/16 08:07, Vineet Gupta wrote: > On 09/02/2016 04:33 PM, Chen Gang wrote: >> On 9/2/16 04:43, Al Viro wrote: >>>> >>>> Can you show a proof that it actually improves anything? He who proposes >>>> a patch gets to defend it, not the other way round... >>>> >>>> Al, bloody annoyed >>>> >> OK, what you said sounds reasonable to me. >> >> It makes the code more readable since they are really pure Boolean >> functions, and let the functions are precisely same in all archs. But >> really, I shall try to prove that it has no negative effect. >> >> e.g. for arc arch. now, I have built the arc raw compiler to build arc >> kernel, but excuse me, I plan to finish proof next week, because during >> these days, I have to work, buy house, and focus on my father's health. > > Since you seem to be have so much stuff to do I decided to help. I did a quick > compile of kernel with and w/o your changes > > bloat-o-meter vmlinux-v4.8rc4-baseline vmlinux-v4.8rc4-bool-in-atomics > add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) > function old new delta > vermagic 49 55 +6 > Total: Before=5967447, After=5967453, chg 0.00% > > I'm mildly surprised that there is no difference so yeah this change is fine > as > far as I'm concerned. > Thank you for your reply :-) And for all: shall I provide the proof for another archs? For me, Boolean gives additional chance to compiler to improve the code. If the compiler can not improve the code, it can treat it as int simply. So theoretically, at least, Boolean should not be worse than int. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)
On 9/3/16 08:07, Vineet Gupta wrote: > On 09/02/2016 04:33 PM, Chen Gang wrote: >> On 9/2/16 04:43, Al Viro wrote: >>>> >>>> Can you show a proof that it actually improves anything? He who proposes >>>> a patch gets to defend it, not the other way round... >>>> >>>> Al, bloody annoyed >>>> >> OK, what you said sounds reasonable to me. >> >> It makes the code more readable since they are really pure Boolean >> functions, and let the functions are precisely same in all archs. But >> really, I shall try to prove that it has no negative effect. >> >> e.g. for arc arch. now, I have built the arc raw compiler to build arc >> kernel, but excuse me, I plan to finish proof next week, because during >> these days, I have to work, buy house, and focus on my father's health. > > Since you seem to be have so much stuff to do I decided to help. I did a quick > compile of kernel with and w/o your changes > > bloat-o-meter vmlinux-v4.8rc4-baseline vmlinux-v4.8rc4-bool-in-atomics > add/remove: 0/0 grow/shrink: 1/0 up/down: 6/0 (6) > function old new delta > vermagic 49 55 +6 > Total: Before=5967447, After=5967453, chg 0.00% > > I'm mildly surprised that there is no difference so yeah this change is fine > as > far as I'm concerned. > Thank you for your reply :-) And for all: shall I provide the proof for another archs? For me, Boolean gives additional chance to compiler to improve the code. If the compiler can not improve the code, it can treat it as int simply. So theoretically, at least, Boolean should not be worse than int. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)
On 9/2/16 04:43, Al Viro wrote: > On Tue, Aug 30, 2016 at 05:49:05AM +0800, Chen Gang wrote: > >> Could you provide the related proof? >> >> Or shall I try to analyze about it and get proof? > > Can you show a proof that it actually improves anything? He who proposes > a patch gets to defend it, not the other way round... > > Al, bloody annoyed > OK, what you said sounds reasonable to me. It makes the code more readable since they are really pure Boolean functions, and let the functions are precisely same in all archs. But really, I shall try to prove that it has no negative effect. e.g. for arc arch. now, I have built the arc raw compiler to build arc kernel, but excuse me, I plan to finish proof next week, because during these days, I have to work, buy house, and focus on my father's health. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: cmsg newgroup alt.sex.fetish.bool (was Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions)
On 9/2/16 04:43, Al Viro wrote: > On Tue, Aug 30, 2016 at 05:49:05AM +0800, Chen Gang wrote: > >> Could you provide the related proof? >> >> Or shall I try to analyze about it and get proof? > > Can you show a proof that it actually improves anything? He who proposes > a patch gets to defend it, not the other way round... > > Al, bloody annoyed > OK, what you said sounds reasonable to me. It makes the code more readable since they are really pure Boolean functions, and let the functions are precisely same in all archs. But really, I shall try to prove that it has no negative effect. e.g. for arc arch. now, I have built the arc raw compiler to build arc kernel, but excuse me, I plan to finish proof next week, because during these days, I have to work, buy house, and focus on my father's health. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
On 8/29/16 21:03, Arnd Bergmann wrote: > On Sunday 28 August 2016, cheng...@emindsoft.com.cn wrote: >> From: Chen Gang <cheng...@emindsoft.com.cn> >> >> Also use the same changing to asm-generic, and also use bool variable >> instead of int variable for mips, mn10300, parisc and tile related >> functions, and also avoid checkpatch.pl to report ERROR. >> >> Originally, except powerpc and xtensa, all another architectures intend >> to return 0 or 1. After this patch, also let powerpc and xtensa return 0 >> or 1. >> >> The patch passes cross building for mips and parisc with default config. >> All related contents are found by "grep test_bit, grep test_and" under >> arch sub-directory. >> >> Signed-off-by: Chen Gang <gang.chen.5...@gmail.com> > > This seems like a good idea overall, and I'm fine with the asm-generic > contents. If there is consensus on changing this, we probably also want > to do some other steps: > > - Change the Documentation/atomic_ops.txt file accordingly > - split up the series per architecture (I don't think there are any > interdependencies) > - For the architectures on which the definition changes (at least > x86 and ARM), do some more sanity checks and see if there are > noticeable changes in object code, and if so whether it looks > better or worse (I'm guessing it will be better if anything) > - See which architectures can still get converted to using the > asm-generic headers instead of providing their own, I think at > least for the nonatomic ones, there are a couple. > Thank you for your ideas, suggestions, and completions. And I guess, at least for arc, I or another related members will try to check the object code. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
On 8/29/16 21:03, Arnd Bergmann wrote: > On Sunday 28 August 2016, cheng...@emindsoft.com.cn wrote: >> From: Chen Gang >> >> Also use the same changing to asm-generic, and also use bool variable >> instead of int variable for mips, mn10300, parisc and tile related >> functions, and also avoid checkpatch.pl to report ERROR. >> >> Originally, except powerpc and xtensa, all another architectures intend >> to return 0 or 1. After this patch, also let powerpc and xtensa return 0 >> or 1. >> >> The patch passes cross building for mips and parisc with default config. >> All related contents are found by "grep test_bit, grep test_and" under >> arch sub-directory. >> >> Signed-off-by: Chen Gang > > This seems like a good idea overall, and I'm fine with the asm-generic > contents. If there is consensus on changing this, we probably also want > to do some other steps: > > - Change the Documentation/atomic_ops.txt file accordingly > - split up the series per architecture (I don't think there are any > interdependencies) > - For the architectures on which the definition changes (at least > x86 and ARM), do some more sanity checks and see if there are > noticeable changes in object code, and if so whether it looks > better or worse (I'm guessing it will be better if anything) > - See which architectures can still get converted to using the > asm-generic headers instead of providing their own, I think at > least for the nonatomic ones, there are a couple. > Thank you for your ideas, suggestions, and completions. And I guess, at least for arc, I or another related members will try to check the object code. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
On 8/30/16 00:48, Vineet Gupta wrote: > On 08/29/2016 06:03 AM, Arnd Bergmann wrote: >> On Sunday 28 August 2016, cheng...@emindsoft.com.cn wrote: >>> From: Chen Gang <cheng...@emindsoft.com.cn> >>> >>> Also use the same changing to asm-generic, and also use bool variable >>> instead of int variable for mips, mn10300, parisc and tile related >>> functions, and also avoid checkpatch.pl to report ERROR. >>> >>> Originally, except powerpc and xtensa, all another architectures intend >>> to return 0 or 1. After this patch, also let powerpc and xtensa return 0 >>> or 1. >>> >>> The patch passes cross building for mips and parisc with default config. >>> All related contents are found by "grep test_bit, grep test_and" under >>> arch sub-directory. >>> >>> Signed-off-by: Chen Gang <gang.chen.5...@gmail.com> >> >> This seems like a good idea overall, and I'm fine with the asm-generic >> contents. If there is consensus on changing this, we probably also want >> to do some other steps: >> >> - Change the Documentation/atomic_ops.txt file accordingly >> - split up the series per architecture (I don't think there are any >> interdependencies) >> - For the architectures on which the definition changes (at least >> x86 and ARM), do some more sanity checks and see if there are >> noticeable changes in object code, and if so whether it looks >> better or worse (I'm guessing it will be better if anything) > > For ARC atleast, it will be slightly worse. As bool is promoted to int in > various > expressions, gcc generates an additional EXTB (extend byte) instruction. > Could you provide the related proof? Or shall I try to analyze about it and get proof? Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
On 8/30/16 00:48, Vineet Gupta wrote: > On 08/29/2016 06:03 AM, Arnd Bergmann wrote: >> On Sunday 28 August 2016, cheng...@emindsoft.com.cn wrote: >>> From: Chen Gang >>> >>> Also use the same changing to asm-generic, and also use bool variable >>> instead of int variable for mips, mn10300, parisc and tile related >>> functions, and also avoid checkpatch.pl to report ERROR. >>> >>> Originally, except powerpc and xtensa, all another architectures intend >>> to return 0 or 1. After this patch, also let powerpc and xtensa return 0 >>> or 1. >>> >>> The patch passes cross building for mips and parisc with default config. >>> All related contents are found by "grep test_bit, grep test_and" under >>> arch sub-directory. >>> >>> Signed-off-by: Chen Gang >> >> This seems like a good idea overall, and I'm fine with the asm-generic >> contents. If there is consensus on changing this, we probably also want >> to do some other steps: >> >> - Change the Documentation/atomic_ops.txt file accordingly >> - split up the series per architecture (I don't think there are any >> interdependencies) >> - For the architectures on which the definition changes (at least >> x86 and ARM), do some more sanity checks and see if there are >> noticeable changes in object code, and if so whether it looks >> better or worse (I'm guessing it will be better if anything) > > For ARC atleast, it will be slightly worse. As bool is promoted to int in > various > expressions, gcc generates an additional EXTB (extend byte) instruction. > Could you provide the related proof? Or shall I try to analyze about it and get proof? Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
Hello all:
I have tried m68k and aarch64, they need include linux/types.h just like
another archs have done (e.g. arc). And then they can pass building for
the default config.
For alpha, it can pass building with my alpha cross compiler (gcc 5.0),
but for safety reason, we'd better let it include linux/types.h, too.
For openrisc, after check the code, I guess, it is the same reason. And
excuse me, I lost my openrisc cross compiler which I originally built,
so I do not give a test now, I can generate it, but really need time.
Thanks.
On 8/28/16 15:10, kbuild test robot wrote:
> Hi Chen,
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.8-rc3 next-20160825]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for
> convenience) to record what (public, well-known) commit your patch series was
> built on]
> [Check https://git-scm.com/docs/git-format-patch for more information]
>
> url:
> https://github.com/0day-ci/linux/commits/chengang-emindsoft-com-cn/arch-all-include-asm-bitops-Use-bool-instead-of-int-for-all-bit-test-functions/20160828-134633
> config: m68k-sun3_defconfig (attached as .config)
> compiler: m68k-linux-gcc (GCC) 4.9.0
> reproduce:
> wget
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
> -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=m68k
>
> All errors (new ones prefixed by >>):
>
>In file included from include/linux/bitops.h:36:0,
> from include/linux/jhash.h:26,
> from net/ipv6/ila/ila_xlat.c:1:
>>> arch/m68k/include/asm/bitops.h:151:15: error: unknown type name 'bool'
> static inline bool test_bit(int nr, const unsigned long *vaddr)
> ^
>arch/m68k/include/asm/bitops.h:157:15: error: unknown type name 'bool'
> static inline bool bset_reg_test_and_set_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:170:15: error: unknown type name 'bool'
> static inline bool bset_mem_test_and_set_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:182:15: error: unknown type name 'bool'
> static inline bool bfset_mem_test_and_set_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:207:15: error: unknown type name 'bool'
> static inline bool bclr_reg_test_and_clear_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:220:15: error: unknown type name 'bool'
> static inline bool bclr_mem_test_and_clear_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:232:15: error: unknown type name 'bool'
> static inline bool bfclr_mem_test_and_clear_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:257:15: error: unknown type name 'bool'
> static inline bool bchg_reg_test_and_change_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:270:15: error: unknown type name 'bool'
> static inline bool bchg_mem_test_and_change_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:282:15: error: unknown type name 'bool'
> static inline bool bfchg_mem_test_and_change_bit(int nr,
> ^
>
> vim +/bool +151 arch/m68k/include/asm/bitops.h
>
>145bfchg_mem_change_bit(nr, vaddr))
>146#endif
>147
>148#define __change_bit(nr, vaddr) change_bit(nr, vaddr)
>149
>150
> > 151static inline bool test_bit(int nr, const unsigned long *vaddr)
>152{
>153return (vaddr[nr >> 5] & (1UL << (nr & 31))) != 0;
>154}
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] arch: all: include: asm: bitops: Use bool instead of int for all bit test functions
Hello all:
I have tried m68k and aarch64, they need include linux/types.h just like
another archs have done (e.g. arc). And then they can pass building for
the default config.
For alpha, it can pass building with my alpha cross compiler (gcc 5.0),
but for safety reason, we'd better let it include linux/types.h, too.
For openrisc, after check the code, I guess, it is the same reason. And
excuse me, I lost my openrisc cross compiler which I originally built,
so I do not give a test now, I can generate it, but really need time.
Thanks.
On 8/28/16 15:10, kbuild test robot wrote:
> Hi Chen,
>
> [auto build test ERROR on linus/master]
> [also build test ERROR on v4.8-rc3 next-20160825]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
> [Suggest to use git(>=2.9.0) format-patch --base= (or --base=auto for
> convenience) to record what (public, well-known) commit your patch series was
> built on]
> [Check https://git-scm.com/docs/git-format-patch for more information]
>
> url:
> https://github.com/0day-ci/linux/commits/chengang-emindsoft-com-cn/arch-all-include-asm-bitops-Use-bool-instead-of-int-for-all-bit-test-functions/20160828-134633
> config: m68k-sun3_defconfig (attached as .config)
> compiler: m68k-linux-gcc (GCC) 4.9.0
> reproduce:
> wget
> https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross
> -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=m68k
>
> All errors (new ones prefixed by >>):
>
>In file included from include/linux/bitops.h:36:0,
> from include/linux/jhash.h:26,
> from net/ipv6/ila/ila_xlat.c:1:
>>> arch/m68k/include/asm/bitops.h:151:15: error: unknown type name 'bool'
> static inline bool test_bit(int nr, const unsigned long *vaddr)
> ^
>arch/m68k/include/asm/bitops.h:157:15: error: unknown type name 'bool'
> static inline bool bset_reg_test_and_set_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:170:15: error: unknown type name 'bool'
> static inline bool bset_mem_test_and_set_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:182:15: error: unknown type name 'bool'
> static inline bool bfset_mem_test_and_set_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:207:15: error: unknown type name 'bool'
> static inline bool bclr_reg_test_and_clear_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:220:15: error: unknown type name 'bool'
> static inline bool bclr_mem_test_and_clear_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:232:15: error: unknown type name 'bool'
> static inline bool bfclr_mem_test_and_clear_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:257:15: error: unknown type name 'bool'
> static inline bool bchg_reg_test_and_change_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:270:15: error: unknown type name 'bool'
> static inline bool bchg_mem_test_and_change_bit(int nr,
> ^
>arch/m68k/include/asm/bitops.h:282:15: error: unknown type name 'bool'
> static inline bool bfchg_mem_test_and_change_bit(int nr,
> ^
>
> vim +/bool +151 arch/m68k/include/asm/bitops.h
>
>145bfchg_mem_change_bit(nr, vaddr))
>146#endif
>147
>148#define __change_bit(nr, vaddr) change_bit(nr, vaddr)
>149
>150
> > 151static inline bool test_bit(int nr, const unsigned long *vaddr)
>152{
>153return (vaddr[nr >> 5] & (1UL << (nr & 31))) != 0;
>154}
>
> ---
> 0-DAY kernel test infrastructureOpen Source Technology Center
> https://lists.01.org/pipermail/kbuild-all Intel Corporation
>
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
On 7/13/16 15:53, Michal Hocko wrote: > On Wed 13-07-16 00:50:12, Chen Gang wrote: >> >> >> On 7/12/16 15:48, Michal Hocko wrote: >>> On Tue 12-07-16 03:47:42, Chen Gang wrote: >>> [...] >>>> In our case, the 2 output size are same, but under x86_64, the insns are >>>> different. After uses bool, it uses push/pop instead of branch, for me, >>>> it should be a little better for catching. >>> >>> The code generated for bool version looks much worse. Look at the fast >>> path. Gcc tries to reuse the retq from the fast path in the bool case >>> and so it has to push rbp and rbx on the stack. >>> >>> That being said, gcc doesn't seem to generate a better code for bool so >>> I do not think this is really worth it. >>> >> >> The code below also merge 3 statements into 1 return statement, although >> for me, it is a little more readable, it will generate a little bad code. >> That is the reason why the output looks a little bad. >> >> In our case, for gcc 6.0, using bool instead of int for bool function >> will get the same output under x86_64. > > If the output is same then there is no reason to change it. > For the new version gcc, the output is same. But bool is a little more readable than int for the pure bool function. But for the current widely used gcc version (I guess, gcc-4.8 is still widely used), bool will get a little better output than int for the pure bool function. >> In our case, for gcc 4.8, using bool instead of int for bool function >> will get a little better output under x86_64. > > I had a different impression and the fast path code had more > instructions. But anyway, is there really a strong reason to change > those return values in the first place? Isn't that just a pointless code > churn? > Excuse me, maybe, I do not quite understand your meanings, but I shall try to explain as far as I can understand (welcome additional detail explanation, e.g. "return values" means c code or assembly output code). In the previous reply, I did not mention 3 things directly and clearly (about my 2 mistakes, and the comparation between gcc 6.0 and 4.8): - Mistake 1: "Use one return statement instead of several statements" is not good, the modification may be a little more readable, but it may get a little bad output code by compiler. - Mistake 2: I only notice there is more branches, but did not notice the real execution path (I guess, your "fast path" is about it). - The optimization of upstream gcc 6.0 is better than redhat gcc 4.8: in this case, gcc 6.0 will: generate the same better code for both bool and int for the pure bool function. optimize the first checking branch (no prologue) -- gcc 4.8 need mark 'likely' for it. skip the 'likely' optimization when "use 1 return statement instead of several statements" -- generation a little bad code too. All together, for me: - Only use bool instead of int for pure bool functions' return value will get a little better code - I shall send patch v2, only change bool to int for all Page_XXX, and keep all the other things no touch. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
On 7/13/16 15:53, Michal Hocko wrote: > On Wed 13-07-16 00:50:12, Chen Gang wrote: >> >> >> On 7/12/16 15:48, Michal Hocko wrote: >>> On Tue 12-07-16 03:47:42, Chen Gang wrote: >>> [...] >>>> In our case, the 2 output size are same, but under x86_64, the insns are >>>> different. After uses bool, it uses push/pop instead of branch, for me, >>>> it should be a little better for catching. >>> >>> The code generated for bool version looks much worse. Look at the fast >>> path. Gcc tries to reuse the retq from the fast path in the bool case >>> and so it has to push rbp and rbx on the stack. >>> >>> That being said, gcc doesn't seem to generate a better code for bool so >>> I do not think this is really worth it. >>> >> >> The code below also merge 3 statements into 1 return statement, although >> for me, it is a little more readable, it will generate a little bad code. >> That is the reason why the output looks a little bad. >> >> In our case, for gcc 6.0, using bool instead of int for bool function >> will get the same output under x86_64. > > If the output is same then there is no reason to change it. > For the new version gcc, the output is same. But bool is a little more readable than int for the pure bool function. But for the current widely used gcc version (I guess, gcc-4.8 is still widely used), bool will get a little better output than int for the pure bool function. >> In our case, for gcc 4.8, using bool instead of int for bool function >> will get a little better output under x86_64. > > I had a different impression and the fast path code had more > instructions. But anyway, is there really a strong reason to change > those return values in the first place? Isn't that just a pointless code > churn? > Excuse me, maybe, I do not quite understand your meanings, but I shall try to explain as far as I can understand (welcome additional detail explanation, e.g. "return values" means c code or assembly output code). In the previous reply, I did not mention 3 things directly and clearly (about my 2 mistakes, and the comparation between gcc 6.0 and 4.8): - Mistake 1: "Use one return statement instead of several statements" is not good, the modification may be a little more readable, but it may get a little bad output code by compiler. - Mistake 2: I only notice there is more branches, but did not notice the real execution path (I guess, your "fast path" is about it). - The optimization of upstream gcc 6.0 is better than redhat gcc 4.8: in this case, gcc 6.0 will: generate the same better code for both bool and int for the pure bool function. optimize the first checking branch (no prologue) -- gcc 4.8 need mark 'likely' for it. skip the 'likely' optimization when "use 1 return statement instead of several statements" -- generation a little bad code too. All together, for me: - Only use bool instead of int for pure bool functions' return value will get a little better code - I shall send patch v2, only change bool to int for all Page_XXX, and keep all the other things no touch. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: gup: Re-define follow_page_mask output parameter page_mask usage
On 7/13/16 15:50, Michal Hocko wrote: > On Wed 13-07-16 01:03:10, Chen Gang wrote: >> On 7/12/16 05:17, Andrew Morton wrote: >>> On Sun, 10 Jul 2016 01:17:05 +0800 cheng...@emindsoft.com.cn wrote: >>> >>>> For a pure output parameter: >>>> >>>> - When callee fails, the caller should not assume the output parameter >>>>is still valid. >>>> >>>> - And callee should not assume the pure output parameter must be >>>>provided by caller -- caller has right to pass NULL when caller does >>>>not care about it. >>> >>> Sorry, I don't think this one is worth merging really. >>> >> >> OK, thanks, I can understand. >> >> It will be better if provide more details: e.g. >> >> - This patch is incorrect, or the comments is not correct. >> >> - The patch is worthless, at present. > > I would say the patch is not really needed. The code you are touching > works just fine and there is no reason to touch it unless this is a part > of a larger change where future changes would be easier to > review/implement. > OK, thanks. I shall try to find other kinds of patches in linux/include, next. :-) -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: gup: Re-define follow_page_mask output parameter page_mask usage
On 7/13/16 15:50, Michal Hocko wrote: > On Wed 13-07-16 01:03:10, Chen Gang wrote: >> On 7/12/16 05:17, Andrew Morton wrote: >>> On Sun, 10 Jul 2016 01:17:05 +0800 cheng...@emindsoft.com.cn wrote: >>> >>>> For a pure output parameter: >>>> >>>> - When callee fails, the caller should not assume the output parameter >>>>is still valid. >>>> >>>> - And callee should not assume the pure output parameter must be >>>>provided by caller -- caller has right to pass NULL when caller does >>>>not care about it. >>> >>> Sorry, I don't think this one is worth merging really. >>> >> >> OK, thanks, I can understand. >> >> It will be better if provide more details: e.g. >> >> - This patch is incorrect, or the comments is not correct. >> >> - The patch is worthless, at present. > > I would say the patch is not really needed. The code you are touching > works just fine and there is no reason to touch it unless this is a part > of a larger change where future changes would be easier to > review/implement. > OK, thanks. I shall try to find other kinds of patches in linux/include, next. :-) -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: gup: Re-define follow_page_mask output parameter page_mask usage
On 7/12/16 05:17, Andrew Morton wrote: > On Sun, 10 Jul 2016 01:17:05 +0800 cheng...@emindsoft.com.cn wrote: > >> For a pure output parameter: >> >> - When callee fails, the caller should not assume the output parameter >>is still valid. >> >> - And callee should not assume the pure output parameter must be >>provided by caller -- caller has right to pass NULL when caller does >>not care about it. > > Sorry, I don't think this one is worth merging really. > OK, thanks, I can understand. It will be better if provide more details: e.g. - This patch is incorrect, or the comments is not correct. - The patch is worthless, at present. - ... By the way, this patch let the callee keep the output parameter no touch if callee no additional outputs, callee assumes caller has initialized the output parameter (for me, it is OK, there are many cases like this). Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: gup: Re-define follow_page_mask output parameter page_mask usage
On 7/12/16 05:17, Andrew Morton wrote: > On Sun, 10 Jul 2016 01:17:05 +0800 cheng...@emindsoft.com.cn wrote: > >> For a pure output parameter: >> >> - When callee fails, the caller should not assume the output parameter >>is still valid. >> >> - And callee should not assume the pure output parameter must be >>provided by caller -- caller has right to pass NULL when caller does >>not care about it. > > Sorry, I don't think this one is worth merging really. > OK, thanks, I can understand. It will be better if provide more details: e.g. - This patch is incorrect, or the comments is not correct. - The patch is worthless, at present. - ... By the way, this patch let the callee keep the output parameter no touch if callee no additional outputs, callee assumes caller has initialized the output parameter (for me, it is OK, there are many cases like this). Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
On 7/12/16 15:48, Michal Hocko wrote: > On Tue 12-07-16 03:47:42, Chen Gang wrote: > [...] >> In our case, the 2 output size are same, but under x86_64, the insns are >> different. After uses bool, it uses push/pop instead of branch, for me, >> it should be a little better for catching. > > The code generated for bool version looks much worse. Look at the fast > path. Gcc tries to reuse the retq from the fast path in the bool case > and so it has to push rbp and rbx on the stack. > > That being said, gcc doesn't seem to generate a better code for bool so > I do not think this is really worth it. > The code below also merge 3 statements into 1 return statement, although for me, it is a little more readable, it will generate a little bad code. That is the reason why the output looks a little bad. In our case, for gcc 6.0, using bool instead of int for bool function will get the same output under x86_64. In our case, for gcc 4.8, using bool instead of int for bool function will get a little better output under x86_64. Thanks. >> The orig: >> >> 1290 : >> 1290: 48 8b 47 08 mov0x8(%rdi),%rax >> 1294: 83 e0 03and$0x3,%eax >> 1297: 48 83 f8 02 cmp$0x2,%rax >> 129b: 74 03 je 12a0 >> <__SetPageMovable+0x12a0> >> 129d: 31 c0 xor%eax,%eax >> 129f: c3 retq >> 12a0: 55 push %rbp >> 12a1: 48 89 e5mov%rsp,%rbp >> 12a4: e8 00 00 00 00 callq 12a9 >> <__SetPageMovable+0x12a9> >> 12a9: 48 85 c0test %rax,%rax >> 12ac: 74 17 je 12c5 >> <__SetPageMovable+0x12c5> >> 12ae: 48 8b 50 68 mov0x68(%rax),%rdx >> 12b2: 48 85 d2test %rdx,%rdx >> 12b5: 74 0e je 12c5 >> <__SetPageMovable+0x12c5> >> 12b7: 48 83 7a 68 00 cmpq $0x0,0x68(%rdx) >> 12bc: b8 01 00 00 00 mov$0x1,%eax >> 12c1: 74 02 je 12c5 >> <__SetPageMovable+0x12c5> >> 12c3: 5d pop%rbp >> 12c4: c3 retq >> 12c5: 31 c0 xor%eax,%eax >> 12c7: 5d pop%rbp >> 12c8: c3 retq >> 12c9: 0f 1f 80 00 00 00 00nopl 0x0(%rax) >> >> The new: >> >> 1290 : >> 1290: 48 8b 47 08 mov0x8(%rdi),%rax >> 1294: 55 push %rbp >> 1295: 48 89 e5mov%rsp,%rbp >> 1298: 53 push %rbx >> 1299: 31 db xor%ebx,%ebx >> 129b: 83 e0 03and$0x3,%eax >> 129e: 48 83 f8 02 cmp$0x2,%rax >> 12a2: 74 05 je 12a9 >> <__SetPageMovable+0x12a9> >> 12a4: 89 d8 mov%ebx,%eax >> 12a6: 5b pop%rbx >> 12a7: 5d pop%rbp >> 12a8: c3 retq >> 12a9: e8 00 00 00 00 callq 12ae >> <__SetPageMovable+0x12ae> >> 12ae: 48 85 c0test %rax,%rax >> 12b1: 74 f1 je 12a4 >> <__SetPageMovable+0x12a4> >> 12b3: 48 8b 40 68 mov0x68(%rax),%rax >> 12b7: 48 85 c0test %rax,%rax >> 12ba: 74 e8 je 12a4 >> <__SetPageMovable+0x12a4> >> 12bc: 48 83 78 68 00 cmpq $0x0,0x68(%rax) >> 12c1: 0f 95 c3setne %bl >> 12c4: 89 d8 mov%ebx,%eax >> 12c6: 5b pop%rbx >> 12c7: 5d pop%rbp >> 12c8: c3 retq >> 12c9: 0f 1f 80 00 00 00 00nopl 0x0(%rax) >> >> Thanks. >> -- >> Chen Gang (陈刚) >> >> Managing Natural Environments is the Duty of Human Beings. > -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
On 7/12/16 15:48, Michal Hocko wrote: > On Tue 12-07-16 03:47:42, Chen Gang wrote: > [...] >> In our case, the 2 output size are same, but under x86_64, the insns are >> different. After uses bool, it uses push/pop instead of branch, for me, >> it should be a little better for catching. > > The code generated for bool version looks much worse. Look at the fast > path. Gcc tries to reuse the retq from the fast path in the bool case > and so it has to push rbp and rbx on the stack. > > That being said, gcc doesn't seem to generate a better code for bool so > I do not think this is really worth it. > The code below also merge 3 statements into 1 return statement, although for me, it is a little more readable, it will generate a little bad code. That is the reason why the output looks a little bad. In our case, for gcc 6.0, using bool instead of int for bool function will get the same output under x86_64. In our case, for gcc 4.8, using bool instead of int for bool function will get a little better output under x86_64. Thanks. >> The orig: >> >> 1290 : >> 1290: 48 8b 47 08 mov0x8(%rdi),%rax >> 1294: 83 e0 03and$0x3,%eax >> 1297: 48 83 f8 02 cmp$0x2,%rax >> 129b: 74 03 je 12a0 >> <__SetPageMovable+0x12a0> >> 129d: 31 c0 xor%eax,%eax >> 129f: c3 retq >> 12a0: 55 push %rbp >> 12a1: 48 89 e5mov%rsp,%rbp >> 12a4: e8 00 00 00 00 callq 12a9 >> <__SetPageMovable+0x12a9> >> 12a9: 48 85 c0test %rax,%rax >> 12ac: 74 17 je 12c5 >> <__SetPageMovable+0x12c5> >> 12ae: 48 8b 50 68 mov0x68(%rax),%rdx >> 12b2: 48 85 d2test %rdx,%rdx >> 12b5: 74 0e je 12c5 >> <__SetPageMovable+0x12c5> >> 12b7: 48 83 7a 68 00 cmpq $0x0,0x68(%rdx) >> 12bc: b8 01 00 00 00 mov$0x1,%eax >> 12c1: 74 02 je 12c5 >> <__SetPageMovable+0x12c5> >> 12c3: 5d pop%rbp >> 12c4: c3 retq >> 12c5: 31 c0 xor%eax,%eax >> 12c7: 5d pop%rbp >> 12c8: c3 retq >> 12c9: 0f 1f 80 00 00 00 00nopl 0x0(%rax) >> >> The new: >> >> 1290 : >> 1290: 48 8b 47 08 mov0x8(%rdi),%rax >> 1294: 55 push %rbp >> 1295: 48 89 e5mov%rsp,%rbp >> 1298: 53 push %rbx >> 1299: 31 db xor%ebx,%ebx >> 129b: 83 e0 03and$0x3,%eax >> 129e: 48 83 f8 02 cmp$0x2,%rax >> 12a2: 74 05 je 12a9 >> <__SetPageMovable+0x12a9> >> 12a4: 89 d8 mov%ebx,%eax >> 12a6: 5b pop%rbx >> 12a7: 5d pop%rbp >> 12a8: c3 retq >> 12a9: e8 00 00 00 00 callq 12ae >> <__SetPageMovable+0x12ae> >> 12ae: 48 85 c0test %rax,%rax >> 12b1: 74 f1 je 12a4 >> <__SetPageMovable+0x12a4> >> 12b3: 48 8b 40 68 mov0x68(%rax),%rax >> 12b7: 48 85 c0test %rax,%rax >> 12ba: 74 e8 je 12a4 >> <__SetPageMovable+0x12a4> >> 12bc: 48 83 78 68 00 cmpq $0x0,0x68(%rax) >> 12c1: 0f 95 c3setne %bl >> 12c4: 89 d8 mov%ebx,%eax >> 12c6: 5b pop%rbx >> 12c7: 5d pop%rbp >> 12c8: c3 retq >> 12c9: 0f 1f 80 00 00 00 00nopl 0x0(%rax) >> >> Thanks. >> -- >> Chen Gang (陈刚) >> >> Managing Natural Environments is the Duty of Human Beings. > -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] include: mman: Use bool instead of int for the return value of arch_validate_prot
On 7/12/16 12:20, Michael Ellerman wrote:
> Chen Gang <cheng...@emindsoft.com.cn> writes:
>
>> On 7/11/16 07:47, Dave Hansen wrote:
>>> On 07/09/2016 09:29 AM, cheng...@emindsoft.com.cn wrote:
>>>> -static inline int arch_validate_prot(unsigned long prot)
>>>> +static inline bool arch_validate_prot(unsigned long prot)
>>>> {
>>>>if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
>>>> - return 0;
>>>> - if ((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO))
>>>> - return 0;
>>>> - return 1;
>>>> + return false;
>>>> + return (prot & PROT_SAO) == 0 || cpu_has_feature(CPU_FTR_SAO);
>>>> }
>>>> #define arch_validate_prot(prot) arch_validate_prot(prot)
>>>
>>> Please don't do things like this. They're not obviously correct and
>>> also have no obvious benefit. You also don't mention why you bothered
>>> to alter the logical structure of these checks.
>>>
>>
>> For all cases, bool is equal or a little better than int, and they are
>> equal in our case (2 final outputs are same). So for me, it may belong
>> to trivial patch, which can be skipped by the normal patch maintainers.
>>
>> As a 'trivial' patch:
>>
>> - For a pure Boolean function, bool return value is more readable than
>>int.
>
> Agreed.
>
> Please send a patch that does that and only that.
>
OK, thanks.
After check the assembly output, for some cases, merging 3 lines to 1
line may be a little more readable, but compiler will generate a little
bad output code.
I shall send patch v2 for it within this weekend.
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] include: mman: Use bool instead of int for the return value of arch_validate_prot
On 7/12/16 12:20, Michael Ellerman wrote:
> Chen Gang writes:
>
>> On 7/11/16 07:47, Dave Hansen wrote:
>>> On 07/09/2016 09:29 AM, cheng...@emindsoft.com.cn wrote:
>>>> -static inline int arch_validate_prot(unsigned long prot)
>>>> +static inline bool arch_validate_prot(unsigned long prot)
>>>> {
>>>>if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
>>>> - return 0;
>>>> - if ((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO))
>>>> - return 0;
>>>> - return 1;
>>>> + return false;
>>>> + return (prot & PROT_SAO) == 0 || cpu_has_feature(CPU_FTR_SAO);
>>>> }
>>>> #define arch_validate_prot(prot) arch_validate_prot(prot)
>>>
>>> Please don't do things like this. They're not obviously correct and
>>> also have no obvious benefit. You also don't mention why you bothered
>>> to alter the logical structure of these checks.
>>>
>>
>> For all cases, bool is equal or a little better than int, and they are
>> equal in our case (2 final outputs are same). So for me, it may belong
>> to trivial patch, which can be skipped by the normal patch maintainers.
>>
>> As a 'trivial' patch:
>>
>> - For a pure Boolean function, bool return value is more readable than
>>int.
>
> Agreed.
>
> Please send a patch that does that and only that.
>
OK, thanks.
After check the assembly output, for some cases, merging 3 lines to 1
line may be a little more readable, but compiler will generate a little
bad output code.
I shall send patch v2 for it within this weekend.
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
On 7/12/16 15:15, Vlastimil Babka wrote: > On 07/11/2016 09:47 PM, Chen Gang wrote: >> >> >> In our case, the 2 output size are same, but under x86_64, the insns are >> different. After uses bool, it uses push/pop instead of branch, for me, >> it should be a little better for catching. > > You mean "caching"? I don't see how this is better for caching. After the > push/pop, the same branch is still there, so it's not eliminated (which would > be indeed better). Somehow the original version just avoids the function > prologue (push rbp, mov rsp, rbp) for the !__PageMovable(page) case. That's > something I would expect e.g. if it was marked likely(), but here it's > probably just accidental that the heuristics think it's likely in the "int" > case and not "bool". So it's not a valid reason for prefering int over bool. > The question is perhaps if it's indeed likely or unlikely and should be > marked as such :) > Oh, sorry, after check the details, the result is a little complex (2 things are mixed together, and likely can be also considered): - One return statement instead of the 3 statements which will change the detail instructions (in fact, it has negative effect). - gcc 6.0 and redhat gcc 4.8 generate the different results. The related output are: - If use one return statement instead of the 3 statements with gcc 6.0, the result is my original outputs which we discussed before. - If still use 3 statements (only use true, false instead of 1, 0) with gcc 6.0, the 2 outputs are equal. - If still use 3 statements (only use true, false instead of 1, 0) with gcc 4.8, the 2 outputs are different, and obviously, the bool will be a little better (no "xor %ebx,%ebx"). - If use one return statement instead of the 3 statements with gcc 4.8, the result is a little bad than keeping 3 statements. - If we add likely(), can get the same result: bool is a little better (no "movzbl %al,%eax"). All together: - For return statement, merging multi-statement together is not a good idea, it will let compiler generates a little bad code. - For gcc 6.0, in our case, the outputs are the same (and both enable 'likely', too). - For gcc 4.8, in our case, 'bool' output is a little better than 'int' (after enable 'likely', also get the same result) The int output by gcc 4.8: 1150 : 1150: 48 8b 57 08 mov0x8(%rdi),%rdx 1154: 55 push %rbp 1155: 48 89 e5mov%rsp,%rbp 1158: 53 push %rbx 1159: 31 db xor%ebx,%ebx 115b: 83 e2 03and$0x3,%edx 115e: 48 83 fa 02 cmp$0x2,%rdx 1162: 74 05 je 1169 <__SetPageMovable+0x1169> 1164: 89 d8 mov%ebx,%eax 1166: 5b pop%rbx 1167: 5d pop%rbp 1168: c3 retq 1169: e8 00 00 00 00 callq 116e <__SetPageMovable+0x116e> 116e: 48 85 c0test %rax,%rax 1171: 74 f1 je 1164 <__SetPageMovable+0x1164> 1173: 48 8b 40 68 mov0x68(%rax),%rax 1177: 48 85 c0test %rax,%rax 117a: 74 e8 je 1164 <__SetPageMovable+0x1164> 117c: 31 db xor%ebx,%ebx 117e: 48 83 78 68 00 cmpq $0x0,0x68(%rax) 1183: 0f 95 c3setne %bl 1186: 89 d8 mov%ebx,%eax 1188: 5b pop%rbx 1189: 5d pop%rbp 118a: c3 retq 118b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) The bool output by gcc 4.8: 1150 : 1150: 48 8b 57 08 mov0x8(%rdi),%rdx 1154: 55 push %rbp 1155: 48 89 e5mov%rsp,%rbp 1158: 53 push %rbx 1159: 31 db xor%ebx,%ebx 115b: 83 e2 03and$0x3,%edx 115e: 48 83 fa 02 cmp$0x2,%rdx 1162: 74 05 je 1169 <__SetPageMovable+0x1169> 1164: 89 d8 mov%ebx,%eax 1166: 5b pop%rbx 1167: 5d pop%rbp 1168: c3 retq 1169: e8 00 00 00 00 callq 116e <__SetPageMovable+0x116e> 116e: 48 85 c0test %r
Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
On 7/12/16 15:15, Vlastimil Babka wrote: > On 07/11/2016 09:47 PM, Chen Gang wrote: >> >> >> In our case, the 2 output size are same, but under x86_64, the insns are >> different. After uses bool, it uses push/pop instead of branch, for me, >> it should be a little better for catching. > > You mean "caching"? I don't see how this is better for caching. After the > push/pop, the same branch is still there, so it's not eliminated (which would > be indeed better). Somehow the original version just avoids the function > prologue (push rbp, mov rsp, rbp) for the !__PageMovable(page) case. That's > something I would expect e.g. if it was marked likely(), but here it's > probably just accidental that the heuristics think it's likely in the "int" > case and not "bool". So it's not a valid reason for prefering int over bool. > The question is perhaps if it's indeed likely or unlikely and should be > marked as such :) > Oh, sorry, after check the details, the result is a little complex (2 things are mixed together, and likely can be also considered): - One return statement instead of the 3 statements which will change the detail instructions (in fact, it has negative effect). - gcc 6.0 and redhat gcc 4.8 generate the different results. The related output are: - If use one return statement instead of the 3 statements with gcc 6.0, the result is my original outputs which we discussed before. - If still use 3 statements (only use true, false instead of 1, 0) with gcc 6.0, the 2 outputs are equal. - If still use 3 statements (only use true, false instead of 1, 0) with gcc 4.8, the 2 outputs are different, and obviously, the bool will be a little better (no "xor %ebx,%ebx"). - If use one return statement instead of the 3 statements with gcc 4.8, the result is a little bad than keeping 3 statements. - If we add likely(), can get the same result: bool is a little better (no "movzbl %al,%eax"). All together: - For return statement, merging multi-statement together is not a good idea, it will let compiler generates a little bad code. - For gcc 6.0, in our case, the outputs are the same (and both enable 'likely', too). - For gcc 4.8, in our case, 'bool' output is a little better than 'int' (after enable 'likely', also get the same result) The int output by gcc 4.8: 1150 : 1150: 48 8b 57 08 mov0x8(%rdi),%rdx 1154: 55 push %rbp 1155: 48 89 e5mov%rsp,%rbp 1158: 53 push %rbx 1159: 31 db xor%ebx,%ebx 115b: 83 e2 03and$0x3,%edx 115e: 48 83 fa 02 cmp$0x2,%rdx 1162: 74 05 je 1169 <__SetPageMovable+0x1169> 1164: 89 d8 mov%ebx,%eax 1166: 5b pop%rbx 1167: 5d pop%rbp 1168: c3 retq 1169: e8 00 00 00 00 callq 116e <__SetPageMovable+0x116e> 116e: 48 85 c0test %rax,%rax 1171: 74 f1 je 1164 <__SetPageMovable+0x1164> 1173: 48 8b 40 68 mov0x68(%rax),%rax 1177: 48 85 c0test %rax,%rax 117a: 74 e8 je 1164 <__SetPageMovable+0x1164> 117c: 31 db xor%ebx,%ebx 117e: 48 83 78 68 00 cmpq $0x0,0x68(%rax) 1183: 0f 95 c3setne %bl 1186: 89 d8 mov%ebx,%eax 1188: 5b pop%rbx 1189: 5d pop%rbp 118a: c3 retq 118b: 0f 1f 44 00 00 nopl 0x0(%rax,%rax,1) The bool output by gcc 4.8: 1150 : 1150: 48 8b 57 08 mov0x8(%rdi),%rdx 1154: 55 push %rbp 1155: 48 89 e5mov%rsp,%rbp 1158: 53 push %rbx 1159: 31 db xor%ebx,%ebx 115b: 83 e2 03and$0x3,%edx 115e: 48 83 fa 02 cmp$0x2,%rdx 1162: 74 05 je 1169 <__SetPageMovable+0x1169> 1164: 89 d8 mov%ebx,%eax 1166: 5b pop%rbx 1167: 5d pop%rbp 1168: c3 retq 1169: e8 00 00 00 00 callq 116e <__SetPageMovable+0x116e> 116e: 48 85 c0test %r
Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
On 7/11/16 08:26, Minchan Kim wrote: > On Sat, Jul 09, 2016 at 11:55:04PM +0800, cheng...@emindsoft.com.cn wrote: >> From: Chen Gang <cheng...@emindsoft.com.cn> >> >> For pure bool function's return value, bool is a little better more or >> less than int. >> >> And return boolean result directly, since 'if' statement is also for >> boolean checking, and return boolean result, too. > > I just wanted to consistent with other PageXXX flags functions, PageAnon, > PageMappingFlags which returns int rather than bool. Although I agree bool > is natural, I want to be consistent with others rather than breaking at > the moment. > > Maybe if you feel it's really helpful, you might be able to handle all > of places I mentioned for better readability and keeping consistency. OK, I guess, we can send another patch for include/linux/page-flags.h for PageXXX. > But I doubt it's worth. > In our case, the 2 output size are same, but under x86_64, the insns are different. After uses bool, it uses push/pop instead of branch, for me, it should be a little better for catching. The orig: 1290 : 1290: 48 8b 47 08 mov0x8(%rdi),%rax 1294: 83 e0 03and$0x3,%eax 1297: 48 83 f8 02 cmp$0x2,%rax 129b: 74 03 je 12a0 <__SetPageMovable+0x12a0> 129d: 31 c0 xor%eax,%eax 129f: c3 retq 12a0: 55 push %rbp 12a1: 48 89 e5mov%rsp,%rbp 12a4: e8 00 00 00 00 callq 12a9 <__SetPageMovable+0x12a9> 12a9: 48 85 c0test %rax,%rax 12ac: 74 17 je 12c5 <__SetPageMovable+0x12c5> 12ae: 48 8b 50 68 mov0x68(%rax),%rdx 12b2: 48 85 d2test %rdx,%rdx 12b5: 74 0e je 12c5 <__SetPageMovable+0x12c5> 12b7: 48 83 7a 68 00 cmpq $0x0,0x68(%rdx) 12bc: b8 01 00 00 00 mov$0x1,%eax 12c1: 74 02 je 12c5 <__SetPageMovable+0x12c5> 12c3: 5d pop%rbp 12c4: c3 retq 12c5: 31 c0 xor%eax,%eax 12c7: 5d pop%rbp 12c8: c3 retq 12c9: 0f 1f 80 00 00 00 00nopl 0x0(%rax) The new: 1290 : 1290: 48 8b 47 08 mov0x8(%rdi),%rax 1294: 55 push %rbp 1295: 48 89 e5mov%rsp,%rbp 1298: 53 push %rbx 1299: 31 db xor%ebx,%ebx 129b: 83 e0 03and$0x3,%eax 129e: 48 83 f8 02 cmp$0x2,%rax 12a2: 74 05 je 12a9 <__SetPageMovable+0x12a9> 12a4: 89 d8 mov%ebx,%eax 12a6: 5b pop%rbx 12a7: 5d pop%rbp 12a8: c3 retq 12a9: e8 00 00 00 00 callq 12ae <__SetPageMovable+0x12ae> 12ae: 48 85 c0test %rax,%rax 12b1: 74 f1 je 12a4 <__SetPageMovable+0x12a4> 12b3: 48 8b 40 68 mov0x68(%rax),%rax 12b7: 48 85 c0test %rax,%rax 12ba: 74 e8 je 12a4 <__SetPageMovable+0x12a4> 12bc: 48 83 78 68 00 cmpq $0x0,0x68(%rax) 12c1: 0f 95 c3setne %bl 12c4: 89 d8 mov%ebx,%eax 12c6: 5b pop%rbx 12c7: 5d pop %rbp 12c8: c3 retq 12c9: 0f 1f 80 00 00 00 00nopl 0x0(%rax) Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm: migrate: Use bool instead of int for the return value of PageMovable
On 7/11/16 08:26, Minchan Kim wrote: > On Sat, Jul 09, 2016 at 11:55:04PM +0800, cheng...@emindsoft.com.cn wrote: >> From: Chen Gang >> >> For pure bool function's return value, bool is a little better more or >> less than int. >> >> And return boolean result directly, since 'if' statement is also for >> boolean checking, and return boolean result, too. > > I just wanted to consistent with other PageXXX flags functions, PageAnon, > PageMappingFlags which returns int rather than bool. Although I agree bool > is natural, I want to be consistent with others rather than breaking at > the moment. > > Maybe if you feel it's really helpful, you might be able to handle all > of places I mentioned for better readability and keeping consistency. OK, I guess, we can send another patch for include/linux/page-flags.h for PageXXX. > But I doubt it's worth. > In our case, the 2 output size are same, but under x86_64, the insns are different. After uses bool, it uses push/pop instead of branch, for me, it should be a little better for catching. The orig: 1290 : 1290: 48 8b 47 08 mov0x8(%rdi),%rax 1294: 83 e0 03and$0x3,%eax 1297: 48 83 f8 02 cmp$0x2,%rax 129b: 74 03 je 12a0 <__SetPageMovable+0x12a0> 129d: 31 c0 xor%eax,%eax 129f: c3 retq 12a0: 55 push %rbp 12a1: 48 89 e5mov%rsp,%rbp 12a4: e8 00 00 00 00 callq 12a9 <__SetPageMovable+0x12a9> 12a9: 48 85 c0test %rax,%rax 12ac: 74 17 je 12c5 <__SetPageMovable+0x12c5> 12ae: 48 8b 50 68 mov0x68(%rax),%rdx 12b2: 48 85 d2test %rdx,%rdx 12b5: 74 0e je 12c5 <__SetPageMovable+0x12c5> 12b7: 48 83 7a 68 00 cmpq $0x0,0x68(%rdx) 12bc: b8 01 00 00 00 mov$0x1,%eax 12c1: 74 02 je 12c5 <__SetPageMovable+0x12c5> 12c3: 5d pop%rbp 12c4: c3 retq 12c5: 31 c0 xor%eax,%eax 12c7: 5d pop%rbp 12c8: c3 retq 12c9: 0f 1f 80 00 00 00 00nopl 0x0(%rax) The new: 1290 : 1290: 48 8b 47 08 mov0x8(%rdi),%rax 1294: 55 push %rbp 1295: 48 89 e5mov%rsp,%rbp 1298: 53 push %rbx 1299: 31 db xor%ebx,%ebx 129b: 83 e0 03and$0x3,%eax 129e: 48 83 f8 02 cmp$0x2,%rax 12a2: 74 05 je 12a9 <__SetPageMovable+0x12a9> 12a4: 89 d8 mov%ebx,%eax 12a6: 5b pop%rbx 12a7: 5d pop%rbp 12a8: c3 retq 12a9: e8 00 00 00 00 callq 12ae <__SetPageMovable+0x12ae> 12ae: 48 85 c0test %rax,%rax 12b1: 74 f1 je 12a4 <__SetPageMovable+0x12a4> 12b3: 48 8b 40 68 mov0x68(%rax),%rax 12b7: 48 85 c0test %rax,%rax 12ba: 74 e8 je 12a4 <__SetPageMovable+0x12a4> 12bc: 48 83 78 68 00 cmpq $0x0,0x68(%rax) 12c1: 0f 95 c3setne %bl 12c4: 89 d8 mov%ebx,%eax 12c6: 5b pop%rbx 12c7: 5d pop %rbp 12c8: c3 retq 12c9: 0f 1f 80 00 00 00 00nopl 0x0(%rax) Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] include: mman: Use bool instead of int for the return value of arch_validate_prot
On 7/11/16 07:47, Dave Hansen wrote:
> On 07/09/2016 09:29 AM, cheng...@emindsoft.com.cn wrote:
>> -static inline int arch_validate_prot(unsigned long prot)
>> +static inline bool arch_validate_prot(unsigned long prot)
>> {
>> if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
>> -return 0;
>> -if ((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO))
>> -return 0;
>> -return 1;
>> +return false;
>> +return (prot & PROT_SAO) == 0 || cpu_has_feature(CPU_FTR_SAO);
>> }
>> #define arch_validate_prot(prot) arch_validate_prot(prot)
>
> Please don't do things like this. They're not obviously correct and
> also have no obvious benefit. You also don't mention why you bothered
> to alter the logical structure of these checks.
>
For all cases, bool is equal or a little better than int, and they are
equal in our case (2 final outputs are same). So for me, it may belong
to trivial patch, which can be skipped by the normal patch maintainers.
As a 'trivial' patch:
- For a pure Boolean function, bool return value is more readable than
int.
- If one statement can express the same expression, and is as simple as
the original 'if' statement, one statement is better than 3 original
statements.
- In our case:
if ((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO))
return 0;
return 1;
equal to:
return !((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO));
equal to:
return !(prot & PROT_SAO) || !!cpu_has_feature(CPU_FTR_SAO);
then:
return (prot & PROT_SAO) == 0 || cpu_has_feature(CPU_FTR_SAO);
Thanks
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] include: mman: Use bool instead of int for the return value of arch_validate_prot
On 7/11/16 07:47, Dave Hansen wrote:
> On 07/09/2016 09:29 AM, cheng...@emindsoft.com.cn wrote:
>> -static inline int arch_validate_prot(unsigned long prot)
>> +static inline bool arch_validate_prot(unsigned long prot)
>> {
>> if (prot & ~(PROT_READ | PROT_WRITE | PROT_EXEC | PROT_SEM | PROT_SAO))
>> -return 0;
>> -if ((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO))
>> -return 0;
>> -return 1;
>> +return false;
>> +return (prot & PROT_SAO) == 0 || cpu_has_feature(CPU_FTR_SAO);
>> }
>> #define arch_validate_prot(prot) arch_validate_prot(prot)
>
> Please don't do things like this. They're not obviously correct and
> also have no obvious benefit. You also don't mention why you bothered
> to alter the logical structure of these checks.
>
For all cases, bool is equal or a little better than int, and they are
equal in our case (2 final outputs are same). So for me, it may belong
to trivial patch, which can be skipped by the normal patch maintainers.
As a 'trivial' patch:
- For a pure Boolean function, bool return value is more readable than
int.
- If one statement can express the same expression, and is as simple as
the original 'if' statement, one statement is better than 3 original
statements.
- In our case:
if ((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO))
return 0;
return 1;
equal to:
return !((prot & PROT_SAO) && !cpu_has_feature(CPU_FTR_SAO));
equal to:
return !(prot & PROT_SAO) || !!cpu_has_feature(CPU_FTR_SAO);
then:
return (prot & PROT_SAO) == 0 || cpu_has_feature(CPU_FTR_SAO);
Thanks
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memory_hotplug.h: Clean up code
On 6/10/16 14:11, kbuild test robot wrote:
> Hi,
>
> [auto build test ERROR on next-20160609]
> [also build test ERROR on v4.7-rc2]
> [cannot apply to v4.7-rc2 v4.7-rc1 v4.6-rc7]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
>
Oh, my patch is for linux-next 20160609 tree, can not apply to v4.7-rc2
directly.
[...]
>
>In file included from include/linux/mmzone.h:741:0,
> from include/linux/gfp.h:5,
> from include/linux/kmod.h:22,
> from include/linux/module.h:13,
> from include/linux/moduleloader.h:5,
> from arch/blackfin/kernel/module.c:9:
>include/linux/memory_hotplug.h: In function 'mhp_notimplemented':
>>> include/linux/memory_hotplug.h:225:2: error: 'mod' undeclared (first use in
>>> this function)
>include/linux/memory_hotplug.h:225:2: note: each undeclared identifier is
> reported only once for each function it appears in
>
> vim +/mod +225 include/linux/memory_hotplug.h
>
>219static inline void zone_span_writelock(struct zone *zone) {}
>220static inline void zone_span_writeunlock(struct zone *zone) {}
>221static inline void zone_seqlock_init(struct zone *zone) {}
>222
>223static inline int mhp_notimplemented(const char *func)
>224{
> > 225pr_warn("%s() called, with CONFIG_MEMORY_HOTPLUG
> disabled\n", func);
>226dump_stack();
>227return -ENOSYS;
>228}
>
After "grep -rn pr_fmt * | grep define" under arch/, for me, it is
blackfin's issue:
we need use
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
instead of
#define pr_fmt(fmt) "module %s: " fmt, mod->name
I shall send one blackfin patch for it.
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memory_hotplug.h: Clean up code
On 6/10/16 14:11, kbuild test robot wrote:
> Hi,
>
> [auto build test ERROR on next-20160609]
> [also build test ERROR on v4.7-rc2]
> [cannot apply to v4.7-rc2 v4.7-rc1 v4.6-rc7]
> [if your patch is applied to the wrong git tree, please drop us a note to
> help improve the system]
>
Oh, my patch is for linux-next 20160609 tree, can not apply to v4.7-rc2
directly.
[...]
>
>In file included from include/linux/mmzone.h:741:0,
> from include/linux/gfp.h:5,
> from include/linux/kmod.h:22,
> from include/linux/module.h:13,
> from include/linux/moduleloader.h:5,
> from arch/blackfin/kernel/module.c:9:
>include/linux/memory_hotplug.h: In function 'mhp_notimplemented':
>>> include/linux/memory_hotplug.h:225:2: error: 'mod' undeclared (first use in
>>> this function)
>include/linux/memory_hotplug.h:225:2: note: each undeclared identifier is
> reported only once for each function it appears in
>
> vim +/mod +225 include/linux/memory_hotplug.h
>
>219static inline void zone_span_writelock(struct zone *zone) {}
>220static inline void zone_span_writeunlock(struct zone *zone) {}
>221static inline void zone_seqlock_init(struct zone *zone) {}
>222
>223static inline int mhp_notimplemented(const char *func)
>224{
> > 225pr_warn("%s() called, with CONFIG_MEMORY_HOTPLUG
> disabled\n", func);
>226dump_stack();
>227return -ENOSYS;
>228}
>
After "grep -rn pr_fmt * | grep define" under arch/, for me, it is
blackfin's issue:
we need use
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
instead of
#define pr_fmt(fmt) "module %s: " fmt, mod->name
I shall send one blackfin patch for it.
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only
On 6/10/16 14:14, Michal Hocko wrote: > On Fri 10-06-16 08:40:30, Chen Gang wrote: >> >> On 6/9/16 23:46, Michal Hocko wrote: > [...] >>> That's being said, I appreciate an interest in making the code cleaner >>> but try to think whether these changes are actually helpful and who is >>> going to benefit from them. >>> >> >> For me, another readers can get benefit more or less from it: if read a >> simple line can know the whole thing (function work), why need we read >> multiple lines? > > Yes I understand that this is a matter of taste but as I've said above. > I do not see this would add a benefit to justify the change. > > If you are doing a reformating or white space cleanups always try to > think about those who want/need to dig into the history to understand > the code and this would add an additional step to get to the original > commit which is added the code. This is just wasting of time. > > Now this would be much more tolerable when the code in question was a > maze but this is not the case. > OK, thanks. Cleaning up code in include/* should face to the whole header files, not only for mm specially. It is not suitable to only focus mm in common shared header files for cleaning up code only. So for me, cleaning up code in include/* is still necessary, but I shall face to all files instead of mm related files. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only
On 6/10/16 14:14, Michal Hocko wrote: > On Fri 10-06-16 08:40:30, Chen Gang wrote: >> >> On 6/9/16 23:46, Michal Hocko wrote: > [...] >>> That's being said, I appreciate an interest in making the code cleaner >>> but try to think whether these changes are actually helpful and who is >>> going to benefit from them. >>> >> >> For me, another readers can get benefit more or less from it: if read a >> simple line can know the whole thing (function work), why need we read >> multiple lines? > > Yes I understand that this is a matter of taste but as I've said above. > I do not see this would add a benefit to justify the change. > > If you are doing a reformating or white space cleanups always try to > think about those who want/need to dig into the history to understand > the code and this would add an additional step to get to the original > commit which is added the code. This is just wasting of time. > > Now this would be much more tolerable when the code in question was a > maze but this is not the case. > OK, thanks. Cleaning up code in include/* should face to the whole header files, not only for mm specially. It is not suitable to only focus mm in common shared header files for cleaning up code only. So for me, cleaning up code in include/* is still necessary, but I shall face to all files instead of mm related files. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only
On 6/9/16 23:46, Michal Hocko wrote: > On Thu 09-06-16 23:23:52, cheng...@emindsoft.com.cn wrote: >> From: Chen Gang <cheng...@emindsoft.com.cn> >> >> Merge several statements to one return statement, since the new return >> statement is still simple enough. >> >> Try to let the second line function parameters almost align with the >> first line parameter (try to be within 80 columns, and in one line). >> >> The comments can fully use 80 columns which can save one line. >> >> Use parameter name newpage instead of new (which will be key word color >> in vim) for dummy mem_cgroup_migrate(), and real mem_cgroup_migrate() >> already uses newpage. > > What is the point of these changes? It removes few lines but does that > actually make the code easier to read? To be honest I am not a big fan > of such a stylist changes unless they are in a series of other changes > which actually tweak the functionality. This just brings more churn > to the git history. > For me, if one line is simple enough, we need not use several lines to express the same thing, especially a line within 80 columns can express the whole function work in headers. The public header files do not like body files, they are simple enough, but their contents are often read through by another readers. So we need more careful for coding styles, but less careful for git history. > That's being said, I appreciate an interest in making the code cleaner > but try to think whether these changes are actually helpful and who is > going to benefit from them. > For me, another readers can get benefit more or less from it: if read a simple line can know the whole thing (function work), why need we read multiple lines? Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only
On 6/9/16 23:46, Michal Hocko wrote: > On Thu 09-06-16 23:23:52, cheng...@emindsoft.com.cn wrote: >> From: Chen Gang >> >> Merge several statements to one return statement, since the new return >> statement is still simple enough. >> >> Try to let the second line function parameters almost align with the >> first line parameter (try to be within 80 columns, and in one line). >> >> The comments can fully use 80 columns which can save one line. >> >> Use parameter name newpage instead of new (which will be key word color >> in vim) for dummy mem_cgroup_migrate(), and real mem_cgroup_migrate() >> already uses newpage. > > What is the point of these changes? It removes few lines but does that > actually make the code easier to read? To be honest I am not a big fan > of such a stylist changes unless they are in a series of other changes > which actually tweak the functionality. This just brings more churn > to the git history. > For me, if one line is simple enough, we need not use several lines to express the same thing, especially a line within 80 columns can express the whole function work in headers. The public header files do not like body files, they are simple enough, but their contents are often read through by another readers. So we need more careful for coding styles, but less careful for git history. > That's being said, I appreciate an interest in making the code cleaner > but try to think whether these changes are actually helpful and who is > going to benefit from them. > For me, another readers can get benefit more or less from it: if read a simple line can know the whole thing (function work), why need we read multiple lines? Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only
On 6/10/16 01:39, Rik van Riel wrote:
> On Thu, 2016-06-09 at 23:23 +0800, cheng...@emindsoft.com.cn wrote:
>> From: Chen Gang <cheng...@emindsoft.com.cn>
>>
>> Merge several statements to one return statement, since the new
>> return
>> statement is still simple enough.
>
> This code is not simple, and any change that
> makes it harder to read needs a good reason.
>
> At least in my opinion, your return statement
> merging thing makes the code harder to read.
>
For me, every member has his/her own taste, we need talk about it case
by case.
[...]
>>
>> static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
>> {
>> -if (mem_cgroup_disabled())
>> -return 0;
>> -
>> -return memcg->css.id;
>> +return mem_cgroup_disabled() ? 0 : memcg->css.id;
>> }
>>
For me, this is the simplest way for using "? :", so it is easy enough (
I guess, Linux kernel does not completely reject "? :").
>> /**
>> @@ -341,10 +338,7 @@ static inline unsigned short
>> mem_cgroup_id(struct mem_cgroup *memcg)
>> */
>> static inline struct mem_cgroup *mem_cgroup_from_id(unsigned short
>> id)
>> {
>> -struct cgroup_subsys_state *css;
>> -
>> -css = css_from_id(id, _cgrp_subsys);
>> -return mem_cgroup_from_css(css);
>> +return mem_cgroup_from_css(css_from_id(id, _cgrp_subsys));
>> }
>>
For me, this case may depend on various members' tastes (although it is
simple enough to me -- it is in one line within 80 columns, and related
with 2 functions' call).
For this case, if any of another member suggests to keep original code
no touch, too, I shall send patch v2 for it (keep it no touch).
>> /**
>> @@ -390,9 +384,7 @@ ino_t page_cgroup_ino(struct page *page);
>>
>> static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
>> {
>> -if (mem_cgroup_disabled())
>> -return true;
>> -return !!(memcg->css.flags & CSS_ONLINE);
>> +return mem_cgroup_disabled() || (memcg->css.flags & CSS_ONLINE);
>> }
>>
For me, this is almost the simplest way for using "||", we can find
this case in include/linux (60+ cases for '||', and 150+ for '&&'), and
the new return statement is in one line within 80 columns.
The new return statement is not the simplest, but it is still simple
enough (which should be better than original several lines statements).
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memcontrol.h: Clean up code only
On 6/10/16 01:39, Rik van Riel wrote:
> On Thu, 2016-06-09 at 23:23 +0800, cheng...@emindsoft.com.cn wrote:
>> From: Chen Gang
>>
>> Merge several statements to one return statement, since the new
>> return
>> statement is still simple enough.
>
> This code is not simple, and any change that
> makes it harder to read needs a good reason.
>
> At least in my opinion, your return statement
> merging thing makes the code harder to read.
>
For me, every member has his/her own taste, we need talk about it case
by case.
[...]
>>
>> static inline unsigned short mem_cgroup_id(struct mem_cgroup *memcg)
>> {
>> -if (mem_cgroup_disabled())
>> -return 0;
>> -
>> -return memcg->css.id;
>> +return mem_cgroup_disabled() ? 0 : memcg->css.id;
>> }
>>
For me, this is the simplest way for using "? :", so it is easy enough (
I guess, Linux kernel does not completely reject "? :").
>> /**
>> @@ -341,10 +338,7 @@ static inline unsigned short
>> mem_cgroup_id(struct mem_cgroup *memcg)
>> */
>> static inline struct mem_cgroup *mem_cgroup_from_id(unsigned short
>> id)
>> {
>> -struct cgroup_subsys_state *css;
>> -
>> -css = css_from_id(id, _cgrp_subsys);
>> -return mem_cgroup_from_css(css);
>> +return mem_cgroup_from_css(css_from_id(id, _cgrp_subsys));
>> }
>>
For me, this case may depend on various members' tastes (although it is
simple enough to me -- it is in one line within 80 columns, and related
with 2 functions' call).
For this case, if any of another member suggests to keep original code
no touch, too, I shall send patch v2 for it (keep it no touch).
>> /**
>> @@ -390,9 +384,7 @@ ino_t page_cgroup_ino(struct page *page);
>>
>> static inline bool mem_cgroup_online(struct mem_cgroup *memcg)
>> {
>> -if (mem_cgroup_disabled())
>> -return true;
>> -return !!(memcg->css.flags & CSS_ONLINE);
>> +return mem_cgroup_disabled() || (memcg->css.flags & CSS_ONLINE);
>> }
>>
For me, this is almost the simplest way for using "||", we can find
this case in include/linux (60+ cases for '||', and 150+ for '&&'), and
the new return statement is in one line within 80 columns.
The new return statement is not the simplest, but it is still simple
enough (which should be better than original several lines statements).
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details
On 2016年05月30日 22:33, Joe Perches wrote: > On Mon, 2016-05-30 at 22:21 +0800, Chen Gang wrote: >> No, they are not necessary. But for me, it will be more clearer, since >> in our kernel (at least in include/linux/), almost all Boolean functions >> use Boolean value or expression for return (and "!!" are often used). > > Opinions vary. > > There seem to be fewer than 20 !! uses in bool return > functions in include/linux/ > ide.h:854 mlx4/driver.h:76 mlx5/device.h:744 mmzone.h:793 mtd/mtd.h:412 nilfs2_fs.h:568 nilfs2_fs.h:602 nilfs2_fs.h:667 nilfs2_fs.h:771 page-flags.h:711 pagemap.h:54 For me, we can change the related functions to Boolean functions directly. > Finding the quantity of bool conversions in include/linux > from something other than 0, 1, true, or false to 0 or 1 > is not trivial, but it is non-zero and seems rather more > than 20. > [root@localhost linux]# grep -rn "\<return\>" * | grep ' & ' | grep -v "==" | grep -v '||' | grep -v 'struct' | grep -v '!=' | grep -v ',' | grep -v '?' | grep -v '!' | grep -v '&&' | wc -l 259 After give a glance, more than 60% are not for bool functions (so I guess about 100 area hint). But I guess, quite a few of none Boolean functions above can be changed to Boolean functions, too (but have to read more code). So I guess, about 200+ matches the hint (not use "!!" for & operation in Boolean functions) in the 1000+ Boolean functions in include/linux. All together, for me: The return statement is much special than normal statements: - It is related with function's type (non-return function, Boolean function, or normal function), not only related with type cast within the statement itself. - Even for normal function, the type cast in return statement is also better: when reading source code, return statements have much more chances to be read than the function return type. - For finding Boolean function in existing normal functions, we often read the return value to know about whether it is a Boolean function or not. So, I still suggest to add type cast explicitly in return statement. Welcome any ideas, suggestions, and completions. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details
On 2016年05月30日 22:33, Joe Perches wrote: > On Mon, 2016-05-30 at 22:21 +0800, Chen Gang wrote: >> No, they are not necessary. But for me, it will be more clearer, since >> in our kernel (at least in include/linux/), almost all Boolean functions >> use Boolean value or expression for return (and "!!" are often used). > > Opinions vary. > > There seem to be fewer than 20 !! uses in bool return > functions in include/linux/ > ide.h:854 mlx4/driver.h:76 mlx5/device.h:744 mmzone.h:793 mtd/mtd.h:412 nilfs2_fs.h:568 nilfs2_fs.h:602 nilfs2_fs.h:667 nilfs2_fs.h:771 page-flags.h:711 pagemap.h:54 For me, we can change the related functions to Boolean functions directly. > Finding the quantity of bool conversions in include/linux > from something other than 0, 1, true, or false to 0 or 1 > is not trivial, but it is non-zero and seems rather more > than 20. > [root@localhost linux]# grep -rn "\" * | grep ' & ' | grep -v "==" | grep -v '||' | grep -v 'struct' | grep -v '!=' | grep -v ',' | grep -v '?' | grep -v '!' | grep -v '&&' | wc -l 259 After give a glance, more than 60% are not for bool functions (so I guess about 100 area hint). But I guess, quite a few of none Boolean functions above can be changed to Boolean functions, too (but have to read more code). So I guess, about 200+ matches the hint (not use "!!" for & operation in Boolean functions) in the 1000+ Boolean functions in include/linux. All together, for me: The return statement is much special than normal statements: - It is related with function's type (non-return function, Boolean function, or normal function), not only related with type cast within the statement itself. - Even for normal function, the type cast in return statement is also better: when reading source code, return statements have much more chances to be read than the function return type. - For finding Boolean function in existing normal functions, we often read the return value to know about whether it is a Boolean function or not. So, I still suggest to add type cast explicitly in return statement. Welcome any ideas, suggestions, and completions. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details
On 5/30/16 22:33, Joe Perches wrote: > On Mon, 2016-05-30 at 22:21 +0800, Chen Gang wrote: >> On 5/29/16 23:08, Joe Perches wrote: >>> These !! uses are't necessary. >>> The compiler makes the bool return 0 or 1. >> No, they are not necessary. But for me, it will be more clearer, since >> in our kernel (at least in include/linux/), almost all Boolean functions >> use Boolean value or expression for return (and "!!" are often used). > > Opinions vary. > OK, thanks. I shall send patch v2 for it within this week. > There seem to be fewer than 20 !! uses in bool return > functions in include/linux/ > [root@localhost linux]# pwd /upstream/linux-next/include/linux [root@localhost linux]# grep -rn '!!' * | grep return | wc -l 32 [root@localhost linux]# except 11 files which use int instead of bool as return value: ide.h:854 mlx4/driver.h:76 mlx5/device.h:744 mmzone.h:793 mtd/mtd.h:412 nilfs2_fs.h:568 nilfs2_fs.h:602 nilfs2_fs.h:667 nilfs2_fs.h:771 page-flags.h:711 pagemap.h:54 > Finding the quantity of bool conversions in include/linux > from something other than 0, 1, true, or false to 0 or 1 > is not trivial, but it is non-zero and seems rather more > than 20. > [root@localhost linux]# grep -rn "\<return\>" * | grep ' & ' | grep -v "==" | grep -v '||' | grep -v 'struct' | grep -v '!=' | grep -v ',' | grep -v '?' | grep -v '!' | grep -v '&&' | wc -l 259 After give a glance, more than 60% are not for bool functions (so I guess about 100 area hint). Thanks -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details
On 5/30/16 22:33, Joe Perches wrote: > On Mon, 2016-05-30 at 22:21 +0800, Chen Gang wrote: >> On 5/29/16 23:08, Joe Perches wrote: >>> These !! uses are't necessary. >>> The compiler makes the bool return 0 or 1. >> No, they are not necessary. But for me, it will be more clearer, since >> in our kernel (at least in include/linux/), almost all Boolean functions >> use Boolean value or expression for return (and "!!" are often used). > > Opinions vary. > OK, thanks. I shall send patch v2 for it within this week. > There seem to be fewer than 20 !! uses in bool return > functions in include/linux/ > [root@localhost linux]# pwd /upstream/linux-next/include/linux [root@localhost linux]# grep -rn '!!' * | grep return | wc -l 32 [root@localhost linux]# except 11 files which use int instead of bool as return value: ide.h:854 mlx4/driver.h:76 mlx5/device.h:744 mmzone.h:793 mtd/mtd.h:412 nilfs2_fs.h:568 nilfs2_fs.h:602 nilfs2_fs.h:667 nilfs2_fs.h:771 page-flags.h:711 pagemap.h:54 > Finding the quantity of bool conversions in include/linux > from something other than 0, 1, true, or false to 0 or 1 > is not trivial, but it is non-zero and seems rather more > than 20. > [root@localhost linux]# grep -rn "\" * | grep ' & ' | grep -v "==" | grep -v '||' | grep -v 'struct' | grep -v '!=' | grep -v ',' | grep -v '?' | grep -v '!' | grep -v '&&' | wc -l 259 After give a glance, more than 60% are not for bool functions (so I guess about 100 area hint). Thanks -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details
On 5/29/16 23:08, Joe Perches wrote:
> On Sun, 2016-05-29 at 22:36 +0800, cheng...@emindsoft.com.cn wrote:
>>
>> Use "!!" to let the boolean function return boolean value directly.
> []
>> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> []
>> @@ -191,12 +190,12 @@ static inline bool movable_node_is_enabled(void)
>>
>> static inline bool memblock_is_mirror(struct memblock_region *m)
>> {
>> -return m->flags & MEMBLOCK_MIRROR;
>> +return !!(m->flags & MEMBLOCK_MIRROR);
>
> These !! uses are't necessary.
> The compiler makes the bool return 0 or 1.
>
No, they are not necessary. But for me, it will be more clearer, since
in our kernel (at least in include/linux/), almost all Boolean functions
use Boolean value or expression for return (and "!!" are often used).
Please help check, and welcome any additional ideas, suggestions, and
completions.
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/memblock.h: Clean up code for several trivial details
On 5/29/16 23:08, Joe Perches wrote:
> On Sun, 2016-05-29 at 22:36 +0800, cheng...@emindsoft.com.cn wrote:
>>
>> Use "!!" to let the boolean function return boolean value directly.
> []
>> diff --git a/include/linux/memblock.h b/include/linux/memblock.h
> []
>> @@ -191,12 +190,12 @@ static inline bool movable_node_is_enabled(void)
>>
>> static inline bool memblock_is_mirror(struct memblock_region *m)
>> {
>> -return m->flags & MEMBLOCK_MIRROR;
>> +return !!(m->flags & MEMBLOCK_MIRROR);
>
> These !! uses are't necessary.
> The compiler makes the bool return 0 or 1.
>
No, they are not necessary. But for me, it will be more clearer, since
in our kernel (at least in include/linux/), almost all Boolean functions
use Boolean value or expression for return (and "!!" are often used).
Please help check, and welcome any additional ideas, suggestions, and
completions.
Thanks.
--
Chen Gang (陈刚)
Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
Hello all: Shall I send patch v2 for it? (if really need, please let me know, and I shall try). Default, I shall continue to try to find and send another patches for mm in "include/linux/*.h". Thanks. On 5/3/16 00:38, Chen Gang wrote: > On 5/3/16 00:23, Chen Gang wrote: >> On 5/2/16 23:35, Alexander Potapenko wrote: >>> On Mon, May 2, 2016 at 5:13 PM, Chen Gang <cheng...@emindsoft.com.cn> wrote: >>>> >>>> OK. But it does not look quite easy to use kasan_disable_current() for >>>> INIT_KASAN which is used in INIT_TASK. >>>> >>>> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in >>>> kasan_enable_current(). >>> Agreed, decrementing the counter in kasan_enable_current() is more natural. >>> I can fix this together with the comments. >> >> OK, thanks. And need I also send patch v2 for include/linux/kasan.h? (or >> you will fix them together). >> >>>> >>>> If we don't prevent the overflow, it will have negative effect with the >>>> caller. When we issue an warning, it means the caller's hope fail, but >>>> can not destroy the caller's original work. In our case: >>>> >>>> - Assume "kasan_depth-- for kasan_enable_current()", the first enable >>>>will let kasan_depth be 0. >>> Sorry, I'm not sure I follow. >>> If we start with kasan_depth=0 (which is the default case for every >>> task except for the init, which also gets kasan_depth=0 short after >>> the kernel starts), >>> then the first call to kasan_disable_current() will make kasan_depth >>> nonzero and will disable KASAN. >>> The subsequent call to kasan_enable_current() will enable KASAN back. >>> >>> There indeed is a problem when someone calls kasan_enable_current() >>> without previously calling kasan_disable_current(). >>> In this case we need to check that kasan_depth was zero and print a >>> warning if it was. >>> It actually does not matter whether we modify kasan_depth after that >>> warning or not, because we are already in inconsistent state. >>> But I think we should modify kasan_depth anyway to ease the debugging. >>> > > Oh, sorry, I forgot one of our original discussing content: > > - If we use signed int kasan_depth, and kasan_depth <= 0 means enable, I >guess, we can always modify kasan_depth. > > - When overflow/underflow (singed int overflow), we can use BUG_ON(), >since it should be rarely happen. > > Thanks. > >> >> For me, BUG_ON() will be better for debugging, but it is really not well >> for using. For WARN_ON(), it already print warnings, so I am not quite >> sure "always modifying kasan_depth will be ease the debugging". >> >> When we are in inconsistent state, for me, what we can do is: >> >> - Still try to do correct things within our control: "when the caller >>make a mistake, if kasan_enable_current() notices about it, it need >>issue warning, and prevent itself to make mistake (causing disable). >> >> - "try to let negative effect smaller to user", e.g. let users "loose >>hope" (call enable has no effect) instead of destroying users' >>original work (call enable, but get disable). >> >> Thanks. >> > -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
Hello all: Shall I send patch v2 for it? (if really need, please let me know, and I shall try). Default, I shall continue to try to find and send another patches for mm in "include/linux/*.h". Thanks. On 5/3/16 00:38, Chen Gang wrote: > On 5/3/16 00:23, Chen Gang wrote: >> On 5/2/16 23:35, Alexander Potapenko wrote: >>> On Mon, May 2, 2016 at 5:13 PM, Chen Gang wrote: >>>> >>>> OK. But it does not look quite easy to use kasan_disable_current() for >>>> INIT_KASAN which is used in INIT_TASK. >>>> >>>> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in >>>> kasan_enable_current(). >>> Agreed, decrementing the counter in kasan_enable_current() is more natural. >>> I can fix this together with the comments. >> >> OK, thanks. And need I also send patch v2 for include/linux/kasan.h? (or >> you will fix them together). >> >>>> >>>> If we don't prevent the overflow, it will have negative effect with the >>>> caller. When we issue an warning, it means the caller's hope fail, but >>>> can not destroy the caller's original work. In our case: >>>> >>>> - Assume "kasan_depth-- for kasan_enable_current()", the first enable >>>>will let kasan_depth be 0. >>> Sorry, I'm not sure I follow. >>> If we start with kasan_depth=0 (which is the default case for every >>> task except for the init, which also gets kasan_depth=0 short after >>> the kernel starts), >>> then the first call to kasan_disable_current() will make kasan_depth >>> nonzero and will disable KASAN. >>> The subsequent call to kasan_enable_current() will enable KASAN back. >>> >>> There indeed is a problem when someone calls kasan_enable_current() >>> without previously calling kasan_disable_current(). >>> In this case we need to check that kasan_depth was zero and print a >>> warning if it was. >>> It actually does not matter whether we modify kasan_depth after that >>> warning or not, because we are already in inconsistent state. >>> But I think we should modify kasan_depth anyway to ease the debugging. >>> > > Oh, sorry, I forgot one of our original discussing content: > > - If we use signed int kasan_depth, and kasan_depth <= 0 means enable, I >guess, we can always modify kasan_depth. > > - When overflow/underflow (singed int overflow), we can use BUG_ON(), >since it should be rarely happen. > > Thanks. > >> >> For me, BUG_ON() will be better for debugging, but it is really not well >> for using. For WARN_ON(), it already print warnings, so I am not quite >> sure "always modifying kasan_depth will be ease the debugging". >> >> When we are in inconsistent state, for me, what we can do is: >> >> - Still try to do correct things within our control: "when the caller >>make a mistake, if kasan_enable_current() notices about it, it need >>issue warning, and prevent itself to make mistake (causing disable). >> >> - "try to let negative effect smaller to user", e.g. let users "loose >>hope" (call enable has no effect) instead of destroying users' >>original work (call enable, but get disable). >> >> Thanks. >> > -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/3/16 00:23, Chen Gang wrote: > On 5/2/16 23:35, Alexander Potapenko wrote: >> On Mon, May 2, 2016 at 5:13 PM, Chen Gang <cheng...@emindsoft.com.cn> wrote: >>> >>> OK. But it does not look quite easy to use kasan_disable_current() for >>> INIT_KASAN which is used in INIT_TASK. >>> >>> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in >>> kasan_enable_current(). >> Agreed, decrementing the counter in kasan_enable_current() is more natural. >> I can fix this together with the comments. > > OK, thanks. And need I also send patch v2 for include/linux/kasan.h? (or > you will fix them together). > >>> >>> If we don't prevent the overflow, it will have negative effect with the >>> caller. When we issue an warning, it means the caller's hope fail, but >>> can not destroy the caller's original work. In our case: >>> >>> - Assume "kasan_depth-- for kasan_enable_current()", the first enable >>>will let kasan_depth be 0. >> Sorry, I'm not sure I follow. >> If we start with kasan_depth=0 (which is the default case for every >> task except for the init, which also gets kasan_depth=0 short after >> the kernel starts), >> then the first call to kasan_disable_current() will make kasan_depth >> nonzero and will disable KASAN. >> The subsequent call to kasan_enable_current() will enable KASAN back. >> >> There indeed is a problem when someone calls kasan_enable_current() >> without previously calling kasan_disable_current(). >> In this case we need to check that kasan_depth was zero and print a >> warning if it was. >> It actually does not matter whether we modify kasan_depth after that >> warning or not, because we are already in inconsistent state. >> But I think we should modify kasan_depth anyway to ease the debugging. >> Oh, sorry, I forgot one of our original discussing content: - If we use signed int kasan_depth, and kasan_depth <= 0 means enable, I guess, we can always modify kasan_depth. - When overflow/underflow (singed int overflow), we can use BUG_ON(), since it should be rarely happen. Thanks. > > For me, BUG_ON() will be better for debugging, but it is really not well > for using. For WARN_ON(), it already print warnings, so I am not quite > sure "always modifying kasan_depth will be ease the debugging". > > When we are in inconsistent state, for me, what we can do is: > > - Still try to do correct things within our control: "when the caller >make a mistake, if kasan_enable_current() notices about it, it need >issue warning, and prevent itself to make mistake (causing disable). > > - "try to let negative effect smaller to user", e.g. let users "loose >hope" (call enable has no effect) instead of destroying users' >original work (call enable, but get disable). > > Thanks. > -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/3/16 00:23, Chen Gang wrote: > On 5/2/16 23:35, Alexander Potapenko wrote: >> On Mon, May 2, 2016 at 5:13 PM, Chen Gang wrote: >>> >>> OK. But it does not look quite easy to use kasan_disable_current() for >>> INIT_KASAN which is used in INIT_TASK. >>> >>> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in >>> kasan_enable_current(). >> Agreed, decrementing the counter in kasan_enable_current() is more natural. >> I can fix this together with the comments. > > OK, thanks. And need I also send patch v2 for include/linux/kasan.h? (or > you will fix them together). > >>> >>> If we don't prevent the overflow, it will have negative effect with the >>> caller. When we issue an warning, it means the caller's hope fail, but >>> can not destroy the caller's original work. In our case: >>> >>> - Assume "kasan_depth-- for kasan_enable_current()", the first enable >>>will let kasan_depth be 0. >> Sorry, I'm not sure I follow. >> If we start with kasan_depth=0 (which is the default case for every >> task except for the init, which also gets kasan_depth=0 short after >> the kernel starts), >> then the first call to kasan_disable_current() will make kasan_depth >> nonzero and will disable KASAN. >> The subsequent call to kasan_enable_current() will enable KASAN back. >> >> There indeed is a problem when someone calls kasan_enable_current() >> without previously calling kasan_disable_current(). >> In this case we need to check that kasan_depth was zero and print a >> warning if it was. >> It actually does not matter whether we modify kasan_depth after that >> warning or not, because we are already in inconsistent state. >> But I think we should modify kasan_depth anyway to ease the debugging. >> Oh, sorry, I forgot one of our original discussing content: - If we use signed int kasan_depth, and kasan_depth <= 0 means enable, I guess, we can always modify kasan_depth. - When overflow/underflow (singed int overflow), we can use BUG_ON(), since it should be rarely happen. Thanks. > > For me, BUG_ON() will be better for debugging, but it is really not well > for using. For WARN_ON(), it already print warnings, so I am not quite > sure "always modifying kasan_depth will be ease the debugging". > > When we are in inconsistent state, for me, what we can do is: > > - Still try to do correct things within our control: "when the caller >make a mistake, if kasan_enable_current() notices about it, it need >issue warning, and prevent itself to make mistake (causing disable). > > - "try to let negative effect smaller to user", e.g. let users "loose >hope" (call enable has no effect) instead of destroying users' >original work (call enable, but get disable). > > Thanks. > -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 23:35, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 5:13 PM, Chen Gang <cheng...@emindsoft.com.cn> wrote: >> >> OK. But it does not look quite easy to use kasan_disable_current() for >> INIT_KASAN which is used in INIT_TASK. >> >> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in >> kasan_enable_current(). > Agreed, decrementing the counter in kasan_enable_current() is more natural. > I can fix this together with the comments. OK, thanks. And need I also send patch v2 for include/linux/kasan.h? (or you will fix them together). >> >> If we don't prevent the overflow, it will have negative effect with the >> caller. When we issue an warning, it means the caller's hope fail, but >> can not destroy the caller's original work. In our case: >> >> - Assume "kasan_depth-- for kasan_enable_current()", the first enable >>will let kasan_depth be 0. > Sorry, I'm not sure I follow. > If we start with kasan_depth=0 (which is the default case for every > task except for the init, which also gets kasan_depth=0 short after > the kernel starts), > then the first call to kasan_disable_current() will make kasan_depth > nonzero and will disable KASAN. > The subsequent call to kasan_enable_current() will enable KASAN back. > > There indeed is a problem when someone calls kasan_enable_current() > without previously calling kasan_disable_current(). > In this case we need to check that kasan_depth was zero and print a > warning if it was. > It actually does not matter whether we modify kasan_depth after that > warning or not, because we are already in inconsistent state. > But I think we should modify kasan_depth anyway to ease the debugging. > For me, BUG_ON() will be better for debugging, but it is really not well for using. For WARN_ON(), it already print warnings, so I am not quite sure "always modifying kasan_depth will be ease the debugging". When we are in inconsistent state, for me, what we can do is: - Still try to do correct things within our control: "when the caller make a mistake, if kasan_enable_current() notices about it, it need issue warning, and prevent itself to make mistake (causing disable). - "try to let negative effect smaller to user", e.g. let users "loose hope" (call enable has no effect) instead of destroying users' original work (call enable, but get disable). Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 23:35, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 5:13 PM, Chen Gang wrote: >> >> OK. But it does not look quite easy to use kasan_disable_current() for >> INIT_KASAN which is used in INIT_TASK. >> >> If we have to set "kasan_depth == 1", we have to use kasan_depth-- in >> kasan_enable_current(). > Agreed, decrementing the counter in kasan_enable_current() is more natural. > I can fix this together with the comments. OK, thanks. And need I also send patch v2 for include/linux/kasan.h? (or you will fix them together). >> >> If we don't prevent the overflow, it will have negative effect with the >> caller. When we issue an warning, it means the caller's hope fail, but >> can not destroy the caller's original work. In our case: >> >> - Assume "kasan_depth-- for kasan_enable_current()", the first enable >>will let kasan_depth be 0. > Sorry, I'm not sure I follow. > If we start with kasan_depth=0 (which is the default case for every > task except for the init, which also gets kasan_depth=0 short after > the kernel starts), > then the first call to kasan_disable_current() will make kasan_depth > nonzero and will disable KASAN. > The subsequent call to kasan_enable_current() will enable KASAN back. > > There indeed is a problem when someone calls kasan_enable_current() > without previously calling kasan_disable_current(). > In this case we need to check that kasan_depth was zero and print a > warning if it was. > It actually does not matter whether we modify kasan_depth after that > warning or not, because we are already in inconsistent state. > But I think we should modify kasan_depth anyway to ease the debugging. > For me, BUG_ON() will be better for debugging, but it is really not well for using. For WARN_ON(), it already print warnings, so I am not quite sure "always modifying kasan_depth will be ease the debugging". When we are in inconsistent state, for me, what we can do is: - Still try to do correct things within our control: "when the caller make a mistake, if kasan_enable_current() notices about it, it need issue warning, and prevent itself to make mistake (causing disable). - "try to let negative effect smaller to user", e.g. let users "loose hope" (call enable has no effect) instead of destroying users' original work (call enable, but get disable). Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 22:23, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 3:51 PM, Chen Gang <cheng...@emindsoft.com.cn> wrote: >> >> OK, thanks. >> >> And for "kasan_depth == 1", I guess, its meaning is related with >> kasan_depth[++|--] in kasan_[en|dis]able_current(): > Assuming you are talking about the assignment of 1 to kasan_depth in > /include/linux/init_task.h, > it's somewhat counterintuitive. I think we just need to replace it > with kasan_disable_current(), and add a corresponding > kasan_enable_current() to the end of kasan_init. > OK. But it does not look quite easy to use kasan_disable_current() for INIT_KASAN which is used in INIT_TASK. If we have to set "kasan_depth == 1", we have to use kasan_depth-- in kasan_enable_current(). >> >> OK, thanks. >> >> I guess, we are agree with each other: "We can both issue a WARNING and >> prevent the actual overflow/underflow.". > No, I am not sure think that we need to prevent the overflow. > As I showed before, this may result in kasan_depth being off even in > the case kasan_enable_current()/kasan_disable_current() are used > consistently. If we don't prevent the overflow, it will have negative effect with the caller. When we issue an warning, it means the caller's hope fail, but can not destroy the caller's original work. In our case: - Assume "kasan_depth-- for kasan_enable_current()", the first enable will let kasan_depth be 0. - If we don't prevent the overflow, 2nd enable will cause disable effect, which will destroy the caller's original work. - Enable/disable mismatch is caused by caller, we can issue warnings, and skip it (since it is not caused by us). But we can not generate new issues to the system only because of the caller's issue. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 22:23, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 3:51 PM, Chen Gang wrote: >> >> OK, thanks. >> >> And for "kasan_depth == 1", I guess, its meaning is related with >> kasan_depth[++|--] in kasan_[en|dis]able_current(): > Assuming you are talking about the assignment of 1 to kasan_depth in > /include/linux/init_task.h, > it's somewhat counterintuitive. I think we just need to replace it > with kasan_disable_current(), and add a corresponding > kasan_enable_current() to the end of kasan_init. > OK. But it does not look quite easy to use kasan_disable_current() for INIT_KASAN which is used in INIT_TASK. If we have to set "kasan_depth == 1", we have to use kasan_depth-- in kasan_enable_current(). >> >> OK, thanks. >> >> I guess, we are agree with each other: "We can both issue a WARNING and >> prevent the actual overflow/underflow.". > No, I am not sure think that we need to prevent the overflow. > As I showed before, this may result in kasan_depth being off even in > the case kasan_enable_current()/kasan_disable_current() are used > consistently. If we don't prevent the overflow, it will have negative effect with the caller. When we issue an warning, it means the caller's hope fail, but can not destroy the caller's original work. In our case: - Assume "kasan_depth-- for kasan_enable_current()", the first enable will let kasan_depth be 0. - If we don't prevent the overflow, 2nd enable will cause disable effect, which will destroy the caller's original work. - Enable/disable mismatch is caused by caller, we can issue warnings, and skip it (since it is not caused by us). But we can not generate new issues to the system only because of the caller's issue. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 20:42, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 2:27 PM, Chen Gang <cheng...@emindsoft.com.cn> wrote: >> On 5/2/16 19:21, Dmitry Vyukov wrote: >>> >>> Signed counter looks good to me. >> >> Oh, sorry, it seems a little mess (originally, I need let the 2 patches >> in one patch set). >> >> If what Alexander's idea is OK (if I did not misunderstand), I guess, >> unsigned counter is still necessary. > I don't think it's critical for us to use an unsigned counter. > If we increment the counter in kasan_disable_current() and decrement > it in kasan_enable_current(), as Dmitry suggested, we'll be naturally > using only positive integers for the counter. > If the counter drops below zero, or exceeds a certain number (say, > 20), we can immediately issue a warning. > OK, thanks. And for "kasan_depth == 1", I guess, its meaning is related with kasan_depth[++|--] in kasan_[en|dis]able_current(): - If kasan_depth++ in kasan_enable_current() with preventing overflow/ underflow, it means "we always want to disable KASAN, if CONFIG_KASAN is not under arm64 or x86_64". - If kasan_depth-- in kasan_enable_current() with preventing overflow/ underflow, it means "we can enable KASAN if CONFIG_KASAN, but firstly we disable it, if it is not under arm64 or x86_64". For me, I don't know which one is correct (or my whole 'guess' is incorrect). Could any members provide your ideas? >>> We can both issue a WARNING and prevent the actual overflow/underflow. >>> But I don't think that there is any sane way to handle the bug (other >>> than just fixing the unmatched disable/enable). If we ignore an >>> excessive disable, then we can end up with ignores enabled >>> permanently. If we ignore an excessive enable, then we can end up with >>> ignores enabled when they should not be enabled. The main point here >>> is to bark loudly, so that the unmatched annotations are noticed and >>> fixed. >>> >> >> How about BUG_ON()? > As noted by Dmitry in an offline discussion, we shouldn't bail out as > long as it's possible to proceed, otherwise the kernel may become very > hard to debug. > A mismatching annotation isn't a case in which we can't proceed with > the execution. OK, thanks. I guess, we are agree with each other: "We can both issue a WARNING and prevent the actual overflow/underflow.". Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 20:42, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 2:27 PM, Chen Gang wrote: >> On 5/2/16 19:21, Dmitry Vyukov wrote: >>> >>> Signed counter looks good to me. >> >> Oh, sorry, it seems a little mess (originally, I need let the 2 patches >> in one patch set). >> >> If what Alexander's idea is OK (if I did not misunderstand), I guess, >> unsigned counter is still necessary. > I don't think it's critical for us to use an unsigned counter. > If we increment the counter in kasan_disable_current() and decrement > it in kasan_enable_current(), as Dmitry suggested, we'll be naturally > using only positive integers for the counter. > If the counter drops below zero, or exceeds a certain number (say, > 20), we can immediately issue a warning. > OK, thanks. And for "kasan_depth == 1", I guess, its meaning is related with kasan_depth[++|--] in kasan_[en|dis]able_current(): - If kasan_depth++ in kasan_enable_current() with preventing overflow/ underflow, it means "we always want to disable KASAN, if CONFIG_KASAN is not under arm64 or x86_64". - If kasan_depth-- in kasan_enable_current() with preventing overflow/ underflow, it means "we can enable KASAN if CONFIG_KASAN, but firstly we disable it, if it is not under arm64 or x86_64". For me, I don't know which one is correct (or my whole 'guess' is incorrect). Could any members provide your ideas? >>> We can both issue a WARNING and prevent the actual overflow/underflow. >>> But I don't think that there is any sane way to handle the bug (other >>> than just fixing the unmatched disable/enable). If we ignore an >>> excessive disable, then we can end up with ignores enabled >>> permanently. If we ignore an excessive enable, then we can end up with >>> ignores enabled when they should not be enabled. The main point here >>> is to bark loudly, so that the unmatched annotations are noticed and >>> fixed. >>> >> >> How about BUG_ON()? > As noted by Dmitry in an offline discussion, we shouldn't bail out as > long as it's possible to proceed, otherwise the kernel may become very > hard to debug. > A mismatching annotation isn't a case in which we can't proceed with > the execution. OK, thanks. I guess, we are agree with each other: "We can both issue a WARNING and prevent the actual overflow/underflow.". Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] include/linux/kasan.h: Notice about 0 for kasan_[dis/en]able_current()
On 5/2/16 19:23, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 1:20 PM, Chen Gang <cheng...@emindsoft.com.cn> wrote: >> On 5/2/16 18:49, Alexander Potapenko wrote: >>> On Mon, May 2, 2016 at 7:35 AM, <cheng...@emindsoft.com.cn> wrote: >>>> >>>> According to their comments and the kasan_depth's initialization, if >>>> kasan_depth is zero, it means disable. So kasan_depth need consider >>>> about the 0 overflow. >>>> >>>> Also remove useless comments for dummy kasan_slab_free(). >>>> >>>> Signed-off-by: Chen Gang <gang.chen.5...@gmail.com> >>> >>> Acked-by: Alexander Potapenko <gli...@google.com> > Nacked-by: Alexander Potapenko <gli...@google.com> >>> >> >> OK, thanks. > Well, on a second thought I take that back, there still might be problems. > I haven't noticed the other CL, and was too hasty reviewing this one. > > As kasan_disable_current() and kasan_enable_current() always go > together, we need to prevent nested calls to them from breaking > everything. > If we ignore some calls to kasan_disable_current() to prevent > overflows, the pairing calls to kasan_enable_current() will bring > |current->kasan_depth| to an invalid state. > > E.g. if I'm understanding your idea correctly, after the following > sequence of calls: > kasan_disable_current(); // #1 > kasan_disable_current(); // #2 > kasan_enable_current(); // #3 > kasan_enable_current(); // #4 > > the value of |current->kasan_depth| will be 2, so a single subsequent > call to kasan_disable_current() won't disable KASAN. > > I think we'd better add BUG checks to bail out if the value of > |current->kasan_depth| is too big or too small. > For me, BUG_ON is OK. e.g. - BUG_ON(!kasan_depth) as soon as be in kasan_enable_current(). - BUG_ON(!(kasan_depth - 1)) as soon as be in kasan_disable_current(). Welcome another members ideas, if no any additional reply within 3 days, I shall send patch v2 for it. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] include/linux/kasan.h: Notice about 0 for kasan_[dis/en]able_current()
On 5/2/16 19:23, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 1:20 PM, Chen Gang wrote: >> On 5/2/16 18:49, Alexander Potapenko wrote: >>> On Mon, May 2, 2016 at 7:35 AM, wrote: >>>> >>>> According to their comments and the kasan_depth's initialization, if >>>> kasan_depth is zero, it means disable. So kasan_depth need consider >>>> about the 0 overflow. >>>> >>>> Also remove useless comments for dummy kasan_slab_free(). >>>> >>>> Signed-off-by: Chen Gang >>> >>> Acked-by: Alexander Potapenko > Nacked-by: Alexander Potapenko >>> >> >> OK, thanks. > Well, on a second thought I take that back, there still might be problems. > I haven't noticed the other CL, and was too hasty reviewing this one. > > As kasan_disable_current() and kasan_enable_current() always go > together, we need to prevent nested calls to them from breaking > everything. > If we ignore some calls to kasan_disable_current() to prevent > overflows, the pairing calls to kasan_enable_current() will bring > |current->kasan_depth| to an invalid state. > > E.g. if I'm understanding your idea correctly, after the following > sequence of calls: > kasan_disable_current(); // #1 > kasan_disable_current(); // #2 > kasan_enable_current(); // #3 > kasan_enable_current(); // #4 > > the value of |current->kasan_depth| will be 2, so a single subsequent > call to kasan_disable_current() won't disable KASAN. > > I think we'd better add BUG checks to bail out if the value of > |current->kasan_depth| is too big or too small. > For me, BUG_ON is OK. e.g. - BUG_ON(!kasan_depth) as soon as be in kasan_enable_current(). - BUG_ON(!(kasan_depth - 1)) as soon as be in kasan_disable_current(). Welcome another members ideas, if no any additional reply within 3 days, I shall send patch v2 for it. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 19:21, Dmitry Vyukov wrote: > On Mon, May 2, 2016 at 1:11 PM, Chen Gang <cheng...@emindsoft.com.cn> wrote: >> On 5/2/16 16:26, Dmitry Vyukov wrote: >>> If you want to improve kasan_depth handling, then please fix the >>> comments and make disable increment and enable decrement (potentially >>> with WARNING on overflow/underflow). It's better to produce a WARNING >>> rather than silently ignore the error. We've ate enough unmatched >>> annotations in user space (e.g. enable is skipped on an error path). >>> These unmatched annotations are hard to notice (they suppress >>> reports). So in user space we bark loudly on overflows/underflows and >>> also check that a thread does not exit with enabled suppressions. >>> >> >> For me, when WARNING on something, it will dummy the related feature >> which should be used (may let user's hope fail), but should not get the >> negative result (hurt user's original work). So in our case: >> >> - When caller calls kasan_report_enabled(), kasan_depth-- to 0, >> >> - When a caller calls kasan_report_enabled() again, the caller will get >>a warning, and notice about this calling is failed, but it is still >>in enable state, should not change to disable state automatically. >> >> - If we report an warning, but still kasan_depth--, it will let things >>much complex. >> >> And for me, another improvements can be done: >> >> - signed int kasan_depth may be a little better. When kasan_depth > 0, >>it is in disable state, else in enable state. It will be much harder >>to generate overflow than unsigned int kasan_depth. >> >> - Let kasan_[en|dis]able_current() return Boolean value to notify the >>caller whether the calling succeeds or fails. > > Signed counter looks good to me. Oh, sorry, it seems a little mess (originally, I need let the 2 patches in one patch set). If what Alexander's idea is OK (if I did not misunderstand), I guess, unsigned counter is still necessary. > We can both issue a WARNING and prevent the actual overflow/underflow. > But I don't think that there is any sane way to handle the bug (other > than just fixing the unmatched disable/enable). If we ignore an > excessive disable, then we can end up with ignores enabled > permanently. If we ignore an excessive enable, then we can end up with > ignores enabled when they should not be enabled. The main point here > is to bark loudly, so that the unmatched annotations are noticed and > fixed. > How about BUG_ON()? Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 19:21, Dmitry Vyukov wrote: > On Mon, May 2, 2016 at 1:11 PM, Chen Gang wrote: >> On 5/2/16 16:26, Dmitry Vyukov wrote: >>> If you want to improve kasan_depth handling, then please fix the >>> comments and make disable increment and enable decrement (potentially >>> with WARNING on overflow/underflow). It's better to produce a WARNING >>> rather than silently ignore the error. We've ate enough unmatched >>> annotations in user space (e.g. enable is skipped on an error path). >>> These unmatched annotations are hard to notice (they suppress >>> reports). So in user space we bark loudly on overflows/underflows and >>> also check that a thread does not exit with enabled suppressions. >>> >> >> For me, when WARNING on something, it will dummy the related feature >> which should be used (may let user's hope fail), but should not get the >> negative result (hurt user's original work). So in our case: >> >> - When caller calls kasan_report_enabled(), kasan_depth-- to 0, >> >> - When a caller calls kasan_report_enabled() again, the caller will get >>a warning, and notice about this calling is failed, but it is still >>in enable state, should not change to disable state automatically. >> >> - If we report an warning, but still kasan_depth--, it will let things >>much complex. >> >> And for me, another improvements can be done: >> >> - signed int kasan_depth may be a little better. When kasan_depth > 0, >>it is in disable state, else in enable state. It will be much harder >>to generate overflow than unsigned int kasan_depth. >> >> - Let kasan_[en|dis]able_current() return Boolean value to notify the >>caller whether the calling succeeds or fails. > > Signed counter looks good to me. Oh, sorry, it seems a little mess (originally, I need let the 2 patches in one patch set). If what Alexander's idea is OK (if I did not misunderstand), I guess, unsigned counter is still necessary. > We can both issue a WARNING and prevent the actual overflow/underflow. > But I don't think that there is any sane way to handle the bug (other > than just fixing the unmatched disable/enable). If we ignore an > excessive disable, then we can end up with ignores enabled > permanently. If we ignore an excessive enable, then we can end up with > ignores enabled when they should not be enabled. The main point here > is to bark loudly, so that the unmatched annotations are noticed and > fixed. > How about BUG_ON()? Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 19:34, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 7:36 AM, <cheng...@emindsoft.com.cn> wrote: >> From: Chen Gang <cheng...@emindsoft.com.cn> >> >> According to kasan_[dis|en]able_current() comments and the kasan_depth' >> s initialization, if kasan_depth is zero, it means disable. > The comments for those functions are really poor, but there's nothing > there that says kasan_depth==0 disables KASAN. > Actually, kasan_report_enabled() is currently the only place that > denotes the semantics of kasan_depth, so it couldn't be wrong. > > init_task.kasan_depth is 1 during bootstrap and is then set to zero by > kasan_init() > For every other thread, current->kasan_depth is zero-initialized. > OK, what you said sound reasonable to me. and do you also mean: - kasan_depth == 0 means enable KASAN, others means disable KASAN. - If always let kasan_[en|dis]able_current() be pair, and notice about the overflow, it should be OK: "kasan_enable_current() can let kasan_depth++, and kasan_disable_current() will let kasan_depth--". - If we check the related overflow, "kasan_depth == 1" mean "the KASAN should be always in disable state". Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 19:34, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 7:36 AM, wrote: >> From: Chen Gang >> >> According to kasan_[dis|en]able_current() comments and the kasan_depth' >> s initialization, if kasan_depth is zero, it means disable. > The comments for those functions are really poor, but there's nothing > there that says kasan_depth==0 disables KASAN. > Actually, kasan_report_enabled() is currently the only place that > denotes the semantics of kasan_depth, so it couldn't be wrong. > > init_task.kasan_depth is 1 during bootstrap and is then set to zero by > kasan_init() > For every other thread, current->kasan_depth is zero-initialized. > OK, what you said sound reasonable to me. and do you also mean: - kasan_depth == 0 means enable KASAN, others means disable KASAN. - If always let kasan_[en|dis]able_current() be pair, and notice about the overflow, it should be OK: "kasan_enable_current() can let kasan_depth++, and kasan_disable_current() will let kasan_depth--". - If we check the related overflow, "kasan_depth == 1" mean "the KASAN should be always in disable state". Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] include/linux/kasan.h: Notice about 0 for kasan_[dis/en]able_current()
On 5/2/16 18:49, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 7:35 AM, <cheng...@emindsoft.com.cn> wrote: >> >> According to their comments and the kasan_depth's initialization, if >> kasan_depth is zero, it means disable. So kasan_depth need consider >> about the 0 overflow. >> >> Also remove useless comments for dummy kasan_slab_free(). >> >> Signed-off-by: Chen Gang <gang.chen.5...@gmail.com> > > Acked-by: Alexander Potapenko <gli...@google.com> > OK, thanks. Another patch thread is also related with this patch thread, please help check. And sorry, originally, I did not let the 2 patches in one patches set. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] include/linux/kasan.h: Notice about 0 for kasan_[dis/en]able_current()
On 5/2/16 18:49, Alexander Potapenko wrote: > On Mon, May 2, 2016 at 7:35 AM, wrote: >> >> According to their comments and the kasan_depth's initialization, if >> kasan_depth is zero, it means disable. So kasan_depth need consider >> about the 0 overflow. >> >> Also remove useless comments for dummy kasan_slab_free(). >> >> Signed-off-by: Chen Gang > > Acked-by: Alexander Potapenko > OK, thanks. Another patch thread is also related with this patch thread, please help check. And sorry, originally, I did not let the 2 patches in one patches set. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 16:26, Dmitry Vyukov wrote: > On Mon, May 2, 2016 at 7:36 AM, <cheng...@emindsoft.com.cn> wrote: >> From: Chen Gang <cheng...@emindsoft.com.cn> >> >> According to kasan_[dis|en]able_current() comments and the kasan_depth' >> s initialization, if kasan_depth is zero, it means disable. >> >> So need use "!!kasan_depth" instead of "!kasan_depth" for checking >> enable. >> > > Hi Chen, > > I don't think this is correct. OK, thanks. > We seem to have some incorrect comments around kasan_depth, and a > weird way of manipulating it (disable should increment, and enable > should decrement). But in the end it is working. This change will > suppress all true reports and enable all false reports. > For me, I guess, what you said above is reasonable. But it is really strange to any newbies (e.g. me), so it will be better to get another member's confirmation, too. If no any additional reply by any other members within 3 days, I shall treat what you said is OK. > If you want to improve kasan_depth handling, then please fix the > comments and make disable increment and enable decrement (potentially > with WARNING on overflow/underflow). It's better to produce a WARNING > rather than silently ignore the error. We've ate enough unmatched > annotations in user space (e.g. enable is skipped on an error path). > These unmatched annotations are hard to notice (they suppress > reports). So in user space we bark loudly on overflows/underflows and > also check that a thread does not exit with enabled suppressions. > For me, when WARNING on something, it will dummy the related feature which should be used (may let user's hope fail), but should not get the negative result (hurt user's original work). So in our case: - When caller calls kasan_report_enabled(), kasan_depth-- to 0, - When a caller calls kasan_report_enabled() again, the caller will get a warning, and notice about this calling is failed, but it is still in enable state, should not change to disable state automatically. - If we report an warning, but still kasan_depth--, it will let things much complex. And for me, another improvements can be done: - signed int kasan_depth may be a little better. When kasan_depth > 0, it is in disable state, else in enable state. It will be much harder to generate overflow than unsigned int kasan_depth. - Let kasan_[en|dis]able_current() return Boolean value to notify the caller whether the calling succeeds or fails. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH] mm/kasan/kasan.h: Fix boolean checking issue for kasan_report_enabled()
On 5/2/16 16:26, Dmitry Vyukov wrote: > On Mon, May 2, 2016 at 7:36 AM, wrote: >> From: Chen Gang >> >> According to kasan_[dis|en]able_current() comments and the kasan_depth' >> s initialization, if kasan_depth is zero, it means disable. >> >> So need use "!!kasan_depth" instead of "!kasan_depth" for checking >> enable. >> > > Hi Chen, > > I don't think this is correct. OK, thanks. > We seem to have some incorrect comments around kasan_depth, and a > weird way of manipulating it (disable should increment, and enable > should decrement). But in the end it is working. This change will > suppress all true reports and enable all false reports. > For me, I guess, what you said above is reasonable. But it is really strange to any newbies (e.g. me), so it will be better to get another member's confirmation, too. If no any additional reply by any other members within 3 days, I shall treat what you said is OK. > If you want to improve kasan_depth handling, then please fix the > comments and make disable increment and enable decrement (potentially > with WARNING on overflow/underflow). It's better to produce a WARNING > rather than silently ignore the error. We've ate enough unmatched > annotations in user space (e.g. enable is skipped on an error path). > These unmatched annotations are hard to notice (they suppress > reports). So in user space we bark loudly on overflows/underflows and > also check that a thread does not exit with enabled suppressions. > For me, when WARNING on something, it will dummy the related feature which should be used (may let user's hope fail), but should not get the negative result (hurt user's original work). So in our case: - When caller calls kasan_report_enabled(), kasan_depth-- to 0, - When a caller calls kasan_report_enabled() again, the caller will get a warning, and notice about this calling is failed, but it is still in enable state, should not change to disable state automatically. - If we report an warning, but still kasan_depth--, it will let things much complex. And for me, another improvements can be done: - signed int kasan_depth may be a little better. When kasan_depth > 0, it is in disable state, else in enable state. It will be much harder to generate overflow than unsigned int kasan_depth. - Let kasan_[en|dis]able_current() return Boolean value to notify the caller whether the calling succeeds or fails. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/29/16 06:23, Theodore Ts'o wrote: > On Sun, Feb 28, 2016 at 08:47:23AM +0800, Chen Gang wrote: >> >> Excuse me, when I sent this patch, I did not know who I shall send to, I >> have to reference to "./scripts/get_maintainer.pl". >> >> If any members have no time to care about it (every members' time are >> really expensive), please let me know (can reply directly). > > Yes, everybody's time is very expensive. So why are you wasting it > all with a "last post wins" style of argumentation? A maintainer has > NAK'ed it. Please drop this. > For me, I don't care about "last post wins". But I care about the technical correctness, and for me, we (all of us) need try our best to let the email reply as correct as we can, so can avoid to mislead another readers. So for me, if any members have new ideas, suggestions, or completions, they can still reply at any time (may be next day, next week, or next month ...). > There is a reason why whitespace fixes are often consider to have > extreme negative value, and a deep suspicion that people are doing > this just to say that they have a patch in the kernel, perhaps in the > misapprehension that this will help them get a job. > For me, I don't think so, at least for me, contribution and learning is my main goal in open source community, so I mainly focus on correctness. All of us know when some related maintainer NAK'ed, the related patch, of cause, must be dropped, the reason why I still reply the mail is: I shall try to make the discussion/communication as correct as I can. For "get a job", I guess, the open source community is helpful, but I also suggest: if someone wants to "get a job", he/she should not depend on the open source community (community has no duty for it). > Let me say that if I were a hiring manager, and I did a Google search > on a potential job application, and came across a thread like this, my > reaction would be extremely negative. > For me, job hunter and HR hunter can use open source community, but the open source community's main goal is not for job hunter or HR hunter. I guess, the open source community's goal should be: - Develop the relate product (we can treat it as urgent thing, e.g. new features, bug fix). - Learning and discussing the product related technical issues (we can treat it as important thing, I guess, "coding styles issues" should be one of these issues). Welcome any members ideas, suggestions, and completions for it. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/29/16 06:23, Theodore Ts'o wrote: > On Sun, Feb 28, 2016 at 08:47:23AM +0800, Chen Gang wrote: >> >> Excuse me, when I sent this patch, I did not know who I shall send to, I >> have to reference to "./scripts/get_maintainer.pl". >> >> If any members have no time to care about it (every members' time are >> really expensive), please let me know (can reply directly). > > Yes, everybody's time is very expensive. So why are you wasting it > all with a "last post wins" style of argumentation? A maintainer has > NAK'ed it. Please drop this. > For me, I don't care about "last post wins". But I care about the technical correctness, and for me, we (all of us) need try our best to let the email reply as correct as we can, so can avoid to mislead another readers. So for me, if any members have new ideas, suggestions, or completions, they can still reply at any time (may be next day, next week, or next month ...). > There is a reason why whitespace fixes are often consider to have > extreme negative value, and a deep suspicion that people are doing > this just to say that they have a patch in the kernel, perhaps in the > misapprehension that this will help them get a job. > For me, I don't think so, at least for me, contribution and learning is my main goal in open source community, so I mainly focus on correctness. All of us know when some related maintainer NAK'ed, the related patch, of cause, must be dropped, the reason why I still reply the mail is: I shall try to make the discussion/communication as correct as I can. For "get a job", I guess, the open source community is helpful, but I also suggest: if someone wants to "get a job", he/she should not depend on the open source community (community has no duty for it). > Let me say that if I were a hiring manager, and I did a Google search > on a potential job application, and came across a thread like this, my > reaction would be extremely negative. > For me, job hunter and HR hunter can use open source community, but the open source community's main goal is not for job hunter or HR hunter. I guess, the open source community's goal should be: - Develop the relate product (we can treat it as urgent thing, e.g. new features, bug fix). - Learning and discussing the product related technical issues (we can treat it as important thing, I guess, "coding styles issues" should be one of these issues). Welcome any members ideas, suggestions, and completions for it. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/28/16 21:27, Mel Gorman wrote: > On Sun, Feb 28, 2016 at 08:21:40AM +0800, Chen Gang wrote: >> >> For me, NAK also needs reasons. >> > > You already got the reasons. Not only does a patch of this type interfere > with git blame which is important even in headers but I do not think the > patch actually improves the readability of the code. For example, the > comments move to the line after the defintions which to my eye at least > looks clumsy and weird. > For me, in local headers, they may be often modified, and also may be complex, so the code analyzing maybe also be used often. But in common shared headers in ./include (e.g. gfp.h), most of them are simple enough. - Since common shared headers are usually simple, code analyzing is still useful, but not like the body files or local headers (code analyzing are very useful for body files and local headers). - Common shared headers are quite often read by most programmers, so common shared headers need take more care about its coding styles. - Then for common shared headers, the coding style is 1st. And for __GFP_MOVABLE definition (with ZONE_MOVABLE), I guess, we can keep it no touch (like what I originally said: if the related member stick to, we can keep it no touch). And for me, the other macro definitions which out of 80 columns, can be fixed in normal ways (let the related comments ahead of macro definition ), does this change also have negative effect? >> I guess they are related with this patch, and their NAKs' reason are: mm >> and trivial don't care about this coding style issue, is it correct? >> > > No. Coding style is important but it's a guideline not a law. Yes. For me, vertical split window in vim is very useful, I almost always use this feature when read source code in full screen under Macbook client, when columns are 86+, it will be wrapped (I feel really not quite good). And occasionally (really not often), we may copy/past part of contents in the header files (e.g. constant definition) to the pdf file as appendix. So except the string broken, or "grep -rn xxx * | grep yyy", 80 columns limitation is always helpful to me. > There are > cases where breaking it results in perfectly readable code. At least one > my my own recent patches was flagged by checkpatch as having style issues > but fixing the style was considerably harder to read so I left it. If the > definitions in that header need to change again in the future and there > are style issues then they can be fixed in the context of a functional > change instead of patching style just for the sake of it. > For me, except just modify the related contents, usually, we need devide the patch into 2: one for real modification, the other for coding styles. And in some of common, base, shared headers in ./include (e.g. gfp.h), I guess, most of contents *should* not be changed quite often, so the bad coding styles probably will be alive in a long term. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/28/16 21:27, Mel Gorman wrote: > On Sun, Feb 28, 2016 at 08:21:40AM +0800, Chen Gang wrote: >> >> For me, NAK also needs reasons. >> > > You already got the reasons. Not only does a patch of this type interfere > with git blame which is important even in headers but I do not think the > patch actually improves the readability of the code. For example, the > comments move to the line after the defintions which to my eye at least > looks clumsy and weird. > For me, in local headers, they may be often modified, and also may be complex, so the code analyzing maybe also be used often. But in common shared headers in ./include (e.g. gfp.h), most of them are simple enough. - Since common shared headers are usually simple, code analyzing is still useful, but not like the body files or local headers (code analyzing are very useful for body files and local headers). - Common shared headers are quite often read by most programmers, so common shared headers need take more care about its coding styles. - Then for common shared headers, the coding style is 1st. And for __GFP_MOVABLE definition (with ZONE_MOVABLE), I guess, we can keep it no touch (like what I originally said: if the related member stick to, we can keep it no touch). And for me, the other macro definitions which out of 80 columns, can be fixed in normal ways (let the related comments ahead of macro definition ), does this change also have negative effect? >> I guess they are related with this patch, and their NAKs' reason are: mm >> and trivial don't care about this coding style issue, is it correct? >> > > No. Coding style is important but it's a guideline not a law. Yes. For me, vertical split window in vim is very useful, I almost always use this feature when read source code in full screen under Macbook client, when columns are 86+, it will be wrapped (I feel really not quite good). And occasionally (really not often), we may copy/past part of contents in the header files (e.g. constant definition) to the pdf file as appendix. So except the string broken, or "grep -rn xxx * | grep yyy", 80 columns limitation is always helpful to me. > There are > cases where breaking it results in perfectly readable code. At least one > my my own recent patches was flagged by checkpatch as having style issues > but fixing the style was considerably harder to read so I left it. If the > definitions in that header need to change again in the future and there > are style issues then they can be fixed in the context of a functional > change instead of patching style just for the sake of it. > For me, except just modify the related contents, usually, we need devide the patch into 2: one for real modification, the other for coding styles. And in some of common, base, shared headers in ./include (e.g. gfp.h), I guess, most of contents *should* not be changed quite often, so the bad coding styles probably will be alive in a long term. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/28/16 07:14, Jiri Kosina wrote: > On Sat, 27 Feb 2016, Chen Gang wrote: > >>> Mel, as an MM developer, has already NACK'ed the patch, which means >>> you should not send the patch to **any** upstream maintainer for >>> inclusion. >> >> I don't think I "should not ...". I only care about correctness and >> contribution, I don't care about any members ideas and their thinking. >> When we have different ideas or thinking, we need discuss. > > If by "discuss" you mean "30+ email thread about where to put a line > break", please drop me from CC next time this discussion is going to > happen. Thanks. > Excuse me, when I sent this patch, I did not know who I shall send to, I have to reference to "./scripts/get_maintainer.pl". If any members have no time to care about it (every members' time are really expensive), please let me know (can reply directly). Thanks. >> For common shared header files, for me, we should really take more care >> about the coding styles. >> >> - If the common shared header files don't care about the coding styles, >>I guess any body files will have much more excuses for "do not care >>about coding styles". >> >> - That means our kernel whole source files need not care about coding >>styles at all!! >> >> - It is really really VERY BAD!! >> >> If someone only dislike me to send the related patches, I suggest: Let >> another member(s) "run checkpatch -file" on the whole "./include" sub- >> directory, and fix all coding styles issues. > > Which is exactly what you shouldn't do. > For me, I also guess, I am not the suitable member to do that (in fact, I dislike to do like that - "run checkpath -file" on "./include"). > The ultimate goal of the Linux kernel is not 100% strict complicance to > the CodingStyle document no matter what. The ultimate goal is to have a > kernel that is under control. By polluting git blame, you are taking on > aspect of the "under control" away. > Yes, the ultimate goal of CodingStyle is to have a kernel that is under control. For me, most of files in "./include" are common, simple, and shared files, which are not quite related with code analyzing (e.g. git log -p, or git blame), but they are read by others in most times. Is it correct? > Common sense needs to be used; horribly terrible coding style needs to be > fixed, sure. Is 82-characters long line horribly terrible coding style? > No, it's not. > For me, what you said above have effect on body files (in kernel, at least, more than 95% source files are body files, I guess). But in "./include", most of files are the interface inside and outside of our kernel, we need take more care about their coding styles. I often use vertical split window in vim in full screen mode to reading source code, when I read c source files, I often split window vertically for the related header files as reference. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/28/16 07:14, Jiri Kosina wrote: > On Sat, 27 Feb 2016, Chen Gang wrote: > >>> Mel, as an MM developer, has already NACK'ed the patch, which means >>> you should not send the patch to **any** upstream maintainer for >>> inclusion. >> >> I don't think I "should not ...". I only care about correctness and >> contribution, I don't care about any members ideas and their thinking. >> When we have different ideas or thinking, we need discuss. > > If by "discuss" you mean "30+ email thread about where to put a line > break", please drop me from CC next time this discussion is going to > happen. Thanks. > Excuse me, when I sent this patch, I did not know who I shall send to, I have to reference to "./scripts/get_maintainer.pl". If any members have no time to care about it (every members' time are really expensive), please let me know (can reply directly). Thanks. >> For common shared header files, for me, we should really take more care >> about the coding styles. >> >> - If the common shared header files don't care about the coding styles, >>I guess any body files will have much more excuses for "do not care >>about coding styles". >> >> - That means our kernel whole source files need not care about coding >>styles at all!! >> >> - It is really really VERY BAD!! >> >> If someone only dislike me to send the related patches, I suggest: Let >> another member(s) "run checkpatch -file" on the whole "./include" sub- >> directory, and fix all coding styles issues. > > Which is exactly what you shouldn't do. > For me, I also guess, I am not the suitable member to do that (in fact, I dislike to do like that - "run checkpath -file" on "./include"). > The ultimate goal of the Linux kernel is not 100% strict complicance to > the CodingStyle document no matter what. The ultimate goal is to have a > kernel that is under control. By polluting git blame, you are taking on > aspect of the "under control" away. > Yes, the ultimate goal of CodingStyle is to have a kernel that is under control. For me, most of files in "./include" are common, simple, and shared files, which are not quite related with code analyzing (e.g. git log -p, or git blame), but they are read by others in most times. Is it correct? > Common sense needs to be used; horribly terrible coding style needs to be > fixed, sure. Is 82-characters long line horribly terrible coding style? > No, it's not. > For me, what you said above have effect on body files (in kernel, at least, more than 95% source files are body files, I guess). But in "./include", most of files are the interface inside and outside of our kernel, we need take more care about their coding styles. I often use vertical split window in vim in full screen mode to reading source code, when I read c source files, I often split window vertically for the related header files as reference. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/28/16 00:53, Theodore Ts'o wrote: > On Sat, Feb 27, 2016 at 10:32:04PM +0800, Chen Gang wrote: >> I don't think so. Of cause NOT the "CODE CHURN". It is not correct to >> make an early decision during discussing. > > There is no discussion. If the maintainer has NAK'ed it. That's the > end of the dicsussion. Period. See: > For me, NAK also needs reasons. And this issue is about "coding styles issue", I am not quite sure whether trivial patch maintainer and mm maintainer are also the maintainer for "coding styles issues". I guess they are related with this patch, and their NAKs' reason are: mm and trivial don't care about this coding style issue, is it correct? > ftp://ftp.kernel.org/pub/linux/kernel/people/rusty/trivial/template-index.html > > Also note the comment from the above: > >NOTE: This means I'll only take whitespace/indentation fixes from the >author or maintainer. OK, thanks, it is really one proof for us. :-) I guess, the file above almost means: except whitespace/indentation, trivial patches don't consider about the coding styles issues. But can we say coding styles issues are not issues in our kernel? (I guess not) If we can not say, I guess one of your suggestion is useful (maybe be as your suggestion): find one suitable member (I guess I am not), run "checkpatch -file" under "./include", and fix all reported issues. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/28/16 00:53, Theodore Ts'o wrote: > On Sat, Feb 27, 2016 at 10:32:04PM +0800, Chen Gang wrote: >> I don't think so. Of cause NOT the "CODE CHURN". It is not correct to >> make an early decision during discussing. > > There is no discussion. If the maintainer has NAK'ed it. That's the > end of the dicsussion. Period. See: > For me, NAK also needs reasons. And this issue is about "coding styles issue", I am not quite sure whether trivial patch maintainer and mm maintainer are also the maintainer for "coding styles issues". I guess they are related with this patch, and their NAKs' reason are: mm and trivial don't care about this coding style issue, is it correct? > ftp://ftp.kernel.org/pub/linux/kernel/people/rusty/trivial/template-index.html > > Also note the comment from the above: > >NOTE: This means I'll only take whitespace/indentation fixes from the >author or maintainer. OK, thanks, it is really one proof for us. :-) I guess, the file above almost means: except whitespace/indentation, trivial patches don't consider about the coding styles issues. But can we say coding styles issues are not issues in our kernel? (I guess not) If we can not say, I guess one of your suggestion is useful (maybe be as your suggestion): find one suitable member (I guess I am not), run "checkpatch -file" under "./include", and fix all reported issues. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/27/16 10:45, Theodore Ts'o wrote: > On Fri, Feb 26, 2016 at 11:26:02PM +0800, Chen Gang wrote: >>> As for coding style, actually IMHO this patch is even _not_ a coding >>> style, more like a code shuffle, indeed. >>> >> >> "80 column limitation" is about coding style, I guess, all of us agree >> with it. > > No, it's been accepted that checkpatch requiring people to reformat > code to within be 80 columns limitation was actively harmful, and it > no longer does that. > > Worse, it now complains when you split a printf string across lines, > so there were patches that split a string across multiple lines to > make checkpatch shut up. And now there are patches that join the > string back together. > > And if you now start submitting patches to split them up again because > you think the 80 column restriction is so darned important, that would > be even ***more*** code churn. > I don't think so. Of cause NOT the "CODE CHURN". It is not correct to make an early decision during discussing. "80 column limitation" is mentioned in "Documentation/CodingStyle", if we have very good reason for it, we can break this limitation (for me, what you said above are really some of good reasons). But in our case (the patch), can anybody find any "good reasons" for it? at least, at present, I can not find: - It is a common shared base header file, it is almost not used for code analyzing (e.g. git diff, git blame). - Is it helpful for "grep xxx filename | grep yyy"? Please check the patch, I can not find (maybe __GFP_MOVABL definition be? but it is still not obvious, if some member stick to, we can keep it no touch). - Could anyone find any good reasons for it within this patch? > Which is one of the reasons why some of us aren't terribly happy with > people who start running checkpatch -file on other people's code and > start submitting patches, either through the trivial patch portal or > not. > For me, as a individual developer, I don't like this way, either. So of cause, I don't care about this way. I am just reading the common shared header files about mm. At least, I can understand some common sense of mm, and also read through the whole other headers to know what they are. When I find something valuable more or less, I shall send related patch for it. > Mel, as an MM developer, has already NACK'ed the patch, which means > you should not send the patch to **any** upstream maintainer for > inclusion. I don't think I "should not ...". I only care about correctness and contribution, I don't care about any members ideas and their thinking. When we have different ideas or thinking, we need discuss. For common shared header files, for me, we should really take more care about the coding styles. - If the common shared header files don't care about the coding styles, I guess any body files will have much more excuses for "do not care about coding styles". - That means our kernel whole source files need not care about coding styles at all!! - It is really really VERY BAD!! If someone only dislike me to send the related patches, I suggest: Let another member(s) "run checkpatch -file" on the whole "./include" sub- directory, and fix all coding styles issues. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/27/16 10:45, Theodore Ts'o wrote: > On Fri, Feb 26, 2016 at 11:26:02PM +0800, Chen Gang wrote: >>> As for coding style, actually IMHO this patch is even _not_ a coding >>> style, more like a code shuffle, indeed. >>> >> >> "80 column limitation" is about coding style, I guess, all of us agree >> with it. > > No, it's been accepted that checkpatch requiring people to reformat > code to within be 80 columns limitation was actively harmful, and it > no longer does that. > > Worse, it now complains when you split a printf string across lines, > so there were patches that split a string across multiple lines to > make checkpatch shut up. And now there are patches that join the > string back together. > > And if you now start submitting patches to split them up again because > you think the 80 column restriction is so darned important, that would > be even ***more*** code churn. > I don't think so. Of cause NOT the "CODE CHURN". It is not correct to make an early decision during discussing. "80 column limitation" is mentioned in "Documentation/CodingStyle", if we have very good reason for it, we can break this limitation (for me, what you said above are really some of good reasons). But in our case (the patch), can anybody find any "good reasons" for it? at least, at present, I can not find: - It is a common shared base header file, it is almost not used for code analyzing (e.g. git diff, git blame). - Is it helpful for "grep xxx filename | grep yyy"? Please check the patch, I can not find (maybe __GFP_MOVABL definition be? but it is still not obvious, if some member stick to, we can keep it no touch). - Could anyone find any good reasons for it within this patch? > Which is one of the reasons why some of us aren't terribly happy with > people who start running checkpatch -file on other people's code and > start submitting patches, either through the trivial patch portal or > not. > For me, as a individual developer, I don't like this way, either. So of cause, I don't care about this way. I am just reading the common shared header files about mm. At least, I can understand some common sense of mm, and also read through the whole other headers to know what they are. When I find something valuable more or less, I shall send related patch for it. > Mel, as an MM developer, has already NACK'ed the patch, which means > you should not send the patch to **any** upstream maintainer for > inclusion. I don't think I "should not ...". I only care about correctness and contribution, I don't care about any members ideas and their thinking. When we have different ideas or thinking, we need discuss. For common shared header files, for me, we should really take more care about the coding styles. - If the common shared header files don't care about the coding styles, I guess any body files will have much more excuses for "do not care about coding styles". - That means our kernel whole source files need not care about coding styles at all!! - It is really really VERY BAD!! If someone only dislike me to send the related patches, I suggest: Let another member(s) "run checkpatch -file" on the whole "./include" sub- directory, and fix all coding styles issues. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/26/16 10:32, Jianyu Zhan wrote: > On Fri, Feb 26, 2016 at 6:29 AM, Chen Gang <cheng...@emindsoft.com.cn> wrote: >> git is a tool mainly for analyzing code, but not mainly for normal >> reading main code. >> >> So for me, the coding styles need not consider about git. > > For you, maybe yes. > > But for most of the developers/learners, git blame does help a lot. > Kernel code was not as complicated as it is now, it is keeping evolving. > Yes. > So basically a history chain is indispensable in studying such a complex > system. > git blame fits in this role. I benefited a lot from using it when I > started to learn the code, > And, a pure coding style fix is sometimes really troublesome as I > have to use git blame > to go another step up along the history chain, which is time > consuming and boring. > > But after all, I bet you will be fond of using it if you dive deeper > into the kernel code studying. > And if you do, you will know why so many developers in this thread > are so upset and allergic > to such coding-style fix. > For me, for discussion, I don't care about "so many developers", I only focus on the proof and the contribution. > As for coding style, actually IMHO this patch is even _not_ a coding > style, more like a code shuffle, indeed. > "80 column limitation" is about coding style, I guess, all of us agree with it. > And for your commit history, I found actually you have already > contributed some quit good patches. For me, I don't care about my history -- except some members find issues related with my original patches, I have duty to analyze the related issues together with the finders. > I don't think it is helpful for a non-layman contributor to keep > generating such code churn. > For me, we are discussing, so it is not quite suitable to make an early conclusion (code churn). For me, I don't care about layman or non-layman, I only focus on the proof and the contribution. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/26/16 10:32, Jianyu Zhan wrote: > On Fri, Feb 26, 2016 at 6:29 AM, Chen Gang wrote: >> git is a tool mainly for analyzing code, but not mainly for normal >> reading main code. >> >> So for me, the coding styles need not consider about git. > > For you, maybe yes. > > But for most of the developers/learners, git blame does help a lot. > Kernel code was not as complicated as it is now, it is keeping evolving. > Yes. > So basically a history chain is indispensable in studying such a complex > system. > git blame fits in this role. I benefited a lot from using it when I > started to learn the code, > And, a pure coding style fix is sometimes really troublesome as I > have to use git blame > to go another step up along the history chain, which is time > consuming and boring. > > But after all, I bet you will be fond of using it if you dive deeper > into the kernel code studying. > And if you do, you will know why so many developers in this thread > are so upset and allergic > to such coding-style fix. > For me, for discussion, I don't care about "so many developers", I only focus on the proof and the contribution. > As for coding style, actually IMHO this patch is even _not_ a coding > style, more like a code shuffle, indeed. > "80 column limitation" is about coding style, I guess, all of us agree with it. > And for your commit history, I found actually you have already > contributed some quit good patches. For me, I don't care about my history -- except some members find issues related with my original patches, I have duty to analyze the related issues together with the finders. > I don't think it is helpful for a non-layman contributor to keep > generating such code churn. > For me, we are discussing, so it is not quite suitable to make an early conclusion (code churn). For me, I don't care about layman or non-layman, I only focus on the proof and the contribution. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/26/16 07:12, SeongJae Park wrote: > > On Fri, 26 Feb 2016, Chen Gang wrote: > >> >> git is a tool mainly for analyzing code, but not mainly for normal >> reading main code. >> >> So for me, the coding styles need not consider about git. > > > It is common to see reject of trivial coding style fixup patch here and > there. Those patches usually be merged for early stage files that only > few people read / write. However, for files that are old and lots of > people read and write, those patches are rejected in usual. I mean, the > negative opinions for this patches are usual in this community. > > I agree that coding style is important and respect your effort. However, > because the code will be seen and written by most kernel hackers, the file > should be maintained to be easily readable and writable by most kernel > hackers, especially, maintainers. What I want to say is, we should > respect maintainers' opinion in usual. > Yes we need consider about the maintainers' options. And my another ideas are replied in the other thread, please check, and welcome any ideas, suggestion, and completions. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/26/16 07:12, SeongJae Park wrote: > > On Fri, 26 Feb 2016, Chen Gang wrote: > >> >> git is a tool mainly for analyzing code, but not mainly for normal >> reading main code. >> >> So for me, the coding styles need not consider about git. > > > It is common to see reject of trivial coding style fixup patch here and > there. Those patches usually be merged for early stage files that only > few people read / write. However, for files that are old and lots of > people read and write, those patches are rejected in usual. I mean, the > negative opinions for this patches are usual in this community. > > I agree that coding style is important and respect your effort. However, > because the code will be seen and written by most kernel hackers, the file > should be maintained to be easily readable and writable by most kernel > hackers, especially, maintainers. What I want to say is, we should > respect maintainers' opinion in usual. > Yes we need consider about the maintainers' options. And my another ideas are replied in the other thread, please check, and welcome any ideas, suggestion, and completions. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/26/16 06:39, Jiri Kosina wrote: > On Fri, 26 Feb 2016, Chen Gang wrote: > >> git is a tool mainly for analyzing code, but not mainly for normal >> reading main code. >> >> So for me, the coding styles need not consider about git. > > You are mistaken here. It's very helpful when debugging; For me, 'debugging' is related with debugger (e.g. kdb or kgdb), and 'tracing' is related with dumping log, and code analyzing is related with "git diff" and "git blame". And yes, for me, "git diff" and "git blame" is really very helpful for code analyzing. > usually you want > to find the commit that introduced particular change, and read its > changelog (at least). Having to cross rather pointless changes just adds > time (need to restart git-blame with commit~1 as a base) for no really > good reason. > That is the reason why I am not quite care about body files, I often use "git log -p filename", the cleanup code patch has negative effect with code analyzing (although for me, it should still need to be cleanup). But in our case, it is for the shared header file: - They are often the common base file, the main contents will not be changed quite often, and their contents are usually simple enough ( e.g. gfp.h in our case), they are not often for "code analyzing". - But they are quite often read in normal reading ways by programmers (e.g. open with normal editors). For normal reading, programmers usually care about the contents, not the changes. - So for me, the common shared header files need always take care about coding styles, and need not consider about code analyzing. And if we reject this kind of patch (in our case), I guess, that almost mean: "for the common shared header files, their bad coding styles will be remain for ever". Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/26/16 06:39, Jiri Kosina wrote: > On Fri, 26 Feb 2016, Chen Gang wrote: > >> git is a tool mainly for analyzing code, but not mainly for normal >> reading main code. >> >> So for me, the coding styles need not consider about git. > > You are mistaken here. It's very helpful when debugging; For me, 'debugging' is related with debugger (e.g. kdb or kgdb), and 'tracing' is related with dumping log, and code analyzing is related with "git diff" and "git blame". And yes, for me, "git diff" and "git blame" is really very helpful for code analyzing. > usually you want > to find the commit that introduced particular change, and read its > changelog (at least). Having to cross rather pointless changes just adds > time (need to restart git-blame with commit~1 as a base) for no really > good reason. > That is the reason why I am not quite care about body files, I often use "git log -p filename", the cleanup code patch has negative effect with code analyzing (although for me, it should still need to be cleanup). But in our case, it is for the shared header file: - They are often the common base file, the main contents will not be changed quite often, and their contents are usually simple enough ( e.g. gfp.h in our case), they are not often for "code analyzing". - But they are quite often read in normal reading ways by programmers (e.g. open with normal editors). For normal reading, programmers usually care about the contents, not the changes. - So for me, the common shared header files need always take care about coding styles, and need not consider about code analyzing. And if we reject this kind of patch (in our case), I guess, that almost mean: "for the common shared header files, their bad coding styles will be remain for ever". Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/26/16 00:07, Mel Gorman wrote: >>> On Thu, Feb 25, 2016 at 06:26:31AM +0800, cheng...@emindsoft.com.cn wrote: > > I do not want this patch to go through the trivial tree. It still adds > another step to identifying relevant commits through git blame and has > limited, if any, benefit to maintainability. > >> "it's preferable to preserve blame than go through a layer of cleanup >> when looking for the commit that defined particular flags". >> > > git blame identifies what commit last altered a line. If a cleanup patch > is encountered then the tree before that commit needs to be examined > which adds time. It's rare that cleanup patches on their own are useful > and this is one of those cases. > git is a tool mainly for analyzing code, but not mainly for normal reading main code. So for me, the coding styles need not consider about git. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/26/16 00:07, Mel Gorman wrote: >>> On Thu, Feb 25, 2016 at 06:26:31AM +0800, cheng...@emindsoft.com.cn wrote: > > I do not want this patch to go through the trivial tree. It still adds > another step to identifying relevant commits through git blame and has > limited, if any, benefit to maintainability. > >> "it's preferable to preserve blame than go through a layer of cleanup >> when looking for the commit that defined particular flags". >> > > git blame identifies what commit last altered a line. If a cleanup patch > is encountered then the tree before that commit needs to be examined > which adds time. It's rare that cleanup patches on their own are useful > and this is one of those cases. > git is a tool mainly for analyzing code, but not mainly for normal reading main code. So for me, the coding styles need not consider about git. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/25/16 23:12, Jiri Kosina wrote: > On Thu, 25 Feb 2016, Chen Gang wrote: > >> I can understand for your NAK, it is a trivial patch. > > Not all trivial patches are NAKed :) But they have to be generally useful. > > Shuffling code around, without actually changing / improving it a bit, > just for the sole purpose of formatting, is kind of pointless (especially > given the fact that the current code as-is is easily readable; it's not > like it'd be a horrible mess difficult to understand). > > Sure, it might had been formatted better at the time it was actually > merged. But changing it "just because" after being in tree for long time > doesn't fix any problem really. > OK, thanks. I have replied the related contents in the other thread. Welcome any ideas, suggestions, and completions in the other related thread. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
Re: [PATCH trivial] include/linux/gfp.h: Improve the coding styles
On 2/25/16 23:12, Jiri Kosina wrote: > On Thu, 25 Feb 2016, Chen Gang wrote: > >> I can understand for your NAK, it is a trivial patch. > > Not all trivial patches are NAKed :) But they have to be generally useful. > > Shuffling code around, without actually changing / improving it a bit, > just for the sole purpose of formatting, is kind of pointless (especially > given the fact that the current code as-is is easily readable; it's not > like it'd be a horrible mess difficult to understand). > > Sure, it might had been formatted better at the time it was actually > merged. But changing it "just because" after being in tree for long time > doesn't fix any problem really. > OK, thanks. I have replied the related contents in the other thread. Welcome any ideas, suggestions, and completions in the other related thread. Thanks. -- Chen Gang (陈刚) Managing Natural Environments is the Duty of Human Beings.
