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

[PATCH CLOUD-DEV v4] Move pool to modular framework #120

Closed
wants to merge 2 commits into
base: cloud-dev
from

Conversation

Projects
None yet
4 participants
@kevinwangsk
Contributor

kevinwangsk commented Aug 9, 2017

Separate with two commits.
First one is to add the pool subsystem in modular framework.
Second one is to add the generic pool implementation to the pool subsystem.

@muvarov muvarov changed the title from Move pool to modular framework to [PATCH CLOUD-DEV v1] Move pool to modular framework Aug 9, 2017

@muvarov muvarov added the Email_sent label Aug 9, 2017

@Bill-Fischofer-Linaro

Numerous Travis failures reported that need to be fixed. Also, please try to update this pull request rather than creating a new PR for each revision.

@muvarov muvarov changed the title from [PATCH CLOUD-DEV v1] Move pool to modular framework to [PATCH CLOUD-DEV v2] Move pool to modular framework Aug 10, 2017

Kevin Wang
linux-gen: pool: apply modular framework and create subsystem
Signed-off-by: Kevin Wang <kevin.wang@arm.com>

@muvarov muvarov changed the title from [PATCH CLOUD-DEV v2] Move pool to modular framework to [PATCH CLOUD-DEV v3] Move pool to modular framework Aug 14, 2017

@heyi-arm

Reviewed-by: Yi He yi.he@linaro.org

I saw build failure but it seems not relevant to these commits, I've triggered travis re-run to check again.

@heyi-arm

This comment has been minimized.

Show comment
Hide comment
@heyi-arm

heyi-arm Aug 14, 2017

Collaborator

It failed again at the same place, Kevin please have a look into the Travis CI log and see if you can replicate the problem.

Collaborator

heyi-arm commented Aug 14, 2017

It failed again at the same place, Kevin please have a look into the Travis CI log and see if you can replicate the problem.

@kevinwangsk

This comment has been minimized.

Show comment
Hide comment
@kevinwangsk

kevinwangsk Aug 14, 2017

Contributor

Seems "pool/generic.lo: CFLAGS += -DIM_ACTIVE_MODULE" doesn't take affect in the 20th test case. So there is not active module in pool subsystem. Need take some more time to figure out the reason.

Contributor

kevinwangsk commented Aug 14, 2017

Seems "pool/generic.lo: CFLAGS += -DIM_ACTIVE_MODULE" doesn't take affect in the 20th test case. So there is not active module in pool subsystem. Need take some more time to figure out the reason.

@muvarov muvarov added the Email_sent label Aug 15, 2017

Kevin Wang
linux-gen: pool: add generic pool module to mempool subsystem
Signed-off-by: Kevin Wang <kevin.wang@arm.com>

@muvarov muvarov changed the title from [PATCH CLOUD-DEV v3] Move pool to modular framework to [PATCH CLOUD-DEV v4] Move pool to modular framework Aug 16, 2017

@muvarov muvarov added Email_sent and removed Email_sent labels Aug 16, 2017

@heyi-arm

This comment has been minimized.

Show comment
Hide comment
@heyi-arm

heyi-arm Aug 16, 2017

Collaborator

@Bill-Fischofer-Linaro Hi, Bill this PR has fixed and passed the Travis CI, I've reviewed and if it looks OK to you I'll merge it.

Collaborator

heyi-arm commented Aug 16, 2017

@Bill-Fischofer-Linaro Hi, Bill this PR has fixed and passed the Travis CI, I've reviewed and if it looks OK to you I'll merge it.

@Bill-Fischofer-Linaro

This comment has been minimized.

Show comment
Hide comment
@Bill-Fischofer-Linaro

Bill-Fischofer-Linaro Aug 16, 2017

Member

I'll review this PR today. Thanks.

Member

Bill-Fischofer-Linaro commented Aug 16, 2017

I'll review this PR today. Thanks.

@Bill-Fischofer-Linaro

This looks very good, but could use a few adjustments we can discuss during tomorrow's call.

Show outdated Hide outdated platform/linux-generic/pool/subsystem.c
ODP_ERR("No defined init_global function "
"in module %s of pool subsystem\n", mod->base.name);
return -1;
}

This comment has been minimized.

@Bill-Fischofer-Linaro

Bill-Fischofer-Linaro Aug 16, 2017

Member

This pattern is highly repetitive, which suggests it should be factored out as part of the modular framework itself rather than having to be repeated for every API. I fact it seems that the entire subsystem.c file for each module could be auto-generated since there's no real variant code here.

@Bill-Fischofer-Linaro

Bill-Fischofer-Linaro Aug 16, 2017

Member

This pattern is highly repetitive, which suggests it should be factored out as part of the modular framework itself rather than having to be repeated for every API. I fact it seems that the entire subsystem.c file for each module could be auto-generated since there's no real variant code here.

This comment has been minimized.

@heyi-arm

heyi-arm Aug 17, 2017

Collaborator

Yes, I leave it to user programmer, so different policies can be impled, like:

  1. IF a subsystem assumes all its api pointers should not be NULL, it can bypass some checks.
  2. IF a subsystem assumes at least one active module exist, it can bypass some checks.
  3. a subsystem can write these checks in subsystem->init_global() to assure active module exists and/or api pointers are not NULL.
  4. in least case, user programmer can write inline/macro to check per their subsystem's requirements.
@heyi-arm

heyi-arm Aug 17, 2017

Collaborator

Yes, I leave it to user programmer, so different policies can be impled, like:

  1. IF a subsystem assumes all its api pointers should not be NULL, it can bypass some checks.
  2. IF a subsystem assumes at least one active module exist, it can bypass some checks.
  3. a subsystem can write these checks in subsystem->init_global() to assure active module exists and/or api pointers are not NULL.
  4. in least case, user programmer can write inline/macro to check per their subsystem's requirements.

This comment has been minimized.

@Bill-Fischofer-Linaro

Bill-Fischofer-Linaro Aug 17, 2017

Member

It makes sense to do these checks at init time when dynamic loading is involved, but once a module is loaded and bound these just add overhead, which should be unnecessary. The odp_pool_xxx() APIs are not executed with great frequency so this overhead may not matter here, but for things like pktio operations such extra overhead would be very undesirable.

@Bill-Fischofer-Linaro

Bill-Fischofer-Linaro Aug 17, 2017

Member

It makes sense to do these checks at init time when dynamic loading is involved, but once a module is loaded and bound these just add overhead, which should be unnecessary. The odp_pool_xxx() APIs are not executed with great frequency so this overhead may not matter here, but for things like pktio operations such extra overhead would be very undesirable.

This comment has been minimized.

@heyi-arm

heyi-arm Aug 17, 2017

Collaborator

Yes, we saw old code also does not check NULL pointer errors in API wrappers, here can do the same and assume all modules understand this requirement.

@heyi-arm

heyi-arm Aug 17, 2017

Collaborator

Yes, we saw old code also does not check NULL pointer errors in API wrappers, here can do the same and assume all modules understand this requirement.

This comment has been minimized.

@kevinwangsk

kevinwangsk Aug 17, 2017

Contributor

Agree that we just need to check the active module at the init function. After that, there should be an active module before invoke any other pool operations. This should be the same with other subsystem.

@kevinwangsk

kevinwangsk Aug 17, 2017

Contributor

Agree that we just need to check the active module at the init function. After that, there should be an active module before invoke any other pool operations. This should be the same with other subsystem.

odp_api_proto(pool, info) info;
odp_api_proto(pool, print) print;
odp_api_proto(pool, to_u64) to_u64;
odp_api_proto(pool, param_init) param_init;

This comment has been minimized.

@Bill-Fischofer-Linaro

Bill-Fischofer-Linaro Aug 16, 2017

Member

Any reason why these shouldn't be alphabetical? That's been a standard we've used in the various .am and related files.

@Bill-Fischofer-Linaro

Bill-Fischofer-Linaro Aug 16, 2017

Member

Any reason why these shouldn't be alphabetical? That's been a standard we've used in the various .am and related files.

This comment has been minimized.

@kevinwangsk

kevinwangsk Aug 17, 2017

Contributor

This is to define the function pointer for the public APIs. So I am just following the sequence in ODP API doc.

@kevinwangsk

kevinwangsk Aug 17, 2017

Contributor

This is to define the function pointer for the public APIs. So I am just following the sequence in ODP API doc.

@heyi-arm

This comment has been minimized.

Show comment
Hide comment
@heyi-arm

heyi-arm Aug 18, 2017

Collaborator

Merged and closed.

Collaborator

heyi-arm commented Aug 18, 2017

Merged and closed.

@heyi-arm heyi-arm closed this Aug 18, 2017

@kevinwangsk kevinwangsk deleted the kevinwangsk:cloud-dev-pool-modular branch Sep 14, 2017

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