ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

PATCH Superseded Souptick Joarder 2 participants 5 messages Dec 1, 2016 – Dec 3, 2016

Subsystems (2)

Network drivers Intel Ethernet

lore

Share & tools

lore

Jeff Kirsher

2016-12-02 19:44 UTC #1 lore raw

PATCH v2 1/1 ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

Quoted 4 lines · Souptick Joarder

In e100_alloc_cbs(), pci_pool_alloc() followed by memset will be replaced by pci_pool_zalloc()
Signed-off-by Souptick joarder <[email protected]>

Why the change? Is pci_pool_alloc() being deprecated? I ask because this is a very old driver and changes will cause a huge regression, where many of the parts/NICs may not be available to test with.

Quoted 27 lines

---
v2:
? - Alignment changes

?drivers/net/ethernet/intel/e100.c | 5 ++---
?1 file changed, 2 insertions(+), 3 deletions(-)

drivers/net/ethernet/intel/e100.c +2 -4

index 068789e..0cf338f 100644
@@ -1910,11 +1910,10 @@ static int e100_alloc_cbs(struct nic *nic)
 ?	nic->cb_to_use = nic->cb_to_send = nic->cb_to_clean = NULL;
 ?	nic->cbs_avail = 0;
 
-	nic->cbs = pci_pool_alloc(nic->cbs_pool, GFP_KERNEL,
-				??&nic->cbs_dma_addr);
+	nic->cbs = pci_pool_zalloc(nic->cbs_pool, GFP_KERNEL,
+				???&nic->cbs_dma_addr);
 ?	if (!nic->cbs)
 ?		return -ENOMEM;
-	memset(nic->cbs, 0, count * sizeof(struct cb));
 
 ?	for (cb = nic->cbs, i = 0; i < count; cb++, i++) {
 ?		cb->next = (i + 1 < count) ? cb + 1 : nic->cbs;
--

Quoted 2 lines

1.9.1

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20161202/6f139877/attachment-0001.asc>

Souptick Joarder

author

2016-12-03 07:28 UTC #2 lore raw

PATCH v2 1/1 ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

Quoted 9 lines · Jeff Kirsher

Quoted 4 lines · Souptick Joarder
In e100_alloc_cbs(), pci_pool_alloc() followed by memset will be replaced by pci_pool_zalloc()
Signed-off-by Souptick joarder <[email protected]>
Why the change? Is pci_pool_alloc() being deprecated? I ask because this is a very old driver and changes will cause a huge regression, where many of the parts/NICs may not be available to test with.

pci_pool_alloc is not deprecated. As pci_pool_zalloc can do the same as pci_pool_alloc/memset, hence this change.

Do we need to go through complete regression to validate this patch?

Quoted 30 lines

---
v2:
- Alignment changes

 drivers/net/ethernet/intel/e100.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

drivers/net/ethernet/intel/e100.c +2 -4

index 068789e..0cf338f 100644
@@ -1910,11 +1910,10 @@ static int e100_alloc_cbs(struct nic *nic)
      nic->cb_to_use = nic->cb_to_send = nic->cb_to_clean = NULL;
      nic->cbs_avail = 0;
 
-     nic->cbs = pci_pool_alloc(nic->cbs_pool, GFP_KERNEL,
-                               &nic->cbs_dma_addr);
+     nic->cbs = pci_pool_zalloc(nic->cbs_pool, GFP_KERNEL,
+                                &nic->cbs_dma_addr);
      if (!nic->cbs)
              return -ENOMEM;
-     memset(nic->cbs, 0, count * sizeof(struct cb));
 
      for (cb = nic->cbs, i = 0; i < count; cb++, i++) {
              cb->next = (i + 1 < count) ? cb + 1 : nic->cbs;
--

1.9.1

Jeff Kirsher

2016-12-03 07:59 UTC #3 lore raw

PATCH v2 1/1 ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

Quoted 14 lines · Souptick Joarder

Quoted 11 lines · Jeff Kirsher
Quoted 4 lines · Souptick Joarder
In e100_alloc_cbs(), pci_pool_alloc() followed by memset will be replaced by pci_pool_zalloc()
Signed-off-by Souptick joarder <[email protected]>
Why the change?? Is pci_pool_alloc() being deprecated?? I ask because this
is a very old driver and changes will cause a huge regression, where many
of the parts/NICs may not be available to test with.

Quoted 4 lines

pci_pool_alloc is not deprecated. As pci_pool_zalloc can do the same as pci_pool_alloc/memset, hence this change.
Do we need to go through complete regression to validate this patch?

Well, I do not think it is good idea to make changes and not test them, even the "simple" patches need to be tested and verified.

Since pci_pool_zalloc() does the same thing as pci_pool_alloc/memset, then what is the benefit of the change? If there is no benefit and just causes a bunch of regression testing, then I am not seeing the gain.

Currently e100 and e1000 drivers are in bug fix only mode (and have been so for some time now) so if you looking to kick the dust off these older drivers for no real good reason other than you see a possible optimization, which you are not wanting to regression test to verify there are no issues.
Then I am not going to take your requested change seriously.

Since we are not actively working on the driver, it makes even more sense
that we should thoroughly test any changes to ensure there are no issues. -------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20161202/c5f6c799/attachment.asc>

Souptick Joarder

author

2016-12-03 08:42 UTC #4 lore raw

PATCH v2 1/1 ethernet: intel: Replace pci_pool_alloc by pci_pool_zalloc

Quoted 27 lines · Jeff Kirsher

Quoted 14 lines · Souptick Joarder
Quoted 11 lines · Jeff Kirsher
Quoted 4 lines · Souptick Joarder
In e100_alloc_cbs(), pci_pool_alloc() followed by memset will be replaced by pci_pool_zalloc()
Signed-off-by Souptick joarder <[email protected]>
Why the change? Is pci_pool_alloc() being deprecated? I ask because this
is a very old driver and changes will cause a huge regression, where many
of the parts/NICs may not be available to test with.
Quoted 4 lines
pci_pool_alloc is not deprecated. As pci_pool_zalloc can do the same as pci_pool_alloc/memset, hence this change.
Do we need to go through complete regression to validate this patch?
Well, I do not think it is good idea to make changes and not test them, even the "simple" patches need to be tested and verified.
Since pci_pool_zalloc() does the same thing as pci_pool_alloc/memset, then what is the benefit of the change? If there is no benefit and just causes a bunch of regression testing, then I am not seeing the gain.

Quoted 5 lines

Currently e100 and e1000 drivers are in bug fix only mode (and have been so for some time now) so if you looking to kick the dust off these older drivers for no real good reason other than you see a possible optimization, which you are not wanting to regression test to verify there are no issues.
Then I am not going to take your requested change seriously.

Well , I mean, rather than running entire regression whether we can run minimum set of test cases to verify it :)

Quoted 3 lines

Since we are not actively working on the driver, it makes even more sense that we should thoroughly test any changes to ensure there are no issues.

Ok, I understand it's really painful to run entire regression test for this small
optimization change. I will drop this patch.