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

Fix detection of Mellanox VFs in switchdev mode #395

Merged

Conversation

almaslennikov
Copy link
Contributor

@almaslennikov almaslennikov commented Oct 29, 2021

This fixes #383 by calling the function implemented in https://github.com/Mellanox/sriovnet/blob/ecc40df73c7c2d53a10fd86368583de1937c40f6/sriovnet_switchdev.go#L60 that correctly determines the name of PF in switchdev mode for given VF by checking phys_port_id for each device in /sys/bus/devices/<vf_address>/physFn/net

The original issue is that current logic selects the first device in physFn/net which might not be the PF if the device is in switchdev mode. The proposed solution is to use a function from https://github.com/Mellanox/sriovnet library that iterates over the devices and determines the PF by validating its physical port

Signed-off-by: Alexander Maslennikov [email protected]

go.mod Outdated Show resolved Hide resolved
pkg/utils/utils.go Outdated Show resolved Hide resolved
@almaslennikov almaslennikov force-pushed the detect_switchdev_vfs branch 2 times, most recently from 2e44eed to 72f5883 Compare November 2, 2021 07:43
Copy link
Contributor

@adrianchiris adrianchiris left a comment

Choose a reason for hiding this comment

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

Hey !

Added some comments. also please add some more info in commit message explaining the problem and proposed solution

Thanks!

pkg/utils/sriovnet_ops.go Outdated Show resolved Hide resolved
pkg/utils/utils.go Outdated Show resolved Hide resolved
@almaslennikov almaslennikov force-pushed the detect_switchdev_vfs branch 2 times, most recently from 40db3b8 to a62e391 Compare November 2, 2021 11:13
pkg/utils/utils.go Outdated Show resolved Hide resolved
pkg/utils/utils.go Outdated Show resolved Hide resolved
pkg/utils/utils_test.go Outdated Show resolved Hide resolved
pkg/utils/netlink_provider.go Outdated Show resolved Hide resolved
pkg/utils/netlink_provider.go Outdated Show resolved Hide resolved
pkg/utils/netlink_provider.go Show resolved Hide resolved
pkg/utils/utils.go Outdated Show resolved Hide resolved
go.mod Outdated Show resolved Hide resolved
pkg/utils/utils.go Show resolved Hide resolved
@almaslennikov almaslennikov force-pushed the detect_switchdev_vfs branch 2 times, most recently from ee86c54 to 82cec98 Compare November 9, 2021 18:21
pkg/utils/netlink_provider.go Outdated Show resolved Hide resolved
pkg/utils/utils.go Outdated Show resolved Hide resolved
@almaslennikov almaslennikov force-pushed the detect_switchdev_vfs branch 3 times, most recently from 812b75a to fd4eb62 Compare November 10, 2021 14:04
Copy link
Contributor

@adrianchiris adrianchiris 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 addressing the comments ! LGTM

@Eoghan1232
Copy link
Collaborator

Eoghan1232 commented Nov 11, 2021

Thanks for working on this! LGTM

@zshi-redhat
Copy link
Collaborator

I tested with current version, it seems the pfName is still retrieved from sysfs, rather than devlink. the pfEswitchMode returned from devlink query is empty with cx-5 NIC. @almaslennikov Does it work for you?

