From mboxrd@z Thu Jan 1 00:00:00 1970 From: "Samudrala, Sridhar" Subject: Re: [PATCH net-next v12 1/5] net: Introduce generic failover module Date: Fri, 25 May 2018 16:04:33 -0700 Message-ID: References: <1527180917-39737-1-git-send-email-sridhar.samudrala@intel.com> <1527180917-39737-2-git-send-email-sridhar.samudrala@intel.com> <20180525153744.2b53c449@xeon-e3> Mime-Version: 1.0 Content-Type: multipart/mixed; boundary="===============0657665207004003949==" Cc: alexander.h.duyck@intel.com, virtio-dev@lists.oasis-open.org, jiri@resnulli.us, mst@redhat.com, kubakici@wp.pl, netdev@vger.kernel.org, virtualization@lists.linux-foundation.org, loseweigh@gmail.com, anjali.singhai@intel.com, aaron.f.brown@intel.com, davem@davemloft.net To: Stephen Hemminger Return-path: In-Reply-To: <20180525153744.2b53c449@xeon-e3> Content-Language: en-US List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: virtualization-bounces@lists.linux-foundation.org Errors-To: virtualization-bounces@lists.linux-foundation.org List-Id: netdev.vger.kernel.org This is a multi-part message in MIME format. --===============0657665207004003949== Content-Type: multipart/alternative; boundary="------------1B0AD3C788667269D61CF141" Content-Language: en-US This is a multi-part message in MIME format. --------------1B0AD3C788667269D61CF141 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit On 5/25/2018 3:37 PM, Stephen Hemminger wrote: > On Thu, 24 May 2018 09:55:13 -0700 > Sridhar Samudrala wrote: > > >> + spin_lock(&failover_lock); > Since register is not in fast path, this should be a mutex? This is Jiri's comment which made me to switch to spinlock from mutex >> Why mutex? Apparently you don't need to sleep while holding a lock. >> Simple spinlock would do. > > >> +int failover_slave_unregister(struct net_device *slave_dev) >> +{ >> + struct net_device *failover_dev; >> + struct failover_ops *fops; >> + >> + if (!netif_is_failover_slave(slave_dev)) >> + goto done; >> + >> + ASSERT_RTNL(); >> + >> + failover_dev = failover_get_bymac(slave_dev->perm_addr, &fops); >> + if (!failover_dev) >> + goto done; > Since the slave device must have a master device set already, why not use > that instead of searching by MAC address on unregister or link change. > We also need to get the fops(failover_ops) --------------1B0AD3C788667269D61CF141 Content-Type: text/html; charset=utf-8 Content-Transfer-Encoding: 7bit



On 5/25/2018 3:37 PM, Stephen Hemminger wrote:
On Thu, 24 May 2018 09:55:13 -0700
Sridhar Samudrala <sridhar.samudrala@intel.com> wrote:



+	spin_lock(&failover_lock);

Since register is not in fast path, this should be a mutex?
This is Jiri's comment which made me to switch to spinlock from mutex

  >> Why mutex? Apparently you don't need to sleep while holding a lock.
  >> Simple spinlock would do.





+int failover_slave_unregister(struct net_device *slave_dev)
+{
+	struct net_device *failover_dev;
+	struct failover_ops *fops;
+
+	if (!netif_is_failover_slave(slave_dev))
+		goto done;
+
+	ASSERT_RTNL();
+
+	failover_dev = failover_get_bymac(slave_dev->perm_addr, &fops);
+	if (!failover_dev)
+		goto done;

Since the slave device must have a master device set already, why not use
that instead of searching by MAC address on unregister or link change.


We also need to get the fops(failover_ops)
--------------1B0AD3C788667269D61CF141-- --===============0657665207004003949== Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline _______________________________________________ Virtualization mailing list Virtualization@lists.linux-foundation.org https://lists.linuxfoundation.org/mailman/listinfo/virtualization --===============0657665207004003949==--