cezanne / usbip-win

USB/IP for Windows
GNU General Public License v3.0
1.99k stars 350 forks source link

ubsip_vhci: locking in irp processing cleaned up #129

Closed bartlomiejcieszkowski closed 4 years ago

bartlomiejcieszkowski commented 4 years ago

This cleans up the locking and handling of irps in usbip_vhci

Also the irps for device ioctl were coming at dpc which caused a lot of pain in handling - changed to process these in a thread - see threaded_csq.[c|h] - a nice wrapper

Tested this with an xbox one controller - as per #122 Did the whole attach / detach scenario, everything works great

Also did some stress testing with following scenario:

  1. Attached 3 devices:

    • Microsoft Xbox One controller
    • Logitech usb mouse
    • Pendrive
  2. Pressed buttons, copied 2 gigabyte iso from pendrive

  3. Detached with usbip detach -p 0

Result: no BSOD

Also it is worth noting that it's safe to reduce SleepEx in usbip_forward.c to 100ms from 500ms

bartlomiejcieszkowski commented 4 years ago

in the meantime ill cleanup the USING_CSQ(...) define as this is some leftover

koudis commented 4 years ago

Thanks for the PR :)

Review

The technical side must be review by @cezanne.

Test

I will/can test branch with

I will do it thru this weekend.

SleepEx

The problem with SleepEx is solved in forwarder branch which deprecates #119

cezanne commented 4 years ago

@bartlomiejcieszkowski :

Also the irps for device ioctl were coming at dpc which caused a lot of pain in handling - changed to process these in a thread

Please, can you explain DPC pain ❓ In a usual case, thread will be more painful than DPC. In my opinion, if a DPC routine is called with an unnecessary lock or something, the DPC routine itself should be fixed.

bartlomiejcieszkowski commented 4 years ago

@cezanne nevermind that comment, i've rechecked the patch with different paths taken - and removed CSQ altogether, as it deemed unnecessary :)

I've explored some paths like 1) CSQ 2) CSQ with passive level lock for urbrs (GuardedMutex) - oh that was fun :D .. but was killed by DPC latency introduced by network adapter :( 3) No CSQ

Yeah and the no csq version works, so i'm dropping it as it is unnecessary, i've simplified the pull request.

koudis commented 4 years ago

@bartlomiejcieszkowski Is the branch prepared for testing?

bartlomiejcieszkowski commented 4 years ago

@koudis and now it is, sorry for delay, had a busy week :)

koudis commented 4 years ago

@bartlomiejcieszkowski I just test the branch. It works really nice! No lags, no BSOD. 3 devices connected at once and no problem.

I will continue tomorrow because i forgot my USB Hub at work - with 7 SSD USB 3.1 Disks (i've got BSOD with 50% probablity with old USBIP vhci driver)

cezanne commented 4 years ago

@bartlomiejcieszkowski : Please apply this patch for beautify.

bartlomiejcieszkowski commented 4 years ago

@cezanne applied, squash and merge?

koudis commented 4 years ago

@bartlomiejcieszkowski please wait a minute :). I will test new changes and then we can merge.

bartlomiejcieszkowski commented 4 years ago

@koudis sure :)

koudis commented 4 years ago

@cezanne @bartlomiejcieszkowski I test the branch after rebase. It seems to work without problems.

I test simultaneously 3x USB 3.1 SSD drives, 1x USB 2.0 flash disk and 2x Logitech unyfing receiver. I copy file from each device to computer (simultaneously) and during "copy" I detach usb devices by usbip.exe detach -p 0 - no problem occurred. I repeated this test five times.

cezanne commented 4 years ago

@bartlomiejcieszkowski :

@cezanne applied, squash and merge?

Please squash, rebase, fast-forward merge. 😃 Great thanks to @bartlomiejcieszkowski and @koudis .