| Message ID | 20230728075840.4022760-2-shaojijie@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | There are some bugfix for the HNS3 ethernet driver | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net |
| netdev/fixes_present | success | Fixes tag present in non-next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 9 this patch: 9 |
| netdev/cc_maintainers | success | CCed 7 of 7 maintainers |
| netdev/build_clang | success | Errors and warnings before: 1351 this patch: 1351 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 1351 this patch: 1351 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 15 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
From: Jijie Shao > Sent: 28 July 2023 08:59 > > num_online_cpus() may call more than once when passing to min_t(), > between calls, it may return different values, so move num_online_cpus() > out of min_t(). Nope, wrong bug: min() (and friends) are careful to only evaluate their arguments once. The bug is using min_t() - especially with a small type. If/when the number of cpu hits 65536 the (u16) cast will convert it to zero. Looking at the code a lot of the local variables should be 'unsigned int' not 'u16. Just because the domain of a value is small doesn't mean you should use a small type (unless you are saving space in a structure). David > > Signed-off-by: Yonglong Liu <liuyonglong@huawei.com> > Signed-off-by: Jijie Shao <shaojijie@huawei.com> > --- > drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 ++- > 1 file changed, 2 insertions(+), 1 deletion(-) > > diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > index 9f6890059666..823e6d2e85f5 100644 > --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c > @@ -4757,6 +4757,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv) > { > struct hnae3_handle *h = priv->ae_handle; > struct hns3_enet_tqp_vector *tqp_vector; > + u32 online_cpus = num_online_cpus(); > struct hnae3_vector_info *vector; > struct pci_dev *pdev = h->pdev; > u16 tqp_num = h->kinfo.num_tqps; > @@ -4766,7 +4767,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv) > > /* RSS size, cpu online and vector_num should be the same */ > /* Should consider 2p/4p later */ > - vector_num = min_t(u16, num_online_cpus(), tqp_num); > + vector_num = min_t(u16, online_cpus, tqp_num); > > vector = devm_kcalloc(&pdev->dev, vector_num, sizeof(*vector), > GFP_KERNEL); > -- > 2.30.0 - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi David： Yes, you're right, min_t() evaluates the arguments only once. In the actual scenario, the number of cpu is far less than 65535. Therefore, the minimum value will not convert to zero. Thanks for your advice, this patch will be withdrawn. Jijie Shao on 2023/7/28 16:29, David Laight wrote: > From: Jijie Shao >> Sent: 28 July 2023 08:59 >> >> num_online_cpus() may call more than once when passing to min_t(), >> between calls, it may return different values, so move num_online_cpus() >> out of min_t(). > Nope, wrong bug: > min() (and friends) are careful to only evaluate their arguments once. > The bug is using min_t() - especially with a small type. > > If/when the number of cpu hits 65536 the (u16) cast will convert > it to zero. > > Looking at the code a lot of the local variables should be > 'unsigned int' not 'u16. > Just because the domain of a value is small doesn't mean > you should use a small type (unless you are saving space in > a structure). > > David > >> Signed-off-by: Yonglong Liu <liuyonglong@huawei.com> >> Signed-off-by: Jijie Shao <shaojijie@huawei.com> >> --- >> drivers/net/ethernet/hisilicon/hns3/hns3_enet.c | 3 ++- >> 1 file changed, 2 insertions(+), 1 deletion(-) >> >> diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >> b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >> index 9f6890059666..823e6d2e85f5 100644 >> --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >> +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c >> @@ -4757,6 +4757,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv) >> { >> struct hnae3_handle *h = priv->ae_handle; >> struct hns3_enet_tqp_vector *tqp_vector; >> + u32 online_cpus = num_online_cpus(); >> struct hnae3_vector_info *vector; >> struct pci_dev *pdev = h->pdev; >> u16 tqp_num = h->kinfo.num_tqps; >> @@ -4766,7 +4767,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv) >> >> /* RSS size, cpu online and vector_num should be the same */ >> /* Should consider 2p/4p later */ >> - vector_num = min_t(u16, num_online_cpus(), tqp_num); >> + vector_num = min_t(u16, online_cpus, tqp_num); >> >> vector = devm_kcalloc(&pdev->dev, vector_num, sizeof(*vector), >> GFP_KERNEL); >> -- >> 2.30.0 > - > Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK > Registration No: 1397386 (Wales) > >
diff --git a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c index 9f6890059666..823e6d2e85f5 100644 --- a/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c +++ b/drivers/net/ethernet/hisilicon/hns3/hns3_enet.c @@ -4757,6 +4757,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv) { struct hnae3_handle *h = priv->ae_handle; struct hns3_enet_tqp_vector *tqp_vector; + u32 online_cpus = num_online_cpus(); struct hnae3_vector_info *vector; struct pci_dev *pdev = h->pdev; u16 tqp_num = h->kinfo.num_tqps; @@ -4766,7 +4767,7 @@ static int hns3_nic_alloc_vector_data(struct hns3_nic_priv *priv) /* RSS size, cpu online and vector_num should be the same */ /* Should consider 2p/4p later */ - vector_num = min_t(u16, num_online_cpus(), tqp_num); + vector_num = min_t(u16, online_cpus, tqp_num); vector = devm_kcalloc(&pdev->dev, vector_num, sizeof(*vector), GFP_KERNEL);