| Message ID | 1385624162-10116-2-git-send-email-jeffrey.t.kirsher@intel.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com> Date: Wed, 27 Nov 2013 23:35:55 -0800 > From: Carolyn Wyborny <carolyn.wyborny@intel.com> > > This patch adds a call to dev_close if the queue reinit fails in order > to make clearer to the user that the device is down. > > Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com> > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> This is a very bad approach to this problem. Users absolutely do not expect their entire interface to go down simply because an ethtool request cannot be satisfied. This is extremely poor quality of implementation. And in this specific case it absolutely is not necessary. The only thing that can fail is the queue allocation, so make a function which can preserve the previous configuration if the queue allocation fails. How about "igb_reinit_interrupt_scheme". Don't free the q vectors until the very last moment, when you know that the allocation of the new q vectors has succeeded. I'm not applying this patch, it needs to be reimplemented more sanely, using the above suggestions or similar. Thanks. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-11-29 at 15:47 -0500, David Miller wrote: > From: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > Date: Wed, 27 Nov 2013 23:35:55 -0800 > > > From: Carolyn Wyborny <carolyn.wyborny@intel.com> > > > > This patch adds a call to dev_close if the queue reinit fails in order > > to make clearer to the user that the device is down. > > > > Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com> > > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > This is a very bad approach to this problem. > > Users absolutely do not expect their entire interface to go down > simply because an ethtool request cannot be satisfied. This is > extremely poor quality of implementation. > > And in this specific case it absolutely is not necessary. > > The only thing that can fail is the queue allocation, so make a > function which can preserve the previous configuration if the queue > allocation fails. How about "igb_reinit_interrupt_scheme". > > Don't free the q vectors until the very last moment, when you know > that the allocation of the new q vectors has succeeded. > > I'm not applying this patch, it needs to be reimplemented more > sanely, using the above suggestions or similar. > > Thanks. Thanks for the feedback Dave, I will drop this patch from the series so that Carolyn can re-work the solution.
> -----Original Message----- > From: David Miller [mailto:davem@davemloft.net] > Sent: Friday, November 29, 2013 12:48 PM > To: Kirsher, Jeffrey T > Cc: Wyborny, Carolyn; netdev@vger.kernel.org; gospo@redhat.com; > sassmann@redhat.com > Subject: Re: [net 1/8] igb: Update queue reinit function to call dev_close when > init of queues fails > > From: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > Date: Wed, 27 Nov 2013 23:35:55 -0800 > > > From: Carolyn Wyborny <carolyn.wyborny@intel.com> > > > > This patch adds a call to dev_close if the queue reinit fails in order > > to make clearer to the user that the device is down. > > > > Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com> > > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > This is a very bad approach to this problem. > > Users absolutely do not expect their entire interface to go down simply because > an ethtool request cannot be satisfied. This is extremely poor quality of > implementation. > > And in this specific case it absolutely is not necessary. > > The only thing that can fail is the queue allocation, so make a function which can > preserve the previous configuration if the queue allocation fails. How about > "igb_reinit_interrupt_scheme". > > Don't free the q vectors until the very last moment, when you know that the > allocation of the new q vectors has succeeded. > > I'm not applying this patch, it needs to be reimplemented more sanely, using the > above suggestions or similar. > I did this per Ben's suggestion on Laura's original patch. You didn't argue with it at the time. I don't think I misunderstood his suggestion, but if so, I'll rework it. Carolyn Carolyn Wyborny Linux Development Networking Division Intel Corporation -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, 2013-12-02 at 16:55 +0000, Wyborny, Carolyn wrote: > > -----Original Message----- > > From: David Miller [mailto:davem@davemloft.net] > > Sent: Friday, November 29, 2013 12:48 PM > > To: Kirsher, Jeffrey T > > Cc: Wyborny, Carolyn; netdev@vger.kernel.org; gospo@redhat.com; > > sassmann@redhat.com > > Subject: Re: [net 1/8] igb: Update queue reinit function to call dev_close when > > init of queues fails > > > > From: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > Date: Wed, 27 Nov 2013 23:35:55 -0800 > > > > > From: Carolyn Wyborny <carolyn.wyborny@intel.com> > > > > > > This patch adds a call to dev_close if the queue reinit fails in order > > > to make clearer to the user that the device is down. > > > > > > Signed-off-by: Carolyn Wyborny <carolyn.wyborny@intel.com> > > > Tested-by: Aaron Brown <aaron.f.brown@intel.com> > > > Signed-off-by: Jeff Kirsher <jeffrey.t.kirsher@intel.com> > > > > This is a very bad approach to this problem. > > > > Users absolutely do not expect their entire interface to go down simply because > > an ethtool request cannot be satisfied. This is extremely poor quality of > > implementation. > > > > And in this specific case it absolutely is not necessary. > > > > The only thing that can fail is the queue allocation, so make a function which can > > preserve the previous configuration if the queue allocation fails. How about > > "igb_reinit_interrupt_scheme". > > > > Don't free the q vectors until the very last moment, when you know that the > > allocation of the new q vectors has succeeded. > > > > I'm not applying this patch, it needs to be reimplemented more sanely, using the > > above suggestions or similar. > > > > I did this per Ben's suggestion on Laura's original patch. You didn't > argue with it at the time. I don't think I misunderstood his > suggestion, but if so, I'll rework it. My concerns were that after a failure where the interface is effectively down, - the interface must be in a consistent state where is it safe to reconfigure the interface again or to unbind the driver, and - it should be obviously down so the user can then try to bring it up again. David is saying that you should implement the reconfiguration in such a way that you can always roll back to the previous working state in case of a failure (which would make my concerns moot). This is definitely a good goal but I'm not convinced that it's always possible. Ben.
> -----Original Message----- > From: Ben Hutchings [mailto:bhutchings@solarflare.com] > Sent: Monday, December 02, 2013 9:18 AM > To: Wyborny, Carolyn > Cc: David Miller; Kirsher, Jeffrey T; netdev@vger.kernel.org; > gospo@redhat.com; sassmann@redhat.com > Subject: Re: [net 1/8] igb: Update queue reinit function to call dev_close when > init of queues fails > [..] > My concerns were that after a failure where the interface is effectively down, > - the interface must be in a consistent state where is it safe to reconfigure the > interface again or to unbind the driver, and > - it should be obviously down so the user can then try to bring it up again. > > David is saying that you should implement the reconfiguration in such a way that > you can always roll back to the previous working state in case of a failure (which > would make my concerns moot). This is definitely a good goal but I'm not > convinced that it's always possible. > Thanks for the clarification Ben, I'll see what I can do to follow Dave's feedback. Carolyn
From: Ben Hutchings <bhutchings@solarflare.com> Date: Mon, 2 Dec 2013 17:18:16 +0000 > David is saying that you should implement the reconfiguration in such a > way that you can always roll back to the previous working state in case > of a failure (which would make my concerns moot). This is definitely a > good goal but I'm not convinced that it's always possible. In this case it is always possible. The only failure possible in these code paths is for a memory allocation failure. Therefore, without a doubt, trying to allocate the memory first before making any changes will provably allow perfect rollback to a working state. In fact, no state will be changed at all if the allocation fails. That's the whole point of my suggestion. Do the one thing that can fail, the memory allocation, before adjusting anything else. -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c b/drivers/net/ethernet/intel/igb/igb_main.c index 025e5f4..5a64aa6 100644 --- a/drivers/net/ethernet/intel/igb/igb_main.c +++ b/drivers/net/ethernet/intel/igb/igb_main.c @@ -7846,6 +7846,7 @@ int igb_reinit_queues(struct igb_adapter *adapter) if (igb_init_interrupt_scheme(adapter, true)) { dev_err(&pdev->dev, "Unable to allocate memory for queues\n"); + dev_close(netdev); return -ENOMEM; }