pfEswitchMode, err := GetPfEswitchMode(pciAddr)
if err != nil {
// If device doesn't support eswitch mode query, fall back to the default implementation
if strings.Contains(strings.ToLower(fmt.Sprint(err)), "no such device") {
Copy link
Contributor

Choose a reason for hiding this comment

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

There is an additional use-case which slipped my mind.

if GetPfName is called with a PF (i.e non-sriov) address and it does not have sriov VFs then devlink will fail with

[root ~]# devlink dev eswitch show pci/0000:03:00.0
devlink answers: Operation not supported

in this case, we should also use sysfs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added a condition handling this case

@adrianchiris
Copy link
Contributor

I tested with current version, it seems the pfName is still retrieved from sysfs, rather than devlink. the pfEswitchMode returned from devlink query is empty with cx-5 NIC. @almaslennikov Does it work for you?

@zshi-redhat when you run devlink dev eswitch show <devlink-device> what are you getting ?
do you have sr-iov enabled ?

@adrianchiris adrianchiris self-requested a review November 15, 2021 07:20
pkg/utils/utils.go Outdated Show resolved Hide resolved
@zshi-redhat
Copy link
Collaborator

zshi-redhat commented Nov 16, 2021

I tested with current version, it seems the pfName is still retrieved from sysfs, rather than devlink. the pfEswitchMode returned from devlink query is empty with cx-5 NIC. @almaslennikov Does it work for you?

@zshi-redhat when you run devlink dev eswitch show <devlink-device> what are you getting ? do you have sr-iov enabled ?

I got different results when using devlink cmd and the device plugin program:

  1. Manually run the devlink cmd, got the following result:
# devlink dev eswitch show pci/0000:3b:00.1
devlink answers: Operation not supported
  1. With sriov device plugin, it returns empty value:

added the following debug message:

@@ -76,6 +79,7 @@ func GetPfName(pciAddr string) (string, error) {
                        return "", err
                }
        }
+       glog.Warningf("Eswitchdev mode for device %s is %s", pciAddr, pfEswitchMode)
        if pfEswitchMode == eswitchModeSwitchdev {
                name, err := GetSriovnetProvider().GetUplinkRepresentor(pciAddr)

got the empty output:

W1116 02:51:38.215605 2262387 utils.go:82] Eswitchdev mode for device 0000:3b:00.1 is

pkg/utils/utils.go Outdated Show resolved Hide resolved
@almaslennikov
Copy link
Contributor Author

@zshi-redhat this could be an issue in the netlink library. The proposed solution is to log a warning when we get an empty eswitch mode and then use the default implementation

@zshi-redhat
Copy link
Collaborator

@zshi-redhat this could be an issue in the netlink library. The proposed solution is to log a warning when we get an empty eswitch mode and then use the default implementation

I think you're right, the netlink ilbrary returns empty for the PF (w/o sriov configured), in which case it doesn't error out in the GetPfEswitchMode and continue to hit the empty string check and then the previous pfname logic.

@zshi-redhat zshi-redhat merged commit 889d422 into k8snetworkplumbingwg:master Nov 17, 2021
@zshi-redhat
Copy link
Collaborator

Merged per three approvals.

@zshi-redhat
Copy link
Collaborator

@almaslennikov Thanks for your persistence in fixing the comments!

@zshi-redhat
Copy link
Collaborator

hmm.. @almaslennikov I just tried the latest master, it seems not getting the eswitch mode correctly.

0000:af:0a.0 is the VF pci address:

W1117 12:23:01.315293 1155807 utils.go:79] Failed to retrieve eswitch mode with devlink query for device 0000:af:0a.0

0000:af:00.1 is the PF pci address

# devlink dev eswitch show pci/0000:af:00.1
pci/0000:af:00.1: mode switchdev inline-mode none encap-mode basic

Do you see the same issue in your environment?

@zshi-redhat
Copy link
Collaborator

My setup is simple that one CX-5 with two ports, one port is switchdev enabled and sriov configured.

ens4f1 is the PF with pci address 0000:af:00.1

# ethtool -i ens4f1
driver: mlx5e_rep
version: 4.18.0-322.el8.mr942_210708_154
firmware-version: 16.31.1014 (MT_0000000008)
expansion-rom-version: 
bus-info: 0000:af:00.1
supports-statistics: yes
supports-test: no
supports-eeprom-access: no
supports-register-dump: no
supports-priv-flags: no

devlink shows the right switchdev mode:

devlink dev eswitch show pci/0000:af:00.1
pci/0000:af:00.1: mode switchdev inline-mode none encap-mode basic

but I got this message (0000:af:0a.0 is the VF pci address of the above PF):

W1117 13:55:41.022025  171331 utils.go:79] Failed to retrieve eswitch mode with devlink query for device 0000:af:0a.0

@almaslennikov
Copy link
Contributor Author

@zshi-redhat the version of netlink used in this commit is broken. I've created another PR fixing it. I've described the netlink issue there. Please, take a look: #398

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fail to detect Mellanox VFs in switchdev mode using pfName nicSelector
5 participants