The variable 'new_usage' in misc_cg_try_charge() may overflow if it
becomes above INT_MAX. This was observed when I implement the new SGX
EPC cgroup[1] as a misc cgroup and test on a platform with large SGX EPC
sizes.
Change type of new_usage to long from int and check overflow.
Fixes: a72232eabdfcf ("cgroup: Add misc cgroup controller")
Signed-off-by: Haitao Huang <[email protected]>
[1] https://lore.kernel.org/linux-sgx/[email protected]/
---
kernel/cgroup/misc.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
index fe3e8a0eb7ed..ff9f900981a3 100644
--- a/kernel/cgroup/misc.c
+++ b/kernel/cgroup/misc.c
@@ -143,7 +143,7 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
struct misc_cg *i, *j;
int ret;
struct misc_res *res;
- int new_usage;
+ long new_usage;
if (!(valid_type(type) && cg && READ_ONCE(misc_res_capacity[type])))
return -EINVAL;
@@ -153,10 +153,10 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
for (i = cg; i; i = parent_misc(i)) {
res = &i->res[type];
-
new_usage = atomic_long_add_return(amount, &res->usage);
if (new_usage > READ_ONCE(res->max) ||
- new_usage > READ_ONCE(misc_res_capacity[type])) {
+ new_usage > READ_ONCE(misc_res_capacity[type]) ||
+ new_usage < 0) {
ret = -EBUSY;
goto err_charge;
}
--
2.25.1
On Mon, 17 Jul 2023 13:57:59 -0500, Tejun Heo <[email protected]> wrote:
> On Mon, Jul 17, 2023 at 06:55:32PM +0000, Jarkko Sakkinen wrote:
>> On Mon Jul 17, 2023 at 6:47 PM UTC, Haitao Huang wrote:
>> > The variable 'new_usage' in misc_cg_try_charge() may overflow if it
>> > becomes above INT_MAX. This was observed when I implement the new SGX
>> > EPC cgroup[1] as a misc cgroup and test on a platform with large SGX
>> EPC
>> > sizes.
>> >
>> > Change type of new_usage to long from int and check overflow.
>> >
>> > Fixes: a72232eabdfcf ("cgroup: Add misc cgroup controller")
>> > Signed-off-by: Haitao Huang <[email protected]>
>> >
>> > [1]
>> https://lore.kernel.org/linux-sgx/[email protected]/
>> > ---
>> > kernel/cgroup/misc.c | 6 +++---
>> > 1 file changed, 3 insertions(+), 3 deletions(-)
>> >
>> > diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
>> > index fe3e8a0eb7ed..ff9f900981a3 100644
>> > --- a/kernel/cgroup/misc.c
>> > +++ b/kernel/cgroup/misc.c
>> > @@ -143,7 +143,7 @@ int misc_cg_try_charge(enum misc_res_type type,
>> struct misc_cg *cg,
>> > struct misc_cg *i, *j;
>> > int ret;
>> > struct misc_res *res;
>> > - int new_usage;
>> > + long new_usage;
>> >
>> > if (!(valid_type(type) && cg && READ_ONCE(misc_res_capacity[type])))
>> > return -EINVAL;
>> > @@ -153,10 +153,10 @@ int misc_cg_try_charge(enum misc_res_type type,
>> struct misc_cg *cg,
>> >
>> > for (i = cg; i; i = parent_misc(i)) {
>> > res = &i->res[type];
>> > -
>>
>> This is extra noise in the patch, please remove the change.
>
> Lemme just revert it. Haitao, can you instead make the resource counters
> and
> all related variables explicit 64bit instead?
>
Will do.
Thanks
Haitao
On Mon, Jul 17, 2023 at 11:47:19AM -0700, Haitao Huang wrote:
> The variable 'new_usage' in misc_cg_try_charge() may overflow if it
> becomes above INT_MAX. This was observed when I implement the new SGX
> EPC cgroup[1] as a misc cgroup and test on a platform with large SGX EPC
> sizes.
>
> Change type of new_usage to long from int and check overflow.
> - int new_usage;
> + long new_usage;
>
> if (!(valid_type(type) && cg && READ_ONCE(misc_res_capacity[type])))
> return -EINVAL;
> @@ -153,10 +153,10 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
>
> for (i = cg; i; i = parent_misc(i)) {
> res = &i->res[type];
> -
> new_usage = atomic_long_add_return(amount, &res->usage);
> if (new_usage > READ_ONCE(res->max) ||
> - new_usage > READ_ONCE(misc_res_capacity[type])) {
> + new_usage > READ_ONCE(misc_res_capacity[type]) ||
> + new_usage < 0) {
Applying to cgroup/for-6.6 (as none of the current users are affected by
this) but I think the right thing to do here is using explicit 64bit types
(s64 or u64) for the resource counters rather than depending on the long
width.
Thanks.
--
tejun
On Mon Jul 17, 2023 at 6:47 PM UTC, Haitao Huang wrote:
> The variable 'new_usage' in misc_cg_try_charge() may overflow if it
> becomes above INT_MAX. This was observed when I implement the new SGX
> EPC cgroup[1] as a misc cgroup and test on a platform with large SGX EPC
> sizes.
>
> Change type of new_usage to long from int and check overflow.
>
> Fixes: a72232eabdfcf ("cgroup: Add misc cgroup controller")
> Signed-off-by: Haitao Huang <[email protected]>
>
> [1] https://lore.kernel.org/linux-sgx/[email protected]/
> ---
> kernel/cgroup/misc.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
> index fe3e8a0eb7ed..ff9f900981a3 100644
> --- a/kernel/cgroup/misc.c
> +++ b/kernel/cgroup/misc.c
> @@ -143,7 +143,7 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
> struct misc_cg *i, *j;
> int ret;
> struct misc_res *res;
> - int new_usage;
> + long new_usage;
>
> if (!(valid_type(type) && cg && READ_ONCE(misc_res_capacity[type])))
> return -EINVAL;
> @@ -153,10 +153,10 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
>
> for (i = cg; i; i = parent_misc(i)) {
> res = &i->res[type];
> -
This is extra noise in the patch, please remove the change.
> new_usage = atomic_long_add_return(amount, &res->usage);
> if (new_usage > READ_ONCE(res->max) ||
> - new_usage > READ_ONCE(misc_res_capacity[type])) {
> + new_usage > READ_ONCE(misc_res_capacity[type]) ||
> + new_usage < 0) {
> ret = -EBUSY;
> goto err_charge;
> }
> --
> 2.25.1
BR, Jarkko
On Mon, Jul 17, 2023 at 06:55:32PM +0000, Jarkko Sakkinen wrote:
> On Mon Jul 17, 2023 at 6:47 PM UTC, Haitao Huang wrote:
> > The variable 'new_usage' in misc_cg_try_charge() may overflow if it
> > becomes above INT_MAX. This was observed when I implement the new SGX
> > EPC cgroup[1] as a misc cgroup and test on a platform with large SGX EPC
> > sizes.
> >
> > Change type of new_usage to long from int and check overflow.
> >
> > Fixes: a72232eabdfcf ("cgroup: Add misc cgroup controller")
> > Signed-off-by: Haitao Huang <[email protected]>
> >
> > [1] https://lore.kernel.org/linux-sgx/[email protected]/
> > ---
> > kernel/cgroup/misc.c | 6 +++---
> > 1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
> > index fe3e8a0eb7ed..ff9f900981a3 100644
> > --- a/kernel/cgroup/misc.c
> > +++ b/kernel/cgroup/misc.c
> > @@ -143,7 +143,7 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
> > struct misc_cg *i, *j;
> > int ret;
> > struct misc_res *res;
> > - int new_usage;
> > + long new_usage;
> >
> > if (!(valid_type(type) && cg && READ_ONCE(misc_res_capacity[type])))
> > return -EINVAL;
> > @@ -153,10 +153,10 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
> >
> > for (i = cg; i; i = parent_misc(i)) {
> > res = &i->res[type];
> > -
>
> This is extra noise in the patch, please remove the change.
Lemme just revert it. Haitao, can you instead make the resource counters and
all related variables explicit 64bit instead?
Thanks.
--
tejun
On Mon, 17 Jul 2023 14:01:03 -0500, Haitao Huang
<[email protected]> wrote:
> On Mon, 17 Jul 2023 13:57:59 -0500, Tejun Heo <[email protected]> wrote:
>
>> On Mon, Jul 17, 2023 at 06:55:32PM +0000, Jarkko Sakkinen wrote:
>>> On Mon Jul 17, 2023 at 6:47 PM UTC, Haitao Huang wrote:
>>> > The variable 'new_usage' in misc_cg_try_charge() may overflow if it
>>> > becomes above INT_MAX. This was observed when I implement the new SGX
>>> > EPC cgroup[1] as a misc cgroup and test on a platform with large SGX
>>> EPC
>>> > sizes.
>>> >
>>> > Change type of new_usage to long from int and check overflow.
>>> >
>>> > Fixes: a72232eabdfcf ("cgroup: Add misc cgroup controller")
>>> > Signed-off-by: Haitao Huang <[email protected]>
>>> >
>>> > [1]
>>> https://lore.kernel.org/linux-sgx/[email protected]/
>>> > ---
>>> > kernel/cgroup/misc.c | 6 +++---
>>> > 1 file changed, 3 insertions(+), 3 deletions(-)
>>> >
>>> > diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
>>> > index fe3e8a0eb7ed..ff9f900981a3 100644
>>> > --- a/kernel/cgroup/misc.c
>>> > +++ b/kernel/cgroup/misc.c
>>> > @@ -143,7 +143,7 @@ int misc_cg_try_charge(enum misc_res_type type,
>>> struct misc_cg *cg,
>>> > struct misc_cg *i, *j;
>>> > int ret;
>>> > struct misc_res *res;
>>> > - int new_usage;
>>> > + long new_usage;
>>> >
>>> > if (!(valid_type(type) && cg &&
>>> READ_ONCE(misc_res_capacity[type])))
>>> > return -EINVAL;
>>> > @@ -153,10 +153,10 @@ int misc_cg_try_charge(enum misc_res_type
>>> type, struct misc_cg *cg,
>>> >
>>> > for (i = cg; i; i = parent_misc(i)) {
>>> > res = &i->res[type];
>>> > -
>>>
>>> This is extra noise in the patch, please remove the change.
>>
>> Lemme just revert it. Haitao, can you instead make the resource
>> counters and
>> all related variables explicit 64bit instead?
>>
>
> Will do.
Actually, we are using atomic_long_t for 'current' which is the same width
as long defined by arch/compiler. So new_usage should be long to be
consistent?
ditto for event counter. Only max is plain unsigned long but I think it is
also OK as it only compared with 'current' without any arithmetic ops
involved.
Did I miss something here?
Thanks
Haitao
Hello,
On Mon, Jul 17, 2023 at 03:19:38PM -0500, Haitao Huang wrote:
> Actually, we are using atomic_long_t for 'current' which is the same width
> as long defined by arch/compiler. So new_usage should be long to be
> consistent?
We can use atomic64_t, right? It's slower on 32bit machines but I think it'd
be better to guarantee resource counter range than micro-optimizing charge
operations. None of the current users are hot enough for this to matter and
if somebody becomes that hot, the difference between atomic_t and atomic64_t
isn't gonna matter that much. We'd need to batch allocations per-cpu and so
on.
> ditto for event counter. Only max is plain unsigned long but I think it is
> also OK as it only compared with 'current' without any arithmetic ops
> involved.
> Did I miss something here?
I'm saying that it'd be better to make everything explicitly 64bit.
Thanks.
--
tejun
The variable 'new_usage' in misc_cg_try_charge() may overflow if it
becomes above INT_MAX. This was observed when I implement the new SGX
EPC cgroup[1] as a misc cgroup and test on a platform with large SGX EPC
sizes.
Change type of new_usage to long from int and check overflow.
Fixes: a72232eabdfc ("cgroup: Add misc cgroup controller")
Signed-off-by: Haitao Huang <[email protected]>
[1] https://lore.kernel.org/linux-sgx/[email protected]/
---
kernel/cgroup/misc.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
index fe3e8a0eb7ed..b127607837c6 100644
--- a/kernel/cgroup/misc.c
+++ b/kernel/cgroup/misc.c
@@ -143,7 +143,7 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
struct misc_cg *i, *j;
int ret;
struct misc_res *res;
- int new_usage;
+ long new_usage;
if (!(valid_type(type) && cg && READ_ONCE(misc_res_capacity[type])))
return -EINVAL;
@@ -156,7 +156,8 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
new_usage = atomic_long_add_return(amount, &res->usage);
if (new_usage > READ_ONCE(res->max) ||
- new_usage > READ_ONCE(misc_res_capacity[type])) {
+ new_usage > READ_ONCE(misc_res_capacity[type]) ||
+ new_usage < 0) {
ret = -EBUSY;
goto err_charge;
}
--
2.25.1
So the variables can account for resources of huge quantities even on
32-bit machines.
Signed-off-by: Haitao Huang <[email protected]>
---
include/linux/misc_cgroup.h | 22 +++++++--------
kernel/cgroup/misc.c | 53 +++++++++++++++++++------------------
2 files changed, 38 insertions(+), 37 deletions(-)
diff --git a/include/linux/misc_cgroup.h b/include/linux/misc_cgroup.h
index c238207d1615..2751cf12796d 100644
--- a/include/linux/misc_cgroup.h
+++ b/include/linux/misc_cgroup.h
@@ -34,9 +34,9 @@ struct misc_cg;
* @failed: True if charged failed for the resource in a cgroup.
*/
struct misc_res {
- unsigned long max;
- atomic_long_t usage;
- atomic_long_t events;
+ u64 max;
+ atomic64_t usage;
+ atomic64_t events;
};
/**
@@ -53,12 +53,12 @@ struct misc_cg {
struct misc_res res[MISC_CG_RES_TYPES];
};
-unsigned long misc_cg_res_total_usage(enum misc_res_type type);
-int misc_cg_set_capacity(enum misc_res_type type, unsigned long capacity);
+u64 misc_cg_res_total_usage(enum misc_res_type type);
+int misc_cg_set_capacity(enum misc_res_type type, u64 capacity);
int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
- unsigned long amount);
+ u64 amount);
void misc_cg_uncharge(enum misc_res_type type, struct misc_cg *cg,
- unsigned long amount);
+ u64 amount);
/**
* css_misc() - Get misc cgroup from the css.
@@ -99,27 +99,27 @@ static inline void put_misc_cg(struct misc_cg *cg)
#else /* !CONFIG_CGROUP_MISC */
-static inline unsigned long misc_cg_res_total_usage(enum misc_res_type type)
+static inline u64 misc_cg_res_total_usage(enum misc_res_type type)
{
return 0;
}
static inline int misc_cg_set_capacity(enum misc_res_type type,
- unsigned long capacity)
+ u64 capacity)
{
return 0;
}
static inline int misc_cg_try_charge(enum misc_res_type type,
struct misc_cg *cg,
- unsigned long amount)
+ u64 amount)
{
return 0;
}
static inline void misc_cg_uncharge(enum misc_res_type type,
struct misc_cg *cg,
- unsigned long amount)
+ u64 amount)
{
}
diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
index b127607837c6..c4546e99cde4 100644
--- a/kernel/cgroup/misc.c
+++ b/kernel/cgroup/misc.c
@@ -14,7 +14,7 @@
#include <linux/misc_cgroup.h>
#define MAX_STR "max"
-#define MAX_NUM ULONG_MAX
+#define MAX_NUM U64_MAX
/* Miscellaneous res name, keep it in sync with enum misc_res_type */
static const char *const misc_res_name[] = {
@@ -37,7 +37,7 @@ static struct misc_cg root_cg;
* more than the actual capacity. We are using Limits resource distribution
* model of cgroup for miscellaneous controller.
*/
-static unsigned long misc_res_capacity[MISC_CG_RES_TYPES];
+static u64 misc_res_capacity[MISC_CG_RES_TYPES];
/**
* parent_misc() - Get the parent of the passed misc cgroup.
@@ -74,10 +74,10 @@ static inline bool valid_type(enum misc_res_type type)
* Context: Any context.
* Return: Current total usage of the resource.
*/
-unsigned long misc_cg_res_total_usage(enum misc_res_type type)
+u64 misc_cg_res_total_usage(enum misc_res_type type)
{
if (valid_type(type))
- return atomic_long_read(&root_cg.res[type].usage);
+ return atomic64_read(&root_cg.res[type].usage);
return 0;
}
@@ -95,7 +95,7 @@ EXPORT_SYMBOL_GPL(misc_cg_res_total_usage);
* * %0 - Successfully registered the capacity.
* * %-EINVAL - If @type is invalid.
*/
-int misc_cg_set_capacity(enum misc_res_type type, unsigned long capacity)
+int misc_cg_set_capacity(enum misc_res_type type, u64 capacity)
{
if (!valid_type(type))
return -EINVAL;
@@ -114,9 +114,9 @@ EXPORT_SYMBOL_GPL(misc_cg_set_capacity);
* Context: Any context.
*/
static void misc_cg_cancel_charge(enum misc_res_type type, struct misc_cg *cg,
- unsigned long amount)
+ u64 amount)
{
- WARN_ONCE(atomic_long_add_negative(-amount, &cg->res[type].usage),
+ WARN_ONCE(atomic64_add_negative(-amount, &cg->res[type].usage),
"misc cgroup resource %s became less than 0",
misc_res_name[type]);
}
@@ -138,12 +138,12 @@ static void misc_cg_cancel_charge(enum misc_res_type type, struct misc_cg *cg,
* capacity.
*/
int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
- unsigned long amount)
+ u64 amount)
{
struct misc_cg *i, *j;
int ret;
struct misc_res *res;
- long new_usage;
+ s64 new_usage;
if (!(valid_type(type) && cg && READ_ONCE(misc_res_capacity[type])))
return -EINVAL;
@@ -154,7 +154,7 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
for (i = cg; i; i = parent_misc(i)) {
res = &i->res[type];
- new_usage = atomic_long_add_return(amount, &res->usage);
+ new_usage = atomic64_add_return(amount, &res->usage);
if (new_usage > READ_ONCE(res->max) ||
new_usage > READ_ONCE(misc_res_capacity[type]) ||
new_usage < 0) {
@@ -166,7 +166,7 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg,
err_charge:
for (j = i; j; j = parent_misc(j)) {
- atomic_long_inc(&j->res[type].events);
+ atomic64_inc(&j->res[type].events);
cgroup_file_notify(&j->events_file);
}
@@ -186,7 +186,7 @@ EXPORT_SYMBOL_GPL(misc_cg_try_charge);
* Context: Any context.
*/
void misc_cg_uncharge(enum misc_res_type type, struct misc_cg *cg,
- unsigned long amount)
+ u64 amount)
{
struct misc_cg *i;
@@ -210,7 +210,7 @@ static int misc_cg_max_show(struct seq_file *sf, void *v)
{
int i;
struct misc_cg *cg = css_misc(seq_css(sf));
- unsigned long max;
+ u64 max;
for (i = 0; i < MISC_CG_RES_TYPES; i++) {
if (READ_ONCE(misc_res_capacity[i])) {
@@ -218,7 +218,7 @@ static int misc_cg_max_show(struct seq_file *sf, void *v)
if (max == MAX_NUM)
seq_printf(sf, "%s max\n", misc_res_name[i]);
else
- seq_printf(sf, "%s %lu\n", misc_res_name[i],
+ seq_printf(sf, "%s %llu\n", misc_res_name[i],
max);
}
}
@@ -242,13 +242,13 @@ static int misc_cg_max_show(struct seq_file *sf, void *v)
* Return:
* * >= 0 - Number of bytes processed in the input.
* * -EINVAL - If buf is not valid.
- * * -ERANGE - If number is bigger than the unsigned long capacity.
+ * * -ERANGE - If number is bigger than the u64 capacity.
*/
static ssize_t misc_cg_max_write(struct kernfs_open_file *of, char *buf,
size_t nbytes, loff_t off)
{
struct misc_cg *cg;
- unsigned long max;
+ u64 max;
int ret = 0, i;
enum misc_res_type type = MISC_CG_RES_TYPES;
char *token;
@@ -272,7 +272,7 @@ static ssize_t misc_cg_max_write(struct kernfs_open_file *of, char *buf,
if (!strcmp(MAX_STR, buf)) {
max = MAX_NUM;
} else {
- ret = kstrtoul(buf, 0, &max);
+ ret = kstrtou64(buf, 0, &max);
if (ret)
return ret;
}
@@ -298,13 +298,13 @@ static ssize_t misc_cg_max_write(struct kernfs_open_file *of, char *buf,
static int misc_cg_current_show(struct seq_file *sf, void *v)
{
int i;
- unsigned long usage;
+ u64 usage;
struct misc_cg *cg = css_misc(seq_css(sf));
for (i = 0; i < MISC_CG_RES_TYPES; i++) {
- usage = atomic_long_read(&cg->res[i].usage);
+ usage = atomic64_read(&cg->res[i].usage);
if (READ_ONCE(misc_res_capacity[i]) || usage)
- seq_printf(sf, "%s %lu\n", misc_res_name[i], usage);
+ seq_printf(sf, "%s %llu\n", misc_res_name[i], usage);
}
return 0;
@@ -323,12 +323,12 @@ static int misc_cg_current_show(struct seq_file *sf, void *v)
static int misc_cg_capacity_show(struct seq_file *sf, void *v)
{
int i;
- unsigned long cap;
+ u64 cap;
for (i = 0; i < MISC_CG_RES_TYPES; i++) {
cap = READ_ONCE(misc_res_capacity[i]);
if (cap)
- seq_printf(sf, "%s %lu\n", misc_res_name[i], cap);
+ seq_printf(sf, "%s %llu\n", misc_res_name[i], cap);
}
return 0;
@@ -337,12 +337,13 @@ static int misc_cg_capacity_show(struct seq_file *sf, void *v)
static int misc_events_show(struct seq_file *sf, void *v)
{
struct misc_cg *cg = css_misc(seq_css(sf));
- unsigned long events, i;
+ u64 events;
+ int i;
for (i = 0; i < MISC_CG_RES_TYPES; i++) {
- events = atomic_long_read(&cg->res[i].events);
+ events = atomic64_read(&cg->res[i].events);
if (READ_ONCE(misc_res_capacity[i]) || events)
- seq_printf(sf, "%s.max %lu\n", misc_res_name[i], events);
+ seq_printf(sf, "%s.max %llu\n", misc_res_name[i], events);
}
return 0;
}
@@ -399,7 +400,7 @@ misc_cg_alloc(struct cgroup_subsys_state *parent_css)
for (i = 0; i < MISC_CG_RES_TYPES; i++) {
WRITE_ONCE(cg->res[i].max, MAX_NUM);
- atomic_long_set(&cg->res[i].usage, 0);
+ atomic64_set(&cg->res[i].usage, 0);
}
return &cg->css;
--
2.25.1
Hi
On Mon, 17 Jul 2023 15:37:08 -0500, Tejun Heo <[email protected]> wrote:
> Hello,
>
> On Mon, Jul 17, 2023 at 03:19:38PM -0500, Haitao Huang wrote:
>> Actually, we are using atomic_long_t for 'current' which is the same
>> width
>> as long defined by arch/compiler. So new_usage should be long to be
>> consistent?
>
> We can use atomic64_t, right? It's slower on 32bit machines but I think
> it'd
> be better to guarantee resource counter range than micro-optimizing
> charge
> operations. None of the current users are hot enough for this to matter
> and
> if somebody becomes that hot, the difference between atomic_t and
> atomic64_t
> isn't gonna matter that much. We'd need to batch allocations per-cpu and
> so
> on.
>
>> ditto for event counter. Only max is plain unsigned long but I think it
>> is
>> also OK as it only compared with 'current' without any arithmetic ops
>> involved.
>> Did I miss something here?
>
> I'm saying that it'd be better to make everything explicitly 64bit.
>
Thanks for the explanation. I think I got it and sent it as a separate
patch now just to be sure.
BR
Haitao
On Mon Jul 17, 2023 at 11:37 PM EEST, Tejun Heo wrote:
> Hello,
>
> On Mon, Jul 17, 2023 at 03:19:38PM -0500, Haitao Huang wrote:
> > Actually, we are using atomic_long_t for 'current' which is the same width
> > as long defined by arch/compiler. So new_usage should be long to be
> > consistent?
>
> We can use atomic64_t, right? It's slower on 32bit machines but I think it'd
> be better to guarantee resource counter range than micro-optimizing charge
> operations. None of the current users are hot enough for this to matter and
> if somebody becomes that hot, the difference between atomic_t and atomic64_t
> isn't gonna matter that much. We'd need to batch allocations per-cpu and so
> on.
In our context, the microcode of SGX could support 32-bit but by design
we only support 64-bit. So at least with the current implementation this
would not be an issue for SGX.
BR, Jarkko
On Mon, Jul 17, 2023 at 06:08:45PM -0700, Haitao Huang wrote:
> So the variables can account for resources of huge quantities even on
> 32-bit machines.
>
> Signed-off-by: Haitao Huang <[email protected]>
Applied to cgroup/for-6.6 with some whitespace adjustments. I think the code
is broken when we cross the signed boundary but that's not a new problem
caused by your patch. I think what we should do is to treat atomic64_t reads
as u64 instead of putting it in s64.
Thanks.
--
tejun
Hi
On Tue, 18 Jul 2023 17:52:10 -0500, Tejun Heo <[email protected]> wrote:
> On Mon, Jul 17, 2023 at 06:08:45PM -0700, Haitao Huang wrote:
>> So the variables can account for resources of huge quantities even on
>> 32-bit machines.
>>
>> Signed-off-by: Haitao Huang <[email protected]>
>
> Applied to cgroup/for-6.6 with some whitespace adjustments. I think the
> code
> is broken when we cross the signed boundary but that's not a new problem
> caused by your patch. I think what we should do is to treat atomic64_t
> reads
> as u64 instead of putting it in s64.
>
Thanks. I think you meant the 'new_usage' in try_charge.
I'll send a patch.
BR
Haitao
Change 'new_usage' type to u64 so it can be compared with unsigned 'max'
and 'capacity' properly even if the value crosses the signed boundary.
Signed-off-by: Haitao Huang <[email protected]>
---
kernel/cgroup/misc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/kernel/cgroup/misc.c b/kernel/cgroup/misc.c
index abbe9aa5cdd1..79a3717a5803 100644
--- a/kernel/cgroup/misc.c
+++ b/kernel/cgroup/misc.c
@@ -142,7 +142,7 @@ int misc_cg_try_charge(enum misc_res_type type, struct misc_cg *cg, u64 amount)
struct misc_cg *i, *j;
int ret;
struct misc_res *res;
- s64 new_usage;
+ u64 new_usage;
if (!(valid_type(type) && cg && READ_ONCE(misc_res_capacity[type])))
return -EINVAL;
base-commit: 32bf85c60ca3584a7ba3bef19da2779b73b2e7d6
--
2.25.1
On Fri, Jul 21, 2023 at 05:02:31AM -0700, Haitao Huang wrote:
> Change 'new_usage' type to u64 so it can be compared with unsigned 'max'
> and 'capacity' properly even if the value crosses the signed boundary.
>
> Signed-off-by: Haitao Huang <[email protected]>
Applied to cgroup/for-6.6.
Thanks.
--
tejun