| Message ID | 155053494572.24125.4002432022455040178.stgit@noble.brown (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | More lustre patches from obdclass | expand |
> This function seems overly complex, the same functionality > is available with much less effort. Actually we are in discussion about removing these functions. The details are at: https://jira.whamcloud.com/browse/LU-11157 Since shells typically don't handle "floating point" representation well you can see the following: sanity 64d with the same error message; '209.9: syntax error: invalid arithmetic operator (error token is ".9")' The second issues is that the read values are not true representation of the desired value. The same issues is with the string helpers as well. Take for example test_string_get_size_one(1100, 1, "1.10 kB", "1.07 KiB"); Which is taken from linux/lib/test-string_helpers.c. If you pass 1100 to string_get_size() you can get either 1.10 kB or 1.07 KiB. Now if do the reverse 1.10 kB you get ~1126 which is not the same as 11000. I discussed how to handle this problem with Andreas and he agreed the best solution is make all the sysfs / debugfs *_mb files turn any values passed in to around up to the nearest MiB. This way we can report the exact MiB settings to the users. We already have a patch to do this for max_dirty_mb which I can push. The other *_mb files need to be updated to round off. If you can wait a few days I can backport the already done patch and push a patch for the rest. > Signed-off-by: NeilBrown <neilb@suse.com> > --- > .../lustre/lustre/obdclass/lprocfs_status.c | 47 +++----------------- > 1 file changed, 7 insertions(+), 40 deletions(-) > > diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c > index 568e6623e0c8..bd24e48f6145 100644 > --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c > +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c > @@ -205,53 +205,20 @@ static void obd_connect_data_seqprint(struct seq_file *m, > int lprocfs_read_frac_helper(char *buffer, unsigned long count, long val, > int mult) > { > - long decimal_val, frac_val; > int prtn; > > if (count < 10) > return -EINVAL; > > - decimal_val = val / mult; > - prtn = snprintf(buffer, count, "%ld", decimal_val); > - frac_val = val % mult; > - > - if (prtn < (count - 4) && frac_val > 0) { > - long temp_frac; > - int i, temp_mult = 1, frac_bits = 0; > - > - temp_frac = frac_val * 10; > - buffer[prtn++] = '.'; > - while (frac_bits < 2 && (temp_frac / mult) < 1) { > - /* only reserved 2 bits fraction */ > - buffer[prtn++] = '0'; > - temp_frac *= 10; > - frac_bits++; > - } > - /* > - * Need to think these cases : > - * 1. #echo x.00 > /sys/xxx output result : x > - * 2. #echo x.0x > /sys/xxx output result : x.0x > - * 3. #echo x.x0 > /sys/xxx output result : x.x > - * 4. #echo x.xx > /sys/xxx output result : x.xx > - * Only reserved 2 bits fraction. > - */ > - for (i = 0; i < (5 - prtn); i++) > - temp_mult *= 10; > - > - frac_bits = min((int)count - prtn, 3 - frac_bits); > - prtn += snprintf(buffer + prtn, frac_bits, "%ld", > - frac_val * temp_mult / mult); > + prtn = snprintf(buffer, count, "%ld.%02lu", > + val / mult, > + (val % mult) / (mult / 100)); > > + /* Strip trailing zeroes, and trailing '.' */ > + while (prtn && buffer[prtn-1] == '0') > + prtn--; > + if (prtn && buffer[prtn-1] == '.') > prtn--; > - while (buffer[prtn] < '1' || buffer[prtn] > '9') { > - prtn--; > - if (buffer[prtn] == '.') { > - prtn--; > - break; > - } > - } > - prtn++; > - } > buffer[prtn++] = '\n'; > return prtn; > } > > >
On Sun, Feb 24 2019, James Simmons wrote: >> This function seems overly complex, the same functionality >> is available with much less effort. > > Actually we are in discussion about removing these functions. > The details are at: > > https://jira.whamcloud.com/browse/LU-11157 > > Since shells typically don't handle "floating point" representation > well you can see the following: > > sanity 64d with the same error message; '209.9: syntax error: invalid > arithmetic operator (error token is ".9")' > > The second issues is that the read values are not true representation > of the desired value. The same issues is with the string helpers as > well. Take for example > > test_string_get_size_one(1100, 1, "1.10 kB", "1.07 KiB"); > > Which is taken from linux/lib/test-string_helpers.c. > > If you pass 1100 to string_get_size() you can get either 1.10 kB or 1.07 > KiB. Now if do the reverse 1.10 kB you get ~1126 which is not the same as > 11000. I discussed how to handle this problem with Andreas and he agreed > the best solution is make all the sysfs / debugfs *_mb files turn any > values passed in to around up to the nearest MiB. This way we can report > the exact MiB settings to the users. We already have a patch to do this > for max_dirty_mb which I can push. The other *_mb files need to be updated > to round off. If you can wait a few days I can backport the already done > patch and push a patch for the rest. My recommendation would be to deprecate all _mb files and instead have _bytes files which report a simple integer - the number of bytes. Leave all the unit conversion to user-space. Linus once told me that he prefers sysfs files to always be in basic units, and that decimals are fine. So use "seconds" for time, even if that means 0.002 for 2 milliseconds. Use bytes for storage, etc. NeilBrown > >> Signed-off-by: NeilBrown <neilb@suse.com> >> --- >> .../lustre/lustre/obdclass/lprocfs_status.c | 47 +++----------------- >> 1 file changed, 7 insertions(+), 40 deletions(-) >> >> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c >> index 568e6623e0c8..bd24e48f6145 100644 >> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c >> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c >> @@ -205,53 +205,20 @@ static void obd_connect_data_seqprint(struct seq_file *m, >> int lprocfs_read_frac_helper(char *buffer, unsigned long count, long val, >> int mult) >> { >> - long decimal_val, frac_val; >> int prtn; >> >> if (count < 10) >> return -EINVAL; >> >> - decimal_val = val / mult; >> - prtn = snprintf(buffer, count, "%ld", decimal_val); >> - frac_val = val % mult; >> - >> - if (prtn < (count - 4) && frac_val > 0) { >> - long temp_frac; >> - int i, temp_mult = 1, frac_bits = 0; >> - >> - temp_frac = frac_val * 10; >> - buffer[prtn++] = '.'; >> - while (frac_bits < 2 && (temp_frac / mult) < 1) { >> - /* only reserved 2 bits fraction */ >> - buffer[prtn++] = '0'; >> - temp_frac *= 10; >> - frac_bits++; >> - } >> - /* >> - * Need to think these cases : >> - * 1. #echo x.00 > /sys/xxx output result : x >> - * 2. #echo x.0x > /sys/xxx output result : x.0x >> - * 3. #echo x.x0 > /sys/xxx output result : x.x >> - * 4. #echo x.xx > /sys/xxx output result : x.xx >> - * Only reserved 2 bits fraction. >> - */ >> - for (i = 0; i < (5 - prtn); i++) >> - temp_mult *= 10; >> - >> - frac_bits = min((int)count - prtn, 3 - frac_bits); >> - prtn += snprintf(buffer + prtn, frac_bits, "%ld", >> - frac_val * temp_mult / mult); >> + prtn = snprintf(buffer, count, "%ld.%02lu", >> + val / mult, >> + (val % mult) / (mult / 100)); >> >> + /* Strip trailing zeroes, and trailing '.' */ >> + while (prtn && buffer[prtn-1] == '0') >> + prtn--; >> + if (prtn && buffer[prtn-1] == '.') >> prtn--; >> - while (buffer[prtn] < '1' || buffer[prtn] > '9') { >> - prtn--; >> - if (buffer[prtn] == '.') { >> - prtn--; >> - break; >> - } >> - } >> - prtn++; >> - } >> buffer[prtn++] = '\n'; >> return prtn; >> } >> >> >>
On Feb 26, 2019, at 16:59, NeilBrown <neilb@suse.com> wrote: > > On Sun, Feb 24 2019, James Simmons wrote: > >>> This function seems overly complex, the same functionality >>> is available with much less effort. >> >> Actually we are in discussion about removing these functions. >> The details are at: >> >> https://jira.whamcloud.com/browse/LU-11157 >> >> Since shells typically don't handle "floating point" representation >> well you can see the following: >> >> sanity 64d with the same error message; '209.9: syntax error: invalid >> arithmetic operator (error token is ".9")' >> >> The second issues is that the read values are not true representation >> of the desired value. The same issues is with the string helpers as >> well. Take for example >> >> test_string_get_size_one(1100, 1, "1.10 kB", "1.07 KiB"); >> >> Which is taken from linux/lib/test-string_helpers.c. >> >> If you pass 1100 to string_get_size() you can get either 1.10 kB or 1.07 >> KiB. Now if do the reverse 1.10 kB you get ~1126 which is not the same as >> 11000. I discussed how to handle this problem with Andreas and he agreed >> the best solution is make all the sysfs / debugfs *_mb files turn any >> values passed in to around up to the nearest MiB. This way we can report >> the exact MiB settings to the users. We already have a patch to do this >> for max_dirty_mb which I can push. The other *_mb files need to be updated >> to round off. If you can wait a few days I can backport the already done >> patch and push a patch for the rest. > > My recommendation would be to deprecate all _mb files and instead have > _bytes files which report a simple integer - the number of bytes. That would break a lot of configurations and documentation. Also, bytes are not very convenient units for a lot of the numbers, for example you don't want to (nor could you) send 47-byte RPCs. > Leave all the unit conversion to user-space. > > Linus once told me that he prefers sysfs files to always be in basic > units, and that decimals are fine. So use "seconds" for time, even if > that means 0.002 for 2 milliseconds. Use bytes for storage, etc. That is not going to be better than allowing "max_dirty_mb=0.5", which is what we wanted to get rid of in the first place. Back in the day when a system had 4MB DIMMs it was useful to allow fractional MB for the cache size or debug log size, but I don't think that is useful today. I'm content to just round these to the nearest MB, in the rare case that someone specifies a fractional unit, but honestly I don't think that anyone does anymore. It is much more likely they will be setting the cache size to 16G per target on their 256GB RAM nodes. Cheers, Andreas >>> Signed-off-by: NeilBrown <neilb@suse.com> >>> --- >>> .../lustre/lustre/obdclass/lprocfs_status.c | 47 +++----------------- >>> 1 file changed, 7 insertions(+), 40 deletions(-) >>> >>> diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c >>> index 568e6623e0c8..bd24e48f6145 100644 >>> --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c >>> +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c >>> @@ -205,53 +205,20 @@ static void obd_connect_data_seqprint(struct seq_file *m, >>> int lprocfs_read_frac_helper(char *buffer, unsigned long count, long val, >>> int mult) >>> { >>> - long decimal_val, frac_val; >>> int prtn; >>> >>> if (count < 10) >>> return -EINVAL; >>> >>> - decimal_val = val / mult; >>> - prtn = snprintf(buffer, count, "%ld", decimal_val); >>> - frac_val = val % mult; >>> - >>> - if (prtn < (count - 4) && frac_val > 0) { >>> - long temp_frac; >>> - int i, temp_mult = 1, frac_bits = 0; >>> - >>> - temp_frac = frac_val * 10; >>> - buffer[prtn++] = '.'; >>> - while (frac_bits < 2 && (temp_frac / mult) < 1) { >>> - /* only reserved 2 bits fraction */ >>> - buffer[prtn++] = '0'; >>> - temp_frac *= 10; >>> - frac_bits++; >>> - } >>> - /* >>> - * Need to think these cases : >>> - * 1. #echo x.00 > /sys/xxx output result : x >>> - * 2. #echo x.0x > /sys/xxx output result : x.0x >>> - * 3. #echo x.x0 > /sys/xxx output result : x.x >>> - * 4. #echo x.xx > /sys/xxx output result : x.xx >>> - * Only reserved 2 bits fraction. >>> - */ >>> - for (i = 0; i < (5 - prtn); i++) >>> - temp_mult *= 10; >>> - >>> - frac_bits = min((int)count - prtn, 3 - frac_bits); >>> - prtn += snprintf(buffer + prtn, frac_bits, "%ld", >>> - frac_val * temp_mult / mult); >>> + prtn = snprintf(buffer, count, "%ld.%02lu", >>> + val / mult, >>> + (val % mult) / (mult / 100)); >>> >>> + /* Strip trailing zeroes, and trailing '.' */ >>> + while (prtn && buffer[prtn-1] == '0') >>> + prtn--; >>> + if (prtn && buffer[prtn-1] == '.') >>> prtn--; >>> - while (buffer[prtn] < '1' || buffer[prtn] > '9') { >>> - prtn--; >>> - if (buffer[prtn] == '.') { >>> - prtn--; >>> - break; >>> - } >>> - } >>> - prtn++; >>> - } >>> buffer[prtn++] = '\n'; >>> return prtn; >>> } Cheers, Andreas --- Andreas Dilger Principal Lustre Architect Whamcloud
diff --git a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c index 568e6623e0c8..bd24e48f6145 100644 --- a/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c +++ b/drivers/staging/lustre/lustre/obdclass/lprocfs_status.c @@ -205,53 +205,20 @@ static void obd_connect_data_seqprint(struct seq_file *m, int lprocfs_read_frac_helper(char *buffer, unsigned long count, long val, int mult) { - long decimal_val, frac_val; int prtn; if (count < 10) return -EINVAL; - decimal_val = val / mult; - prtn = snprintf(buffer, count, "%ld", decimal_val); - frac_val = val % mult; - - if (prtn < (count - 4) && frac_val > 0) { - long temp_frac; - int i, temp_mult = 1, frac_bits = 0; - - temp_frac = frac_val * 10; - buffer[prtn++] = '.'; - while (frac_bits < 2 && (temp_frac / mult) < 1) { - /* only reserved 2 bits fraction */ - buffer[prtn++] = '0'; - temp_frac *= 10; - frac_bits++; - } - /* - * Need to think these cases : - * 1. #echo x.00 > /sys/xxx output result : x - * 2. #echo x.0x > /sys/xxx output result : x.0x - * 3. #echo x.x0 > /sys/xxx output result : x.x - * 4. #echo x.xx > /sys/xxx output result : x.xx - * Only reserved 2 bits fraction. - */ - for (i = 0; i < (5 - prtn); i++) - temp_mult *= 10; - - frac_bits = min((int)count - prtn, 3 - frac_bits); - prtn += snprintf(buffer + prtn, frac_bits, "%ld", - frac_val * temp_mult / mult); + prtn = snprintf(buffer, count, "%ld.%02lu", + val / mult, + (val % mult) / (mult / 100)); + /* Strip trailing zeroes, and trailing '.' */ + while (prtn && buffer[prtn-1] == '0') + prtn--; + if (prtn && buffer[prtn-1] == '.') prtn--; - while (buffer[prtn] < '1' || buffer[prtn] > '9') { - prtn--; - if (buffer[prtn] == '.') { - prtn--; - break; - } - } - prtn++; - } buffer[prtn++] = '\n'; return prtn; }
This function seems overly complex, the same functionality is available with much less effort. Signed-off-by: NeilBrown <neilb@suse.com> --- .../lustre/lustre/obdclass/lprocfs_status.c | 47 +++----------------- 1 file changed, 7 insertions(+), 40 deletions(-)