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

Use Devlink when possible for NewPciNetDevice #399

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Use Devlink when possible for NewPciNetDevice #399

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion pkg/netdevice/pciNetDevice.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,13 @@ func NewPciNetDevice(dev *ghw.PCIDevice, rFactory types.ResourceFactory, rc *typ
}

linkType := ""
if len(ifName) > 0 {
if _, err = utils.GetNetlinkProvider().GetDevLinkDevice(pciAddr); err == nil {
linkType = "ether"
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@adrianchiris question if the nic is InfiniBand will it get reported in devlink dev command?

I don't have a card to test this

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is there a linkType field in devlink device ?
I only see the port Type field in devlink port attribute, but not in devlink device.
I was thinking we can reliably get the linkType from devlink interface for devlink device.

Copy link
Contributor

Choose a reason for hiding this comment

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

@adrianchiris question if the nic is InfiniBand will it get reported in devlink dev command?

it will report a devlink device. (tested on cx5)

however you need devlink port as Zenghui mentioned.
Now, for infiniband port, i dont think mlx driver has implemented the feature :(.

running on my system i didnt see devlink port for infiniband PF

// PF 03:00.1 is infiniband
$ devlink dev show
pci/0000:03:00.0
pci/0000:03:00.1
$ devlink port show
pci/0000:03:00.0/65535: type eth netdev enp3s0f0 flavour physical port 0

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for the comment I have to say this is not working for me.

I try it on my system

lspci | grep Mellanox
5e:00.0 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5]
5e:00.1 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5]
5e:00.2 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
5e:00.3 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
5e:00.4 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
5e:00.5 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
5e:00.6 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
5e:00.7 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
5e:01.0 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
5e:01.1 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
5e:01.2 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]
5e:01.3 Ethernet controller: Mellanox Technologies MT27800 Family [ConnectX-5 Virtual Function]

I try the devlink port show and it's empty on my system do I miss something?

[root@test core]# devlink port show
[root@test core]#

Copy link
Contributor

Choose a reason for hiding this comment

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

I try the devlink port show and it's empty on my system do I miss something?

hmmm, maybe you need a newer kernel ?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

your version is a bit newer

[core@cnfdt04 ~]$ uname -a
Linux cnfdt04 4.18.0-305.19.1.el8_4.x86_64 #1 SMP Tue Sep 7 07:07:31 EDT 2021 x86_64 x86_64 x86_64 GNU/Linux
[core@cnfdt04 ~]$ devlink port
[core@cnfdt04 ~]$

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @bn222 I try it with the same kernel you did and I am still not able to get the information

[root@cnfdd8 core]# devlink dev
pci/0000:19:00.0
pci/0000:19:00.1
pci/0000:3b:00.0
pci/0000:3b:00.1
pci/0000:86:00.0
pci/0000:86:00.1
[root@cnfdd8 core]# devlink port
[root@cnfdd8 core]# 
[root@cnfdd8 core]# 
[root@cnfdd8 core]# uname -a
Linux cnfdd8 4.18.0-305.28.1.el8_4.x86_64 #1 SMP Mon Nov 8 07:45:47 EST 2021 x86_64 x86_64 x86_64 GNU/Linux

Copy link
Contributor

@bn222 bn222 Dec 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Hi @SchSeba , I just checked this again. I was confused by other NICs on my system. I can confirm that I also don't have it showing up in devlink port.
I'm using a slightly different ConnectX-5:

d8:00.0 Ethernet controller: Mellanox Technologies MT28800 Family [ConnectX-5 Ex] d8:00.1 Ethernet controller: Mellanox Technologies MT28800 Family [ConnectX-5 Ex]

I also tried the latest RHEL kernel, also not showing anything there.

However, I also just built an upstream kernel, and there it works. I get:

pci/0000:d8:00.0/65535: type eth netdev ens8f0np0 flavour physical port 0 splittable false pci/0000:d8:00.1/131071: type eth netdev ens8f1np1 flavour physical port 1 splittable false

I suspect RHEL is missing some patches. @adrianchiris Do you have an idea what patches could be missing?

I could start a bisecting effort to pinpoint the patches if we have no clue.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@bn222 will the type by ib if the nic is in InfiniBand mode?

I must say I have a problem needing a specific kernel and the solution working only for some time of network nics.

Is it possible to drop this one now that k8snetworkplumbingwg/sriov-network-operator#203 was merged?

Or we are going to have a problem with we want to use InfiniBand?

Copy link
Contributor

@bn222 bn222 Dec 14, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

@SchSeba It's not that we need a specific kernel. Rather, I think we just miss a fix/patch in the kernel. Once I can pinpoint what changes we need, we would be in a better place to decide.

Regarding k8snetworkplumbingwg/sriov-network-operator#203 which removes the selector, I'm just wondering if there are other issues elsewhere with having an empty link type.

I think that having an additional way to query the link type is still useful. That, or the question becomes what the real validity is of having the link type stored in the first place.

}

if err != nil && len(ifName) > 0 {
glog.Warningf("Devlink query for device %s named %s is not supported trying netlink", pciAddr, ifName)

la, err := utils.GetNetlinkProvider().GetLinkAttrs(ifName)
if err != nil {
return nil, err
Expand Down
23 changes: 23 additions & 0 deletions pkg/utils/mocks/NetlinkProvider.go
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

11 changes: 11 additions & 0 deletions pkg/utils/netlink_provider.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,8 @@ type NetlinkProvider interface {
GetLinkAttrs(ifName string) (*nl.LinkAttrs, error)
// GetDevLinkDeviceEswitchAttrs returns a devlink device's attributes
GetDevLinkDeviceEswitchAttrs(ifName string) (*nl.DevlinkDevEswitchAttr, error)
// GetDevLinkDevice returns a devlink device
GetDevLinkDevice(pfAddr string) (*nl.DevlinkDevice, error)
}

type defaultNetlinkProvider struct {
Expand All @@ -47,6 +49,15 @@ func (defaultNetlinkProvider) GetLinkAttrs(ifName string) (*nl.LinkAttrs, error)
return link.Attrs(), nil
}

// GetDevLinkDevice returns a devlink device
func (defaultNetlinkProvider) GetDevLinkDevice(pfAddr string) (*nl.DevlinkDevice, error) {
dev, err := nl.DevLinkGetDeviceByName("pci", pfAddr)
if err != nil {
return nil, fmt.Errorf("error getting devlink device for net device %s %v", pfAddr, err)
}
return dev, nil
}

// GetDevLinkDeviceEswitchAttrs returns a devlink device's attributes
func (defaultNetlinkProvider) GetDevLinkDeviceEswitchAttrs(pfAddr string) (*nl.DevlinkDevEswitchAttr, error) {
dev, err := nl.DevLinkGetDeviceByName("pci", pfAddr)
Expand Down
3 changes: 3 additions & 0 deletions pkg/utils/testing.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ func SetDefaultMockNetlinkProvider() {
mockProvider.
On("GetDevLinkDeviceEswitchAttrs", mock.AnythingOfType("string")).
Return(&nl.DevlinkDevEswitchAttr{Mode: "fakeMode"}, nil)
mockProvider.
On("GetDevLinkDevice", mock.AnythingOfType("string")).
Return(nil, fmt.Errorf("error getting devlink device for net device"))

SetNetlinkProviderInst(&mockProvider)
}