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.

Sign up for GitHub

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

Jump to bottom

Avoid redfine of IO_SIZE if made configurable #265

Closed
wants to merge 1 commit into from
Closed

Avoid redfine of IO_SIZE if made configurable #265

wants to merge 1 commit into from

Conversation

ericherman
Copy link
Contributor

@ericherman ericherman commented Dec 31, 2024 •
edited
Loading

If the server is to make IO_SIZE configurable, we need to avoid either redefining it, or defining it to be different.

See: MariaDB/server#3726 and https://jira.mariadb.org/browse/MDEV-35740

@ericherman ericherman mentioned this pull request Dec 31, 2024
Draft
4 tasks
ericherman added a commit to ericherman/mariadb-server that referenced this pull request Dec 31, 2024
@ericherman
Make IO_SIZE configurable
ddad018 
The default IO_SIZE of 4096 as defined in include/my_global.h and also
in libmariadb's include/ma_global.h matches to the memory page size of
most systems. Larger page sizes are widely supported, called "huge
pages" in Linux, "superpages" in FreeBSD, and "large pages" in MS
Windows.

On POSIX systems, obtaining the page size can be done via:

	page_size= sysconf(_SC_PAGESIZE);

On Windows:

	SYSTEM_INFO si;
	GetSystemInfo(&si);
	page_size= si.dwPageSize;

Making mariadb's IO_SIZE configurable enables more straight-forward
investigation of the performance implications of having an IO_SIZE which
is different than the memory page size.

Note that libmariadb's include/ma_global.h should also be adjusted to
avoid a double #define of IO_SIZE and to ensure they are defined to be
the same.
See: mariadb-corporation/mariadb-connector-c#265

Signed-off-by: Eric Herman <[email protected]>
@vuvova vuvova self-requested a review January 1, 2025 13:40
vuvova
vuvova requested changes Jan 1, 2025
View reviewed changes
include/ma_global.h Outdated
#endif
#if ((IO_SIZE <= 0) || ((IO_SIZE % 512) != 0) || ((IO_SIZE & (IO_SIZE-1)) != 0))
#error "IO_SIZE must be a positive multiple of 512 and power of 2"
#endif
Copy link
Member

Choose a reason for hiding this comment

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

I don't like that, there's no reason — that I can see — why server's IO_SIZE should affect the client. I'd suggest to rename IO_SIZE in the libmariadb. See how it's used, perhaps a NET_BUFF_ALIGN could be a good name

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 would be perfectly happy to rename client constant instead as I also feel like these should be able to be distinct; @9E0R9 what do you think?

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've updated and --force pushed

ericherman added a commit to ericherman/mariadb-server that referenced this pull request Jan 3, 2025
@ericherman
Make IO_SIZE compile-time configurable
494fc45 
Making mariadb's IO_SIZE compile-time configurable enables more
straight-forward investigation of the performance implications of having
an IO_SIZE which is different than the memory page size.

The default IO_SIZE of 4096 as defined in include/my_global.h and also
in libmariadb's include/ma_global.h matches to the memory page size of
most systems. Larger page sizes are widely supported, called "huge
pages" in Linux, "superpages" in FreeBSD, and "large pages" in MS
Windows.

On POSIX systems, obtaining the page size can be done via:

	page_size= sysconf(_SC_PAGESIZE);

On Windows:

	SYSTEM_INFO si;
	GetSystemInfo(&si);
	page_size= si.dwPageSize;

In https://jira.mariadb.org/browse/MDEV-35740 Marko highlights that
there are vastly different uses of IO_SIZE. This "one size fits all"
nature of IO_SIZE is not ideal, future work could split this into
separate constants based upon usage.

Note that libmariadb's include/ma_global.h should also be adjusted to
avoid a double #define of IO_SIZE and to ensure they are defined to be
the same.
See: mariadb-corporation/mariadb-connector-c#265

Signed-off-by: Eric Herman <[email protected]>
ericherman added a commit to ericherman/mariadb-server that referenced this pull request Jan 3, 2025
@ericherman
Make IO_SIZE compile-time configurable
780b924 
Making mariadb's IO_SIZE compile-time configurable enables more
straight-forward investigation of the performance implications of having
an IO_SIZE which is different than the memory page size.

The default IO_SIZE of 4096 as defined in include/my_global.h and also
in libmariadb's include/ma_global.h matches to the memory page size of
most systems. Larger page sizes are widely supported, called "huge
pages" in Linux, "superpages" in FreeBSD, and "large pages" in MS
Windows.

On POSIX systems, obtaining the page size can be done via:

	page_size= sysconf(_SC_PAGESIZE);

On Windows:

	SYSTEM_INFO si;
	GetSystemInfo(&si);
	page_size= si.dwPageSize;

In https://jira.mariadb.org/browse/MDEV-35740 Marko highlights that
there are vastly different uses of IO_SIZE. This "one size fits all"
nature of IO_SIZE is not ideal, future work could split this into
separate constants based upon usage.

Note that libmariadb's include/ma_global.h should also be adjusted to
avoid a double #define of IO_SIZE and to ensure they are defined to be
the same.
See: mariadb-corporation/mariadb-connector-c#265

Signed-off-by: Eric Herman <[email protected]>
@ericherman ericherman force-pushed the eherman-io-size-20241231 branch from a6e5a17 to 8079810 Compare January 3, 2025 12:22
ericherman added a commit to ericherman/mariadb-server that referenced this pull request Jan 3, 2025
@ericherman
Make IO_SIZE compile-time configurable
9879b3d 
Making mariadb's IO_SIZE compile-time configurable enables more
straight-forward investigation of the performance implications of having
an IO_SIZE which is different than the memory page size.

The default IO_SIZE of 4096 as defined in include/my_global.h and also
in libmariadb's include/ma_global.h matches to the memory page size of
most systems. Larger page sizes are widely supported, called "huge
pages" in Linux, "superpages" in FreeBSD, and "large pages" in MS
Windows.

On POSIX systems, obtaining the page size can be done via:

	page_size= sysconf(_SC_PAGESIZE);

On Windows:

	SYSTEM_INFO si;
	GetSystemInfo(&si);
	page_size= si.dwPageSize;

In https://jira.mariadb.org/browse/MDEV-35740 Marko highlights that
there are vastly different uses of IO_SIZE. This "one size fits all"
nature of IO_SIZE is not ideal, future work could split this into
separate constants based upon usage.

Note that libmariadb's include/ma_global.h should also be adjusted to
avoid a double #define of IO_SIZE and to ensure they are defined to be
the same.
See: mariadb-corporation/mariadb-connector-c#265

Signed-off-by: Eric Herman <[email protected]>
@ericherman
Create #define for network buffer alignment
3389246 
The server's definition of IO_SIZE is re-used here in the client for
network buffer alignment, however IO_SIZE is used in the server for
many different things and the client's buffer alignment is not related
to many of those uses.

If the server is to make IO_SIZE configurable, we need to avoid either
redefining it, or defining it to be different. By creating a specific
define for this, we avoid redfine and clarify the code.

See: MariaDB/server#3726

Signed-off-by: Eric Herman <[email protected]>
@ericherman ericherman force-pushed the eherman-io-size-20241231 branch from 8079810 to 3389246 Compare January 5, 2025 08:58
@ericherman ericherman marked this pull request as ready for review January 5, 2025 08:59
vuvova
vuvova approved these changes Jan 5, 2025
View reviewed changes
Copy link
Member

@vuvova vuvova left a comment

Choose a reason for hiding this comment

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

look good to me

@9EOR9 9EOR9 deleted the branch mariadb-corporation:master January 5, 2025 16:24
@9EOR9 9EOR9 closed this Jan 5, 2025
@ericherman ericherman mentioned this pull request Jan 5, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vuvova vuvova vuvova approved these changes

Assignees

@9EOR9 9EOR9

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@ericherman @vuvova @9EOR9