Re: [PATCH net-next 4/7] ixgbe: protect vxlan_get_rx_port in ixgbe_service_task with rtnl_lock
On Mon, Apr 18, 2016 at 12:19 PM, Hannes Frederic Sowa wrote: > vxlan_get_rx_port requires rtnl_lock to be held. > > Cc: Jeff Kirsher > Cc: Jesse Brandeburg > Cc: Shannon Nelson > Cc: Carolyn Wyborny > Cc: Don Skidmore > Cc: Bruce Allan > Cc: John Ronciak > Cc: Mitch Williams > Signed-off-by: Hannes Frederic Sowa > --- > 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
Re: [PATCH net-next 4/7] ixgbe: protect vxlan_get_rx_port in ixgbe_service_task with rtnl_lock
On Fri, Jun 10, 2016 at 2:17 PM, Eric Dumazet 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 >> Cc: Jesse Brandeburg >> Cc: Shannon Nelson >> Cc: Carolyn Wyborny >> Cc: Don Skidmore >> Cc: Bruce Allan >> Cc: John Ronciak >> Cc: Mitch Williams >> Signed-off-by: Hannes Frederic Sowa >> --- >> 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
Re: [PATCH net-next 4/7] ixgbe: protect vxlan_get_rx_port in ixgbe_service_task with rtnl_lock
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 > Cc: Jesse Brandeburg > Cc: Shannon Nelson > Cc: Carolyn Wyborny > Cc: Don Skidmore > Cc: Bruce Allan > Cc: John Ronciak > Cc: Mitch Williams > Signed-off-by: Hannes Frederic Sowa > --- > 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()
[PATCH net-next 4/7] ixgbe: protect vxlan_get_rx_port in ixgbe_service_task with rtnl_lock
vxlan_get_rx_port requires rtnl_lock to be held. Cc: Jeff Kirsher Cc: Jesse Brandeburg Cc: Shannon Nelson Cc: Carolyn Wyborny Cc: Don Skidmore Cc: Bruce Allan Cc: John Ronciak Cc: Mitch Williams Signed-off-by: Hannes Frederic Sowa --- 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