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

Add iommu groups and drivers info to resource pool #20

Merged
merged 4 commits into from
Aug 5, 2020

Conversation

lvfxx
Copy link
Member

@lvfxx lvfxx commented Jul 30, 2020

No description provided.

Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>

return nil
}

// PhysicalFunction contains information about physical function
type PhysicalFunction struct {

Choose a reason for hiding this comment

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

It looks like PhysicalFunction duplicates all VirtualFunction fields, so please consider using extension:

type PhysicalFunction struct {
    TargetPCIAddress         string
    Capability               string
    VirtualFunctionsCapacity int
    VirtualFunctions         map[*VirtualFunction]VirtualFunctionState

    VirtualFunction
}

Copy link
Member Author

@lvfxx lvfxx Jul 30, 2020

Choose a reason for hiding this comment

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

I wonder will it be a little confusing

Copy link
Member Author

Choose a reason for hiding this comment

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

Initialization example:

physfun := &PhysicalFunction{
			VirtualFunction: VirtualFunction{
				PCIAddress:       pfPciAddr,
				BoundDriver:      KernelDriver, // Kernel driver is bound by default
				KernelDriverName: pfKernelDriverName,
				IommuGroup:       pfIommuGroup,
				NetInterfaceName: pfIfaceNames[0],
			},
			TargetPCIAddress:         device.Target.PCIAddress,
			Capability:               device.Capability,
			VirtualFunctionsCapacity: vfCapacity,
			VirtualFunctions:         map[*VirtualFunction]VirtualFunctionState{},
		}

Copy link
Member Author

Choose a reason for hiding this comment

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

What about having something like that?

type NetworkFunction struct {
	PCIAddress       string
	BoundDriver      DriverType
	KernelDriverName string
	IommuGroup       int
	NetInterfaceName string
}

type VirtualFunction NetworkFunction

type PhysicalFunction struct {
        NetworkFunction
        other fields...
}

)

// NetResourcePool provides contains information about net devices
type NetResourcePool struct {
HostName string
PhysicalFunctions []*PhysicalFunction
IommuGroups map[int]DriverType
sriovProvider utils.SriovProvider

Choose a reason for hiding this comment

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

Do we need utils.SriovProvider as a part of NetResourcePool? I believe it should be only used in InitResourcePool and so should be passed in there as an argument.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree with you

HostName: config.HostName,
PhysicalFunctions: nil,
IommuGroups: map[int]DriverType{},
sriovProvider: utils.NewSriovProvider(pciDevicesPath, pciDriversPath, iommuGroupsPath),

Choose a reason for hiding this comment

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

I believe such things make testing difficult. Let it be passed to the InitResourcePool as an argument?

Signed-off-by: Sergey Semenov <sergey.semenov@xored.com>
@haiodo haiodo merged commit a29fcac into networkservicemesh:master Aug 5, 2020
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.

None yet

4 participants