* ixgbe: replace rtnl_lock with rcu_read_lock @ 2016-06-05 9:14 zyjzyj2000 2016-06-05 9:14 ` [PATCH 1/1] " zyjzyj2000 0 siblings, 1 reply; 8+ messages in thread From: zyjzyj2000 @ 2016-06-05 9:14 UTC (permalink / raw) To: e1000-devel, netdev Hi,all I am using the ixgbe nic. I found that the rtnl_lock is being used to enable upper device, such as macvlan. I am curious why rcu_read_lock is not used here. I replaced rtnl_lock with rcu_read_lock here. Then I made tests and it can work well. So can the rtnl_lock be replaced with rcu_read_lock? Anyone can explain it in details? Any reply is appreciated. Zhu Yanjun ^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH 1/1] ixgbe: replace rtnl_lock with rcu_read_lock 2016-06-05 9:14 ixgbe: replace rtnl_lock with rcu_read_lock zyjzyj2000 @ 2016-06-05 9:14 ` zyjzyj2000 2016-06-05 11:14 ` Eric Dumazet 2016-06-05 18:40 ` Alexander Duyck 0 siblings, 2 replies; 8+ messages in thread From: zyjzyj2000 @ 2016-06-05 9:14 UTC (permalink / raw) To: e1000-devel, netdev; +Cc: Zhu Yanjun From: Zhu Yanjun <zyjzyj2000@gmail.com> Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> --- drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c index 088c47c..cb19cbc 100644 --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c @@ -6840,7 +6840,7 @@ static void ixgbe_watchdog_link_is_up(struct ixgbe_adapter *adapter) netif_tx_wake_all_queues(adapter->netdev); /* enable any upper devices */ - rtnl_lock(); + rcu_read_lock(); netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter) { if (netif_is_macvlan(upper)) { struct macvlan_dev *vlan = netdev_priv(upper); @@ -6849,7 +6849,7 @@ static void ixgbe_watchdog_link_is_up(struct ixgbe_adapter *adapter) netif_tx_wake_all_queues(upper); } } - rtnl_unlock(); + rcu_read_unlock(); /* update the default user priority for VFs */ ixgbe_update_default_up(adapter); -- 1.7.9.5 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ixgbe: replace rtnl_lock with rcu_read_lock 2016-06-05 9:14 ` [PATCH 1/1] " zyjzyj2000 @ 2016-06-05 11:14 ` Eric Dumazet 2016-06-05 18:40 ` Alexander Duyck 1 sibling, 0 replies; 8+ messages in thread From: Eric Dumazet @ 2016-06-05 11:14 UTC (permalink / raw) To: zyjzyj2000; +Cc: e1000-devel, netdev On Sun, 2016-06-05 at 17:14 +0800, zyjzyj2000@gmail.com wrote: > From: Zhu Yanjun <zyjzyj2000@gmail.com> > > > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> > --- You do not explain if this is a bug fix (targeting net tree) or simply an optimization in the slow path (targeting net-next tree) ? A changelog, even small, would be nice, even if the patch looks valid. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ixgbe: replace rtnl_lock with rcu_read_lock 2016-06-05 9:14 ` [PATCH 1/1] " zyjzyj2000 2016-06-05 11:14 ` Eric Dumazet @ 2016-06-05 18:40 ` Alexander Duyck 2016-06-06 13:37 ` zhuyj 1 sibling, 1 reply; 8+ messages in thread From: Alexander Duyck @ 2016-06-05 18:40 UTC (permalink / raw) To: zhuyj; +Cc: e1000-devel, Netdev On Sun, Jun 5, 2016 at 2:14 AM, <zyjzyj2000@gmail.com> wrote: > From: Zhu Yanjun <zyjzyj2000@gmail.com> > > > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> > --- > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > index 088c47c..cb19cbc 100644 > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > @@ -6840,7 +6840,7 @@ static void ixgbe_watchdog_link_is_up(struct ixgbe_adapter *adapter) > netif_tx_wake_all_queues(adapter->netdev); > > /* enable any upper devices */ > - rtnl_lock(); > + rcu_read_lock(); > netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter) { > if (netif_is_macvlan(upper)) { > struct macvlan_dev *vlan = netdev_priv(upper); > @@ -6849,7 +6849,7 @@ static void ixgbe_watchdog_link_is_up(struct ixgbe_adapter *adapter) > netif_tx_wake_all_queues(upper); > } > } > - rtnl_unlock(); > + rcu_read_unlock(); > > /* update the default user priority for VFs */ > ixgbe_update_default_up(adapter); The rtnl_lock is being used to prevent any changes to the upper devices while the interface is going through and updating the Tx queue configuration on them. Without that lock you introduce possible bugs since you could have queues freed or added while this loop is in the middle of trying to update the state of it. As a general rule you use rcu_read_lock when you are only reading an RCU protected structure, you use rtnl_lock when you have to protect the system from any other changes while you are updating network configuration. In this case netif_tx_wake_all_queues changes the state of the upper device. The use of rtnl_lock here is intentional and it is best to just leave it as is unless you have some actual bug you are seeing. - Alex ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ixgbe: replace rtnl_lock with rcu_read_lock 2016-06-05 18:40 ` Alexander Duyck @ 2016-06-06 13:37 ` zhuyj 2016-06-06 16:12 ` Alexander Duyck 0 siblings, 1 reply; 8+ messages in thread From: zhuyj @ 2016-06-06 13:37 UTC (permalink / raw) To: Alexander Duyck; +Cc: e1000-devel, Netdev [-- Attachment #1.1: Type: text/plain, Size: 2551 bytes --] Hi, Alex Thanks for your reply. I checked all the nic driver in driver/net/ethernet/intel. And I found that only here the rtnl_lock is used. So I am curios why rtnl_lock is used when waking up the tupper device tx queue here. And the rtnl_lock is not used in other places. Best Regards! Zhu Yanjun On Mon, Jun 6, 2016 at 2:40 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote: > On Sun, Jun 5, 2016 at 2:14 AM, <zyjzyj2000@gmail.com> wrote: > > From: Zhu Yanjun <zyjzyj2000@gmail.com> > > > > > > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> > > --- > > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > index 088c47c..cb19cbc 100644 > > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > > @@ -6840,7 +6840,7 @@ static void ixgbe_watchdog_link_is_up(struct > ixgbe_adapter *adapter) > > netif_tx_wake_all_queues(adapter->netdev); > > > > /* enable any upper devices */ > > - rtnl_lock(); > > + rcu_read_lock(); > > netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter) { > > if (netif_is_macvlan(upper)) { > > struct macvlan_dev *vlan = netdev_priv(upper); > > @@ -6849,7 +6849,7 @@ static void ixgbe_watchdog_link_is_up(struct > ixgbe_adapter *adapter) > > netif_tx_wake_all_queues(upper); > > } > > } > > - rtnl_unlock(); > > + rcu_read_unlock(); > > > > /* update the default user priority for VFs */ > > ixgbe_update_default_up(adapter); > > The rtnl_lock is being used to prevent any changes to the upper > devices while the interface is going through and updating the Tx queue > configuration on them. Without that lock you introduce possible bugs > since you could have queues freed or added while this loop is in the > middle of trying to update the state of it. > > As a general rule you use rcu_read_lock when you are only reading an > RCU protected structure, you use rtnl_lock when you have to protect > the system from any other changes while you are updating network > configuration. In this case netif_tx_wake_all_queues changes the > state of the upper device. The use of rtnl_lock here is intentional > and it is best to just leave it as is unless you have some actual bug > you are seeing. > > - Alex > [-- Attachment #2: Type: text/plain, Size: 453 bytes --] ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e [-- Attachment #3: Type: text/plain, Size: 257 bytes --] _______________________________________________ E1000-devel mailing list E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ixgbe: replace rtnl_lock with rcu_read_lock 2016-06-06 13:37 ` zhuyj @ 2016-06-06 16:12 ` Alexander Duyck 2016-06-07 6:30 ` zhuyj 0 siblings, 1 reply; 8+ messages in thread From: Alexander Duyck @ 2016-06-06 16:12 UTC (permalink / raw) To: zhuyj; +Cc: e1000-devel, Netdev Just a quick scan has me wondering what code you are comparing it to? The key bit here that is the reason for taking the RTNL lock is because this section is handled in the watchdog which is not an RTNL protected region, and because it is messing with devices other than the ones controlled by the ixgbe driver itself. As far as I can tell I don't see similar code in the other drivers. You end up having to take the RTNL lock any time you start trying to manipulate some state of a device that is not protected through other means. For example pretty much all the net device operations expect that the RTNL lock has already been taken before calling them, however a driver can call into those functions if it is maintaining the state for the devices without the RTNL lock assuming it has some other means of keeping the state consistent such as a __IXGBE_DOWN state bit in the ixgbe driver. - Alex On Mon, Jun 6, 2016 at 6:37 AM, zhuyj <zyjzyj2000@gmail.com> wrote: > Hi, Alex > > Thanks for your reply. > > I checked all the nic driver in driver/net/ethernet/intel. And I found that > only here the rtnl_lock is used. > So I am curios why rtnl_lock is used when waking up the tupper device tx > queue here. And the rtnl_lock is not used in other places. > > Best Regards! > Zhu Yanjun > > On Mon, Jun 6, 2016 at 2:40 AM, Alexander Duyck <alexander.duyck@gmail.com> > wrote: >> >> On Sun, Jun 5, 2016 at 2:14 AM, <zyjzyj2000@gmail.com> wrote: >> > From: Zhu Yanjun <zyjzyj2000@gmail.com> >> > >> > >> > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> >> > --- >> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++-- >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> > >> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> > index 088c47c..cb19cbc 100644 >> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> > @@ -6840,7 +6840,7 @@ static void ixgbe_watchdog_link_is_up(struct >> > ixgbe_adapter *adapter) >> > netif_tx_wake_all_queues(adapter->netdev); >> > >> > /* enable any upper devices */ >> > - rtnl_lock(); >> > + rcu_read_lock(); >> > netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter) >> > { >> > if (netif_is_macvlan(upper)) { >> > struct macvlan_dev *vlan = netdev_priv(upper); >> > @@ -6849,7 +6849,7 @@ static void ixgbe_watchdog_link_is_up(struct >> > ixgbe_adapter *adapter) >> > netif_tx_wake_all_queues(upper); >> > } >> > } >> > - rtnl_unlock(); >> > + rcu_read_unlock(); >> > >> > /* update the default user priority for VFs */ >> > ixgbe_update_default_up(adapter); >> >> The rtnl_lock is being used to prevent any changes to the upper >> devices while the interface is going through and updating the Tx queue >> configuration on them. Without that lock you introduce possible bugs >> since you could have queues freed or added while this loop is in the >> middle of trying to update the state of it. >> >> As a general rule you use rcu_read_lock when you are only reading an >> RCU protected structure, you use rtnl_lock when you have to protect >> the system from any other changes while you are updating network >> configuration. In this case netif_tx_wake_all_queues changes the >> state of the upper device. The use of rtnl_lock here is intentional >> and it is best to just leave it as is unless you have some actual bug >> you are seeing. >> >> - Alex > > ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ E1000-devel mailing list E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ixgbe: replace rtnl_lock with rcu_read_lock 2016-06-06 16:12 ` Alexander Duyck @ 2016-06-07 6:30 ` zhuyj 2016-06-07 15:31 ` Alexander Duyck 0 siblings, 1 reply; 8+ messages in thread From: zhuyj @ 2016-06-07 6:30 UTC (permalink / raw) To: Alexander Duyck; +Cc: e1000-devel, Netdev [-- Attachment #1.1: Type: text/plain, Size: 5005 bytes --] Hi, Alex You are very nice to explain it in details. In the file ixgbe_main.c, the function is 6638 static void ixgbe_watchdog_link_is_up(struct ixgbe_adapter *adapter) ... 6705 netif_carrier_on(netdev); 6706 ixgbe_check_vf_rate_limit(adapter); 6707 6708 /* enable transmits */ *6709 netif_tx_wake_all_queues(adapter->netdev); <---This will change ixgbe nic state* 6710 6711 /* enable any upper devices */ 6712 rtnl_lock(); 6713 netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter) { 6714 if (netif_is_macvlan(upper)) { 6715 struct macvlan_dev *vlan = netdev_priv(upper); 6716 6717 if (vlan->fwd_priv) *6718 netif_tx_wake_all_queues(upper); <---This will change upper(macvlan) nic state* 6719 } 6720 } 6721 rtnl_unlock(); ... In the above, the ixgbe nic state is changed without rtnl_lock protection. But upper device state change needs the rtnl_lock protection. I am confused about this. Thanks for your reply. Zhu Yanjun On Tue, Jun 7, 2016 at 12:12 AM, Alexander Duyck <alexander.duyck@gmail.com> wrote: > Just a quick scan has me wondering what code you are comparing it to? > > The key bit here that is the reason for taking the RTNL lock is > because this section is handled in the watchdog which is not an RTNL > protected region, and because it is messing with devices other than > the ones controlled by the ixgbe driver itself. As far as I can tell > I don't see similar code in the other drivers. You end up having to > take the RTNL lock any time you start trying to manipulate some state > of a device that is not protected through other means. For example > pretty much all the net device operations expect that the RTNL lock > has already been taken before calling them, however a driver can call > into those functions if it is maintaining the state for the devices > without the RTNL lock assuming it has some other means of keeping the > state consistent such as a __IXGBE_DOWN state bit in the ixgbe driver. > > - Alex > > On Mon, Jun 6, 2016 at 6:37 AM, zhuyj <zyjzyj2000@gmail.com> wrote: > > Hi, Alex > > > > Thanks for your reply. > > > > I checked all the nic driver in driver/net/ethernet/intel. And I found > that > > only here the rtnl_lock is used. > > So I am curios why rtnl_lock is used when waking up the tupper device tx > > queue here. And the rtnl_lock is not used in other places. > > > > Best Regards! > > Zhu Yanjun > > > > On Mon, Jun 6, 2016 at 2:40 AM, Alexander Duyck < > alexander.duyck@gmail.com> > > wrote: > >> > >> On Sun, Jun 5, 2016 at 2:14 AM, <zyjzyj2000@gmail.com> wrote: > >> > From: Zhu Yanjun <zyjzyj2000@gmail.com> > >> > > >> > > >> > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> > >> > --- > >> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++-- > >> > 1 file changed, 2 insertions(+), 2 deletions(-) > >> > > >> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > >> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > >> > index 088c47c..cb19cbc 100644 > >> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > >> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c > >> > @@ -6840,7 +6840,7 @@ static void ixgbe_watchdog_link_is_up(struct > >> > ixgbe_adapter *adapter) > >> > netif_tx_wake_all_queues(adapter->netdev); > >> > > >> > /* enable any upper devices */ > >> > - rtnl_lock(); > >> > + rcu_read_lock(); > >> > netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, > iter) > >> > { > >> > if (netif_is_macvlan(upper)) { > >> > struct macvlan_dev *vlan = netdev_priv(upper); > >> > @@ -6849,7 +6849,7 @@ static void ixgbe_watchdog_link_is_up(struct > >> > ixgbe_adapter *adapter) > >> > netif_tx_wake_all_queues(upper); > >> > } > >> > } > >> > - rtnl_unlock(); > >> > + rcu_read_unlock(); > >> > > >> > /* update the default user priority for VFs */ > >> > ixgbe_update_default_up(adapter); > >> > >> The rtnl_lock is being used to prevent any changes to the upper > >> devices while the interface is going through and updating the Tx queue > >> configuration on them. Without that lock you introduce possible bugs > >> since you could have queues freed or added while this loop is in the > >> middle of trying to update the state of it. > >> > >> As a general rule you use rcu_read_lock when you are only reading an > >> RCU protected structure, you use rtnl_lock when you have to protect > >> the system from any other changes while you are updating network > >> configuration. In this case netif_tx_wake_all_queues changes the > >> state of the upper device. The use of rtnl_lock here is intentional > >> and it is best to just leave it as is unless you have some actual bug > >> you are seeing. > >> > >> - Alex > > > > > [-- Attachment #2: Type: text/plain, Size: 453 bytes --] ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e [-- Attachment #3: Type: text/plain, Size: 257 bytes --] _______________________________________________ E1000-devel mailing list E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH 1/1] ixgbe: replace rtnl_lock with rcu_read_lock 2016-06-07 6:30 ` zhuyj @ 2016-06-07 15:31 ` Alexander Duyck 0 siblings, 0 replies; 8+ messages in thread From: Alexander Duyck @ 2016-06-07 15:31 UTC (permalink / raw) To: zhuyj; +Cc: e1000-devel, Netdev I pretty much spelled this out already. The watchdog gets synced and disabled when ixgbe_down is called so you are guaranteed that the number of queues will not be changing while this function is running. What is occurring is that the ixgbe driver can update the netdevs controlled by the ixgbe driver without needing to use the RTNL lock because it can use its own synchronization primitives. It is only really when you start calling between drivers or you are calling into functions that expect the RTNL lock to be held that you need to take it. - Alex On Mon, Jun 6, 2016 at 11:30 PM, zhuyj <zyjzyj2000@gmail.com> wrote: > Hi, Alex > > You are very nice to explain it in details. > > In the file ixgbe_main.c, the function is > > 6638 static void ixgbe_watchdog_link_is_up(struct ixgbe_adapter *adapter) > > ... > 6705 netif_carrier_on(netdev); > 6706 ixgbe_check_vf_rate_limit(adapter); > 6707 > 6708 /* enable transmits */ > 6709 netif_tx_wake_all_queues(adapter->netdev); <---This will > change ixgbe nic state > 6710 > 6711 /* enable any upper devices */ > 6712 rtnl_lock(); > 6713 netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, iter) > { > 6714 if (netif_is_macvlan(upper)) { > 6715 struct macvlan_dev *vlan = netdev_priv(upper); > 6716 > 6717 if (vlan->fwd_priv) > 6718 netif_tx_wake_all_queues(upper); > <---This will change upper(macvlan) nic state > 6719 } > 6720 } > 6721 rtnl_unlock(); > > ... > > In the above, the ixgbe nic state is changed without rtnl_lock protection. > But upper device state change needs the rtnl_lock protection. > > I am confused about this. > > Thanks for your reply. > > Zhu Yanjun > > On Tue, Jun 7, 2016 at 12:12 AM, Alexander Duyck <alexander.duyck@gmail.com> > wrote: >> >> Just a quick scan has me wondering what code you are comparing it to? >> >> The key bit here that is the reason for taking the RTNL lock is >> because this section is handled in the watchdog which is not an RTNL >> protected region, and because it is messing with devices other than >> the ones controlled by the ixgbe driver itself. As far as I can tell >> I don't see similar code in the other drivers. You end up having to >> take the RTNL lock any time you start trying to manipulate some state >> of a device that is not protected through other means. For example >> pretty much all the net device operations expect that the RTNL lock >> has already been taken before calling them, however a driver can call >> into those functions if it is maintaining the state for the devices >> without the RTNL lock assuming it has some other means of keeping the >> state consistent such as a __IXGBE_DOWN state bit in the ixgbe driver. >> >> - Alex >> >> On Mon, Jun 6, 2016 at 6:37 AM, zhuyj <zyjzyj2000@gmail.com> wrote: >> > Hi, Alex >> > >> > Thanks for your reply. >> > >> > I checked all the nic driver in driver/net/ethernet/intel. And I found >> > that >> > only here the rtnl_lock is used. >> > So I am curios why rtnl_lock is used when waking up the tupper device tx >> > queue here. And the rtnl_lock is not used in other places. >> > >> > Best Regards! >> > Zhu Yanjun >> > >> > On Mon, Jun 6, 2016 at 2:40 AM, Alexander Duyck >> > <alexander.duyck@gmail.com> >> > wrote: >> >> >> >> On Sun, Jun 5, 2016 at 2:14 AM, <zyjzyj2000@gmail.com> wrote: >> >> > From: Zhu Yanjun <zyjzyj2000@gmail.com> >> >> > >> >> > >> >> > Signed-off-by: Zhu Yanjun <zyjzyj2000@gmail.com> >> >> > --- >> >> > drivers/net/ethernet/intel/ixgbe/ixgbe_main.c | 4 ++-- >> >> > 1 file changed, 2 insertions(+), 2 deletions(-) >> >> > >> >> > diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> >> > b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> >> > index 088c47c..cb19cbc 100644 >> >> > --- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> >> > +++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c >> >> > @@ -6840,7 +6840,7 @@ static void ixgbe_watchdog_link_is_up(struct >> >> > ixgbe_adapter *adapter) >> >> > netif_tx_wake_all_queues(adapter->netdev); >> >> > >> >> > /* enable any upper devices */ >> >> > - rtnl_lock(); >> >> > + rcu_read_lock(); >> >> > netdev_for_each_all_upper_dev_rcu(adapter->netdev, upper, >> >> > iter) >> >> > { >> >> > if (netif_is_macvlan(upper)) { >> >> > struct macvlan_dev *vlan = >> >> > netdev_priv(upper); >> >> > @@ -6849,7 +6849,7 @@ static void ixgbe_watchdog_link_is_up(struct >> >> > ixgbe_adapter *adapter) >> >> > netif_tx_wake_all_queues(upper); >> >> > } >> >> > } >> >> > - rtnl_unlock(); >> >> > + rcu_read_unlock(); >> >> > >> >> > /* update the default user priority for VFs */ >> >> > ixgbe_update_default_up(adapter); >> >> >> >> The rtnl_lock is being used to prevent any changes to the upper >> >> devices while the interface is going through and updating the Tx queue >> >> configuration on them. Without that lock you introduce possible bugs >> >> since you could have queues freed or added while this loop is in the >> >> middle of trying to update the state of it. >> >> >> >> As a general rule you use rcu_read_lock when you are only reading an >> >> RCU protected structure, you use rtnl_lock when you have to protect >> >> the system from any other changes while you are updating network >> >> configuration. In this case netif_tx_wake_all_queues changes the >> >> state of the upper device. The use of rtnl_lock here is intentional >> >> and it is best to just leave it as is unless you have some actual bug >> >> you are seeing. >> >> >> >> - Alex >> > >> > > > ------------------------------------------------------------------------------ What NetFlow Analyzer can do for you? Monitors network bandwidth and traffic patterns at an interface-level. Reveals which users, apps, and protocols are consuming the most bandwidth. Provides multi-vendor support for NetFlow, J-Flow, sFlow and other flows. Make informed decisions using capacity planning reports. https://ad.doubleclick.net/ddm/clk/305295220;132659582;e _______________________________________________ E1000-devel mailing list E1000-devel@lists.sourceforge.net https://lists.sourceforge.net/lists/listinfo/e1000-devel To learn more about Intel® Ethernet, visit http://communities.intel.com/community/wired ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2016-06-07 15:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-06-05 9:14 ixgbe: replace rtnl_lock with rcu_read_lock zyjzyj2000 2016-06-05 9:14 ` [PATCH 1/1] " zyjzyj2000 2016-06-05 11:14 ` Eric Dumazet 2016-06-05 18:40 ` Alexander Duyck 2016-06-06 13:37 ` zhuyj 2016-06-06 16:12 ` Alexander Duyck 2016-06-07 6:30 ` zhuyj 2016-06-07 15:31 ` Alexander Duyck
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.