Page MenuHomeFreeBSD

Introducing sleepable locks into if_lagg
ClosedPublic

Authored by mav on Apr 26 2017, 5:32 PM.

Details

Summary

if_lagg is now using non-sleepable rmlocks to protect its internal state. In many cases it calls if_ioctl() interface method while still holding the lock. According to my conversation with glebius@ that is not correct, since if_ioctl() should be allowed to sleep. This patch introduces another sx lock to protect code paths that require sleeping, while still uses old rmlock to protect hot non-sleepable data paths.

This change allows to remove taskqueue decoupling used before to change interface addresses without holding the lock. Instead the new code uses sx lock to protect direct if_ioctl() calls.

As another bonus, the new code synchronizes enabled capabilities of member interfaces, and allows to control them with ifconfig laggX, that was impossible before. This part should fix inter-operation with if_bridge, that may need to disable some capabilities, such as TXCSUM or LRO, to allow bridging with non-capable interfaces.

Diff Detail

Repository
rS FreeBSD src repository
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mav created this revision.Apr 26 2017, 5:32 PM
mav edited the summary of this revision. (Show Details)Apr 26 2017, 5:34 PM
mav updated this revision to Diff 27761.Apr 26 2017, 5:55 PM
glebius edited edge metadata.Apr 26 2017, 9:32 PM

I haven't reviewed in detail (yet), but I absolutely agree with the description of the change. This is the right approach to fix multiple issues in this module.

This revision was automatically updated to reflect the committed changes.
araujo added a subscriber: araujo.May 3 2017, 1:18 AM
araujo added inline comments.May 3 2017, 2:35 AM
head/sys/net/if_lagg.c
1536

Looks like we don't need the lagg_softc struct here.

1539

All other function from where you call lagg_clrmulti does the ASSERT right before call the clrmulti function. And here we don't directly access the lagg_softc struct. So, it seems safe to remove it.

mav marked an inline comment as done.May 3 2017, 2:55 AM
mav added inline comments.
head/sys/net/if_lagg.c
1539

lp_mc_head access require locking, so I'd prefer to leave ASSERT there. The variable is removed though.