Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Calculation of mixed units will fail in RAM total size #107

Closed
luddinho opened this issue Jun 30, 2023 · 7 comments
Closed

Calculation of mixed units will fail in RAM total size #107

luddinho opened this issue Jun 30, 2023 · 7 comments

Comments

@luddinho
Copy link

Environment

script version: v3.0.56
OS: DSM 7.2
Device: 1522+
RAM: 1x 8GB, 1x 32GB; Total: 40 GB

Problem description

On this device the command for RAM size determintation will return the following size.

dmidecode -t memory | grep "[Ss]ize"

# Result
Size: 8192 MB
Size: 32 GB

The total ram size will therfore calculated as 8192 + 32 = 8224 and the unit of the last occurrence form the loop will win. In this case the unit is determined as GB. Unfortunatelly the system is providing values with different units.
Therefore the result is:

ramtotal: 8224
bytes: GB

This will lead to the assumption that all determined values have the unit GB and therefore the value will be multiplied with the factor 1024.

if [[ $bytes == "GB" ]]; then      # DSM 7.2 dmidecode returns GB
    ramtotal=$((ramtotal * 1024))  # Convert to MB
fi

At the end the calculated total RAM size will reported wrongly with 8421376 bytes.

Proposed solution

Avoid mixing units in calculation.
Consider the evaluation of the units for each value inside the loop before adding those whit different units.

@007revad
Copy link
Owner

007revad commented Jun 30, 2023

Size: 8192 MB
Size: 32 GB

I knew dmidecode in DSM 7.2 sometimes reported memory in GB instead of MB but it never occurred to me that it could report different units for 2 different memory modules in the same NAS.

Thanks again for your help.

007revad added a commit that referenced this issue Jun 30, 2023
v3.1.57
- Added enabling E10M20-T1, M2D20 and M2D18 for DS1821+, DS1621+ and DS1520+.
- Fixed enabling E10M20-T1, M2D20 and M2D18 cards in models that don't officially support them.
- Fixed bugs where the calculated amount of installed memory could be incorrect:
   - If last memory socket was empty an invalid unit of bytes could be used. Issue #106
   - When dmidecode returned MB for one ram module and GB for another ram module. Issue #107
@007revad
Copy link
Owner

Do you want to try this version to confirm that if solves issue #106 and this issue.

https://github.com/007revad/Synology_HDD_db/releases/tag/v3.1.57-RC

@luddinho
Copy link
Author

The assets might not updated at the linked release candidate of v3.1.57-RC. I can only find the changes from the commit ada7eff

There is unfortunately no relevant change from the difference report compared to v3.0.56
v3.0.56...v3.1.57-RC

@007revad
Copy link
Owner

Oops. I forgot to set the target to develop.

I made sure to set the target to develop on this version.
https://github.com/007revad/Synology_HDD_db/releases/tag/v3.1.58-RC

@luddinho
Copy link
Author

We have checked the behavior and the result is as following.

The console output informed that RAM size was changed with this message:

Max memory is set to 40 GB.

Thats the good point that the evaluation of the max installed RAM is now correct.

On the other hand there might an issue when the latest size is less than the previous one from the database.
In this case the wrong evaluated value in the databse from the last script execution was 8421376 and therefore the new value of 40 GB (40960 MB) is less than the previous one.

else [[ $setting -lt "$ramtotal" ]]
  #echo -e "\nMax memory is set to $ramtotal MB."
  ramgb=$((ramtotal / 1024))
  echo -e "\nMax memory is set to $ramgb GB."
fi

This will cause the issue that the new value is only reported on the console out but will never updated in the database with synosetkeyvalue.

What is the idea of the function to set the mem_max_mb value?
Only to update database in case of lareger values of RAM installed than the max limitation of Synology default setting to support more RAM in the system?

@007revad
Copy link
Owner

007revad commented Jul 1, 2023

Only to update database in case of larger values of RAM installed than the max limitation of Synology default setting to support more RAM in the system?

Yes. If you install more memory than the mem_max_mb setting DSM will only use memory up to the mem_max_mb setting, but it will use the rest of the memory as a cache.

That else [[ $setting -lt "$ramtotal" ]] section in that if else then statement was definitely wrong, and never would have been processed because the if [[ $ramtotal -gt "$setting" ]] would have been processed instead.

I've changed it to:

  else [[ $ramtotal -lt "$setting" ]]
    #echo -e "\nMax memory is set to $setting MB."
    ramgb=$((setting / 1024))
    echo -e "\nMax memory is set to $ramgb GB."
  fi

To restore incorrectly set mem_max_mb like yours the script now checks if the existing value is greater than both the installed memory amount and the default value in the backup syninfo.conf - then restores it to the default if needed.

https://github.com/007revad/Synology_HDD_db/releases/tag/v3.1.59-RC

@luddinho
Copy link
Author

Sorry that I didn't had the chance to test the second update. But the diff looks pretty promising.
Thanks for fixing the issues.

@007revad 007revad mentioned this issue Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants