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

Allocation class map interface #1986

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
5 participants
@pbalcer
Contributor

pbalcer commented Jun 7, 2017

Second part of the allocation classes.
Includes #1985


This change is Reviewable

@pbalcer pbalcer referenced this pull request Jun 7, 2017

Closed

obj: alloc class interface #1887

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 8, 2017

Codecov Report

Merging #1986 into master will decrease coverage by <.01%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1986      +/-   ##
==========================================
- Coverage   83.08%   83.07%   -0.01%     
==========================================
  Files         332      332              
  Lines       36249    36353     +104     
  Branches     5397     5410      +13     
==========================================
+ Hits        30116    30201      +85     
- Misses       6133     6152      +19

codecov-io commented Jun 8, 2017

Codecov Report

Merging #1986 into master will decrease coverage by <.01%.
The diff coverage is 84.61%.

@@            Coverage Diff             @@
##           master    #1986      +/-   ##
==========================================
- Coverage   83.08%   83.07%   -0.01%     
==========================================
  Files         332      332              
  Lines       36249    36353     +104     
  Branches     5397     5410      +13     
==========================================
+ Hits        30116    30201      +85     
- Misses       6133     6152      +19
@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jul 31, 2017

Contributor

Reviewed 15 of 42 files at r1, 3 of 11 files at r2, 9 of 12 files at r3.
Review status: all files reviewed at latest revision, 14 unresolved discussions.


doc/libpmemobj.3.md, line 2648 at r3 (raw file):

heap.alloc_class.map.reset | -w | - | - | `struct pobj_alloc_class_params` | integer, integer, integer

This entry point deletes all of the internal definitions of allocation classes

