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

Support only one kernel at run-time, #156

Merged
merged 23 commits into from
Jan 11, 2022

Conversation

ossama-othman
Copy link
Member

@ossama-othman ossama-othman commented Oct 4, 2021

Support only one MPTCP capable kernel at run-time, either the upstream or multipath-tcp.org kernel but not both. This simplifies the kernel support in mptcpd, and further prepares mptcpd to support the upcoming user space MPTCP path management commands in the upstream kernel.

@ossama-othman ossama-othman added the enhancement New feature or request label Oct 4, 2021
@ossama-othman ossama-othman added this to the Q4-2021 milestone Oct 4, 2021
@ossama-othman ossama-othman self-assigned this Oct 4, 2021
@ossama-othman ossama-othman added this to In progress in mptcpd 0.9 via automation Oct 4, 2021
@mjmartineau
Copy link
Member

Feedback by way of git commits: https://github.com/mjmartineau/mptcpd/commits/user-pm-cmds

@ossama-othman ossama-othman removed the request for review from mjmartineau October 20, 2021 03:38
@ossama-othman ossama-othman changed the title Support user space path management commands for the upstream kernel Support only one kernel at run-time, Jan 4, 2022
@ossama-othman
Copy link
Member Author

@mjmartineau, I'm splitting the new user space PM command support into a pull request separate from this one. I think this one is self-contained enough.

@ossama-othman ossama-othman marked this pull request as ready for review January 4, 2022 19:49
@ossama-othman ossama-othman marked this pull request as draft January 4, 2022 21:25
@ossama-othman
Copy link
Member Author

Switched to draft. Some additional changes are still pending for this pull request.

@ossama-othman ossama-othman removed the request for review from mjmartineau January 4, 2022 21:25
Refactor the multipath-tcp.org kernel path management commands
implementations so that they may be leveraged by the corresponding and
pending upstream kernel user space commands.
Allow the user to choose build-time support for either the upstream or
multipath-tcp.org kernel, e.g.:

    ./configure --with-kernel=multipath-tcp.org

Valid kernel options are "upstream" or "multipath-tcp.org", with the
former being the default.  Mptcpd will no longer support both kernels
at run-time.
The MPTCP path management generic netlink API commands between the
upstream and multipath-tcp.org kernels will not be source compatible.
Move the refactored command implementation back
netlink_pm_mptcp_org.c.

This reverts commit dc92d3b.
The common netlink PM command implementation in mptcpd will not work
both the upstream and multipath-tcp.org kernels since their netlink
path management APIs are not the same.  Move kernel-specific code to
their respective netlink_pm_{mptcp_org,upstream}.c source file.
Don't bother running the mptcpd unit tests for the multipath-tcp.org
kernel build since the GitHub Ubuntu environment doesn't run it.
Refactor upstream and multipath-tcp.org <linux/mptcp.h> header
detection checks to a new `m4/mptcpd_kernel.m4' Autoconf macro file.
The HAVE_LINUX_MPTCP_H_UPSTREAM_EVENTS preprocessor symbol is no
longer used.  Drop references to it.
@coveralls
Copy link

coveralls commented Jan 7, 2022

Pull Request Test Coverage Report for Build 1670502137

  • 41 of 41 (100.0%) changed or added relevant lines in 3 files are covered.
  • 2 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+4.4%) to 53.266%

Files with Coverage Reduction New Missed Lines %
src/path_manager.c 2 14.39%
Totals Coverage Status
Change from base Build 1664962841: 4.4%
Covered Lines: 1060
Relevant Lines: 1990

💛 - Coveralls

@@ -84,6 +86,8 @@ enum {
MPTCP_PM_CMD_SET_LIMITS,
MPTCP_PM_CMD_GET_LIMITS,
MPTCP_PM_CMD_SET_FLAGS,
MPTCP_PM_CMD_ANNOUNCE,
MPTCP_PM_CMD_REMOVE,
Copy link
Member Author

Choose a reason for hiding this comment

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

I know there's more to the new API than this. More to come in a subsequent pull request.

@ossama-othman ossama-othman marked this pull request as ready for review January 8, 2022 05:24
@ossama-othman
Copy link
Member Author

@mjmartineau I don't have any other changes pending for this pull request. A subsequent pull request will replace the stubs for the new upstream netlink PM API in src/netlink_pm_upstream.c.

Copy link
Member

@mjmartineau mjmartineau left a comment

Choose a reason for hiding this comment

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

Looks ok to me. I did build and run the tests with the upstream kernel configuration, but only configured and built the multipath-tcp.org version (no self tests attempted). Would it be helpful for me to dust off my multipath-tcp.org VM and run the tests there, or have you covered that already?

@@ -280,14 +274,14 @@ LIBS="$LIBS $ELL_LIBS"
dnl l_genl_msg_get_extended_error() was introduced in ELL v0.31.
AC_CHECK_FUNC([l_genl_msg_get_extended_error],
[AC_DEFINE([HAVE_L_GENL_MSG_GET_EXTENDED_ERROR],
[],
[ELL has l_genl_msg_get_extended_error()])])
[],
Copy link
Member

Choose a reason for hiding this comment

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

Tab to space conversion? Wasn't sure if it was intentional, but looks like everything has now been normalized to spaces, so that seems fine.

Copy link
Member Author

Choose a reason for hiding this comment

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

The spaces were intended. It seems there were left over tabs in this file from previous commits that I missed.

mptcpd 0.9 automation moved this from In progress to Reviewer approved Jan 10, 2022
@ossama-othman
Copy link
Member Author

ossama-othman commented Jan 10, 2022

Looks ok to me. I did build and run the tests with the upstream kernel configuration, but only configured and built the multipath-tcp.org version (no self tests attempted). Would it be helpful for me to dust off my multipath-tcp.org VM and run the tests there, or have you covered that already?

Only if you have time. Since I've been focusing on the upstream kernel, I haven't cranked up my multipath-tcp.org kernel VM in a while, so I'll give it a try.

Thanks!

@ossama-othman
Copy link
Member Author

I'll commit any potential changes or fixes for the multipath-tcp.org kernel in a separate pull request.

@ossama-othman ossama-othman merged commit 567afaa into multipath-tcp:master Jan 11, 2022
mptcpd 0.9 automation moved this from Reviewer approved to Done Jan 11, 2022
@ossama-othman ossama-othman deleted the user-pm-cmds branch January 11, 2022 06:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants