Page MenuHomeFreeBSD
Differential D19930

Track device's NUMA domain in ifnet & allocate ifnet from NUMA local memory
ClosedPublic
Actions

Authored by gallatin on Apr 17 2019, 12:09 AM.
Tags
None
Referenced Files
Unknown Object (File)
Wed, Nov 27, 9:21 PM
Unknown Object (File)
Oct 6 2024, 3:45 PM
Unknown Object (File)
Sep 24 2024, 12:22 PM
Unknown Object (File)
Sep 24 2024, 12:22 PM
Unknown Object (File)
Sep 21 2024, 8:01 AM
Unknown Object (File)
Sep 12 2024, 8:12 PM
Unknown Object (File)
Sep 6 2024, 9:27 PM
Unknown Object (File)
Sep 4 2024, 8:43 PM
View All 91 Files
Subscribers
ae
imp

Details

Reviewers
kib
np
jhb
jeff
• hselasky
markj
glebius
slavash
scottl
Group Reviewers
manpages
Commits
rS346579: Track device's NUMA domain in ifnet & alloc ifnet from NUMA local memory
Summary

I have added a new if_alloc_domain() and if_alloc_dev() and ported 2 example drivers to use this new interface. (Assuming this patch is accepted, I will follow on with patches to at least iflib(4) and mxge(4)).
When called with a domain on a NUMA machine, ifalloc_domain() will record the NUMA domain in the ifnet, and it will allocate the ifnet struct from memory which is local to the device's NUMA node.

Drivers can call if_alloc_dev() to automatically record the NUMA domain in the ifnet and allocate the ifnet from local memory.

The NUMA node is used in a follow on patch to enhance lagg lacp egress port selection based on NUMA domain. (in addition to another patch which records NUMA domain information in the inp).

I have intentionally avoided ifdef NUMA as much as possible; all code is out of the critical path (only run at alloc/free) and gracefully falls back to the old behavior when run on a non-NUMA kernel or with a NUMA kernel on non-NUMA hardware.

The new if_numa_domain field fits in an alignment pad in struct ifnet, and so does not alter the size of the structure.

Diff Detail

Repository
rS FreeBSD src repository - subversion
Lint
Lint Skipped
Unit
Tests Skipped
Build Status
Buildable 23715

Event Timeline

gallatin created this revision.Apr 17 2019, 12:09 AM
Herald added a reviewer: manpages. · View Herald TranscriptApr 17 2019, 12:09 AM
Herald added subscribers: ae, imp. · View Herald Transcript
Harbormaster completed remote builds in B23705: Diff 56266.Apr 17 2019, 12:09 AM
gallatin added a reviewer: glebius.Apr 17 2019, 12:09 AM
• hselasky added a reviewer: slavash.Apr 17 2019, 7:38 AM
kib added a comment.Apr 17 2019, 8:20 AM

I wonder if a KPI of the format

struct ifnet *if_alloc_dev(u_char type, dev_t device);

would be more useful. It would hide all NUMA/non-NUMA/bus_get_domain() failed details that driver authors will copy/paste ad infinitum.

I do not propose to not add if_alloc_domain(), just think that if_alloc_dev() should be the primari KPI. Also it might allow to get more device_t context in if_alloc().

sys/net/if.c
540

There should be spaces around binary op.

594

There should be a blank line after '{' if there is no locals.

gallatin added a comment.Apr 17 2019, 12:21 PM
In D19930#428329, @kib wrote:

I wonder if a KPI of the format

struct ifnet *if_alloc_dev(u_char type, dev_t device);

would be more useful. It would hide all NUMA/non-NUMA/bus_get_domain() failed details that driver authors will copy/paste ad infinitum.

I do not propose to not add if_alloc_domain(), just think that if_alloc_dev() should be the primari KPI. Also it might allow to get more device_t context in if_alloc().

I think I agree with you. I was torn between the 2 different KPIs. Given that you suggested what I already was thinking about, I will implement it.

I'm trying to think now how if_alloc_domain() could be useful for non-device ifnets, and it does not seem to me at all clear that it would be. So I think I will replace if_alloc_domain() with if_alloc_dev()

kib added a comment.Apr 17 2019, 1:13 PM
In D19930#428396, @gallatin wrote:
In D19930#428329, @kib wrote:

I wonder if a KPI of the format

struct ifnet *if_alloc_dev(u_char type, dev_t device);

would be more useful. It would hide all NUMA/non-NUMA/bus_get_domain() failed details that driver authors will copy/paste ad infinitum.

I do not propose to not add if_alloc_domain(), just think that if_alloc_dev() should be the primari KPI. Also it might allow to get more device_t context in if_alloc().

I think I agree with you. I was torn between the 2 different KPIs. Given that you suggested what I already was thinking about, I will implement it.

I'm trying to think now how if_alloc_domain() could be useful for non-device ifnets, and it does not seem to me at all clear that it would be. So I think I will replace if_alloc_domain() with if_alloc_dev()

if_alloc_domain() could be useful for specific cases of synthetic ifnets, e.g. the mentioned lagg when both ifnets belong to the same domain, or even more clear, if_vlan for the domain of the main interface.

gallatin updated this revision to Diff 56286.Apr 17 2019, 3:34 PM
gallatin edited the summary of this revision. (Show Details)
  • Added if_alloc_dev() and changed example mlx5en and cxgbe code to use them, as suggested by kib.
  • Added links to if_alloc_domain and if_alloc_dev for man pages
  • Fixed style issues pointed out by kib
Harbormaster completed remote builds in B23711: Diff 56286.Apr 17 2019, 3:34 PM
gallatin marked 2 inline comments as done.Apr 17 2019, 3:34 PM
kib added inline comments.Apr 17 2019, 4:41 PM
sys/net/if.c
42

Thus header is excessive if domainset.h is included ?

599

I would write it as

if (dev == NULL || bus_get_domain(dev, &numa_domain) != 0)
   return (if_alloc_domain(type, IF_NODOM);
return (if_alloc_domain(type, numa_domain);
gallatin updated this revision to Diff 56302.Apr 17 2019, 5:53 PM

Address kib's feedback

  • remove unneeded _domainset.h include
  • re-write if_alloc_dev() to be more concise
Harbormaster completed remote builds in B23715: Diff 56302.Apr 17 2019, 5:53 PM
gallatin marked an inline comment as done.Apr 17 2019, 5:54 PM
gallatin added inline comments.
sys/net/if.c
42

Indeed, you are correct. I confess to just copying from the domainset(9) man page.

kib accepted this revision.Apr 17 2019, 6:19 PM
This revision is now accepted and ready to land.Apr 17 2019, 6:19 PM
gallatin added a reviewer: scottl.Apr 18 2019, 12:43 PM
markj accepted this revision.Apr 18 2019, 12:58 PM
markj added inline comments.
sys/net/if.c
596

There are redundant parens around the call.

gallatin updated this revision to Diff 56348.Apr 18 2019, 3:18 PM

Removed redundant parens around bus_get_domain() call as pointed out by markj

This revision now requires review to proceed.Apr 18 2019, 3:18 PM
Harbormaster completed remote builds in B23742: Diff 56348.Apr 18 2019, 3:18 PM
gallatin marked an inline comment as done.Apr 18 2019, 3:19 PM
This revision was not accepted when it landed; it landed in state Needs Review.Apr 22 2019, 7:24 PM
Closed by commit rS346579: Track device's NUMA domain in ifnet & alloc ifnet from NUMA local memory (authored by gallatin). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathSize
share/
man/
man9/
2 lines
36 lines
sys/
dev/
cxgbe/
2 lines
mlx5/
mlx5_en/
2 lines
net/
33 lines
5 lines

Diff 56302

View Options

share/man/man9/Makefile

Loading...
View Options

share/man/man9/ifnet.9

Loading...
View Options

sys/dev/cxgbe/t4_main.c

Loading...
View Options

sys/dev/mlx5/mlx5_en/mlx5_en_main.c

Loading...
View Options

sys/net/if.c

Loading...
View Options

sys/net/if_var.h

Loading...