| Message ID | 1339591464-10554-9-git-send-email-meravs@broadcom.com |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | David Miller |
| Headers | show |
On Wed, 2012-06-13 at 15:44 +0300, Merav Sicron wrote: > This patch removed the limitation in the code for 16 RSS queues. The default > (without other instruction from the user) number of queues is determined > according to the number of MSI-X vectors supported for that function, the number > of CPUs in the system, and a maximum of 8 queues. Thats a very confusing changelog You meant : " a minimum of 8 queues" ? You should give more explanations, because its a sensible area for performances. -- 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 Wed, 2012-06-13 at 18:14 +0300, Merav Sicron wrote: > Hi Eric, > > On Wed, 2012-06-13 at 11:52 +0200, Eric Dumazet wrote: > > On Wed, 2012-06-13 at 15:44 +0300, Merav Sicron wrote: > > > This patch removed the limitation in the code for 16 RSS queues. The default > > > (without other instruction from the user) number of queues is determined > > > according to the number of MSI-X vectors supported for that function, the number > > > of CPUs in the system, and a maximum of 8 queues. > > > > Thats a very confusing changelog > > > > You meant : " a minimum of 8 queues" ? > > > No, I meant maximum...for example in a system with 16 CPUs we will > allocate 8 RSS queues. We saw that in most scenarios we tested there was > no need for more than 8 queues to get the maximal throughput, and by > limiting to 8 by default we reduce the allocated memory. We do provide > ethtool -L support so that the user could request more RSS queues if > desired. > > > You should give more explanations, because its a sensible area for > > performances. > > Just to emphasis, since this is the patch series that enable the users to control the number of queues, we can reduce the default number and allow the user to increase it if he has a setup that needs more than 8 parallel CPUs to receive the traffic. When using a new FW on the board, the number can be increased up to 64, so using the maximal number can be an overkill (even if the machine has 64 CPUs, it does not mean that the user would like us to consume 64 MSI-X vectors and all the memory to set up 64 queues) - so a lower default value can be used to satisfy most users while allowing them to increase the number if they wish. Thanks, Eilon -- 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 Wed, 2012-06-13 at 16:53 +0300, Eilon Greenstein wrote: > On Wed, 2012-06-13 at 18:14 +0300, Merav Sicron wrote: > > Hi Eric, > > > > On Wed, 2012-06-13 at 11:52 +0200, Eric Dumazet wrote: > > > On Wed, 2012-06-13 at 15:44 +0300, Merav Sicron wrote: > > > > This patch removed the limitation in the code for 16 RSS queues. The default > > > > (without other instruction from the user) number of queues is determined > > > > according to the number of MSI-X vectors supported for that function, the number > > > > of CPUs in the system, and a maximum of 8 queues. > > > > > > Thats a very confusing changelog > > > > > > You meant : " a minimum of 8 queues" ? > > > > > No, I meant maximum...for example in a system with 16 CPUs we will > > allocate 8 RSS queues. We saw that in most scenarios we tested there was > > no need for more than 8 queues to get the maximal throughput, and by > > limiting to 8 by default we reduce the allocated memory. We do provide > > ethtool -L support so that the user could request more RSS queues if > > desired. > > > > > You should give more explanations, because its a sensible area for > > > performances. > > > > > Just to emphasis, since this is the patch series that enable the users > to control the number of queues, we can reduce the default number and > allow the user to increase it if he has a setup that needs more than 8 > parallel CPUs to receive the traffic. When using a new FW on the board, > the number can be increased up to 64, so using the maximal number can be > an overkill (even if the machine has 64 CPUs, it does not mean that the > user would like us to consume 64 MSI-X vectors and all the memory to set > up 64 queues) - so a lower default value can be used to satisfy most > users while allowing them to increase the number if they wish. I am all for a reduction of default number of queues. Even a laptop has now 8 cpus, so on servers, using one queue per cpu is overkill for most uses. 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
Hi Eric, On Wed, 2012-06-13 at 11:52 +0200, Eric Dumazet wrote: > On Wed, 2012-06-13 at 15:44 +0300, Merav Sicron wrote: > > This patch removed the limitation in the code for 16 RSS queues. The default > > (without other instruction from the user) number of queues is determined > > according to the number of MSI-X vectors supported for that function, the number > > of CPUs in the system, and a maximum of 8 queues. > > Thats a very confusing changelog > > You meant : " a minimum of 8 queues" ? > No, I meant maximum...for example in a system with 16 CPUs we will allocate 8 RSS queues. We saw that in most scenarios we tested there was no need for more than 8 queues to get the maximal throughput, and by limiting to 8 by default we reduce the allocated memory. We do provide ethtool -L support so that the user could request more RSS queues if desired. > You should give more explanations, because its a sensible area for > performances. > Thanks, Merav -- 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
From: "Eilon Greenstein" <eilong@broadcom.com> Date: Wed, 13 Jun 2012 16:53:29 +0300 > Just to emphasis, since this is the patch series that enable the users > to control the number of queues, we can reduce the default number and > allow the user to increase it if he has a setup that needs more than 8 > parallel CPUs to receive the traffic. When using a new FW on the board, > the number can be increased up to 64, so using the maximal number can be > an overkill (even if the machine has 64 CPUs, it does not mean that the > user would like us to consume 64 MSI-X vectors and all the memory to set > up 64 queues) - so a lower default value can be used to satisfy most > users while allowing them to increase the number if they wish. I think you should look at other drivers for guidance in this area. There is zero value in each and every driver author deciding what is a good default strategy, because this means the user gets a very inconsistent experience based purely upon the driver author's whims. -- 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 Wed, 2012-06-13 at 15:35 -0700, David Miller wrote: > From: "Eilon Greenstein" <eilong@broadcom.com> > Date: Wed, 13 Jun 2012 16:53:29 +0300 > > > Just to emphasis, since this is the patch series that enable the users > > to control the number of queues, we can reduce the default number and > > allow the user to increase it if he has a setup that needs more than 8 > > parallel CPUs to receive the traffic. When using a new FW on the board, > > the number can be increased up to 64, so using the maximal number can be > > an overkill (even if the machine has 64 CPUs, it does not mean that the > > user would like us to consume 64 MSI-X vectors and all the memory to set > > up 64 queues) - so a lower default value can be used to satisfy most > > users while allowing them to increase the number if they wish. > > I think you should look at other drivers for guidance in this area. > > There is zero value in each and every driver author deciding what > is a good default strategy, because this means the user gets a > very inconsistent experience based purely upon the driver author's > whims. > We looked at few other drivers - their current behavior is similar to what bnx2x had before this change: Minimum between the number of CPU and a defined maximum (probably the HW limit). bnx2x HW limit is 64 (in most other drivers it is smaller, but this can change). The number of CPUs in new systems becomes bigger and bigger, and allocaintg so many RSS queues seems like a waste. With the relatively new ethtool -L feature the user can change the number of queues. That's why we think (and so does Eric Dumazet) that it is better to have a smaller default number which is good for most cases. Do you agree with that? Thanks, Merav -- 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
From: "Merav Sicron" <meravs@broadcom.com> Date: Thu, 14 Jun 2012 18:34:06 +0300 > That's why we think (and so does Eric Dumazet) that it is better > to have a smaller default number which is good for most cases. > Do you agree with that? What I think is that the thing which is more important than the default we choose, is that it is consistently followed by all multiqueue drivers. By blazing your own unique path here, that is nearly guaranteed not to happen. I'd much rather have a bad default that every driver adheres to. -- 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 Thu, 2012-06-14 at 13:36 -0700, David Miller wrote: > From: "Merav Sicron" <meravs@broadcom.com> > Date: Thu, 14 Jun 2012 18:34:06 +0300 > > > That's why we think (and so does Eric Dumazet) that it is better > > to have a smaller default number which is good for most cases. > > Do you agree with that? > > What I think is that the thing which is more important than the > default we choose, is that it is consistently followed by all > multiqueue drivers. > > By blazing your own unique path here, that is nearly guaranteed not to > happen. > > I'd much rather have a bad default that every driver adheres to. > The increasing number of CPUs together with the increasing number of supported MSI-X vectors per device, is causing a lot of memory to be allocated for the networking channels. This is a problem on low memory platforms such as 32bits installations. 64 queues per device (and in most cases, there are few devices) is a becoming a real problem. If the device does not implement the ethtool .set_channels operation, then the default is practically the only way to go, and there is no such thing as a good default - it really depends on the use case. This is the reason why we are only setting the default in the series that adds support for this operation. So what do you say about setting a new default only for devices that supports .set_channels? There are only 2 of those right now (qlcnic, bnx2 and hopefully bnx2x soon). Is it acceptable to keep the default hard coded value or do you want it to be configurable? If the latter, is a kernel parameter a valid option? Please note that today, on a system with enough CPUs, you cannot really tell how many channels will be created per device - each vendor is allocating up to its HW limit - usually 8 or 16, but there is no easy way to determine it beside reading the code. So adding a kernel parameter to limit the number can actually increase the consistency. Thanks, Eilon -- 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
If you're not going to read what I wrote, don't pretend that you did. That really irritates me and makes me want to not review or apply your patches. I didn't say that there wasn't a problem. I said that I don't want you to just blaze your own trail without getting agreement and coordinating to adjust the other drivers in unison as well. I refuse to allow you to just change the default of your driver and then well... maybe other drivers follow and maybe others do not. That hodge-podge inconsistent situation sucks for users. So now are you going to try again to explain the problem to me or are you going to understand what my actual concern is? 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 Sun, 2012-06-17 at 01:08 -0700, David Miller wrote: > If you're not going to read what I wrote, don't pretend that you did. > That really irritates me and makes me want to not review or apply your > patches. > > I didn't say that there wasn't a problem. > > I said that I don't want you to just blaze your own trail without > getting agreement and coordinating to adjust the other drivers in > unison as well. I refuse to allow you to just change the default of > your driver and then well... maybe other drivers follow and maybe > others do not. That hodge-podge inconsistent situation sucks for > users. > > So now are you going to try again to explain the problem to me or > are you going to understand what my actual concern is? > I understood what you are saying, and if it looks like I was still pushing for this patch to be accepted then I apologize, it is not what I'm trying to do. What I am trying to do now, is to find a way to set a default for all drivers. Since set_channels is applicable to only two drivers now, it does not sound like a good path - so I was asking if you can consider a kernel parameter as acceptable path. To be clear, I'm asking if adding the default for all multi-queue drivers in the form of a kernel parameter is a good path to explore. -- 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
From: "Eilon Greenstein" <eilong@broadcom.com> Date: Sun, 17 Jun 2012 11:33:56 +0300 > drivers now, it does not sound like a good path - so I was asking if you > can consider a kernel parameter as acceptable path. To be clear, I'm > asking if adding the default for all multi-queue drivers in the form of > a kernel parameter is a good path to explore. If it's the default, there's no need for a kernel command line parameter. Those suck, just pick a good default. -- 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 Sun, 2012-06-17 at 02:00 -0700, David Miller wrote: > From: "Eilon Greenstein" <eilong@broadcom.com> > Date: Sun, 17 Jun 2012 11:33:56 +0300 > > > drivers now, it does not sound like a good path - so I was asking if you > > can consider a kernel parameter as acceptable path. To be clear, I'm > > asking if adding the default for all multi-queue drivers in the form of > > a kernel parameter is a good path to explore. > > If it's the default, there's no need for a kernel command line > parameter. Those suck, just pick a good default. > OK. So we will send an RFC series setting the default (hard coded define) to 8 for all multi-queue drivers and that will start a discussion if this is a good default number or not. The side-effect of this series will be that drivers that did not implement set_channels will not be able to change this default back to the HW limit like they run today (assuming the HW limit is higher than the chosen default). On top of that, we will send this series for the bnx2x again without any default -allowing up to 64 queues unless set_channels is used. Eilon -- 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/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h index 60a72f5..4dde45a 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h @@ -1643,7 +1643,7 @@ extern int num_queues; #define BNX2X_MAX_QUEUES(bp) BNX2X_MAX_RSS_COUNT(bp) /* #define is_eth_multi(bp) (BNX2X_NUM_ETH_QUEUES(bp) > 1) */ - +#define BNX2X_DEFAULT_NUM_QUEUES 8 #define RSS_IPV4_CAP_MASK \ TSTORM_ETH_FUNCTION_COMMON_CONFIG_RSS_IPV4_CAPABILITY diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h index 11afe5d..2865575 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h @@ -815,9 +815,11 @@ static inline void bnx2x_disable_msi(struct bnx2x *bp) static inline int bnx2x_calc_num_queues(struct bnx2x *bp) { + int num = min_t(int, num_online_cpus(), BNX2X_DEFAULT_NUM_QUEUES); + return num_queues ? min_t(int, num_queues, BNX2X_MAX_QUEUES(bp)) : - min_t(int, num_online_cpus(), BNX2X_MAX_QUEUES(bp)); + min_t(int, num, BNX2X_MAX_QUEUES(bp)); } static inline void bnx2x_clear_sge_mask_next_elems(struct bnx2x_fastpath *fp) diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index fbade5c..11bd0b6 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c @@ -11813,13 +11813,6 @@ static int __devinit bnx2x_init_one(struct pci_dev *pdev, max_non_def_sbs = bnx2x_get_num_non_def_sbs(pdev); - /* !!! FIXME !!! - * Do not allow the maximum SB count to grow above 16 - * since Special CIDs starts from 16*BNX2X_MULTI_TX_COS=48. - * We will use the FP_SB_MAX_E1x macro for this matter. - */ - max_non_def_sbs = min_t(int, FP_SB_MAX_E1x, max_non_def_sbs); - WARN_ON(!max_non_def_sbs); /* Maximum number of RSS queues: one IGU SB goes to CNIC */ @@ -11841,9 +11834,6 @@ static int __devinit bnx2x_init_one(struct pci_dev *pdev, bp = netdev_priv(dev); - BNX2X_DEV_INFO("Allocated netdev with %d tx and %d rx queues\n", - tx_count, rx_count); - bp->igu_sb_cnt = max_non_def_sbs; bp->msg_enable = debug; pci_set_drvdata(pdev, dev); @@ -11856,6 +11846,9 @@ static int __devinit bnx2x_init_one(struct pci_dev *pdev, BNX2X_DEV_INFO("max_non_def_sbs %d\n", max_non_def_sbs); + BNX2X_DEV_INFO("Allocated netdev with %d tx and %d rx queues\n", + tx_count, rx_count); + rc = bnx2x_init_bp(bp); if (rc) goto init_one_exit;