...including default ones (id's 1-127)...


src/include/libpmemobj/ctl.h, line 80 at r3 (raw file):

 *	allowing applications to have complete control over the allocator.
 *
 * - heap.alloc_class.[class_id].desc

duplicate - this entry is already described a few lines above


src/libpmemobj/alloc_class.c, line 632 at r3 (raw file):

 */
int
alloc_class_reset(struct alloc_class_collection *ac,

It partially duplicates the code from alloc_class_collection_new(). Could be a subject for refactoring.


src/libpmemobj/alloc_class.c, line 635 at r3 (raw file):

	size_t granularity, size_t limit, int fail_on_missing_class)
{
	size_t maps_size = (limit / granularity) + 1;

LOG();


src/libpmemobj/alloc_class.c, line 639 at r3 (raw file):

	uint8_t *class_map_by_unit_size;
	if ((class_map_by_alloc_size = Malloc(maps_size)) == NULL) {
		return -1;

ERR();


src/libpmemobj/alloc_class.c, line 643 at r3 (raw file):

	if ((class_map_by_unit_size = Malloc(maps_size)) == NULL) {
		Free(class_map_by_alloc_size);
		return -1;

ERR()


src/libpmemobj/alloc_class.c, line 659 at r3 (raw file):

	ac->class_map_by_alloc_size = class_map_by_alloc_size;
	ac->class_map_by_unit_size = class_map_by_unit_size;
	memset(ac->class_map_by_alloc_size, 0xFF, maps_size);

Why not 0? Is 255 an invalid class id or default one?


src/libpmemobj/alloc_class.c, line 675 at r3 (raw file):

	struct alloc_class *c, size_t start, size_t end)
{
	/* only single unit allocs are supported with minimal header */

LOG()


src/libpmemobj/alloc_class.c, line 678 at r3 (raw file):

	if (c->header_type == HEADER_NONE) {
		if (CALC_SIZE_IDX(c->unit_size, start) > 1)
			return -1;

errno = EINVAL;
ERR();


src/libpmemobj/alloc_class.c, line 680 at r3 (raw file):

			return -1;
		if (CALC_SIZE_IDX(c->unit_size, end) > 1)
			return -1;

errno = EINVAL;
ERR();


src/libpmemobj/pmalloc.c, line 493 at r3 (raw file):

	enum ctl_query_type type, void *arg, struct ctl_indexes *indexes)
{
	struct pobj_alloc_class_map_range *range = arg;

LOG()
same for other functions


src/libpmemobj/pmalloc.c, line 499 at r3 (raw file):

	if (c == NULL)
		return -1;

errno / ERR()


src/test/obj_ctl_alloc_class_config/alloc_class_config, line 4 at r3 (raw file):

	4096, # handle sizes up to 4k, leave the rest to the default
	16, # 16 byte granularity
	1 # use default for missing classes

Shouldn't it be "fail for missing classes"


Comments from Reviewable

Contributor

krzycz commented Jul 31, 2017

Reviewed 15 of 42 files at r1, 3 of 11 files at r2, 9 of 12 files at r3.
Review status: all files reviewed at latest revision, 14 unresolved discussions.


doc/libpmemobj.3.md, line 2648 at r3 (raw file):

heap.alloc_class.map.reset | -w | - | - | `struct pobj_alloc_class_params` | integer, integer, integer

This entry point deletes all of the internal definitions of allocation classes

...including default ones (id's 1-127)...


src/include/libpmemobj/ctl.h, line 80 at r3 (raw file):

 *	allowing applications to have complete control over the allocator.
 *
 * - heap.alloc_class.[class_id].desc

duplicate - this entry is already described a few lines above


src/libpmemobj/alloc_class.c, line 632 at r3 (raw file):

 */
int
alloc_class_reset(struct alloc_class_collection *ac,

It partially duplicates the code from alloc_class_collection_new(). Could be a subject for refactoring.


src/libpmemobj/alloc_class.c, line 635 at r3 (raw file):

	size_t granularity, size_t limit, int fail_on_missing_class)
{
	size_t maps_size = (limit / granularity) + 1;

LOG();


src/libpmemobj/alloc_class.c, line 639 at r3 (raw file):

	uint8_t *class_map_by_unit_size;
	if ((class_map_by_alloc_size = Malloc(maps_size)) == NULL) {
		return -1;

ERR();


src/libpmemobj/alloc_class.c, line 643 at r3 (raw file):

	if ((class_map_by_unit_size = Malloc(maps_size)) == NULL) {
		Free(class_map_by_alloc_size);
		return -1;

ERR()


src/libpmemobj/alloc_class.c, line 659 at r3 (raw file):

	ac->class_map_by_alloc_size = class_map_by_alloc_size;
	ac->class_map_by_unit_size = class_map_by_unit_size;
	memset(ac->class_map_by_alloc_size, 0xFF, maps_size);

Why not 0? Is 255 an invalid class id or default one?


src/libpmemobj/alloc_class.c, line 675 at r3 (raw file):

	struct alloc_class *c, size_t start, size_t end)
{
	/* only single unit allocs are supported with minimal header */

LOG()


src/libpmemobj/alloc_class.c, line 678 at r3 (raw file):

	if (c->header_type == HEADER_NONE) {
		if (CALC_SIZE_IDX(c->unit_size, start) > 1)
			return -1;

errno = EINVAL;
ERR();


src/libpmemobj/alloc_class.c, line 680 at r3 (raw file):

			return -1;
		if (CALC_SIZE_IDX(c->unit_size, end) > 1)
			return -1;

errno = EINVAL;
ERR();


src/libpmemobj/pmalloc.c, line 493 at r3 (raw file):

	enum ctl_query_type type, void *arg, struct ctl_indexes *indexes)
{
	struct pobj_alloc_class_map_range *range = arg;

LOG()
same for other functions


src/libpmemobj/pmalloc.c, line 499 at r3 (raw file):

	if (c == NULL)
		return -1;

errno / ERR()


src/test/obj_ctl_alloc_class_config/alloc_class_config, line 4 at r3 (raw file):

	4096, # handle sizes up to 4k, leave the rest to the default
	16, # 16 byte granularity
	1 # use default for missing classes

Shouldn't it be "fail for missing classes"


Comments from Reviewable

@krzycz

This comment has been minimized.

Show comment
Hide comment
@krzycz

krzycz Jul 31, 2017

Contributor

Review status: all files reviewed at latest revision, 14 unresolved discussions.


doc/libpmemobj.3.md, line 2648 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

...including default ones (id's 1-127)...

Or, maybe 'reset' should only reset user-defined classes (128-...). Default ones (that are also used for internal allocations) should never be modified by the user?
Then, if the requested size cannot be mapped to any user-defined class (128-...), it may fall back to default classes (or fail). IOW, the same size could be mapped to one of the user-defined classes and to one of the default classes. (In practice, this means two co-existing aloc classes collections.) For allocation requests from the program, the allocator would first look into user-defined classes, then to default ones (in 'fail' flag is not set). For internal allocations, only default classes would be used.
With such design, there is no risk that user-defined alloc classes would affect the internal metadata allocations.


Comments from Reviewable

Contributor

krzycz commented Jul 31, 2017

Review status: all files reviewed at latest revision, 14 unresolved discussions.


doc/libpmemobj.3.md, line 2648 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

...including default ones (id's 1-127)...

Or, maybe 'reset' should only reset user-defined classes (128-...). Default ones (that are also used for internal allocations) should never be modified by the user?
Then, if the requested size cannot be mapped to any user-defined class (128-...), it may fall back to default classes (or fail). IOW, the same size could be mapped to one of the user-defined classes and to one of the default classes. (In practice, this means two co-existing aloc classes collections.) For allocation requests from the program, the allocator would first look into user-defined classes, then to default ones (in 'fail' flag is not set). For internal allocations, only default classes would be used.
With such design, there is no risk that user-defined alloc classes would affect the internal metadata allocations.


Comments from Reviewable

@plebioda

This comment has been minimized.

Show comment
Hide comment
@plebioda

plebioda Sep 19, 2017

Contributor

Reviewed 1 of 42 files at r1, 2 of 11 files at r2, 7 of 12 files at r3.
Review status: all files reviewed at latest revision, 21 unresolved discussions.


doc/libpmemobj.3.md, line 2613 at r3 (raw file):

The mapping between allocation class and object sizes.

This entry point takes a complex argument.

: ?


doc/libpmemobj.3.md, line 2632 at r3 (raw file):

= 0 and <= 255, or > 0 and < 255 ?
how about defining a macro for max class id ?


doc/libpmemobj.3.md, line 2668 at r3 (raw file):

The field `limit` defines the size in bytes to which allocations are handled
using custom allocation classes. Everything above is handled using entire blocks
and best-fit.

best-fit algorithm
is the "block" term already known to the reader ? What is the size of a block etc ?


doc/libpmemobj.3.md, line 2671 at r3 (raw file):

The field `granularity` defines the size to which all object sizes are aligned
to. Together with `limit`, it determines the size of the array containing the

"abstract array" ? is this really an array ? is it relevant for user that this is an array ?


doc/libpmemobj.3.md, line 2677 at r3 (raw file):

should fail when no mapping between a requested size and an allocation class
exists. If not set, all sizes without corresponding allocation class will be
handled using entire blocks (256 kilobytes).

do we have a macro for the block (chunk) size ? I can imagine that someone would like to create allocation classes programatically based on the user's data sizes and some of allocator's properties could be exposed through API.


src/libpmemobj/alloc_class.c, line 635 at r3 (raw file):

	size_t granularity, size_t limit, int fail_on_missing_class)
{
	size_t maps_size = (limit / granularity) + 1;

SIGFPE


src/libpmemobj/alloc_class.c, line 659 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Why not 0? Is 255 an invalid class id or default one?

good question ?


src/libpmemobj/pmalloc.c, line 505 at r3 (raw file):

	if (ret != 0) {
		ERR("range setting would be invalid for the class");

invalid range settings for the allocation class ?

why would ?


Comments from Reviewable

Contributor

plebioda commented Sep 19, 2017

Reviewed 1 of 42 files at r1, 2 of 11 files at r2, 7 of 12 files at r3.
Review status: all files reviewed at latest revision, 21 unresolved discussions.


doc/libpmemobj.3.md, line 2613 at r3 (raw file):

The mapping between allocation class and object sizes.

This entry point takes a complex argument.

: ?


doc/libpmemobj.3.md, line 2632 at r3 (raw file):

= 0 and <= 255, or > 0 and < 255 ?
how about defining a macro for max class id ?


doc/libpmemobj.3.md, line 2668 at r3 (raw file):

The field `limit` defines the size in bytes to which allocations are handled
using custom allocation classes. Everything above is handled using entire blocks
and best-fit.

best-fit algorithm
is the "block" term already known to the reader ? What is the size of a block etc ?


doc/libpmemobj.3.md, line 2671 at r3 (raw file):

The field `granularity` defines the size to which all object sizes are aligned
to. Together with `limit`, it determines the size of the array containing the

"abstract array" ? is this really an array ? is it relevant for user that this is an array ?


doc/libpmemobj.3.md, line 2677 at r3 (raw file):

should fail when no mapping between a requested size and an allocation class
exists. If not set, all sizes without corresponding allocation class will be
handled using entire blocks (256 kilobytes).

do we have a macro for the block (chunk) size ? I can imagine that someone would like to create allocation classes programatically based on the user's data sizes and some of allocator's properties could be exposed through API.


src/libpmemobj/alloc_class.c, line 635 at r3 (raw file):

	size_t granularity, size_t limit, int fail_on_missing_class)
{
	size_t maps_size = (limit / granularity) + 1;

SIGFPE


src/libpmemobj/alloc_class.c, line 659 at r3 (raw file):

Previously, krzycz (Krzysztof Czurylo) wrote…

Why not 0? Is 255 an invalid class id or default one?

good question ?


src/libpmemobj/pmalloc.c, line 505 at r3 (raw file):

	if (ret != 0) {
		ERR("range setting would be invalid for the class");

invalid range settings for the allocation class ?

why would ?


Comments from Reviewable

@pbalcer

This comment has been minimized.

Show comment
Hide comment
@pbalcer

pbalcer Oct 30, 2017

Contributor

This is a big API change and I'm hesitant on the semantics this introduced.
We should rethink the interface and answer the question what we want to accomplish, and if it's not served in a sufficient manner by the current alloc class API.

Contributor

pbalcer commented Oct 30, 2017

This is a big API change and I'm hesitant on the semantics this introduced.
We should rethink the interface and answer the question what we want to accomplish, and if it's not served in a sufficient manner by the current alloc class API.

@pbalcer pbalcer closed this Oct 30, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment