Message ID | 1461007188-1603-5-git-send-email-hannes@stressinduktion.org |
---|---|
State | Accepted, archived |
Delegated to: | David Miller |
Headers | show |
On Mon, 2016-04-18 at 21:19 +0200, Hannes Frederic Sowa wrote: > vxlan_get_rx_port requires rtnl_lock to be held. > > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> > Cc: Shannon Nelson <shannon.nelson@intel.com> > Cc: Carolyn Wyborny <carolyn.wyborny@intel.com> > Cc: Don Skidmore <donald.c.skidmore@intel.com> > Cc: Bruce Allan <bruce.w.allan@intel.com> > Cc: John Ronciak <john.ronciak@intel.com> > Cc: Mitch Williams <mitch.a.williams@intel.com> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index 2976df77bf14f5..b2f2cf40f06a87 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -7192,10 +7192,12 @@ static void ixgbe_service_task(struct work_struct *work) > return; > } > #ifdef CONFIG_IXGBE_VXLAN > + rtnl_lock(); > if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) { > adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED; > vxlan_get_rx_port(adapter->netdev); > } > + rtnl_unlock(); > #endif /* CONFIG_IXGBE_VXLAN */ > ixgbe_reset_subtask(adapter); > ixgbe_phy_interrupt_subtask(adapter); Although there might be a deadlock with a concurrent cancel_work_sync(&adapter->service_task); from ixgbe_remove()
On Fri, Jun 10, 2016 at 2:17 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote: > On Mon, 2016-04-18 at 21:19 +0200, Hannes Frederic Sowa wrote: >> vxlan_get_rx_port requires rtnl_lock to be held. >> >> Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> >> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> >> Cc: Shannon Nelson <shannon.nelson@intel.com> >> Cc: Carolyn Wyborny <carolyn.wyborny@intel.com> >> Cc: Don Skidmore <donald.c.skidmore@intel.com> >> Cc: Bruce Allan <bruce.w.allan@intel.com> >> Cc: John Ronciak <john.ronciak@intel.com> >> Cc: Mitch Williams <mitch.a.williams@intel.com> >> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> >> --- >> drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++ >> 1 file changed, 2 insertions(+) >> >> diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> index 2976df77bf14f5..b2f2cf40f06a87 100644 >> --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> @@ -7192,10 +7192,12 @@ static void ixgbe_service_task(struct work_struct *work) >> return; >> } >> #ifdef CONFIG_IXGBE_VXLAN >> + rtnl_lock(); >> if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) { >> adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED; >> vxlan_get_rx_port(adapter->netdev); >> } >> + rtnl_unlock(); >> #endif /* CONFIG_IXGBE_VXLAN */ >> ixgbe_reset_subtask(adapter); >> ixgbe_phy_interrupt_subtask(adapter); > > Although there might be a deadlock with a concurrent > cancel_work_sync(&adapter->service_task); from ixgbe_remove() I don't think we are holding the rtnl_lock when that is called. The ixgbe_remove function is a PCI function. The ixgbe_close routine is the one called with the rtnl_lock held. - Alex
On Mon, Apr 18, 2016 at 12:19 PM, Hannes Frederic Sowa <hannes@stressinduktion.org> wrote: > vxlan_get_rx_port requires rtnl_lock to be held. > > Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> > Cc: Shannon Nelson <shannon.nelson@intel.com> > Cc: Carolyn Wyborny <carolyn.wyborny@intel.com> > Cc: Don Skidmore <donald.c.skidmore@intel.com> > Cc: Bruce Allan <bruce.w.allan@intel.com> > Cc: John Ronciak <john.ronciak@intel.com> > Cc: Mitch Williams <mitch.a.williams@intel.com> > Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index 2976df77bf14f5..b2f2cf40f06a87 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -7192,10 +7192,12 @@ static void ixgbe_service_task(struct work_struct *work) > return; > } > #ifdef CONFIG_IXGBE_VXLAN > + rtnl_lock(); > if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) { > adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED; > vxlan_get_rx_port(adapter->netdev); > } > + rtnl_unlock(); > #endif /* CONFIG_IXGBE_VXLAN */ > ixgbe_reset_subtask(adapter); > ixgbe_phy_interrupt_subtask(adapter); > -- > 2.5.5 > Would it be possible to only take the lock inside of the conditional statement? There isn't really any need to be holding the rtnl_lock to check the adapter flag. - Alex
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 2976df77bf14f5..b2f2cf40f06a87 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -7192,10 +7192,12 @@ static void ixgbe_service_task(struct work_struct *work) return; } #ifdef CONFIG_IXGBE_VXLAN + rtnl_lock(); if (adapter->flags2 & IXGBE_FLAG2_VXLAN_REREG_NEEDED) { adapter->flags2 &= ~IXGBE_FLAG2_VXLAN_REREG_NEEDED; vxlan_get_rx_port(adapter->netdev); } + rtnl_unlock(); #endif /* CONFIG_IXGBE_VXLAN */ ixgbe_reset_subtask(adapter); ixgbe_phy_interrupt_subtask(adapter);
vxlan_get_rx_port requires rtnl_lock to be held. Cc: Jeff Kirsher <jeffrey.t.kirsher@intel.com> Cc: Jesse Brandeburg <jesse.brandeburg@intel.com> Cc: Shannon Nelson <shannon.nelson@intel.com> Cc: Carolyn Wyborny <carolyn.wyborny@intel.com> Cc: Don Skidmore <donald.c.skidmore@intel.com> Cc: Bruce Allan <bruce.w.allan@intel.com> Cc: John Ronciak <john.ronciak@intel.com> Cc: Mitch Williams <mitch.a.williams@intel.com> Signed-off-by: Hannes Frederic Sowa <hannes@stressinduktion.org> --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 2 ++ 1 file changed, 2 insertions(+)