Skip to content

Reduce allocation size of cp_time_n and cp_time_o on FreeBSD and DragonFlyBSD #873

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

Merged
merged 1 commit into from
Nov 19, 2021

Conversation

GuillaumeGomez
Copy link
Contributor

Unless I'm missing something, these two arrays are only ever used to read one set of CPU data, not X (where X is the number of CPUs).

Usage is here and allocation is here.

@BenBE
Copy link
Member

BenBE commented Nov 16, 2021

A similar change might be necessary for other BSD variants too. Can you have a look and include the necessary changes too?

@BenBE BenBE added BSD 🐡 Issues related to *BSD FreeBSD 👹 FreeBSD related issues NetBSD 🎏 NetBSD related issues labels Nov 16, 2021
Comment on lines 112 to 113
fpl->cp_time_o = xCalloc(1, sizeof_cp_time_array);
fpl->cp_time_n = xCalloc(1, sizeof_cp_time_array);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given this allocates an array of unsigned long this should better be:

Suggested change
fpl->cp_time_o = xCalloc(1, sizeof_cp_time_array);
fpl->cp_time_n = xCalloc(1, sizeof_cp_time_array);
fpl->cp_time_o = xCalloc(CPU_STATES, sizeof(unsigned long));
fpl->cp_time_n = xCalloc(CPU_STATES, sizeof(unsigned long));

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I thought about it but find it better to re-use the same variable instead to make more obvious that we're allocating it for only one "CPU state". If you really prefer me to update it though, I'll (please confirm whether or not you want me to update it).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a somewhat subtle issue here: calloc does not only an overflow check that the current version is missing, but also ensures proper alignment. Depending on the exact values used here, this may make a difference, causing the actual allocation being larger due to proper alignment of allocated elements (See note here).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CPU_STATES -> CPUSTATES in the suggested change

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the extra explanations @BenBE! Totally makes sense, I updated.

@fasterit: Thanks for the correction.

@GuillaumeGomez GuillaumeGomez changed the title Reduce allocation size of cp_time_n and cp_time_o on FreeBSD Reduce allocation size of cp_time_n and cp_time_o on FreeBSD and DragonFlyBSD Nov 16, 2021
@GuillaumeGomez
Copy link
Contributor Author

I looked into all *bsd and into solaris but only DragonFlyBSD has the issue. Fixed it as well.

@BenBE BenBE merged commit 1284ab4 into htop-dev:main Nov 19, 2021
@GuillaumeGomez GuillaumeGomez deleted the reduce-allocation branch November 19, 2021 12:00
@BenBE BenBE added this to the 3.1.2 milestone Dec 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
BSD 🐡 Issues related to *BSD FreeBSD 👹 FreeBSD related issues NetBSD 🎏 NetBSD related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants