Skip to content

Commit

Permalink
[Intel82599] Unbind PCI device before using it.
Browse files Browse the repository at this point in the history
  • Loading branch information
@eugeneia
eugeneia committed Feb 17, 2015
1 parent 74aaea6 commit c1c0e08
Showing 1 changed file with 4 additions and 0 deletions.
4 changes: 4 additions & 0 deletions src/apps/intel/intel_app.lua
View file
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@ module(...,package.seeall)
local zone = require("jit.zone")
local basic_apps = require("apps.basic.basic_apps")
local lib = require("core.lib")
local pci = require("lib.hardware.pci")
local register = require("lib.hardware.register")
local intel10g = require("apps.intel.intel10g")
local freelist = require("core.freelist")
Expand All @@ -27,6 +28,9 @@ end
function Intel82599:new (arg)
local conf = config.parse_app_arg(arg)

-- Unbind PCI device form linux.
pci.unbind_device_from_linux(conf.pciaddr)

if conf.vmdq then
if devices[conf.pciaddr] == nil then
devices[conf.pciaddr] = {pf=intel10g.new_pf(conf.pciaddr):open(), vflist={}}
Expand Down

9 comments on commit c1c0e08

@lukego
Copy link

@lukego lukego commented on c1c0e08 Feb 18, 2015

Choose a reason for hiding this comment

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

This change looks correct to me.

The same change is also needed anywhere else a PCI device is opened though, for example in the loadgen app.

I wonder what the best place to do this really is? Options:

  1. In both intel_app and loadgen and other apps that use PCI drivers.
  2. In intel10g and any other drivers.
  3. In pci module e.g. before any/all calls that mess with the PCI device.

what do you think?

@eugeneia
Copy link
Owner Author

Choose a reason for hiding this comment

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

The same change is also needed anywhere else a PCI device is opened though, for example in the loadgen app.

At a second glance the code should be a bit different. In the pci module there is a open_usable_devices routine which is AFAIK unused and broken (it uses open_device of which I can not find the definition, might be a stub).

I want to:

  • Fully document lib.hardware.pci, that means finding out which functions are used/working and what for.
  • I think I prefer to reduce the pci lib instead of extending it further. If calling unbind... before using it is enough of general PCI initialization then I think it's reasonable to have a minimal API with functions X,Y,Z that requires you to call unbind... first.
  • Then the right place for unbinding the device would be M_sf:open(edit: and M_pf:open) in intel10g.lua which would cover both LoadGen and our userspace driver.

@eugeneia
Copy link
Owner Author

Choose a reason for hiding this comment

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

@lukego I noticed that we do enable PCI bus mastering (pci.set_bus_master(self.pciaddress, true)) but we never disable it (e.g. pci.set_bus_master(self.pciaddress, false)) after closing/unmapping DMA memory. Is that intended?

@lukego
Copy link

@lukego lukego commented on c1c0e08 Feb 25, 2015

Choose a reason for hiding this comment

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

Good point! Shutting down bus mastering would be a good cleanup step. In fact, ideally we would do this even on abrupt shutdown (e.g. ^C or SEGV). Like the trap in bash scripts. ljsyscall and glibc both have mechanisms for this.

I'd quite like to see the unused PCI scanning code deleted :).

@eugeneia
Copy link
Owner Author

Choose a reason for hiding this comment

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

Hmm I see a conflict there. PCI bus mastering should really be disabled by M_pf:close() and M_sf:close() and the handler for signals might not know which PCI devices do have bus mastering enabled. Add it to :close() for now and see what happens?

@lukego
Copy link

@lukego lukego commented on c1c0e08 Feb 26, 2015

Choose a reason for hiding this comment

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

Sure, start simple.

@eugeneia
Copy link
Owner Author

@eugeneia eugeneia commented on c1c0e08 Feb 27, 2015 via email

Choose a reason for hiding this comment

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

@lukego
Copy link

@lukego lukego commented on c1c0e08 Mar 1, 2015

Choose a reason for hiding this comment

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

Sounds reasonable!

Could also replace the C code with Lua code using ljsyscall?

@eugeneia
Copy link
Owner Author

Choose a reason for hiding this comment

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

Shutting down bus mastering would be a good cleanup step. In fact, ideally we would do this even on abrupt shutdown (e.g. ^C or SEGV). Like the trap in bash scripts. ljsyscall and glibc both have mechanisms for this.

So... I explored some options regarding clean shutdown on receiving various signals using ljsyscall (see this thread). It seems like the safest way would be to use sigprocmask and signalfd to queue up fatal signals in a queue. The question is when to read/handle them? My first thought was to do it in breathe but then again Snabb won't always crash during main meaning this could lead to becoming a zombie process.

Please sign in to comment.