Re: [PATCH net-next 2/3] net: use skb_for_each_frag() helper where possible

2021-04-09 Thread Matteo Croce 
On Fri, Apr 9, 2021 at 11:28 PM Jakub Kicinski  wrote:
>
> On Fri, 9 Apr 2021 22:44:50 +0200 Matteo Croce wrote:
> > > What pops to mind (although quite nit picky) is the question if the
> > > assembly changes much between driver which used to cache nr_frags and
> > > now always going skb_shinfo(skb)->nr_frags? It's a relatively common
> > > pattern.
> >
> > Since skb_shinfo() is a macro and skb_end_pointer() a static inline,
> > it should be the same, but I was curious to check so, this is a diff
> > between the following snippet before and afer the macro:
> >
> > int frags = skb_shinfo(skb)->nr_frags;
> > int i;
> > for (i = 0; i < frags; i++)
> > kfree(skb->frags[i]);
> >
> >  1 file changed, 8 insertions(+), 7 deletions(-)
> >
> > --- ins1.s 2021-04-09 22:35:59.384523865 +0200
> > +++ ins2.s 2021-04-09 22:36:08.132594737 +0200
> > @@ -1,26 +1,27 @@
> >  iter:
> >  movsx   rax, DWORD PTR [rdi+16]
> >  mov rdx, QWORD PTR [rdi+8]
> >  mov eax, DWORD PTR [rdx+rax]
> >  testeax, eax
> >  jle .L6
> >  pushrbp
> > -sub eax, 1
> > +mov rbp, rdi
> >  pushrbx
> > -lea rbp, [rdi+32+rax*8]
> > -lea rbx, [rdi+24]
> > +xor ebx, ebx
> >  sub rsp, 8
> >  .L3:
> > -mov rdi, QWORD PTR [rbx]
> > -add rbx, 8
> > +mov rdi, QWORD PTR [rbp+24+rbx*8]
> > +add rbx, 1
> >  callkfree
> > -cmp rbx, rbp
> > -jne .L3
> > +movsx   rax, DWORD PTR [rbp+16]
> > +mov rdx, QWORD PTR [rbp+8]
> > +cmp DWORD PTR [rdx+rax], ebx
> > +jg  .L3
> >  add rsp, 8
> >  xor eax, eax
> >  pop rbx
> >  pop rbp
> >  ret
> >  .L6:
> >  xor eax, eax
> >  for (i = 0; i < frags; i++)ret
> >
>
> So looks like before compiler generated:
>
> end = &frags[nfrags]
> for (ptr = &frag[0]; ptr < end; ptr++)
>
> and now it has to use the actual value of i, read nfrags in the loop
> each time and compare it to i.
>
> That makes sense, since it can't prove kfree() doesn't change nr_frags.
>
> IDK if we care, but at least commit message should mention this.

Anyway, the chunks using a local nr_frags are too few and not worth it.
I think you're right and that's better to use the cached value, I see
the instructions here being ligther.
Drop the series, I will make a new one which only acts where
skb_shinfo(skb) is accessed.

Thanks,
-- 
per aspera ad upstream

Re: [PATCH net-next 2/3] net: use skb_for_each_frag() helper where possible

2021-04-09 Thread kernel test robot 
Hi Matteo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:
https://github.com/0day-ci/linux/commits/Matteo-Croce/introduce-skb_for_each_frag/20210410-020828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 
4438669eb703d1a7416c2b19a8a15b0400b36738
config: um-allmodconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-22) 9.3.0
reproduce (this is a W=1 build):
# 
https://github.com/0day-ci/linux/commit/9a46b324d3f1ca289db31c0011a6bbfd5ae06918
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review 
Matteo-Croce/introduce-skb_for_each_frag/20210410-020828
git checkout 9a46b324d3f1ca289db31c0011a6bbfd5ae06918
# save the attached .config to linux build tree
make W=1 ARCH=um 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All warnings (new ones prefixed by >>):

   cc1: warning: arch/um/include/uapi: No such file or directory 
[-Wmissing-include-dirs]
   arch/um/drivers/vector_kern.c: In function 'get_bpf_flash':
   arch/um/drivers/vector_kern.c:145:18: warning: ordered comparison of pointer 
with integer zero [-Wextra]
 145 |return (allow > 0);
 |  ^
   arch/um/drivers/vector_kern.c: In function 'prep_skb':
>> arch/um/drivers/vector_kern.c:627:11: warning: variable 'nr_frags' set but 
>> not used [-Wunused-but-set-variable]
 627 |  int err, nr_frags, frag;
 |   ^~~~
   arch/um/drivers/vector_kern.c: In function 'vector_parse':
   arch/um/drivers/vector_kern.c:719:9: warning: variable 'len' set but not 
