RE: WARNING in kobject_add_internal

[Yuan, Linyu (NSB - CN/Shanghai)]

Hi,

I have a question,
"can syzbot auto test each tree with newest changeset" ?

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Dmitry Vyukov
> Sent: Wednesday, April 11, 2018 10:58 PM
> To: syzbot
> Cc: bri...@lists.linux-foundation.org; David Miller; Greg Kroah-Hartman;
> LKML; netdev; stephen hemminger; syzkaller-bugs
> Subject: Re: WARNING in kobject_add_internal
> 
> On Fri, Jan 5, 2018 at 10:41 PM, syzbot
>  il.com>
> wrote:
> > syzkaller has found reproducer for the following crash on
> > 89876f275e8d562912d9c238cd888b52065cf25c
> > git://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/master
> > compiler: gcc (GCC) 7.1.1 20170620
> > .config is attached
> > Raw console output is attached.
> > C reproducer is attached
> > syzkaller reproducer is attached. See https://goo.gl/kgGztJ
> > for information about syzkaller reproducers
> >
> >
> > IMPORTANT: if you fix the bug, please add the following tag to the commit:
> > Reported-by:
> >
> syzbot+e204ced820ef739d71ef5438f5e1976a874abc8d@syzkaller.appspotmail
> .com
> > It will help syzbot understand when the bug is fixed.
> 
> #syz dup: WARNING: kobject bug in device_add
> 
> > [ cut here ]
> > kobject_add_internal failed for   (error: -12 parent: net)
> > WARNING: CPU: 1 PID: 3494 at lib/kobject.c:244
> > kobject_add_internal+0x3f6/0xbc0 lib/kobject.c:242
> > Kernel panic - not syncing: panic_on_warn set ...
> >
> > CPU: 1 PID: 3494 Comm: syzkaller425998 Not tainted 4.15.0-rc6+ #249
> > Hardware name: Google Google Compute Engine/Google Compute Engine,
> BIOS
> > Google 01/01/2011
> > Call Trace:
> >  __dump_stack lib/dump_stack.c:17 [inline]
> >  dump_stack+0x194/0x257 lib/dump_stack.c:53
> >  panic+0x1e4/0x41c kernel/panic.c:183
> >  __warn+0x1dc/0x200 kernel/panic.c:547
> >  report_bug+0x211/0x2d0 lib/bug.c:184
> >  fixup_bug.part.11+0x37/0x80 arch/x86/kernel/traps.c:178
> >  fixup_bug arch/x86/kernel/traps.c:247 [inline]
> >  do_error_trap+0x2d7/0x3e0 arch/x86/kernel/traps.c:296
> >  do_invalid_op+0x1b/0x20 arch/x86/kernel/traps.c:315
> >  invalid_op+0x22/0x40 arch/x86/entry/entry_64.S:1079
> > RIP: 0010:kobject_add_internal+0x3f6/0xbc0 lib/kobject.c:242
> > RSP: 0018:8801c53c76f0 EFLAGS: 00010286
> > RAX: dc08 RBX: 8801bf5a88d8 RCX: 8159da9e
> > RDX:  RSI: 110038a78e99 RDI: 8801c53c73f8
> > RBP: 8801c53c77e8 R08: 110038a78e5b R09: 
> > R10: 8801c53c74b0 R11:  R12: 110038a78ee4
> > R13: fff4 R14: 8801d8359a80 R15: 86201980
> >  kobject_add_varg lib/kobject.c:366 [inline]
> >  kobject_add+0x132/0x1f0 lib/kobject.c:411
> >  device_add+0x35d/0x1650 drivers/base/core.c:1787
> >  netdev_register_kobject+0x183/0x360 net/core/net-sysfs.c:1604
> >  register_netdevice+0xb2b/0x1010 net/core/dev.c:7698
> >  tun_set_iff drivers/net/tun.c:2319 [inline]
> >  __tun_chr_ioctl+0x1d89/0x3dd0 drivers/net/tun.c:2524
> >  tun_chr_ioctl+0x2a/0x40 drivers/net/tun.c:2773
> >  vfs_ioctl fs/ioctl.c:46 [inline]
> >  do_vfs_ioctl+0x1b1/0x1520 fs/ioctl.c:686
> >  SYSC_ioctl fs/ioctl.c:701 [inline]
> >  SyS_ioctl+0x8f/0xc0 fs/ioctl.c:692
> >  entry_SYSCALL_64_fastpath+0x23/0x9a
> > RIP: 0033:0x444fc9
> > RSP: 002b:7fff53389dc8 EFLAGS: 0246 ORIG_RAX:
> 0010
> > RAX: ffda RBX: 0001 RCX: 00444fc9
> > RDX: 20533000 RSI: 400454ca RDI: 0004
> > RBP: 0005 R08: 0002 R09: 006f3131
> > R10:  R11: 0246 R12: 00402500
> > R13: 00402590 R14:  R15: 
> >
> > Dumping ftrace buffer:
> >(ftrace buffer empty)
> > Kernel Offset: disabled
> > Rebooting in 86400 seconds..
> >

