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

SRIOV support for non-network device (FEC) #196

Closed
wants to merge 2 commits into from
Closed

SRIOV support for non-network device (FEC) #196

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
401661e
SRIOV feature is not just applicable for Network interfaces but any I…
damiankopyto Jan 30, 2020
61c4050
Renaming FEC to ACC (accelerator) to make name more generic.
damiankopyto Jan 31, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
92 changes: 68 additions & 24 deletions cmd/sriovdp/manager.go
View file
Open in desktop
Expand Up @@ -31,7 +31,9 @@ import (

const (
socketSuffix = "sock"
netClass = 0x02 // Device class - Network controller. ref: https://pci-ids.ucw.cz/read/PD/02 (for Sub-Classes)
netClass = 0x02 // Device class - Network controller. ref: https://pci-ids.ucw.cz/read/PD/02 (for Sub-Classes)
accClass = 0x12 // Device class - Processing accelerator
rsuDevice = 0x0b30 // RSU device - only used for programming the FPGA
Copy link
Contributor

@cbf123 cbf123 Feb 4, 2020 •
edited

Choose a reason for hiding this comment

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

What does "RSU" mean in this context, and do we need to document that somewhere? (Also at line 334.)

)

/*
Expand All @@ -46,6 +48,7 @@ Network controller subclasses. ref: https://pci-ids.ucw.cz/read/PD/02
07 Infiniband controller
08 Fabric controller
80 Network controller
12 Processing Accelerator
Copy link
Contributor

@cbf123 cbf123 Feb 4, 2020 •
edited

Choose a reason for hiding this comment

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

I think this is wrong. "12" is correct for the PCI device class for processing accelerators as specified at https://pci-ids.ucw.cz/read/PD/ and as I read it, the processing accelerator subclass should be 00 for this device.

However, "12" does not appear to be a valid option for a network controller subclass as described at the URL listed on line 40.

*/

type cliParams struct {
Expand Down Expand Up @@ -194,8 +197,8 @@ func (rm *resourceManager) discoverHostDevices() error {
continue
}

// only interested in network class
if devClass == netClass {
// only interested in network class and accelerator class
if devClass == netClass || devClass == accClass {
vendor := device.Vendor
vendorName := vendor.Name
if len(vendor.Name) > 20 {
Expand All @@ -208,28 +211,14 @@ func (rm *resourceManager) discoverHostDevices() error {
}
glog.Infof("discoverDevices(): device found: %-12s\t%-12s\t%-20s\t%-40s", device.Address, device.Class.ID, vendorName, productName)

// exclude device in-use in host
if isDefaultRoute, _ := hasDefaultRoute(device.Address); !isDefaultRoute {

aPF := utils.IsSriovPF(device.Address)
aVF := utils.IsSriovVF(device.Address)

if aPF || !aVF {
// add to linkWatchList
rm.addToLinkWatchList(device.Address)
}

if aPF && utils.SriovConfigured(device.Address) {
// do not add this device in net device list
continue
}

if newDevice, err := resources.NewPciNetDevice(device, rm.rFactory); err == nil {
rm.netDeviceList = append(rm.netDeviceList, newDevice)
} else {
glog.Errorf("discoverDevices() error adding new device: %q", err)
}
// Network devices
if devClass == netClass {
Copy link
Contributor

Choose a reason for hiding this comment

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

consider refactoring the two branches into their own handler functions for increased readability

rm.netDevHandler(device)
}

// Processing accelerators
if devClass == accClass {
Copy link
Contributor

@cbf123 cbf123 Feb 4, 2020 •
edited

Choose a reason for hiding this comment

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

It seems a little strange to have two totally separate if statements where only one of them can be true...normally I'd expect to see an "else if" here or even a switch statement.

rm.accDevHandler(device)
}
}
}
Expand Down Expand Up @@ -290,3 +279,58 @@ type linkWatcher struct {
func (lw *linkWatcher) Subscribe() {

}

func (rm *resourceManager) netDevHandler(dev *ghw.PCIDevice) {
// exclude device in-use in host
if isDefaultRoute, _ := hasDefaultRoute(dev.Address); !isDefaultRoute {

aPF := utils.IsSriovPF(dev.Address)
aVF := utils.IsSriovVF(dev.Address)

if aPF || !aVF {
// add to linkWatchList
rm.addToLinkWatchList(dev.Address)
}

if aPF && utils.SriovConfigured(dev.Address) {
// do not add this device in net device list
return
}

if newDevice, err := resources.NewPciNetDevice(dev, rm.rFactory); err == nil {
rm.netDeviceList = append(rm.netDeviceList, newDevice)
} else {
glog.Errorf("discoverDevices() error adding new device: %q", err)
}

}
}

func (rm *resourceManager) accDevHandler(dev *ghw.PCIDevice) {
//Check if device is a physical function (PF)
aPF := utils.IsSriovPF(dev.Address)
devID, err := strconv.ParseInt(dev.Product.ID, 16, 64)
if err != nil {
glog.Warningf("discoverDevices(): unable to parse device ID for device %+v %q", dev, err)
return
}
//Check if device ID = 0b30, it means it is a RSU (Remote System Update) device and not accelerator
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems messy to need to be worrying about vendor-specific PCI device IDs in the middle of this code. Is there nothing else we can look at to determine the class of a device, or to make this more generic so that adding other accelerators would be simpler?

if devID != rsuDevice {
if aPF && utils.SriovConfigured(dev.Address) {
// If it is a PF and SRIOV is configured do not add to device list
return
}

if newDevice, err := resources.NewPciNetDevice(dev, rm.rFactory); err == nil {
if newDevice.GetDriver() == "igb_uio" || newDevice.GetDriver() == "vfio-pci" {
Copy link
Contributor

@cbf123 cbf123 Feb 4, 2020 •
edited

Choose a reason for hiding this comment

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

This looks like more device-specific logic. How hard will it be to support other accelerators in the future?

rm.netDeviceList = append(rm.netDeviceList, newDevice)
} else {
glog.Infof("Excluding FEC device %-12s because it is not bound to userspace driver.", dev.Address)
Copy link
Contributor

Choose a reason for hiding this comment

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

I thought we were dropping the "FEC" terminology?

}
} else {
glog.Errorf("discoverDevices() error adding new device: %q", err)
}
} else {
Copy link

Choose a reason for hiding this comment

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

It might be better to check if devID == rsuDevice { log and return } at line 318. I'm thinking of the (future) case of any other accelerator classed PCI devices that would not be appropriate to discover here. It might be useful to have a list of 'blacklisted' device Ids per class that can be looped through rather than individual checks.

glog.Infof("Excluding device %-12s , it is a RSU device.", dev.Address)
}
}