used [-Wunused-but-set-variable]
 719 |  int n, len, err;
 | ^~~


vim +/nr_frags +627 arch/um/drivers/vector_kern.c

49da7e64f33e80 Anton Ivanov 2017-11-20  609  
49da7e64f33e80 Anton Ivanov 2017-11-20  610  /*
49da7e64f33e80 Anton Ivanov 2017-11-20  611   * We do not use the RX queue as a 
proper wraparound queue for now
49da7e64f33e80 Anton Ivanov 2017-11-20  612   * This is not necessary because 
the consumption via netif_rx()
49da7e64f33e80 Anton Ivanov 2017-11-20  613   * happens in-line. While we can 
try using the return code of
49da7e64f33e80 Anton Ivanov 2017-11-20  614   * netif_rx() for flow control 
there are no drivers doing this today.
49da7e64f33e80 Anton Ivanov 2017-11-20  615   * For this RX specific use we 
ignore the tail/head locks and
49da7e64f33e80 Anton Ivanov 2017-11-20  616   * just read into a prepared queue 
filled with skbuffs.
49da7e64f33e80 Anton Ivanov 2017-11-20  617   */
49da7e64f33e80 Anton Ivanov 2017-11-20  618  
49da7e64f33e80 Anton Ivanov 2017-11-20  619  static struct sk_buff *prep_skb(
49da7e64f33e80 Anton Ivanov 2017-11-20  620 struct vector_private *vp,
49da7e64f33e80 Anton Ivanov 2017-11-20  621 struct user_msghdr *msg)
49da7e64f33e80 Anton Ivanov 2017-11-20  622  {
49da7e64f33e80 Anton Ivanov 2017-11-20  623 int linear = vp->max_packet + 
vp->headroom + SAFETY_MARGIN;
49da7e64f33e80 Anton Ivanov 2017-11-20  624 struct sk_buff *result;
49da7e64f33e80 Anton Ivanov 2017-11-20  625 int iov_index = 0, len;
49da7e64f33e80 Anton Ivanov 2017-11-20  626 struct iovec *iov = 
msg->msg_iov;
49da7e64f33e80 Anton Ivanov 2017-11-20 @627 int err, nr_frags, frag;
49da7e64f33e80 Anton Ivanov 2017-11-20  628 skb_frag_t *skb_frag;
49da7e64f33e80 Anton Ivanov 2017-11-20  629  
49da7e64f33e80 Anton Ivanov 2017-11-20  630 if (vp->req_size <= linear)
49da7e64f33e80 Anton Ivanov 2017-11-20  631 len = linear;
49da7e64f33e80 Anton Ivanov 2017-11-20  632 else
49da7e64f33e80 Anton Ivanov 2017-11-20  633 len = vp->req_size;
49da7e64f33e80 Anton Ivanov 2017-11-20  634 result = alloc_skb_with_frags(
49da7e64f33e80 Anton Ivanov 2017-11-20  635 linear,
49da7e64f33e80 Anton Ivanov 2017-11-20  636 len - vp->max_packet,
49da7e64f33e80 Anton Ivanov 2017-11-20  637 3,
49da7e64f33e80 Anton Ivanov 2017-11-20  638 &err,
49da7e64f33e80 Anton Ivanov 2017-11-20  639 GFP_ATOMIC
49da7e64f33e80 Anton Ivanov 2017-11-20  640 );
49da7e64f33e80 Anton Ivanov 2017-11-20  641 if (vp->header_size > 0)
49da7e64f33e80 Anton Ivanov 2017-11-20  642 iov_index++;
49da7e64f33e80 Anton Ivanov 2017-11-20  643 if (result == NULL) {
49da7e64f33e80 Anton Ivanov 2017-11-20  644 iov[iov_index].iov_base 
= NULL;
49da7e64f33e80 Anton Ivanov 2017-11-20  645 iov[iov_index].iov_len 
= 0;
49da7e64f33e80 Anton Ivanov 2017-11-20  646 goto done;
49da7e64f33e80 Anton Ivanov 2017-11-20  647 }
49da7e64f33e80 Anton Ivanov 2017-11-20  648 skb_reserve(result, 
vp->headroom);
49da7e64f33e80 Anton Ivanov 2017-11-20  649 result->dev = vp->dev;
49da7e64f33e80 Anton Ivanov 2017-11-20  650 skb_put(result, vp->max_packet);
49da7e64f33e80 Anton Ivanov 2017-11

Re: [PATCH net-next 2/3] net: use skb_for_each_frag() helper where possible

2021-04-09 Thread Jakub Kicinski 
On Fri, 9 Apr 2021 22:44:50 +0200 Matteo Croce wrote:
> > What pops to mind (although quite nit picky) is the question if the
> > assembly changes much between driver which used to cache nr_frags and
> > now always going skb_shinfo(skb)->nr_frags? It's a relatively common
> > pattern.  
> 
> Since skb_shinfo() is a macro and skb_end_pointer() a static inline,
> it should be the same, but I was curious to check so, this is a diff
> between the following snippet before and afer the macro:
> 
> int frags = skb_shinfo(skb)->nr_frags;
> int i;
> for (i = 0; i < frags; i++)
> kfree(skb->frags[i]);
> 
>  1 file changed, 8 insertions(+), 7 deletions(-)
> 
> --- ins1.s 2021-04-09 22:35:59.384523865 +0200
> +++ ins2.s 2021-04-09 22:36:08.132594737 +0200
> @@ -1,26 +1,27 @@
>  iter:
>  movsx   rax, DWORD PTR [rdi+16]
>  mov rdx, QWORD PTR [rdi+8]
>  mov eax, DWORD PTR [rdx+rax]
>  testeax, eax
>  jle .L6
>  pushrbp
> -sub eax, 1
> +mov rbp, rdi
>  pushrbx
> -lea rbp, [rdi+32+rax*8]
> -lea rbx, [rdi+24]
> +xor ebx, ebx
>  sub rsp, 8
>  .L3:
> -mov rdi, QWORD PTR [rbx]
> -add rbx, 8
> +mov rdi, QWORD PTR [rbp+24+rbx*8]
> +add rbx, 1
>  callkfree
> -cmp rbx, rbp
> -jne .L3
> +movsx   rax, DWORD PTR [rbp+16]
> +mov rdx, QWORD PTR [rbp+8]
> +cmp DWORD PTR [rdx+rax], ebx
> +jg  .L3
>  add rsp, 8
>  xor eax, eax
>  pop rbx
>  pop rbp
>  ret
>  .L6:
>  xor eax, eax
>  for (i = 0; i < frags; i++)ret
> 

So looks like before compiler generated:

end = &frags[nfrags]
for (ptr = &frag[0]; ptr < end; ptr++)

and now it has to use the actual value of i, read nfrags in the loop
each time and compare it to i.

That makes sense, since it can't prove kfree() doesn't change nr_frags.

IDK if we care, but at least commit message should mention this.

Re: [PATCH net-next 2/3] net: use skb_for_each_frag() helper where possible

2021-04-09 Thread kernel test robot 
Hi Matteo,

Thank you for the patch! Perhaps something to improve:

[auto build test WARNING on net-next/master]

url:
https://github.com/0day-ci/linux/commits/Matteo-Croce/introduce-skb_for_each_frag/20210410-020828
base:   https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git 
4438669eb703d1a7416c2b19a8a15b0400b36738
config: xtensa-allyesconfig (attached as .config)
compiler: xtensa-linux-gcc (GCC) 9.3.0
reproduce (this is a W=1 build):
wget 
https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O 
~/bin/make.cross
chmod +x ~/bin/make.cross
# 
https://github.com/0day-ci/linux/commit/9a46b324d3f1ca289db31c0011a6bbfd5ae06918
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review 
Matteo-Croce/introduce-skb_for_each_frag/20210410-020828
git checkout 9a46b324d3f1ca289db31c0011a6bbfd5ae06918
# save the attached .config to linux build tree
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-9.3.0 make.cross 
ARCH=xtensa 

If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot 

All warnings (new ones prefixed by >>):

   drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c: In function 
'netxen_map_tx_skb':
>> drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c:1975:9: warning: 
>> variable 'nr_frags' set but not used [-Wunused-but-set-variable]
1975 |  int i, nr_frags;
 | ^~~~
--
   drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c: In function 
'qlcnic_map_tx_skb':
>> drivers/net/ethernet/qlogic/qlcnic/qlcnic_io.c:584:9: warning: variable 
>> 'nr_frags' set but not used [-Wunused-but-set-variable]
 584 |  int i, nr_frags;
 | ^~~~


vim +/nr_frags +1975 drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c

cd1f8160e015cd drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2008-07-21  1968  
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1969  static int
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1970  netxen_map_tx_skb(struct pci_dev *pdev,
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1971struct sk_buff *skb, struct 
netxen_cmd_buffer *pbuf)
6f70340698333f drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-01-14  1972  {
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1973struct netxen_skb_frag *nf;
d7840976e39156 drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c Matthew 
Wilcox (Oracle  2019-07-22  1974)   skb_frag_t *frag;
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23 @1975int i, nr_frags;
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1976dma_addr_t map;
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1977  
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1978nr_frags = skb_shinfo(skb)->nr_frags;
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1979nf = &pbuf->frag_array[0];
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1980  
297af515d75f5c drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c Christophe 
JAILLET  2021-01-13  1981map = dma_map_single(&pdev->dev, skb->data, 
skb_headlen(skb),
297af515d75f5c drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c Christophe 
JAILLET  2021-01-13  1982 DMA_TO_DEVICE);
297af515d75f5c drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c Christophe 
JAILLET  2021-01-13  1983if (dma_mapping_error(&pdev->dev, map))
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1984goto out_err;
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1985  
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1986nf->dma = map;
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1987nf->length = skb_headlen(skb);
6f70340698333f drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-01-14  1988  
9a46b324d3f1ca drivers/net/ethernet/qlogic/netxen/netxen_nic_main.c Matteo 
Croce2021-04-09  1989skb_for_each_frag(skb, i) {
ce644ed4db3ee1 drivers/net/netxen/netxen_nic_main.c Dhananjay 
Phadke2009-08-23  1990frag = &skb_shinfo(skb)->frags[i];
ce644ed4db3ee1 drivers/n

Re: [PATCH net-next 2/3] net: use skb_for_each_frag() helper where possible

2021-04-09 Thread Matteo Croce 
On Fri, Apr 9, 2021 at 8:54 PM Jakub Kicinski  wrote:
>
> On Fri,  9 Apr 2021 20:06:04 +0200 Matteo Croce wrote:
> > From: Matteo Croce 
> >
> > use the new helper macro skb_for_each_frag() which allows to iterate
> > through all the SKB fragments.
> >
> > The patch was created with Coccinelle, this was the semantic patch:
>
> Bunch of set but not used warnings here. Please make sure the code
> builds cleanly allmodconfig, W=1 C=1 before posting.
>

Will do.

> What pops to mind (although quite nit picky) is the question if the
> assembly changes much between driver which used to cache nr_frags and
> now always going skb_shinfo(skb)->nr_frags? It's a relatively common
> pattern.

Since skb_shinfo() is a macro and skb_end_pointer() a static inline,
it should be the same, but I was curious to check so, this is a diff
between the following snippet before and afer the macro:

int frags = skb_shinfo(skb)->nr_frags;
int i;
for (i = 0; i < frags; i++)
kfree(skb->frags[i]);

 1 file changed, 8 insertions(+), 7 deletions(-)

--- ins1.s 2021-04-09 22:35:59.384523865 +0200
+++ ins2.s 2021-04-09 22:36:08.132594737 +0200
@@ -1,26 +1,27 @@
 iter:
 movsx   rax, DWORD PTR [rdi+16]
 mov rdx, QWORD PTR [rdi+8]
 mov eax, DWORD PTR [rdx+rax]
 testeax, eax
 jle .L6
 pushrbp
-sub eax, 1
+mov rbp, rdi
 pushrbx
-lea rbp, [rdi+32+rax*8]
-lea rbx, [rdi+24]
+xor ebx, ebx
 sub rsp, 8
 .L3:
-mov rdi, QWORD PTR [rbx]
-add rbx, 8
+mov rdi, QWORD PTR [rbp+24+rbx*8]
+add rbx, 1
 callkfree
-cmp rbx, rbp
-jne .L3
+movsx   rax, DWORD PTR [rbp+16]
+mov rdx, QWORD PTR [rbp+8]
+cmp DWORD PTR [rdx+rax], ebx
+jg  .L3
 add rsp, 8
 xor eax, eax
 pop rbx
 pop rbp
 ret
 .L6:
 xor eax, eax
 ret

-- 
per aspera ad upstream

Re: [PATCH net-next 2/3] net: use skb_for_each_frag() helper where possible

2021-04-09 Thread Jakub Kicinski 
On Fri,  9 Apr 2021 20:06:04 +0200 Matteo Croce wrote:
> From: Matteo Croce 
> 
> use the new helper macro skb_for_each_frag() which allows to iterate
> through all the SKB fragments.
> 
> The patch was created with Coccinelle, this was the semantic patch:

Bunch of set but not used warnings here. Please make sure the code
builds cleanly allmodconfig, W=1 C=1 before posting.

What pops to mind (although quite nit picky) is the question if the
assembly changes much between driver which used to cache nr_frags and
now always going skb_shinfo(skb)->nr_frags? It's a relatively common
pattern.