RE: [PATCH net-next 1/2] net: permit skb_segment on head_frag frag_list skb

[Yuan, Linyu (NSB - CN/Shanghai)]

Sorry, I should not add "Here cause next BUG_ON always false."
It cause misunderstanding, I just comment on BUG_ON in else branch.

> -Original Message-
> From: Yonghong Song [mailto:y...@fb.com]
> Sent: Tuesday, March 20, 2018 1:54 PM
> To: Yuan, Linyu (NSB - CN/Shanghai); eduma...@google.com; a...@fb.com;
> dan...@iogearbox.net; dipt...@fb.com; netdev@vger.kernel.org
> Cc: kernel-t...@fb.com
> Subject: Re: [PATCH net-next 1/2] net: permit skb_segment on head_frag
> frag_list skb
> 
> 
> 
> On 3/19/18 10:30 PM, Yuan, Linyu (NSB - CN/Shanghai) wrote:
> >
> >
> >> -Original Message-
> >> From: netdev-ow...@vger.kernel.org
> [mailto:netdev-ow...@vger.kernel.org]
> >> On Behalf Of Yonghong Song
> >> Sent: Tuesday, March 20, 2018 1:16 PM
> >> To: eduma...@google.com; a...@fb.com; dan...@iogearbox.net;
> >> dipt...@fb.com; netdev@vger.kernel.org
> >> Cc: kernel-t...@fb.com
> >> Subject: [PATCH net-next 1/2] net: permit skb_segment on head_frag
> frag_list
> >> skb
> >>
> >>
> >>while (pos < offset + len) {
> >>if (i >= nfrags) {
> >> -  BUG_ON(skb_headlen(list_skb));
> >> +  if (skb_headlen(list_skb) && check_list_skb == 
> >> list_skb) {
> > Here cause next BUG_ON always false.
> 
> The idea is since in this branch, we did not do list_skb =
> list_skb->next. So we update check_list_skb. Next time, when the
> control reaches here, list_skb may still be the same, but check_list_skb
> is not, so we proceed to process list_skb->frags in the else branch.
> 
> In the else branch, we have
> list_skb = list_skb->next;
> check_list_skb = list_skb;
> 
> So when the current frags are processed and ready for the list_skb.
> list_skb will be equal to check_list_skb and it will be processed again.
> 
> It is a little bit convoluted. Please let me know you have better idea.
> 
> >> +  } else {
> >> +  BUG_ON(skb_headlen(list_skb) && 
> >> check_list_skb ==
> >> list_skb);
> > Just according code logic, no need BUG_ON, right?
> 
> Oh, yes, we do not need this. Will remove in the next version.
> 
> >>
> >> -  i = 0;
> >> -  nfrags = skb_shinfo(list_skb)->nr_frags;
> >> -  frag = skb_shinfo(list_skb)->frags;
> >> -  frag_skb = list_skb;
> >> +  i = 0;
> >> +  nfrags = skb_shinfo(list_skb)->nr_frags;
> >> +  frag = skb_shinfo(list_skb)->frags;
> >> +  frag_skb = list_skb;
> >>
> >> -  BUG_ON(!nfrags);
> >> +  BUG_ON(!nfrags);
> >>
> >> -  if (skb_orphan_frags(frag_skb, GFP_ATOMIC) ||
> >> -  skb_zerocopy_clone(nskb, frag_skb,
> >> - GFP_ATOMIC))
> >> -  goto err;
> >> +  if (skb_orphan_frags(frag_skb, 
> >> GFP_ATOMIC) ||
> >> +  skb_zerocopy_clone(nskb, frag_skb,
> >> GFP_ATOMIC))
> >> +  goto err;
> >>
> >> -  list_skb = list_skb->next;
> >> +  list_skb = list_skb->next;
> >> +  check_list_skb = list_skb;
> >> +  }
> >>}
> >>
> >>if (unlikely(skb_shinfo(nskb)->nr_frags >=
> >> --
> >> 2.9.5
> >

RE: [PATCH net-next 1/2] net: permit skb_segment on head_frag frag_list skb

[Yuan, Linyu (NSB - CN/Shanghai)]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Yonghong Song
> Sent: Tuesday, March 20, 2018 1:16 PM
> To: eduma...@google.com; a...@fb.com; dan...@iogearbox.net;
> dipt...@fb.com; netdev@vger.kernel.org
> Cc: kernel-t...@fb.com
> Subject: [PATCH net-next 1/2] net: permit skb_segment on head_frag frag_list
> skb
> 
> 
>   while (pos < offset + len) {
>   if (i >= nfrags) {
> - BUG_ON(skb_headlen(list_skb));
> + if (skb_headlen(list_skb) && check_list_skb == 
> list_skb) {
Here cause next BUG_ON always false.
> + } else {
> + BUG_ON(skb_headlen(list_skb) && 
> check_list_skb ==
> list_skb);
Just according code logic, no need BUG_ON, right? 
> 
> - i = 0;
> - nfrags = skb_shinfo(list_skb)->nr_frags;
> - frag = skb_shinfo(list_skb)->frags;
> - frag_skb = list_skb;
> + i = 0;
> + nfrags = skb_shinfo(list_skb)->nr_frags;
> + frag = skb_shinfo(list_skb)->frags;
> + frag_skb = list_skb;
> 
> - BUG_ON(!nfrags);
> + BUG_ON(!nfrags);
> 
> - if (skb_orphan_frags(frag_skb, GFP_ATOMIC) ||
> - skb_zerocopy_clone(nskb, frag_skb,
> -GFP_ATOMIC))
> - goto err;
> + if (skb_orphan_frags(frag_skb, 
> GFP_ATOMIC) ||
> + skb_zerocopy_clone(nskb, frag_skb,
> GFP_ATOMIC))
> + goto err;
> 
> - list_skb = list_skb->next;
> + list_skb = list_skb->next;
> + check_list_skb = list_skb;
> + }
>   }
> 
>   if (unlikely(skb_shinfo(nskb)->nr_frags >=
> --
> 2.9.5

RE: macvlan devices and vlan interaction

[Yuan, Linyu (NSB - CN/Shanghai)]

https://www.spinics.net/lists/netdev/msg476083.html

I also have a macvlan device question, but get no answer.

But my original thought is in __netif_receive_skb_core() we should check packet 
destination mac address,
if it match macvlan device, change packet as receive from macvlan device, not 
lower device, then packet go to upper layer.

But I don't know how to process broadcast mac address. Do macvlan device can 
receive broadcast packet ?

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Keller, Jacob E
> Sent: Tuesday, January 30, 2018 7:02 AM
> To: netdev@vger.kernel.org
> Cc: Duyck, Alexander H
> Subject: macvlan devices and vlan interaction
> 
> Hi,
> 
> I'm currently investigating how macvlan devices behave in regards to vlan
> support, and found some interesting behavior that I am not sure how best to
> correct, or what the right path forward is.
> 
> If I create a macvlan device:
> 
> ip link add link ens0 name macvlan0 type macvlan:
> 
> and then add a VLAN to it:
> 
> ip link add link macvlan0 name vlan10 type vlan id 10
> 
> This works to pass VLAN 10 traffic over the macvlan device. This seems like
> expected behavior.
> 
> However, if I then also add vlan 10 to the lowerdev:
> 
> ip link add link ens0 name lowervlan10  type vlan id 10
> 
> Then traffic stops flowing to the VLAN on the macvlan device.
> 
> This happens, as far as I can tell, because of how the VLAN traffic is 
> filtered
> first, and then forwarded to the VLAN device, which doesn't know about how
> the macvlan device exists.
> 
> It seems, essentially, that vlan stacked on top of a macvlan shouldn't work.
> Because the vlan code basically expects each vlan to apply to every MAC
> address, and the macvlan device works by putting its MAC address into the
> unicast address list, there's no way for a device driver to know when or how 
> to
> apply the vlan.
> 
> This gets a bit more confusing when we add in the l2 fwd hardware offload.
> 
> Currently, at least for the Intel network parts, this isn't supported, 
> because of a
> bug in which the device drivers don't apply the VLANs to the macvlan
> accelerated addresses. If we fix this, at least for fm10k, the behavior is 
> slightly
> better, because of how the hardware filtering at the MAC address happens
> first, and we direct the traffic to the proper device regardless of VLAN.
> 
> In addition to this peculiarity of VLANs on both the macvlan and lowerdev, is
> that when a macvlan device adds a VLAN, the lowerdev gets an indication to
> add the vlan via its .ndo_vlan_rx_add_vid(), which doesn't distinguish between
> which addresses the VLAN might apply to. It thus simply, depending on
> hardware design, enables the VLAN for all its unicast and multicast addresses.
> Some hardware could theoretically support MAC+VLAN pairs, where it could
> distinguish that a VLAN should only be added for some subset of addresses.
> Other hardware might not be so lucky..
> 
> Unfortunately, this has the weird consequence that if we have the following
> stack of devices:
> 
> vlan10@macvlan0
> macvlan0@ens0
> ens0
> 
> Then ens0 will receive VLAN10 traffic on every address. So VLAN 10 traffic
> destined to the MAC of the lowerdev will be received, instead of dropped.
> 
> If we add VLAN 10 to the lowerdev so we have both the above stack and also
> 
> lowervlan10@ens0
> ens0 (mac gg:hh:ii:jj:kk)
> 
> then all vlan 10 traffic will be received on the lowerdev VLAN 10, without any
> being forwarded to the VLAN10 attached to the macvlan.
> 
> However, if we add two macvlans, and each add the vlan10, so we have the
> following:
> 
> avlan10@macvlan0
> macvlan0@ens0
> ens0
> 
> bvlan10@macvlan1
> macvlan1@ens0
> ens0
> 
> In this case, it does appear that traffic is sorted out correctly. It seems 
> that
> only if the lowerdev gets the VLAN does it end up breaking. If I remove 
> bvlan10
> from macvlan1, the traffic associated with vlan10 is still received by 
> macvlan1,
> even though in principle it should no longer be.
> 
> What is the correct behavior here? Should this just be "administrators should
> know better"? I don't think that's a great argument, and either way we're 
> still
> essentially leaking VLANs across the macvlan interfaces, which I don't think 
> is
> ideal.
> 
> I see two possible solutions:
> 
> 1) modify macvlan driver so that it is marked as VLAN_CHALLENGED, and thus
> indicate it cannot handle VLAN traffic on top of it.
>   a. In order to get the VLANs associated, administrator could instead add the
> VLAN first, and then add the macvlan on top. This I think is a better
> configuration.
>   b. that doesn't work in the offload case, unless/until we fix the VLAN
> interface to forward the l2_dfwd_add_station() along with a vid.
>   c. this could appear as loss of functionality, since in some cases these 
> VLAN
> on top of macvlan work today (with the interesting caveats listed above).
> 
> 2) modify how 

question about __netif_receive_skb_core() work on macvlan device

[Yuan, Linyu (NSB - CN/Shanghai)]

Hi,

I have a question about __netif_receive_skb_core(),
1. create a macvlan device on a real ethernet device and configure a mac 
address to this macvlan device
2. create a AF_PACKET socket and bind to the real ethernet device in step 1
3. user application will receive packet which destination mac address equal to 
step1 through socket created by step2

Is it correct for a macvlan device configured as step1?

thanks

RE: [PATCH net-next] net: assign err to 0 at begin in do_setlink() function

[Yuan, Linyu (NSB - CN/Shanghai)]

Thanks, I will provide a new patch

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of David Miller
> Sent: Wednesday, November 15, 2017 1:20 PM
> To: Yuan, Linyu (NSB - CN/Shanghai)
> Cc: cug...@163.com; netdev@vger.kernel.org
> Subject: Re: [PATCH net-next] net: assign err to 0 at begin in do_setlink()
> function
> 
> From: "Yuan, Linyu (NSB - CN/Shanghai)" <linyu.y...@nokia-sbell.com>
> Date: Wed, 15 Nov 2017 05:15:35 +
> 
> >
> >
> >> -Original Message-
> >> From: netdev-ow...@vger.kernel.org
> [mailto:netdev-ow...@vger.kernel.org]
> >> On Behalf Of David Miller
> >> Sent: Wednesday, November 15, 2017 1:08 PM
> >> To: cug...@163.com
> >> Cc: netdev@vger.kernel.org; Yuan, Linyu (NSB - CN/Shanghai)
> >> Subject: Re: [PATCH net-next] net: assign err to 0 at begin in do_setlink()
> >> function
> >>
> >> From: yuan linyu <cug...@163.com>
> >> Date: Tue, 14 Nov 2017 22:30:59 +0800
> >>
> >> > From: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> >> >
> >> > each netlink attribute have proper process when error happen,
> >> > when exit on attribute process, it implies that no error,
> >> > so err = 0; is useless.
> >> >
> >> > assign err = 0; at beginning if all attributes not set.
> >> >
> >> > Signed-off-by: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> >>
> >> This change is not correct.
> >>
> >> The IFLA_VF_PORTS code block can finish with err set non-zero.
> >
> > Do you mean this block can return err = -EOPNOTSUPP;  ?
> > It means nla_for_each_nested() in this block will never enter, right ?
> 
> Yes, that is what I mean.

RE: [PATCH net-next] net: assign err to 0 at begin in do_setlink() function

[Yuan, Linyu (NSB - CN/Shanghai)]

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of David Miller
> Sent: Wednesday, November 15, 2017 1:08 PM
> To: cug...@163.com
> Cc: netdev@vger.kernel.org; Yuan, Linyu (NSB - CN/Shanghai)
> Subject: Re: [PATCH net-next] net: assign err to 0 at begin in do_setlink()
> function
> 
> From: yuan linyu <cug...@163.com>
> Date: Tue, 14 Nov 2017 22:30:59 +0800
> 
> > From: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> >
> > each netlink attribute have proper process when error happen,
> > when exit on attribute process, it implies that no error,
> > so err = 0; is useless.
> >
> > assign err = 0; at beginning if all attributes not set.
> >
> > Signed-off-by: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> 
> This change is not correct.
> 
> The IFLA_VF_PORTS code block can finish with err set non-zero.

Do you mean this block can return err = -EOPNOTSUPP;  ?
It means nla_for_each_nested() in this block will never enter, right ?

> The assignment to zero there is really necessary.

RE: [PATCH net-next] net: dpaa: remove init which already done in per-cpu allocation

[Yuan, Linyu (NSB - CN/Shanghai)]

I just saw below accepted commit, it said "per cpu allocations are already 
zeroed, no need to clear them again."

https://git.kernel.org/pub/scm/linux/kernel/git/davem/net-next.git/commit/?id=bfd8e5a407133e58a92a38ccf3d0ba6db81f22d8

I will not touch memory sub-system, so I will not change this function 
description.


> -Original Message-
> From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> Sent: Monday, October 30, 2017 1:56 PM
> To: Yuan, Linyu (NSB - CN/Shanghai); yuan linyu; netdev@vger.kernel.org;
> gre...@linuxfoundation.org
> Cc: David S . Miller
> Subject: RE: [PATCH net-next] net: dpaa: remove init which already done in
> per-cpu allocation
> 
> Also here:
> 
> http://elixir.free-electrons.com/linux/latest/source/mm/percpu.c#L717
> 
> Should the fact that the memory is zeroed be included in the function
> description?
> 
> /**
>  * pcpu_alloc - the percpu allocator
>  * @size: size of area to allocate in bytes
>  * @align: alignment of area (max PAGE_SIZE)
>  * @reserved: allocate from the reserved chunk if available
>  * @gfp: allocation flags
>  *
>  * Allocate percpu area of @size bytes aligned at @align.  If @gfp doesn't
>  * contain %GFP_KERNEL, the allocation is atomic.
>  *
>  * RETURNS:
>  * Percpu pointer to the allocated area on success, NULL on failure.
>  */
> 
> Now it seems to be an implementation detail rather than a guarantee.
> 
> Looking at Documentation/driver-model/devres.txt, the memset is not
> mentioned there either.
> 
> > -Original Message-
> > From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.y...@nokia-sbell.com]
> > Sent: Monday, October 30, 2017 7:21 AM
> > To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; yuan linyu
> > <cug...@163.com>; netdev@vger.kernel.org
> > Cc: David S . Miller <da...@davemloft.net>
> > Subject: RE: [PATCH net-next] net: dpaa: remove init which already done in
> > per-cpu allocation
> >
> > http://elixir.free-electrons.com/linux/latest/source/mm/percpu.c#L1018
> >
> > > -Original Message-
> > > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > > Sent: Monday, October 30, 2017 1:15 PM
> > > To: Yuan, Linyu (NSB - CN/Shanghai); yuan linyu; netdev@vger.kernel.org
> > > Cc: David S . Miller
> > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already done
> > in
> > > per-cpu allocation
> > >
> > > Is the memset part documented?
> > > Can you point to the specific comment & code that does it?
> > >
> > > Thanks
> > >
> > > > -Original Message-
> > > > From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.yuan@nokia-
> > sbell.com]
> > > > Sent: Monday, October 30, 2017 7:12 AM
> > > > To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; yuan linyu
> > > > <cug...@163.com>; netdev@vger.kernel.org
> > > > Cc: David S . Miller <da...@davemloft.net>
> > > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> > done in
> > > > per-cpu allocation
> > > >
> > > > Hi,
> > > >
> > > > devm_alloc_percpu() will allocate per-cpu memory and memset allocated
> > > > block content to 0.
> > > >
> > > > > -Original Message-
> > > > > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > > > > Sent: Monday, October 30, 2017 1:08 PM
> > > > > To: yuan linyu; netdev@vger.kernel.org
> > > > > Cc: David S . Miller; Yuan, Linyu (NSB - CN/Shanghai)
> > > > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already
> > done
> > > > in
> > > > > per-cpu allocation
> > > > >
> > > > > Hi Yuan,
> > > > >
> > > > > Can you please give more details about this change you are
> > proposing?
> > > > >
> > > > > Regards,
> > > > > Madalin
> > > > >
> > > > > > -Original Message-
> > > > > > From: netdev-ow...@vger.kernel.org
> > > > > [mailto:netdev-ow...@vger.kernel.org]
> > > > > > On Behalf Of yuan linyu
> > > > > > Sent: Sunday, October 29, 2017 3:49 AM
> > > > > > To: netdev@vger.kernel.org
> > > > > > Cc: David S . Miller <da...@davemloft.net>; yuan linyu
> > > > > > <linyu.y...@alcatel-sbell.co

RE: [PATCH net-next] net: dpaa: remove init which already done in per-cpu allocation

[Yuan, Linyu (NSB - CN/Shanghai)]

Hi,

devm_alloc_percpu() will allocate per-cpu memory and memset allocated block 
content to 0.

> -Original Message-
> From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> Sent: Monday, October 30, 2017 1:08 PM
> To: yuan linyu; netdev@vger.kernel.org
> Cc: David S . Miller; Yuan, Linyu (NSB - CN/Shanghai)
> Subject: RE: [PATCH net-next] net: dpaa: remove init which already done in
> per-cpu allocation
> 
> Hi Yuan,
> 
> Can you please give more details about this change you are proposing?
> 
> Regards,
> Madalin
> 
> > -Original Message-
> > From: netdev-ow...@vger.kernel.org
> [mailto:netdev-ow...@vger.kernel.org]
> > On Behalf Of yuan linyu
> > Sent: Sunday, October 29, 2017 3:49 AM
> > To: netdev@vger.kernel.org
> > Cc: David S . Miller <da...@davemloft.net>; yuan linyu
> > <linyu.y...@alcatel-sbell.com.cn>
> > Subject: [PATCH net-next] net: dpaa: remove init which already done in
> > per-cpu allocation
> >
> > From: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> >
> > Signed-off-by: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> > ---
> >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 
> >  1 file changed, 4 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > index a8d0be8..1ccc316 100644
> > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > @@ -2814,10 +2814,6 @@ static int dpaa_eth_probe(struct
> platform_device
> > *pdev)
> > err = -ENOMEM;
> > goto free_dpaa_fqs;
> > }
> > -   for_each_possible_cpu(i) {
> > -   percpu_priv = per_cpu_ptr(priv->percpu_priv, i);
> > -   memset(percpu_priv, 0, sizeof(*percpu_priv));
> > -   }
> >
> > priv->num_tc = 1;
> > netif_set_real_num_tx_queues(net_dev, priv->num_tc *
> > DPAA_TC_TXQ_NUM);
> > --
> > 2.7.4
> >

RE: [PATCH net-next] net: dpaa: remove init which already done in per-cpu allocation

[Yuan, Linyu (NSB - CN/Shanghai)]

http://elixir.free-electrons.com/linux/latest/source/mm/percpu.c#L1018

> -Original Message-
> From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> Sent: Monday, October 30, 2017 1:15 PM
> To: Yuan, Linyu (NSB - CN/Shanghai); yuan linyu; netdev@vger.kernel.org
> Cc: David S . Miller
> Subject: RE: [PATCH net-next] net: dpaa: remove init which already done in
> per-cpu allocation
> 
> Is the memset part documented?
> Can you point to the specific comment & code that does it?
> 
> Thanks
> 
> > -----Original Message-
> > From: Yuan, Linyu (NSB - CN/Shanghai) [mailto:linyu.y...@nokia-sbell.com]
> > Sent: Monday, October 30, 2017 7:12 AM
> > To: Madalin-cristian Bucur <madalin.bu...@nxp.com>; yuan linyu
> > <cug...@163.com>; netdev@vger.kernel.org
> > Cc: David S . Miller <da...@davemloft.net>
> > Subject: RE: [PATCH net-next] net: dpaa: remove init which already done in
> > per-cpu allocation
> >
> > Hi,
> >
> > devm_alloc_percpu() will allocate per-cpu memory and memset allocated
> > block content to 0.
> >
> > > -Original Message-
> > > From: Madalin-cristian Bucur [mailto:madalin.bu...@nxp.com]
> > > Sent: Monday, October 30, 2017 1:08 PM
> > > To: yuan linyu; netdev@vger.kernel.org
> > > Cc: David S . Miller; Yuan, Linyu (NSB - CN/Shanghai)
> > > Subject: RE: [PATCH net-next] net: dpaa: remove init which already done
> > in
> > > per-cpu allocation
> > >
> > > Hi Yuan,
> > >
> > > Can you please give more details about this change you are proposing?
> > >
> > > Regards,
> > > Madalin
> > >
> > > > -Original Message-
> > > > From: netdev-ow...@vger.kernel.org
> > > [mailto:netdev-ow...@vger.kernel.org]
> > > > On Behalf Of yuan linyu
> > > > Sent: Sunday, October 29, 2017 3:49 AM
> > > > To: netdev@vger.kernel.org
> > > > Cc: David S . Miller <da...@davemloft.net>; yuan linyu
> > > > <linyu.y...@alcatel-sbell.com.cn>
> > > > Subject: [PATCH net-next] net: dpaa: remove init which already done in
> > > > per-cpu allocation
> > > >
> > > > From: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> > > >
> > > > Signed-off-by: yuan linyu <linyu.y...@alcatel-sbell.com.cn>
> > > > ---
> > > >  drivers/net/ethernet/freescale/dpaa/dpaa_eth.c | 4 
> > > >  1 file changed, 4 deletions(-)
> > > >
> > > > diff --git a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > index a8d0be8..1ccc316 100644
> > > > --- a/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > +++ b/drivers/net/ethernet/freescale/dpaa/dpaa_eth.c
> > > > @@ -2814,10 +2814,6 @@ static int dpaa_eth_probe(struct
> > > platform_device
> > > > *pdev)
> > > > err = -ENOMEM;
> > > > goto free_dpaa_fqs;
> > > > }
> > > > -   for_each_possible_cpu(i) {
> > > > -   percpu_priv = per_cpu_ptr(priv->percpu_priv, i);
> > > > -   memset(percpu_priv, 0, sizeof(*percpu_priv));
> > > > -   }
> > > >
> > > > priv->num_tc = 1;
> > > > netif_set_real_num_tx_queues(net_dev, priv->num_tc *
> > > > DPAA_TC_TXQ_NUM);
> > > > --
> > > > 2.7.4
> > > >

RE: [PATCH net-next] skbuff: Add BUG_ON in skb_init.

[Yuan, Linyu (NSB - CN/Shanghai)]

Original this function have return value but not used, and check allocate 
result internal, 
When I change this function to void return, I add this BUG_ON.

> -Original Message-
> From: netdev-ow...@vger.kernel.org [mailto:netdev-ow...@vger.kernel.org]
> On Behalf Of Tonghao Zhang
> Sent: Thursday, August 10, 2017 2:10 PM
> To: David Miller
> Cc: Linux Kernel Network Developers
> Subject: Re: [PATCH net-next] skbuff: Add BUG_ON in skb_init.
> 
> Thanks a lot. I found it when reviewing this codes. and BUG_ON is
> added  in the init_inodecache(). We may remove it or not ?
> 
> diff --git a/net/socket.c b/net/socket.c
> index b332d1e..ebee3ee 100644
> --- a/net/socket.c
> +++ b/net/socket.c
> @@ -296,7 +296,6 @@ static void init_inodecache(void)
> 
> SLAB_RECLAIM_ACCOUNT |
>SLAB_MEM_SPREAD
> | SLAB_ACCOUNT),
>   init_once);
> -   BUG_ON(sock_inode_cachep == NULL);
>  }
> 
>  static const struct super_operations sockfs_ops = {
> 
> On Thu, Aug 10, 2017 at 1:49 PM, David Miller 
> wrote:
> > From: Tonghao Zhang 
> > Date: Wed,  9 Aug 2017 05:04:38 -0700
> >
> >> When initializing the skbuff SLAB cache, we should make
> >> sure it is successful. Adding BUG_ON to check it and
> >> init_inodecache() is in the same case.
> >>
> >> Signed-off-by: Tonghao Zhang 
> >> ---
> >>  net/core/skbuff.c | 2 ++
> >>  1 file changed, 2 insertions(+)
> >>
> >> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> >> index 42b62c716a33..9513de519870 100644
> >> --- a/net/core/skbuff.c
> >> +++ b/net/core/skbuff.c
> >> @@ -3904,6 +3904,8 @@ void __init skb_init(void)
> >>   0,
> >>
> SLAB_HWCACHE_ALIGN|SLAB_PANIC,
> >>   NULL);
> >> + BUG_ON(skbuff_head_cache == NULL);
> >> + BUG_ON(skbuff_fclone_cache == NULL);
> >>  }
> >
> > I know you guys want every allocation to be explicitly checked so that
> > everything is consistent for static code analysis checkers.
> >
> > But this is just wasted code.
> >
> > The first allocation will take a NULL dereference and the backtrace
> > will make it completely clear which SLAB cache was NULL and couldn't
> > be allocated.
> >
> > So there is no real value to adding these checks.
> >
> > So I'm not applying this, sorry.
> >
> > The same logic goes for your other patch of this nature.
> >