2012-05-28 09:04:18

by Alex Shi

[permalink] [raw]
Subject: [PATCH] cpumask: add a few comments of cpumask functions

Current few cpumask function purpose are not quite clear. Stupid
user like myself need to dig into details for clear function
purpose and return value.
Add few explanation for them is helpful.

Signed-off-by: Alex Shi <[email protected]>
---
include/linux/cpumask.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index a2c819d..8436e61 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -271,6 +271,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
* cpumask_test_cpu - test for a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
*
* No static inline type checking - see Subtlety (1) above.
*/
@@ -281,6 +282,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
* cpumask_test_and_set_cpu - atomically test and set a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
*
* test_and_set_bit wrapper for cpumasks.
*/
@@ -293,6 +295,7 @@ static inline int cpumask_test_and_set_cpu(int cpu, struct cpumask *cpumask)
* cpumask_test_and_clear_cpu - atomically test and clear a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
*
* test_and_clear_bit wrapper for cpumasks.
*/
@@ -324,6 +327,7 @@ static inline void cpumask_clear(struct cpumask *dstp)
* @dstp: the cpumask result
* @src1p: the first input
* @src2p: the second input
+ * if *dstp is empty, return 0, otherwise return 1
*/
static inline int cpumask_and(struct cpumask *dstp,
const struct cpumask *src1p,
@@ -365,6 +369,7 @@ static inline void cpumask_xor(struct cpumask *dstp,
* @dstp: the cpumask result
* @src1p: the first input
* @src2p: the second input
+ * if *dstp is empty, return 0, otherwise return 1
*/
static inline int cpumask_andnot(struct cpumask *dstp,
const struct cpumask *src1p,
@@ -414,6 +419,7 @@ static inline bool cpumask_intersects(const struct cpumask *src1p,
* cpumask_subset - (*src1p & ~*src2p) == 0
* @src1p: the first input
* @src2p: the second input
+ * return 1 if the *src1p is the subset of *src2p, otherwise return 0
*/
static inline int cpumask_subset(const struct cpumask *src1p,
const struct cpumask *src2p)
--
1.7.5.4


2012-05-28 12:55:19

by Srivatsa S. Bhat

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions

On 05/28/2012 02:32 PM, Alex Shi wrote:

> Current few cpumask function purpose are not quite clear. Stupid
> user like myself need to dig into details for clear function
> purpose and return value.


You can just see how it is used elsewhere and figure it out ;-)
Anyway, in principle, I don't have any objections to adding comments
that are actually helpful. But I don't think all the comments this
patch adds fall into that category..

> Add few explanation for them is helpful.
>
> Signed-off-by: Alex Shi <[email protected]>
> ---
> include/linux/cpumask.h | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index a2c819d..8436e61 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -271,6 +271,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
> * cpumask_test_cpu - test for a cpu in a cpumask
> * @cpu: cpu number (< nr_cpu_ids)
> * @cpumask: the cpumask pointer
> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
> *


s/return/Returns

What do you mean by "old" cpumask?

> * No static inline type checking - see Subtlety (1) above.
> */
> @@ -281,6 +282,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
> * cpumask_test_and_set_cpu - atomically test and set a cpu in a cpumask
> * @cpu: cpu number (< nr_cpu_ids)
> * @cpumask: the cpumask pointer
> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
> *


"Test and Set" already has a very well-defined and well-known meaning.. Not sure if the
comment adds any extra value..

> * test_and_set_bit wrapper for cpumasks.
> */
> @@ -293,6 +295,7 @@ static inline int cpumask_test_and_set_cpu(int cpu, struct cpumask *cpumask)
> * cpumask_test_and_clear_cpu - atomically test and clear a cpu in a cpumask
> * @cpu: cpu number (< nr_cpu_ids)
> * @cpumask: the cpumask pointer
> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
> *


Same here. Pretty well known meaning.

> * test_and_clear_bit wrapper for cpumasks.
> */
> @@ -324,6 +327,7 @@ static inline void cpumask_clear(struct cpumask *dstp)
> * @dstp: the cpumask result
> * @src1p: the first input
> * @src2p: the second input
> + * if *dstp is empty, return 0, otherwise return 1


I don't think anybody would be interested in the return value of this function!
The functionality (rather than the return value) is what is more interesting
and useful, and is already well-documented.

> */
> static inline int cpumask_and(struct cpumask *dstp,
> const struct cpumask *src1p,
> @@ -365,6 +369,7 @@ static inline void cpumask_xor(struct cpumask *dstp,
> * @dstp: the cpumask result
> * @src1p: the first input
> * @src2p: the second input
> + * if *dstp is empty, return 0, otherwise return 1


Same here. Functionality is more interesting.

> */
> static inline int cpumask_andnot(struct cpumask *dstp,
> const struct cpumask *src1p,
> @@ -414,6 +419,7 @@ static inline bool cpumask_intersects(const struct cpumask *src1p,
> * cpumask_subset - (*src1p & ~*src2p) == 0
> * @src1p: the first input
> * @src2p: the second input
> + * return 1 if the *src1p is the subset of *src2p, otherwise return 0


s/return/Returns
(I think the function name itself is pretty descriptive, but anyway...)

> */
> static inline int cpumask_subset(const struct cpumask *src1p,
> const struct cpumask *src2p)



Regards,
Srivatsa S. Bhat

2012-05-28 13:49:54

by Alex Shi

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions

On 05/28/2012 08:47 PM, Srivatsa S. Bhat wrote:

> On 05/28/2012 02:32 PM, Alex Shi wrote:
>
>> Current few cpumask function purpose are not quite clear. Stupid
>> user like myself need to dig into details for clear function
>> purpose and return value.
>
>
> You can just see how it is used elsewhere and figure it out ;-)
> Anyway, in principle, I don't have any objections to adding comments
> that are actually helpful. But I don't think all the comments this
> patch adds fall into that category..
>
>> Add few explanation for them is helpful.
>>
>> Signed-off-by: Alex Shi <[email protected]>
>> ---
>> include/linux/cpumask.h | 6 ++++++
>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>
>> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
>> index a2c819d..8436e61 100644
>> --- a/include/linux/cpumask.h
>> +++ b/include/linux/cpumask.h
>> @@ -271,6 +271,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
>> * cpumask_test_cpu - test for a cpu in a cpumask
>> * @cpu: cpu number (< nr_cpu_ids)
>> * @cpumask: the cpumask pointer
>> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
>> *
>
>
> s/return/Returns
>
> What do you mean by "old" cpumask?




Thanks for comments!
Should be "the old bitmap of cpumask"?

>
>> * No static inline type checking - see Subtlety (1) above.
>> */
>> @@ -281,6 +282,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
>> * cpumask_test_and_set_cpu - atomically test and set a cpu in a cpumask
>> * @cpu: cpu number (< nr_cpu_ids)
>> * @cpumask: the cpumask pointer
>> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
>> *
>
>
> "Test and Set" already has a very well-defined and well-known meaning.. Not sure if the
> comment adds any extra value..




Yes, but it is still helpful for newbie. :)

>
>> * test_and_set_bit wrapper for cpumasks.
>> */
>> @@ -293,6 +295,7 @@ static inline int cpumask_test_and_set_cpu(int cpu, struct cpumask *cpumask)
>> * cpumask_test_and_clear_cpu - atomically test and clear a cpu in a cpumask
>> * @cpu: cpu number (< nr_cpu_ids)
>> * @cpumask: the cpumask pointer
>> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
>> *
>
>
> Same here. Pretty well known meaning.
>
>> * test_and_clear_bit wrapper for cpumasks.
>> */
>> @@ -324,6 +327,7 @@ static inline void cpumask_clear(struct cpumask *dstp)
>> * @dstp: the cpumask result
>> * @src1p: the first input
>> * @src2p: the second input
>> + * if *dstp is empty, return 0, otherwise return 1
>
>
> I don't think anybody would be interested in the return value of this function!
> The functionality (rather than the return value) is what is more interesting
> and useful, and is already well-documented.




:) it is true none is using the return value, but why not to have a comments
if we have this return value.

>
>> */
>> static inline int cpumask_and(struct cpumask *dstp,
>> const struct cpumask *src1p,
>> @@ -365,6 +369,7 @@ static inline void cpumask_xor(struct cpumask *dstp,
>> * @dstp: the cpumask result
>> * @src1p: the first input
>> * @src2p: the second input
>> + * if *dstp is empty, return 0, otherwise return 1
>
>
> Same here. Functionality is more interesting.
>
>> */
>> static inline int cpumask_andnot(struct cpumask *dstp,
>> const struct cpumask *src1p,
>> @@ -414,6 +419,7 @@ static inline bool cpumask_intersects(const struct cpumask *src1p,
>> * cpumask_subset - (*src1p & ~*src2p) == 0
>> * @src1p: the first input
>> * @src2p: the second input
>> + * return 1 if the *src1p is the subset of *src2p, otherwise return 0
>
>
> s/return/Returns
> (I think the function name itself is pretty descriptive, but anyway...)




Just this function make me confusing, because I missed the '== 0' in
"*src1p & ~*src2p) == 0", than I thought scr1p should be the mother mask
and scr2p is daughter. :)

>
>> */
>> static inline int cpumask_subset(const struct cpumask *src1p,
>> const struct cpumask *src2p)
>
>
>
> Regards,
> Srivatsa S. Bhat



Thanks for all comments, I updated the patch according to your some suggestion.
Anyway, I don't mind if the patch is picked up. Maybe there is only one stupid
guy in the world. :)
----------

>From caf6e32ec4f335064f067904f24fb9f9f90312df Mon Sep 17 00:00:00 2001
From: Alex Shi <[email protected]>
Date: Mon, 28 May 2012 15:53:51 +0800
Subject: [PATCH] cpumask: add a few comments of cpumask functions

Current few cpumask functions' purposes are not quite clear. Stupid
user like myself need to dig into details for clear function
purpose and return value.
Add few explanation for them is helpful.

Signed-off-by: Alex Shi <[email protected]>
---
include/linux/cpumask.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index a2c819d..177e5e8 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -271,6 +271,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
* cpumask_test_cpu - test for a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * Return 1 if the 'cpu' is in the old bitmap of 'cpumask', otherwise return 0
*
* No static inline type checking - see Subtlety (1) above.
*/
@@ -281,6 +282,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
* cpumask_test_and_set_cpu - atomically test and set a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * Return 1 if the 'cpu' is in the old bitmap of 'cpumask', otherwise return 0
*
* test_and_set_bit wrapper for cpumasks.
*/
@@ -293,6 +295,7 @@ static inline int cpumask_test_and_set_cpu(int cpu, struct cpumask *cpumask)
* cpumask_test_and_clear_cpu - atomically test and clear a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * Return 1 if the 'cpu' is in the old bitmap of 'cpumask', otherwise return 0
*
* test_and_clear_bit wrapper for cpumasks.
*/
@@ -324,6 +327,7 @@ static inline void cpumask_clear(struct cpumask *dstp)
* @dstp: the cpumask result
* @src1p: the first input
* @src2p: the second input
+ * If *dstp is empty, return 0, otherwise return 1
*/
static inline int cpumask_and(struct cpumask *dstp,
const struct cpumask *src1p,
@@ -365,6 +369,7 @@ static inline void cpumask_xor(struct cpumask *dstp,
* @dstp: the cpumask result
* @src1p: the first input
* @src2p: the second input
+ * If *dstp is empty, return 0, otherwise return 1
*/
static inline int cpumask_andnot(struct cpumask *dstp,
const struct cpumask *src1p,
@@ -414,6 +419,7 @@ static inline bool cpumask_intersects(const struct cpumask *src1p,
* cpumask_subset - (*src1p & ~*src2p) == 0
* @src1p: the first input
* @src2p: the second input
+ * Return 1 if the *src1p is the subset of *src2p, otherwise return 0
*/
static inline int cpumask_subset(const struct cpumask *src1p,
const struct cpumask *src2p)
--
1.7.5.4

2012-05-28 14:23:58

by Alex Shi

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions

>

> Thanks for all comments, I updated the patch according to your some suggestion.
> Anyway, I don't mind if the patch is picked up. Maybe there is only one stupid
> guy in the world. :)
> ----------




Sorry, missed 's' for verb's singular form in third personal.

---
>From e33d4fb4a6730832c08979f2cfdf65795cefd08a Mon Sep 17 00:00:00 2001
From: Alex Shi <[email protected]>
Date: Mon, 28 May 2012 15:53:51 +0800
Subject: [PATCH] cpumask: add a few comments of cpumask functions

Current few cpumask functions' purposes are not quite clear. Stupid
user like myself need to dig into details for clear function
purpose and return value.
Add few explanation for them is helpful.

Signed-off-by: Alex Shi <[email protected]>
---
include/linux/cpumask.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index a2c819d..7fb07ac 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -271,6 +271,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
* cpumask_test_cpu - test for a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * Returns 1 if the 'cpu' is in the old bitmap of 'cpumask', otherwise returns 0
*
* No static inline type checking - see Subtlety (1) above.
*/
@@ -281,6 +282,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
* cpumask_test_and_set_cpu - atomically test and set a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * Returns 1 if the 'cpu' is in the old bitmap of 'cpumask', otherwise returns 0
*
* test_and_set_bit wrapper for cpumasks.
*/
@@ -293,6 +295,7 @@ static inline int cpumask_test_and_set_cpu(int cpu, struct cpumask *cpumask)
* cpumask_test_and_clear_cpu - atomically test and clear a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * Returns 1 if the 'cpu' is in the old bitmap of 'cpumask', otherwise returns 0
*
* test_and_clear_bit wrapper for cpumasks.
*/
@@ -324,6 +327,7 @@ static inline void cpumask_clear(struct cpumask *dstp)
* @dstp: the cpumask result
* @src1p: the first input
* @src2p: the second input
+ * If *dstp is empty, returns 0, otherwise returns 1
*/
static inline int cpumask_and(struct cpumask *dstp,
const struct cpumask *src1p,
@@ -365,6 +369,7 @@ static inline void cpumask_xor(struct cpumask *dstp,
* @dstp: the cpumask result
* @src1p: the first input
* @src2p: the second input
+ * If *dstp is empty, returns 0, otherwise returns 1
*/
static inline int cpumask_andnot(struct cpumask *dstp,
const struct cpumask *src1p,
@@ -414,6 +419,7 @@ static inline bool cpumask_intersects(const struct cpumask *src1p,
* cpumask_subset - (*src1p & ~*src2p) == 0
* @src1p: the first input
* @src2p: the second input
+ * Returns 1 if the *src1p is the subset of *src2p, otherwise returns 0
*/
static inline int cpumask_subset(const struct cpumask *src1p,
const struct cpumask *src2p)
--
1.7.5.4

2012-05-28 15:26:45

by Srivatsa S. Bhat

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions

On 05/28/2012 07:19 PM, Alex Shi wrote:

> On 05/28/2012 08:47 PM, Srivatsa S. Bhat wrote:
>
>> On 05/28/2012 02:32 PM, Alex Shi wrote:
>>
>>> Current few cpumask function purpose are not quite clear. Stupid
>>> user like myself need to dig into details for clear function
>>> purpose and return value.
>>
>>
>> You can just see how it is used elsewhere and figure it out ;-)
>> Anyway, in principle, I don't have any objections to adding comments
>> that are actually helpful. But I don't think all the comments this
>> patch adds fall into that category..
>>
>>> Add few explanation for them is helpful.
>>>
>>> Signed-off-by: Alex Shi <[email protected]>
>>> ---
>>> include/linux/cpumask.h | 6 ++++++
>>> 1 files changed, 6 insertions(+), 0 deletions(-)
>>>
>>> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
>>> index a2c819d..8436e61 100644
>>> --- a/include/linux/cpumask.h
>>> +++ b/include/linux/cpumask.h
>>> @@ -271,6 +271,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
>>> * cpumask_test_cpu - test for a cpu in a cpumask
>>> * @cpu: cpu number (< nr_cpu_ids)
>>> * @cpumask: the cpumask pointer
>>> + * return 1 if the 'cpu' is in the old 'cpumask', otherwise return 0
>>> *
>>
>>
>> s/return/Returns
>>
>> What do you mean by "old" cpumask?
>
> Thanks for comments!
> Should be "the old bitmap of cpumask"?
>


No, there is no "old" or "new" cpumask here because this function doesn't
change the cpumask. It just checks if that bit is set.
So something like:
Returns 1 if 'cpu' is set in 'cpumask', else returns 0.

Regards,
Srivatsa S. Bhat

2012-05-29 01:18:13

by Alex Shi

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions


>>> s/return/Returns
>>>
>>> What do you mean by "old" cpumask?
>>
>> Thanks for comments!
>> Should be "the old bitmap of cpumask"?
>>
>
>
> No, there is no "old" or "new" cpumask here because this function doesn't
> change the cpumask. It just checks if that bit is set.
> So something like:
> Returns 1 if 'cpu' is set in 'cpumask', else returns 0.
>



Sure, thanks! and comments changed again following your style.
Is the following new update patch ok?


>From 1daa52caf1ba83eea0975a9250becd85dd5a5315 Mon Sep 17 00:00:00 2001
From: Alex Shi <[email protected]>
Date: Mon, 28 May 2012 22:23:51 +0800
Subject: [PATCH] cpumask: add a few comments of cpumask functions

Current few cpumask functions' purposes are not quite clear. Stupid
user like myself needs to dig into details for clear function
purpose and return value.
Add few explanation for them is helpful.

Thanks for Srivatsa's comments and correction!

Signed-off-by: Alex Shi <[email protected]>
---
include/linux/cpumask.h | 6 ++++++
1 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
index a2c819d..170c5af 100644
--- a/include/linux/cpumask.h
+++ b/include/linux/cpumask.h
@@ -271,6 +271,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
* cpumask_test_cpu - test for a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * Returns 1 if 'cpu' is set in 'cpumask', else returns 0
*
* No static inline type checking - see Subtlety (1) above.
*/
@@ -281,6 +282,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
* cpumask_test_and_set_cpu - atomically test and set a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * Returns 1 if 'cpu' is set in old bitmap of 'cpumask', else returns 0
*
* test_and_set_bit wrapper for cpumasks.
*/
@@ -293,6 +295,7 @@ static inline int cpumask_test_and_set_cpu(int cpu, struct cpumask *cpumask)
* cpumask_test_and_clear_cpu - atomically test and clear a cpu in a cpumask
* @cpu: cpu number (< nr_cpu_ids)
* @cpumask: the cpumask pointer
+ * Returns 1 if 'cpu' is set in old bitmap of 'cpumask', else returns 0
*
* test_and_clear_bit wrapper for cpumasks.
*/
@@ -324,6 +327,7 @@ static inline void cpumask_clear(struct cpumask *dstp)
* @dstp: the cpumask result
* @src1p: the first input
* @src2p: the second input
+ * If *dstp is empty, returns 0, else returns 1
*/
static inline int cpumask_and(struct cpumask *dstp,
const struct cpumask *src1p,
@@ -365,6 +369,7 @@ static inline void cpumask_xor(struct cpumask *dstp,
* @dstp: the cpumask result
* @src1p: the first input
* @src2p: the second input
+ * If *dstp is empty, returns 0, else returns 1
*/
static inline int cpumask_andnot(struct cpumask *dstp,
const struct cpumask *src1p,
@@ -414,6 +419,7 @@ static inline bool cpumask_intersects(const struct cpumask *src1p,
* cpumask_subset - (*src1p & ~*src2p) == 0
* @src1p: the first input
* @src2p: the second input
+ * Returns 1 if *src1p is a subset of *src2p, else returns 0
*/
static inline int cpumask_subset(const struct cpumask *src1p,
const struct cpumask *src2p)
--
1.7.5.4


2012-05-30 10:41:55

by Alex Shi

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions

On 05/29/2012 09:16 AM, Alex Shi wrote:

>
>>>> s/return/Returns
>>>>
>>>> What do you mean by "old" cpumask?
>>>
>>> Thanks for comments!
>>> Should be "the old bitmap of cpumask"?
>>>
>>
>>
>> No, there is no "old" or "new" cpumask here because this function doesn't
>> change the cpumask. It just checks if that bit is set.
>> So something like:
>> Returns 1 if 'cpu' is set in 'cpumask', else returns 0.
>>
>
>
>
> Sure, thanks! and comments changed again following your style.
> Is the following new update patch ok?


Any further comments for this patch? :)

>
>
> From 1daa52caf1ba83eea0975a9250becd85dd5a5315 Mon Sep 17 00:00:00 2001
> From: Alex Shi <[email protected]>
> Date: Mon, 28 May 2012 22:23:51 +0800
> Subject: [PATCH] cpumask: add a few comments of cpumask functions
>
> Current few cpumask functions' purposes are not quite clear. Stupid
> user like myself needs to dig into details for clear function
> purpose and return value.
> Add few explanation for them is helpful.
>
> Thanks for Srivatsa's comments and correction!
>
> Signed-off-by: Alex Shi <[email protected]>
> ---
> include/linux/cpumask.h | 6 ++++++
> 1 files changed, 6 insertions(+), 0 deletions(-)
>
> diff --git a/include/linux/cpumask.h b/include/linux/cpumask.h
> index a2c819d..170c5af 100644
> --- a/include/linux/cpumask.h
> +++ b/include/linux/cpumask.h
> @@ -271,6 +271,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
> * cpumask_test_cpu - test for a cpu in a cpumask
> * @cpu: cpu number (< nr_cpu_ids)
> * @cpumask: the cpumask pointer
> + * Returns 1 if 'cpu' is set in 'cpumask', else returns 0
> *
> * No static inline type checking - see Subtlety (1) above.
> */
> @@ -281,6 +282,7 @@ static inline void cpumask_clear_cpu(int cpu, struct cpumask *dstp)
> * cpumask_test_and_set_cpu - atomically test and set a cpu in a cpumask
> * @cpu: cpu number (< nr_cpu_ids)
> * @cpumask: the cpumask pointer
> + * Returns 1 if 'cpu' is set in old bitmap of 'cpumask', else returns 0
> *
> * test_and_set_bit wrapper for cpumasks.
> */
> @@ -293,6 +295,7 @@ static inline int cpumask_test_and_set_cpu(int cpu, struct cpumask *cpumask)
> * cpumask_test_and_clear_cpu - atomically test and clear a cpu in a cpumask
> * @cpu: cpu number (< nr_cpu_ids)
> * @cpumask: the cpumask pointer
> + * Returns 1 if 'cpu' is set in old bitmap of 'cpumask', else returns 0
> *
> * test_and_clear_bit wrapper for cpumasks.
> */
> @@ -324,6 +327,7 @@ static inline void cpumask_clear(struct cpumask *dstp)
> * @dstp: the cpumask result
> * @src1p: the first input
> * @src2p: the second input
> + * If *dstp is empty, returns 0, else returns 1
> */
> static inline int cpumask_and(struct cpumask *dstp,
> const struct cpumask *src1p,
> @@ -365,6 +369,7 @@ static inline void cpumask_xor(struct cpumask *dstp,
> * @dstp: the cpumask result
> * @src1p: the first input
> * @src2p: the second input
> + * If *dstp is empty, returns 0, else returns 1
> */
> static inline int cpumask_andnot(struct cpumask *dstp,
> const struct cpumask *src1p,
> @@ -414,6 +419,7 @@ static inline bool cpumask_intersects(const struct cpumask *src1p,
> * cpumask_subset - (*src1p & ~*src2p) == 0
> * @src1p: the first input
> * @src2p: the second input
> + * Returns 1 if *src1p is a subset of *src2p, else returns 0
> */
> static inline int cpumask_subset(const struct cpumask *src1p,
> const struct cpumask *src2p)

2012-05-30 10:50:20

by KOSAKI Motohiro

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions

On Wed, May 30, 2012 at 6:40 AM, Alex Shi <[email protected]> wrote:
> On 05/29/2012 09:16 AM, Alex Shi wrote:
>
>>
>>>>> s/return/Returns
>>>>>
>>>>> What do you mean by "old" cpumask?
>>>>
>>>> Thanks for comments!
>>>> Should be "the old bitmap of cpumask"?
>>>>
>>>
>>>
>>> No, there is no "old" or "new" cpumask here because this function doesn't
>>> change the cpumask. It just checks if that bit is set.
>>> So something like:
>>> Returns 1 if 'cpu' is set in 'cpumask', else returns 0.
>>>
>>
>>
>>
>> Sure, thanks! and comments changed again following your style.
>> Is the following new update patch ok?
>
>
> Any further comments for this patch? :)

Acked-by: KOSAKI Motohiro <[email protected]>

2012-05-31 08:17:10

by Alex Shi

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions

On 05/30/2012 06:49 PM, KOSAKI Motohiro wrote:

> On Wed, May 30, 2012 at 6:40 AM, Alex Shi <[email protected]> wrote:
>> On 05/29/2012 09:16 AM, Alex Shi wrote:
>>
>>>
>>>>>> s/return/Returns
>>>>>>
>>>>>> What do you mean by "old" cpumask?
>>>>>
>>>>> Thanks for comments!
>>>>> Should be "the old bitmap of cpumask"?
>>>>>
>>>>
>>>>
>>>> No, there is no "old" or "new" cpumask here because this function doesn't
>>>> change the cpumask. It just checks if that bit is set.
>>>> So something like:
>>>> Returns 1 if 'cpu' is set in 'cpumask', else returns 0.
>>>>
>>>
>>>
>>>
>>> Sure, thanks! and comments changed again following your style.
>>> Is the following new update patch ok?
>>
>>
>> Any further comments for this patch? :)
>
> Acked-by: KOSAKI Motohiro <[email protected]>


Thanks!
Does someone like to pick up this patch? :)

2012-06-04 04:48:44

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions

On Mon, 28 May 2012 21:49:43 +0800, Alex Shi <[email protected]> wrote:
> On 05/28/2012 08:47 PM, Srivatsa S. Bhat wrote:
> > "Test and Set" already has a very well-defined and well-known meaning.. Not sure if the
> > comment adds any extra value..
>
> Yes, but it is still helpful for newbie. :)

I wonder if we should just make it clear that you should see
linux/bitmap.h, as these are designed to be exactly synonymous.

Thanks,
Rusty.

2012-06-04 04:48:43

by Rusty Russell

[permalink] [raw]
Subject: Re: [PATCH] cpumask: add a few comments of cpumask functions

On Thu, 31 May 2012 16:15:25 +0800, Alex Shi <[email protected]> wrote:
> On 05/30/2012 06:49 PM, KOSAKI Motohiro wrote:
>
> > On Wed, May 30, 2012 at 6:40 AM, Alex Shi <[email protected]> wrote:
> >> On 05/29/2012 09:16 AM, Alex Shi wrote:
> >>
> >>>
> >>>>>> s/return/Returns
> >>>>>>
> >>>>>> What do you mean by "old" cpumask?
> >>>>>
> >>>>> Thanks for comments!
> >>>>> Should be "the old bitmap of cpumask"?
> >>>>>
> >>>>
> >>>>
> >>>> No, there is no "old" or "new" cpumask here because this function doesn't
> >>>> change the cpumask. It just checks if that bit is set.
> >>>> So something like:
> >>>> Returns 1 if 'cpu' is set in 'cpumask', else returns 0.
> >>>>
> >>>
> >>>
> >>>
> >>> Sure, thanks! and comments changed again following your style.
> >>> Is the following new update patch ok?
> >>
> >>
> >> Any further comments for this patch? :)
> >
> > Acked-by: KOSAKI Motohiro <[email protected]>
>
>
> Thanks!
> Does someone like to pick up this patch? :)

Applied, thanks!

Cheers,
Rusty.