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

Add support for new upstream kernel PM commands. #199

Merged
merged 75 commits into from Aug 9, 2022

Conversation

ossama-othman
Copy link
Member

@ossama-othman ossama-othman commented Jan 14, 2022

Implement support for the upcoming MPTCP netlink user space path commands commands in the upstream kernel. See multipath-tcp/mptcp_net-next#186 for the corresponding kernel-side GitHub issue.

Closes #129.
Closes #244.

@ossama-othman ossama-othman added the enhancement New feature or request label Jan 14, 2022
@ossama-othman ossama-othman self-assigned this Jan 14, 2022
@coveralls
Copy link

coveralls commented Jan 14, 2022

Pull Request Test Coverage Report for Build 2806147104

  • 303 of 351 (86.32%) changed or added relevant lines in 9 files are covered.
  • 2 unchanged lines in 2 files lost coverage.
  • Overall coverage increased (+2.3%) to 64.601%

Changes Missing Coverage Covered Lines Changed/Added Lines %
lib/id_manager.c 23 24 95.83%
lib/hash_sockaddr.c 39 41 95.12%
src/path_manager.c 3 12 25.0%
lib/listener_manager.c 112 130 86.15%
src/netlink_pm_upstream.c 111 129 86.05%
Files with Coverage Reduction New Missed Lines %
src/netlink_pm_upstream.c 1 71.01%
src/path_manager.c 1 20.65%
Totals Coverage Status
Change from base Build 2734055202: 2.3%
Covered Lines: 1376
Relevant Lines: 2130

💛 - Coveralls

@mjmartineau mjmartineau changed the title Add support for new upsttream kernel PM commands. Add support for new upstream kernel PM commands. Jan 14, 2022
@ossama-othman
Copy link
Member Author

Support for MPTCP_PM_CMD_SUBFLOW_CREATE and MPTCP_PM_CMD_SUBFLOW_DESTROY to be added shortly.

@ossama-othman
Copy link
Member Author

@kmaloor, FYI.

@ossama-othman
Copy link
Member Author

ossama-othman commented Jan 14, 2022

I need to fix the sspi plugin so that it generates real address IDs before I can do real testing with new user space PM support. That should be straightforward with the mptcpd address ID manager in <mptcpd/id_manager.h>

Update: This is done in PR #234.

@ossama-othman ossama-othman added this to In progress in mptcpd 0.10 via automation Feb 10, 2022
@ossama-othman ossama-othman added this to the Q1-2022 milestone Feb 10, 2022
@ossama-othman
Copy link
Member Author

Latest user space PM patch set for upstream kernel is available here.

@ossama-othman
Copy link
Member Author

Commit b3ffeba works around the symbol redefinition problem described by issue multipath-tcp/mptcp_net-next#276.

@ossama-othman
Copy link
Member Author

The test-commands unit test now fails with the introduction of the MPTCP listener manager code. The failure occurs due to an attempt to bind() the test address used in test-commands during a call to mptcpd_lm_listen() via mptcpd_pm_add_addr(). The test address is not associated with a network interface, Either we explicitly associate the test address with a network interface prior to running the test case or we use the loopback address instead, as is done in test-listener-manager.

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.

Thanks for pushing the listener changes!

I have a few suggestions on the listener manager, but this definitely gives a solid basis for the ndiffports plugin work.

src/listener_manager.c Outdated Show resolved Hide resolved
return false;
}

if (!l_hashmap_insert(lm, L_UINT_TO_PTR(id), L_UINT_TO_PTR(fd))) {
Copy link
Member

Choose a reason for hiding this comment

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

Should check for hash collision before creating the socket.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe "hash collision" isn't the right word. Better to say: check to see if the key is already used in the hashmap.

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 listener manager now does a lookup before creating a new hash map entry. If an entry exists, the ref count is incremented. Otherwise a new entry is created.

return l_test_run();
}


Copy link
Member

Choose a reason for hiding this comment

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

Suggestions for more coverage: Create multiple listeners. Test for collision.

Copy link
Member Author

Choose a reason for hiding this comment

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

Quite a few test cases were added to the test-listener-manager unit test. Hopefully they are enough. :)

@ossama-othman ossama-othman removed this from In progress in mptcpd 0.10 Jun 27, 2022
@ossama-othman ossama-othman added this to In progress in mptcpd 0.11 via automation Jun 27, 2022
In some cases it is useful to copy the contents of a sockaddr based on
the address family.  Add a new mptcpd_sockaddr_copy() function that
does so.  Only AF_INET and AF_INET6 address families are supported.
The mptcpd_pm_add_addr() function now expects a pointer to a non-const
struct sockaddr.  Update the sspi plugin accordingly.
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.

New batch of commits looks good to me

@ossama-othman
Copy link
Member Author

Thanks Mat! All I have left in my queue are the test-commands fixes.

Overhaul the test-commands unit test with the following notable
changes:
    * Use different addresses for user and kernel space address
      advertising related commands.  This is needed to prevent
      listening on the same TCP address and port.
    * Use the same local and remote addresses for all user space
      subflow related commands.
    * Log addresses obtained from mptcpd_kpm_get_addr().
@ossama-othman ossama-othman marked this pull request as ready for review August 5, 2022 06:53
@ossama-othman
Copy link
Member Author

ossama-othman commented Aug 5, 2022

The test-commands unit test fails in the GitHub Actions builds despite the push of the commits (73facae and cc7021e) that address the failures I was seeing. Perhaps the failure is kernel version related (5.15 on my Ubuntu 22.04 box). More debugging ...

Update: Fixed in commit c1883e4.

Split kernel and user space test cases to make it easier to determine
causes of failure.  The code is also cleaner with the split.

#include <ell/util.h>
Copy link
Member

Choose a reason for hiding this comment

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

This breaks with a recent ELL - needs the wrapper pragmas for these ELL includes

Copy link
Member Author

Choose a reason for hiding this comment

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

Fixed in commit e80db71.

Propagate errors to callers by returning 0 on success, and -1 or errno
on error.  This allows for better error handling and diagnostics.
The EADDRNOTAVAIL error will generally occur during a call to
mptcpd_pm_add_addr() if the test is run without sufficient
capabilities (CAP_NET_ADMIN) to set up the test addresses by
associating them with a network interface.  Ignore the failure for
now.
msg,
mptcpd_family_send_callback,
"remove_subflow", /* user data */
NULL /* destroy */) == 0;
}

static int upstream_set_backup(struct mptcpd_pm *pm,
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 upstream_set_backup() implementation is missing. Rather than delay this pull request further, I'd like to create another pull request for the upstream_set_backup() implementation. This is related to #237.

@ossama-othman ossama-othman merged commit fcf9afc into multipath-tcp:master Aug 9, 2022
mptcpd 0.11 automation moved this from In progress to Done Aug 9, 2022
@ossama-othman ossama-othman deleted the upstream-pm-cmds branch August 9, 2022 00:17
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.

Add parsing and plumbing for MPTCP_ATTR_SERVER_SIDE Support new upstream kernel netlink commands
3 participants