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

deserialization module optimization #331

Merged
merged 4 commits into from
May 20, 2024
Merged

deserialization module optimization #331

merged 4 commits into from
May 20, 2024

Conversation

nlgwcy
Copy link
Contributor

@nlgwcy nlgwcy commented May 15, 2024

What type of PR is this?
/kind cleanup
/kind enhancement
/kind documentation

What this PR does / why we need it:

  • deserialization module dfx optimization
  • add document
  • cleanup redundant process code

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


@kmesh-bot kmesh-bot added kind/cleanup kind/enhancement New feature or request labels May 15, 2024
@kmesh-bot
Copy link
Collaborator

@nlgwcy: The label(s) kind/documentation cannot be applied, because the repository doesn't have them.

In response to this:

What type of PR is this?
/kind cleanup
/kind enhancement
/kind documentation

What this PR does / why we need it:

  • deserialization module dfx optimization
  • add document
  • cleanup redundant process code

Which issue(s) this PR fixes:
Fixes #

Special notes for your reviewer:

Does this PR introduce a user-facing change?:


Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@nlgwcy
Copy link
Contributor Author

nlgwcy commented May 15, 2024

/status in-progress

@nlgwcy nlgwcy changed the title deserialization module optimization [WIP]deserialization module optimization May 16, 2024
@@ -590,7 +595,7 @@ int deserial_update_elem(void *key, void *value)

ret = get_map_fd_info(id, &map_fd, &info);
if (ret < 0) {
LOG_ERR("invlid MAP_ID: %d\n", id);
LOG_ERR("invlid MAP_ID: %d, errno:%d\n", id, errno);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
LOG_ERR("invlid MAP_ID: %d, errno:%d\n", id, errno);
LOG_ERR("invlid MAP_ID: %d, get map failed:%d\n", id, errno);

I'm not sure what fd means, representing the actual operation in the logs allows for a better understanding of the error

@nlgwcy nlgwcy changed the title [WIP]deserialization module optimization deserialization module optimization May 18, 2024
The current implementation scheme is as follows:

- Bitmap is used to manage which inner_map records are free, and the bitmap information is stored in the first record of the outter_map.
- During xDS configuration creation, the outter_map is searched for a `idle`idx, and the corresponding inner_map is created. The inner_map information is added to the outter_map table, and the bitmap information is updated as `used`(the outter_map table is also updated).
Copy link
Member

Choose a reason for hiding this comment

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

outter_map size is 8192 , and i believe it is not enough, As we can see there are almost 8192/100+1 threads to do some initiaztion work. How can we adapt to the size increase later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

in Risks and Mitigations section, possible solution are mentioned, and can be tracked in #330


As we can see, one xDS change involves multiple outter_map refreshes. Here is the optimization approach (space-time trade-off):

- When kmesh-daemon starts, create all outter_map records (including inner_map) at once, using multiple threads to parallelize the refresh process since the outter_map table updates slowly.
Copy link
Member

Choose a reason for hiding this comment

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

why does outter_map table update slowly? Is this because BPF_MAP_TYPE_ARRAY_OF_MAPS

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yes, kernel has set synchronization operations in this PR and has been updated in the docs.

nlgwcy added 4 commits May 20, 2024 16:22
Signed-off-by: wuchangye <wuchangye@huawei.com>
Signed-off-by: wuchangye <wuchangye@huawei.com>
Signed-off-by: wuchangye <wuchangye@huawei.com>
Signed-off-by: wuchangye <wuchangye@huawei.com>
printf("inner_map_create %d failed:%d\n", i, fd);
}
if (i < MAX_OUTTER_MAP_ENTRIES)
LOG_WARN("[warning]inner_map_create (%d->%d) failed:%d, errno:%d\n", i, MAX_OUTTER_MAP_ENTRIES, fd, errno);
Copy link
Member

Choose a reason for hiding this comment

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

donot we need to return erro nunber? ANd what will happend if later we need to r/w on the map

Copy link
Member

@hzxuzhonghu hzxuzhonghu left a comment

Choose a reason for hiding this comment

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

/lgtm

@kmesh-bot
Copy link
Collaborator

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: hzxuzhonghu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@kmesh-bot kmesh-bot merged commit 2a3d566 into kmesh-net:main May 20, 2024
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants