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.

Sign up for GitHub

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

Jump to bottom

UCT/UD: Filter incoming packets by DGID #1850

Merged
merged 2 commits into from
Sep 21, 2017
Merged

UCT/UD: Filter incoming packets by DGID #1850

merged 2 commits into from
Sep 21, 2017

Conversation

brminich
Copy link
Contributor

@brminich brminich commented Sep 19, 2017 •
edited
Loading

When UD is used with RoCE, packets intended for other peers may arrive (as a result of unicast flooding). These packets need to be dropped.

@brminich
UCT/UD: Filter incoming packets by DGID
7d8d339 
When UD is used with RoCE, packets intended for other peers may arrive
(as a result of unicast flooding). These packets needs to be dropped.
@brminich
Copy link
Contributor Author

fixes #1005

@swx-jenkins1
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2643/ for details.

yosefe
yosefe reviewed Sep 19, 2017
View reviewed changes
src/uct/ib/ud/base/ud_iface.c Outdated
@@ -511,6 +550,9 @@ ucs_config_field_t uct_ud_iface_config_table[] = {
{"SLOW_TIMER_BACKOFF", "2.0", "Timeout multiplier for resending trigger",
ucs_offsetof(uct_ud_iface_config_t, slow_timer_backoff),
UCS_CONFIG_TYPE_DOUBLE},
{"ETH_GID_FILTER_ENABLE", "y",
Copy link
Contributor

Choose a reason for hiding this comment

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

ETH_DGID_CHECK

src/uct/ib/ud/base/ud_iface.c Outdated
@@ -511,6 +550,9 @@ ucs_config_field_t uct_ud_iface_config_table[] = {
{"SLOW_TIMER_BACKOFF", "2.0", "Timeout multiplier for resending trigger",
ucs_offsetof(uct_ud_iface_config_t, slow_timer_backoff),
UCS_CONFIG_TYPE_DOUBLE},
{"ETH_GID_FILTER_ENABLE", "y",
"Enable packets filtering by destination GIDs on an Ethernet network",
Copy link
Contributor

Choose a reason for hiding this comment

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

Enable checking destination GID for incoming packets of Ethernet networks. Mismatched packets are silently dropped.

Copy link
Contributor

Choose a reason for hiding this comment

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

Btw,shall we have counter for such drop eventsfor future perf work?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will add in the next PR (for this to be merged asap). Opened issue for that #1855

src/uct/ib/ud/base/ud_iface.c Outdated
* entire 40 bytes. IPv4 headers use the 20 bytes in the second half of the
* reserved 40 bytes area (i.e. offset 20 from the beginning of the receive
* buffer). In this case, the content of the first 20 bytes is undefined." */
static void uct_ud_iface_parse_gid_format(uct_ud_iface_t *iface)
Copy link
Contributor

Choose a reason for hiding this comment

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

either rename to "uct_ud_iface_calc_dgid_len" or this function should accept GID (as const *) and return its length, then it should be placed in common IB code.

src/uct/ib/ud/base/ud_iface.c Outdated
* buffer). In this case, the content of the first 20 bytes is undefined." */
static void uct_ud_iface_parse_gid_format(uct_ud_iface_t *iface)
{
const int ipv4_len = 4;
Copy link
Contributor

Choose a reason for hiding this comment

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

const size_t ipv4_len = sizeof(struct in_addr)
const size_t ipv6_len = sizeof(struct in6_addr)

src/uct/ib/ud/base/ud_iface.c Outdated
uint16_t *first = (uint16_t*)iface->super.gid.raw;
uint16_t *filled = (uint16_t*)iface->super.gid.raw + 5;

/* Make sure that daddr in IPv4 takes last 4 bytes in GRH */
Copy link
Contributor

Choose a reason for hiding this comment

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

/* Make sure that daddr in IPv4 resides in the last 4 bytes in GRH */

src/uct/ib/ud/base/ud_iface.h Outdated
@@ -123,6 +124,8 @@ struct uct_ud_iface {
double slow_timer_backoff;
unsigned tx_qp_len;
unsigned max_inline;
int filter_enabled;
Copy link
Contributor

Choose a reason for hiding this comment

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

int check_grh_dgid

src/uct/ib/ud/base/ud_iface.h Outdated
@@ -215,6 +218,33 @@ static UCS_F_ALWAYS_INLINE void uct_ud_leave(uct_ud_iface_t *iface)
UCS_ASYNC_UNBLOCK(iface->super.super.worker->async);
}

static UCS_F_ALWAYS_INLINE int
uct_ud_iface_filter_dgid(uct_ud_iface_t *iface,void *grh_end,
Copy link
Contributor

Choose a reason for hiding this comment

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

rename: uct_ud_iface_check_grh
code style: space after ","

src/uct/ib/ud/base/ud_iface.h Outdated
dgid = (char*)grh_end - iface->config.dgid_len;

if (memcmp(sgid, dgid, iface->config.dgid_len)) {
ucs_mpool_put_inline(desc);
Copy link
Contributor

Choose a reason for hiding this comment

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

better release desc outside the function. the function should focus on just checking the condition and not cause any side-effects.

src/uct/ib/ud/base/ud_iface.h Outdated
}

if (ucs_unlikely(!is_grh_present)) {
ucs_error("RoCE packet does not contain GRH");
Copy link
Contributor

Choose a reason for hiding this comment

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

ucs_warn

src/uct/ib/ud/base/ud_iface.h Outdated
return 0;
}

sgid = (char*)iface->super.gid.raw + (16 - iface->config.dgid_len);
Copy link
Contributor

Choose a reason for hiding this comment

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

this is not "sgid", this is "local_gid"

@mellanox-github
Copy link
Contributor

Test FAILed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4618/ for details (Mellanox internal link).

@brminich brminich mentioned this pull request Sep 20, 2017
Closed
@mellanox-github
Copy link
Contributor

Test PASSed.
See http://hpc-master.lab.mtl.com:8080/job/hpc-ucx-pr/4634/ for details (Mellanox internal link).

@swx-jenkins1
Copy link

Test FAILed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2658/ for details.

@brminich
Copy link
Contributor Author

bot:bgate:retest

@swx-jenkins1
Copy link

Test PASSed.
See http://bgate.mellanox.com/jenkins/job/gh-ucx-pr/2669/ for details.

@yosefe yosefe added the Bugfix label Sep 21, 2017
@yosefe yosefe merged commit e68a822 into openucx:master Sep 21, 2017
@brminich brminich mentioned this pull request Sep 22, 2017
Merged
@brminich brminich deleted the topic/ud_filtering branch September 24, 2017 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mike-dubman mike-dubman mike-dubman left review comments

@yosefe yosefe yosefe approved these changes

Assignees
No one assigned
Labels
Bugfix
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@brminich @swx-jenkins1 @mellanox-github @mike-dubman @yosefe