* RE: [PATCH] bnx2: turn off the network statck during initialization
From: Michael Chan @ 2012-08-16 19:28 UTC (permalink / raw)
To: Jiang Wang
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Chaitanya Lala, Francis St. Amant
In-Reply-To: <CEE05A0D7856E14880AD5560634EBEA834DB2ABC@SFO1EXC-MBXP06.nbttech.com>
On Thu, 2012-08-16 at 19:15 +0000, Jiang Wang wrote:
> Hi Michael,
>
> I just checked the code for netif_carrier_off(), and it will return if
> the dev is not initialized (registered) as follows:
>
> void netif_carrier_off(struct net_device *dev)
> {
> if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
> if (dev->reg_state == NETREG_UNINITIALIZED)
> return;
> linkwatch_fire_event(dev);
> }
> }
>
> So linkwatch_fire_event should not fire in this case, right? Thanks
> for the quick response.
>
Yes, you're right. Your patch should not cause any problem then.
But I still don't understand what problem you're trying to solve. The
link status from ethtool is determined by bnx2_get_link() and it should
always return link down before bnx2_open() is called. Can you please
elborate? Thanks.
> Regards,
>
> Jiang
>
> -------------------------------------
> Jiang Wang
> Member of Technical Staff
> Riverbed Technology
> Tel: (408) 522-5109
> Email: Jiang.Wang@riverbed.com
> www.riverbed.com
>
>
> -----Original Message-----
> From: Michael Chan [mailto:mchan@broadcom.com]
> Sent: Thursday, August 16, 2012 11:57 AM
> To: Jiang Wang
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Chaitanya Lala; Francis St. Amant; Jiang Wang
> Subject: Re: [PATCH] bnx2: turn off the network statck during initialization
>
> On Thu, 2012-08-16 at 11:21 -0700, Jiang Wang wrote:
> > The initialization state of bnx2 driver is wrong. It does not turn of
> > the Linux network stack using netif_carrier_off. This may lead to
> > inconsistent report from ethtool as the link is up but speed is
> > unknown when the cable is not plugged in.
> >
> > E.g.
> > Speed: Unknown! (0)<--------------------------------------
> > Duplex: Half <--------------------------------------
> > MDI: Unknown! (0)
> > Port: Twisted Pair
> > PHYAD: 1
> > Transceiver: internal
> > Auto-negotiation: on
> > Supports Wake-on: g
> > Wake-on: d
> > Link detected: yes <---------------------------------------
> >
> > This patches fixed the problem by turning off the network stack during
> > initialization.
> >
> > Signed-off-by: Jiang Wang <jwang@riverbed.com>
> > ---
> > drivers/net/ethernet/broadcom/bnx2.c | 4 ++++
> > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnx2.c
> > b/drivers/net/ethernet/broadcom/bnx2.c
> > index ac7b744..ce4548d 100644
> > --- a/drivers/net/ethernet/broadcom/bnx2.c
> > +++ b/drivers/net/ethernet/broadcom/bnx2.c
> > @@ -8463,6 +8463,10 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> > dev->features |= dev->hw_features;
> > dev->priv_flags |= IFF_UNICAST_FLT;
> >
> > + /* tell the stack to leave us alone until bnx2_open() is called */
> > + netif_carrier_off(dev);
>
> We have tried this before and this didn't work. netif_carrier_off() calls linkwatch_fire_event() to schedule the event. If
> register_netdev() fails for whatever reason, the netdev will be freed but the link_watch event may still be scheduled.
>
> Calling netif_carrier_off() after register_netdev() returns successfully also may not work as it will race with bnx2_open() which enables IRQ.
> An linkup IRQ can happen at time and we may end up with the wrong carrier state because of the race condition.
>
> > + netif_stop_queue(dev);
> > +
> > if ((rc = register_netdev(dev))) {
> > dev_err(&pdev->dev, "Cannot register net device\n");
> > goto error;
>
>
>
>
^ permalink raw reply
* Re: [PATCH net-next] sctp: fix compile issue with disabled CONFIG_NET_NS
From: Ben Hutchings @ 2012-08-16 19:36 UTC (permalink / raw)
To: Eric W. Biederman; +Cc: Ulrich Weber, davem, netdev
In-Reply-To: <87fw7m64v9.fsf@xmission.com>
On Thu, 2012-08-16 at 11:58 -0700, Eric W. Biederman wrote:
> Ulrich Weber <ulrich.weber@sophos.com> writes:
>
> > struct seq_net_private has no struct net
> > if CONFIG_NET_NS is not enabled
>
> My mistake.
>
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
> > Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
>
> Ulrich do you get any performance advantage by disabling the network
> namespace?
>
> I am wondering if there is any benefit to keeping it possible to disable
> the network namespace?
>
> The original reason for the option was so that distributions and
> other who wanted to avoid new code could protect their users, and
> that reasons seems to have long since passed.
[...]
Network namespaces are now used by common applications such as Chrome/
Chromium and vsftpd (which flushed out some of the early implementation
problems). They undoubtedly have some overhead that some users would
like to avoid, but I wouldn't expect distributions to disable the
option.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [PATCH net-next] sctp: fix compile issue with disabled CONFIG_NET_NS
From: Eric W. Biederman @ 2012-08-16 20:00 UTC (permalink / raw)
To: Ben Hutchings; +Cc: Ulrich Weber, davem, netdev
In-Reply-To: <1345145802.2832.6.camel@bwh-desktop.uk.solarflarecom.com>
Ben Hutchings <bhutchings@solarflare.com> writes:
> On Thu, 2012-08-16 at 11:58 -0700, Eric W. Biederman wrote:
>> Ulrich Weber <ulrich.weber@sophos.com> writes:
>>
>> > struct seq_net_private has no struct net
>> > if CONFIG_NET_NS is not enabled
>>
>> My mistake.
>>
>> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
>>
>> > Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
>>
>> Ulrich do you get any performance advantage by disabling the network
>> namespace?
>>
>> I am wondering if there is any benefit to keeping it possible to disable
>> the network namespace?
>>
>> The original reason for the option was so that distributions and
>> other who wanted to avoid new code could protect their users, and
>> that reasons seems to have long since passed.
> [...]
>
> Network namespaces are now used by common applications such as Chrome/
> Chromium and vsftpd (which flushed out some of the early implementation
> problems). They undoubtedly have some overhead that some users would
> like to avoid, but I wouldn't expect distributions to disable the
> option.
When the network namespace code was merged we could not mearsure the
overhead, at least not on gigabit ethernet. And there is very little
code change when network namespaces go away.
So I am starting to look for people who care. Especially for people who
can measure overhead associated with network namespaces.
Eric
^ permalink raw reply
* Re: [PATCH v0 0/5] cgroup: Assign subsystem IDs during compile time
From: Daniel Wagner @ 2012-08-16 20:25 UTC (permalink / raw)
To: Neil Horman
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, cgroups-u79uwXL29TY76Z2rM5mHXA,
Daniel Wagner, David S. Miller, Andrew Morton, Eric Dumazet,
Gao feng, Glauber Costa, Jamal Hadi Salim, John Fastabend,
Kamezawa Hiroyuki, Li Zefan, Tejun Heo
In-Reply-To: <20120816190542.GA8203-B26myB8xz7F8NnZeBjwnZQMhkBWG/bsMQH7oEaQurus@public.gmane.org>
Hi Neil,
On 08/16/2012 09:05 PM, Neil Horman wrote:
>> Daniel Wagner (5):
>> cgroup: Use empty task_cls_classid() when !CONFIG_NET_CLS(_MODULE)
>> cgroup: Move sock_update_classid() decleration to cls_cgroup.h
>> cgroup: Protect access to task_cls_classid() when built as module
>> cgroup: Protect access to task_netprioidx() when built as module
>> cgroup: Assign subsystem IDs during compile time
>
> The series seems reasonable. I presume you've testing building and running both
> net_prio and net_cls as modules and monolithically?
> Neil
Yep, I spend a good bit of time rebuilding and testing all patches in
the different configuration. I hope I really got the small semantic
differences between net_cls and net_prio regarding the loading and
unloading correct. So please have a close look at the jump label
patches (#3 and #4).
BTW, I have a few mores on top of these patches, e.g. merging the
builtin and module version of task_cls_classid()/task_netprioidx()
implementation together and getting rid of the many ifdefs in the
header. But let's first get this part reviewed.
thanks,
daniel
^ permalink raw reply
* RE: [PATCH] bnx2: turn off the network statck during initialization
From: Jiang Wang @ 2012-08-16 20:28 UTC (permalink / raw)
To: Michael Chan
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Chaitanya Lala, Francis St. Amant
In-Reply-To: <1345145318.6916.42.camel@LTIRV-MCHAN1.corp.ad.broadcom.com>
OK. I see what you mean. In fact, I am working on an old RHEL based kernel and it still reports the link via ethtool_op_get_link. That is why I got the inconsistent report from the ethtool as I described in the patch. (the ethtool showed the link is up, but speed is unknown, duplex is half).
I guess I can just port in the patch for bnx2_get_link() to solve this problem.
Also, I have another comment related to link state.
Right now, the bnx2 driver powers up the device in bnx2_init_board(), regardless the netif_carrier is on or off. This may introduce following inconsistent behaviors:
1) suppose the cable is plugged in to the NIC and the other end is connected to a switch
2) user powers up the box
3) the Linux does not bring up the interface; i.e, ifconfig ethx shows it is down
4) ethtool ethx will show no link
5) if the user goes to check the light on the physical NIC, he will see the green link light is ON. That means the link is up, right?
I think it is better to power down the device until bnx2_open is called. In this way, ethtool report and the physical link light will be consistent.
Any suggestions? Thanks.
Regards,
Jiang
-----Original Message-----
From: Michael Chan [mailto:mchan@broadcom.com]
Sent: Thursday, August 16, 2012 12:29 PM
To: Jiang Wang
Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Chaitanya Lala; Francis St. Amant
Subject: RE: [PATCH] bnx2: turn off the network statck during initialization
On Thu, 2012-08-16 at 19:15 +0000, Jiang Wang wrote:
> Hi Michael,
>
> I just checked the code for netif_carrier_off(), and it will return if
> the dev is not initialized (registered) as follows:
>
> void netif_carrier_off(struct net_device *dev) {
> if (!test_and_set_bit(__LINK_STATE_NOCARRIER, &dev->state)) {
> if (dev->reg_state == NETREG_UNINITIALIZED)
> return;
> linkwatch_fire_event(dev);
> }
> }
>
> So linkwatch_fire_event should not fire in this case, right? Thanks
> for the quick response.
>
Yes, you're right. Your patch should not cause any problem then.
But I still don't understand what problem you're trying to solve. The link status from ethtool is determined by bnx2_get_link() and it should always return link down before bnx2_open() is called. Can you please elborate? Thanks.
> Regards,
>
> Jiang
>
> -------------------------------------
> Jiang Wang
> Member of Technical Staff
> Riverbed Technology
> Tel: (408) 522-5109
> Email: Jiang.Wang@riverbed.com
> www.riverbed.com
>
>
> -----Original Message-----
> From: Michael Chan [mailto:mchan@broadcom.com]
> Sent: Thursday, August 16, 2012 11:57 AM
> To: Jiang Wang
> Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; Chaitanya
> Lala; Francis St. Amant; Jiang Wang
> Subject: Re: [PATCH] bnx2: turn off the network statck during
> initialization
>
> On Thu, 2012-08-16 at 11:21 -0700, Jiang Wang wrote:
> > The initialization state of bnx2 driver is wrong. It does not turn
> > of the Linux network stack using netif_carrier_off. This may lead to
> > inconsistent report from ethtool as the link is up but speed is
> > unknown when the cable is not plugged in.
> >
> > E.g.
> > Speed: Unknown! (0)<--------------------------------------
> > Duplex: Half <--------------------------------------
> > MDI: Unknown! (0)
> > Port: Twisted Pair
> > PHYAD: 1
> > Transceiver: internal
> > Auto-negotiation: on
> > Supports Wake-on: g
> > Wake-on: d
> > Link detected: yes <---------------------------------------
> >
> > This patches fixed the problem by turning off the network stack
> > during initialization.
> >
> > Signed-off-by: Jiang Wang <jwang@riverbed.com>
> > ---
> > drivers/net/ethernet/broadcom/bnx2.c | 4 ++++
> > 1 files changed, 4 insertions(+), 0 deletions(-)
> >
> > diff --git a/drivers/net/ethernet/broadcom/bnx2.c
> > b/drivers/net/ethernet/broadcom/bnx2.c
> > index ac7b744..ce4548d 100644
> > --- a/drivers/net/ethernet/broadcom/bnx2.c
> > +++ b/drivers/net/ethernet/broadcom/bnx2.c
> > @@ -8463,6 +8463,10 @@ bnx2_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
> > dev->features |= dev->hw_features;
> > dev->priv_flags |= IFF_UNICAST_FLT;
> >
> > + /* tell the stack to leave us alone until bnx2_open() is called */
> > + netif_carrier_off(dev);
>
> We have tried this before and this didn't work. netif_carrier_off()
> calls linkwatch_fire_event() to schedule the event. If
> register_netdev() fails for whatever reason, the netdev will be freed but the link_watch event may still be scheduled.
>
> Calling netif_carrier_off() after register_netdev() returns successfully also may not work as it will race with bnx2_open() which enables IRQ.
> An linkup IRQ can happen at time and we may end up with the wrong carrier state because of the race condition.
>
> > + netif_stop_queue(dev);
> > +
> > if ((rc = register_netdev(dev))) {
> > dev_err(&pdev->dev, "Cannot register net device\n");
> > goto error;
>
>
>
>
^ permalink raw reply
* Re: [RFC v3 1/1] net/hsr: Add support for IEC 62439-3 High-availability Seamless Redundancy
From: David Miller @ 2012-08-16 20:30 UTC (permalink / raw)
To: Arvid.Brodin; +Cc: netdev, joe, shemminger, kuznet, jboticario, balferreira
In-Reply-To: <502D475C.9090208@xdin.com>
This is not the correct way to post a patch.
You should provide a proper full Subject: and commit message text in
the body of the email before the patch itself.
^ permalink raw reply
* Re: [PATCH net-next] sctp: fix compile issue with disabled CONFIG_NET_NS
From: David Miller @ 2012-08-16 20:36 UTC (permalink / raw)
To: ebiederm; +Cc: ulrich.weber, netdev
In-Reply-To: <87fw7m64v9.fsf@xmission.com>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 16 Aug 2012 11:58:34 -0700
> Ulrich Weber <ulrich.weber@sophos.com> writes:
>
>> struct seq_net_private has no struct net
>> if CONFIG_NET_NS is not enabled
>
> My mistake.
>
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
>
>> Signed-off-by: Ulrich Weber <ulrich.weber@sophos.com>
Applied.
^ permalink raw reply
* Re: [patch] sctp: fix bogus if statement in sctp_auth_recv_cid()
From: David Miller @ 2012-08-16 20:37 UTC (permalink / raw)
To: ebiederm; +Cc: dan.carpenter, vyasevich, sri, linux-sctp, netdev,
kernel-janitors
In-Reply-To: <87txw266hk.fsf@xmission.com>
From: ebiederm@xmission.com (Eric W. Biederman)
Date: Thu, 16 Aug 2012 11:23:35 -0700
> Dan Carpenter <dan.carpenter@oracle.com> writes:
>
>> There is an extra semi-colon here, so we always return 0 instead of
>> calling __sctp_auth_cid().
>
> Doh.
>
> Reviewed-by: "Eric W. Biederman" <ebiederm@xmission.com>
Applied.
^ permalink raw reply
* $650,500.00 USD.
From: United Nations Trust Funds @ 2012-08-16 21:08 UTC (permalink / raw)
To: Recipients
Reply with this details to claim.
Full Name:.....................
Address:.......................
Phone Number:..................
Country:......................
^ permalink raw reply
* RE: [PATCH] bnx2: turn off the network statck during initialization
From: Michael Chan @ 2012-08-16 21:13 UTC (permalink / raw)
To: Jiang Wang
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Chaitanya Lala, Francis St. Amant
In-Reply-To: <CEE05A0D7856E14880AD5560634EBEA834DB2B40@SFO1EXC-MBXP06.nbttech.com>
On Thu, 2012-08-16 at 20:28 +0000, Jiang Wang wrote:
> Also, I have another comment related to link state.
>
> Right now, the bnx2 driver powers up the device in bnx2_init_board(),
> regardless the netif_carrier is on or off.
We actually don't power up the device. bnx2_init_board() just probes
the device. If link is already up, it will stay up. If it is down, it
will stay down.
> This may introduce following inconsistent behaviors:
> 1) suppose the cable is plugged in to the NIC and the other end is
> connected to a switch
> 2) user powers up the box
The link may already be up before or when you power up the box because
of management firmware (iLO, etc) or WoL.
> 3) the Linux does not bring up the interface; i.e, ifconfig ethx shows
> it is down
> 4) ethtool ethx will show no link
> 5) if the user goes to check the light on the physical NIC, he will
> see the green link light is ON. That means the link is up, right?
>
> I think it is better to power down the device until bnx2_open is
> called. In this way, ethtool report and the physical link light will
> be consistent.
We cannot power it down. If link is up, it is up for a reason (e.g. it
is an iLO port, etc).
Thanks.
^ permalink raw reply
* Re: [RFC v3 1/1] net/hsr: Add support for IEC 62439-3 High-availability Seamless Redundancy
From: Arvid Brodin @ 2012-08-16 21:16 UTC (permalink / raw)
To: David Miller
Cc: netdev@vger.kernel.org, joe@perches.com, shemminger@vyatta.com,
kuznet@ms2.inr.ac.ru, jboticario@gmail.com,
balferreira@googlemail.com
In-Reply-To: <20120816.133036.1422214989121265534.davem@davemloft.net>
On 2012-08-16 22:30, David Miller wrote:
>
> This is not the correct way to post a patch.
>
> You should provide a proper full Subject: and commit message text in
> the body of the email before the patch itself.
I fail to see what's missing from the Subject: line; please elaborate.
(Please note that I posted this as an RFC since there's still FIXMEs in the code, and
there may still be issues pointed out by Stephen Hemminger to be solved. I'm not sure
it's ready to go into the kernel just yet.)
--
Arvid Brodin | Consultant (Linux)
XDIN AB | Jan Stenbecks Torg 17 | SE-164 40 Kista | Sweden | xdin.com
^ permalink raw reply
* RE: [PATCH] bnx2: turn off the network statck during initialization
From: Ben Hutchings @ 2012-08-16 21:25 UTC (permalink / raw)
To: Jiang Wang
Cc: Michael Chan, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org, Chaitanya Lala, Francis St. Amant
In-Reply-To: <CEE05A0D7856E14880AD5560634EBEA834DB2B40@SFO1EXC-MBXP06.nbttech.com>
On Thu, 2012-08-16 at 20:28 +0000, Jiang Wang wrote:
[...]
> Also, I have another comment related to link state.
>
> Right now, the bnx2 driver powers up the device in bnx2_init_board(),
> regardless the netif_carrier is on or off. This may introduce
> following inconsistent behaviors:
> 1) suppose the cable is plugged in to the NIC and the other end is
> connected to a switch
> 2) user powers up the box
> 3) the Linux does not bring up the interface; i.e, ifconfig ethx shows
> it is down
Most distributions will bring up all interfaces at boot, by default.
> 4) ethtool ethx will show no link
> 5) if the user goes to check the light on the physical NIC, he will
> see the green link light is ON. That means the link is up, right?
>
> I think it is better to power down the device until bnx2_open is
> called. In this way, ethtool report and the physical link light will
> be consistent.
[...]
In general, a physical network interface may need to be enabled to allow
management traffic to a BMC, even when the interface is in the 'down'
state on the host.
The link state should be considered unknown whenever the interface is
down, and /sys/class/net/$IFACE/carrier is not readable then. However
ETHTOOL_GLINK is not expected to fail in this way, and must always
return 0 (down) or 1 (up). The convention (which is now enforced in
ethtool_get_link()) is that when the interface is down it should always
return 0.
Ben.
--
Ben Hutchings, Staff Engineer, Solarflare
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.
^ permalink raw reply
* Re: [RFC v3 1/1] net/hsr: Add support for IEC 62439-3 High-availability Seamless Redundancy
From: David Miller @ 2012-08-16 21:46 UTC (permalink / raw)
To: Arvid.Brodin; +Cc: netdev, joe, shemminger, kuznet, jboticario, balferreira
In-Reply-To: <502D6332.1030303@xdin.com>
From: Arvid Brodin <Arvid.Brodin@xdin.com>
Date: Thu, 16 Aug 2012 21:16:34 +0000
> On 2012-08-16 22:30, David Miller wrote:
>>
>> This is not the correct way to post a patch.
>>
>> You should provide a proper full Subject: and commit message text in
>> the body of the email before the patch itself.
>
> I fail to see what's missing from the Subject: line; please elaborate.
You left in the "Re: " etc. prefix. You should never post new versions
of a patch as a reply, you should make new, fresh, mailing list postings.
And you failed to include the commit message body. Everything you seem
to thing belongs in the "0/1" posting, at a minimum actually belongs
here in 1/1 too.
^ permalink raw reply
* RE: [PATCH] bnx2: turn off the network statck during initialization
From: Jiang Wang @ 2012-08-16 21:48 UTC (permalink / raw)
To: Michael Chan
Cc: netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
Chaitanya Lala, Francis St. Amant
In-Reply-To: <1345151593.6916.62.camel@LTIRV-MCHAN1.corp.ad.broadcom.com>
On Thu, 2012-08-16 at 20:28 +0000, Jiang Wang wrote:
> Also, I have another comment related to link state.
>
> Right now, the bnx2 driver powers up the device in bnx2_init_board(),
> regardless the netif_carrier is on or off.
We actually don't power up the device. bnx2_init_board() just probes the device. If link is already up, it will stay up. If it is down, it will stay down.
---- I see. I was confused by the name of bnx2_set_power_state() and I think the PHY is actually powered up by bnx2_reset_nic, right?
> This may introduce following inconsistent behaviors:
> 1) suppose the cable is plugged in to the NIC and the other end is
> connected to a switch
> 2) user powers up the box
The link may already be up before or when you power up the box because of management firmware (iLO, etc) or WoL.
---- OK.
> 3) the Linux does not bring up the interface; i.e, ifconfig ethx shows
> it is down
> 4) ethtool ethx will show no link
> 5) if the user goes to check the light on the physical NIC, he will
> see the green link light is ON. That means the link is up, right?
>
> I think it is better to power down the device until bnx2_open is
> called. In this way, ethtool report and the physical link light will
> be consistent.
We cannot power it down. If link is up, it is up for a reason (e.g. it is an iLO port, etc).
---- OK.
Thanks.
^ permalink raw reply
* Re: [RFC PATCH v1.0] af_packet: don't emit packet on orig fanout group
From: David Miller @ 2012-08-16 21:52 UTC (permalink / raw)
To: eric; +Cc: netdev
In-Reply-To: <1345131890-15991-1-git-send-email-eric@regit.org>
From: Eric Leblond <eric@regit.org>
Date: Thu, 16 Aug 2012 17:44:50 +0200
> If a packet is emitted on one socket in one group of fanout sockets,
> it is transmitted again. It is thus read again on one of the sockets
> of the fanout group. This result in a loop for software which
> generate packets when receiving one.
> This retransmission is not the intended behavior: a fanout group
> must behave like a single socket. The packet should not be
> transmitted on a socket if it originates from a socket belonging
> to the same fanout group.
>
> This patch fixes the issue by changing the transmission check to
> take fanout group info account.
This looks mostly fine, thanks for fixing this.
I wonder if it wouldn't be better to simply have a callback? That
would eliminate all of the ifdefs:
if (ptype->id_match) {
if (ptype->id_match(ptype, skb->sk))
return true;
} else if (ptype->af_packet_priv == skb->sk)
return true;
It's a shame that we have a user of af_packet_priv outside of
AF_PACKET, in TIPC. If we could get rid of that we could simplify
things even futher.
^ permalink raw reply
* Re: [RFC PATCH v1.0] af_packet: don't emit packet on orig fanout group
From: David Miller @ 2012-08-16 21:52 UTC (permalink / raw)
To: eric; +Cc: netdev
In-Reply-To: <20120816.145200.694410419765575459.davem@davemloft.net>
BTW, please also give "Aleksandr Kotov <a1k@mail.ru>" credit for
reporting this problem recently with a Reported-by tag, thanks.
^ permalink raw reply
* Re: AF_PACKET + PACKET_FANOUT: tx packet delivered back in rx
From: David Miller @ 2012-08-16 21:53 UTC (permalink / raw)
To: a1k; +Cc: netdev, miquels
In-Reply-To: <1342081817.394175102@f168.mail.ru>
From: Aleksandr Kotov <a1k@mail.ru>
Date: Thu, 12 Jul 2012 12:30:17 +0400
> I have a problem using AF_PACKET socket with PACKET_RX_RING, PACKET_TX_RING and PACKET_FANOUT options enabled.
> I am seeing my TX packets get back to the same socket in RX ring. Socket is created and gets bind with ETH_P_ALL protocol type. The problem is in the fanout_add function
> which sets af_packet_priv to "match" variable, but not sk one describing socket and in dev_queue_xmit_nit skb get bounced back to the socket despite the comment left in the function.
Eric Leblong is posting fixes for this problem recently:
http://patchwork.ozlabs.org/patch/178044/
^ permalink raw reply
* Re: [net V2] act_mirred: do not drop packets when fails to mirror it
From: David Miller @ 2012-08-16 21:55 UTC (permalink / raw)
To: jhs; +Cc: jasowang, netdev, linux-kernel
In-Reply-To: <1345119107.5398.0.camel@mojatatu>
From: Jamal Hadi Salim <jhs@mojatatu.com>
Date: Thu, 16 Aug 2012 08:11:47 -0400
> On Thu, 2012-08-16 at 14:44 +0800, Jason Wang wrote:
>> We drop packet unconditionally when we fail to mirror it. This is not intended
>> in some cases. Consdier for kvm guest, we may mirror the traffic of the bridge
>> to a tap device used by a VM. When kernel fails to mirror the packet in
>> conditions such as when qemu crashes or stop polling the tap, it's hard for the
>> management software to detect such condition and clean the the mirroring
>> before. This would lead all packets to the bridge to be dropped and break the
>> netowrk of other virtual machines.
>>
>> To solve the issue, the patch does not drop packets when kernel fails to mirror
>> it, and only drop the redirected packets.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Signed-off-by: Jamal Hadi Salim <jhs@mojatatu.com>
Applied, thanks.
^ permalink raw reply
* Re: [net PATCH v3 1/3] net: netprio: fix files lock and remove useless d_path bits
From: David Miller @ 2012-08-16 22:11 UTC (permalink / raw)
To: john.r.fastabend; +Cc: viro, nhorman, netdev, linux-kernel, joe
In-Reply-To: <20120814223424.3551.21608.stgit@jf-dev1-dcblab>
From: John Fastabend <john.r.fastabend@intel.com>
Date: Tue, 14 Aug 2012 15:34:24 -0700
> Add lock to prevent a race with a file closing and also remove
> useless and ugly sscanf code. The extra code was never needed
> and the case it supposedly protected against is in fact handled
> correctly by sock_from_file as pointed out by Al Viro.
>
> CC: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Applied.
^ permalink raw reply
* Re: [net PATCH v3 2/3] net: netprio: fd passed in SCM_RIGHTS datagram not set correctly
From: David Miller @ 2012-08-16 22:11 UTC (permalink / raw)
To: john.r.fastabend; +Cc: viro, nhorman, netdev, linux-kernel, joe
In-Reply-To: <20120814223430.3551.88909.stgit@jf-dev1-dcblab>
From: John Fastabend <john.r.fastabend@intel.com>
Date: Tue, 14 Aug 2012 15:34:30 -0700
> A socket fd passed in a SCM_RIGHTS datagram was not getting
> updated with the new tasks cgrp prioidx. This leaves IO on
> the socket tagged with the old tasks priority.
>
> To fix this add a check in the scm recvmsg path to update the
> sock cgrp prioidx with the new tasks value.
>
> Thanks to Al Viro for catching this.
>
> CC: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Applied.
^ permalink raw reply
* Re: [net PATCH v3 3/3] net: netprio: fix cgrp create and write priomap race
From: David Miller @ 2012-08-16 22:11 UTC (permalink / raw)
To: john.r.fastabend; +Cc: viro, nhorman, netdev, linux-kernel, joe
In-Reply-To: <20120814223435.3551.62387.stgit@jf-dev1-dcblab>
From: John Fastabend <john.r.fastabend@intel.com>
Date: Tue, 14 Aug 2012 15:34:35 -0700
> A race exists where creating cgroups and also updating the priomap
> may result in losing a priomap update. This is because priomap
> writers are not protected by rtnl_lock.
>
> Move priority writer into rtnl_lock()/rtnl_unlock().
>
> CC: Neil Horman <nhorman@tuxdriver.com>
> Reported-by: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Applied.
^ permalink raw reply
* [PATCH net-next 2/9] tipc: fix lockdep warning during bearer initialization
From: Paul Gortmaker @ 2012-08-16 22:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Jon Maloy, Ying Xue, Paul Gortmaker
In-Reply-To: <1345154954-12526-1-git-send-email-paul.gortmaker@windriver.com>
From: Ying Xue <ying.xue@windriver.com>
When the lockdep validator is enabled, it will report the below
warning when we enable a TIPC bearer:
[ INFO: possible irq lock inversion dependency detected ]
---------------------------------------------------------
Possible interrupt unsafe locking scenario:
CPU0 CPU1
---- ----
lock(ptype_lock);
local_irq_disable();
lock(tipc_net_lock);
lock(ptype_lock);
<Interrupt>
lock(tipc_net_lock);
*** DEADLOCK ***
the shortest dependencies between 2nd lock and 1st lock:
-> (ptype_lock){+.+...} ops: 10 {
[...]
SOFTIRQ-ON-W at:
[<c1089418>] __lock_acquire+0x528/0x13e0
[<c108a360>] lock_acquire+0x90/0x100
[<c1553c38>] _raw_spin_lock+0x38/0x50
[<c14651ca>] dev_add_pack+0x3a/0x60
[<c182da75>] arp_init+0x1a/0x48
[<c182dce5>] inet_init+0x181/0x27e
[<c1001114>] do_one_initcall+0x34/0x170
[<c17f7329>] kernel_init+0x110/0x1b2
[<c155b6a2>] kernel_thread_helper+0x6/0x10
[...]
... key at: [<c17e4b10>] ptype_lock+0x10/0x20
... acquired at:
[<c108a360>] lock_acquire+0x90/0x100
[<c1553c38>] _raw_spin_lock+0x38/0x50
[<c14651ca>] dev_add_pack+0x3a/0x60
[<c8bc18d2>] enable_bearer+0xf2/0x140 [tipc]
[<c8bb283a>] tipc_enable_bearer+0x1ba/0x450 [tipc]
[<c8bb3a04>] tipc_cfg_do_cmd+0x5c4/0x830 [tipc]
[<c8bbc032>] handle_cmd+0x42/0xd0 [tipc]
[<c148e802>] genl_rcv_msg+0x232/0x280
[<c148d3f6>] netlink_rcv_skb+0x86/0xb0
[<c148e5bc>] genl_rcv+0x1c/0x30
[<c148d144>] netlink_unicast+0x174/0x1f0
[<c148ddab>] netlink_sendmsg+0x1eb/0x2d0
[<c1456bc1>] sock_aio_write+0x161/0x170
[<c1135a7c>] do_sync_write+0xac/0xf0
[<c11360f6>] vfs_write+0x156/0x170
[<c11361e2>] sys_write+0x42/0x70
[<c155b0df>] sysenter_do_call+0x12/0x38
[...]
}
-> (tipc_net_lock){+..-..} ops: 4 {
[...]
IN-SOFTIRQ-R at:
[<c108953a>] __lock_acquire+0x64a/0x13e0
[<c108a360>] lock_acquire+0x90/0x100
[<c15541cd>] _raw_read_lock_bh+0x3d/0x50
[<c8bb874d>] tipc_recv_msg+0x1d/0x830 [tipc]
[<c8bc195f>] recv_msg+0x3f/0x50 [tipc]
[<c146a5fa>] __netif_receive_skb+0x22a/0x590
[<c146ab0b>] netif_receive_skb+0x2b/0xf0
[<c13c43d2>] pcnet32_poll+0x292/0x780
[<c146b00a>] net_rx_action+0xfa/0x1e0
[<c103a4be>] __do_softirq+0xae/0x1e0
[...]
}
>From the log, we can see three different call chains between
CPU0 and CPU1:
Time 0 on CPU0:
kernel_init()->inet_init()->dev_add_pack()
At time 0, the ptype_lock is held by CPU0 in dev_add_pack();
Time 1 on CPU1:
tipc_enable_bearer()->enable_bearer()->dev_add_pack()
At time 1, tipc_enable_bearer() first holds tipc_net_lock, and then
wants to take ptype_lock to register TIPC protocol handler into the
networking stack. But the ptype_lock has been taken by dev_add_pack()
on CPU0, so at this time the dev_add_pack() running on CPU1 has to be
busy looping.
Time 2 on CPU0:
netif_receive_skb()->recv_msg()->tipc_recv_msg()
At time 2, an incoming TIPC packet arrives at CPU0, hence
tipc_recv_msg() will be invoked. In tipc_recv_msg(), it first wants
to hold tipc_net_lock. At the moment, below scenario happens:
On CPU0, below is our sequence of taking locks:
lock(ptype_lock)->lock(tipc_net_lock)
On CPU1, our sequence of taking locks looks like:
lock(tipc_net_lock)->lock(ptype_lock)
Obviously deadlock may happen in this case.
But please note the deadlock possibly doesn't occur at all when the
first TIPC bearer is enabled. Before enable_bearer() -- running on
CPU1 does not hold ptype_lock, so the TIPC receive handler (i.e.
recv_msg()) is not registered successfully via dev_add_pack(), so
the tipc_recv_msg() cannot be called by recv_msg() even if a TIPC
message comes to CPU0. But when the second TIPC bearer is
registered, the deadlock can perhaps really happen.
To fix it, we will push the work of registering TIPC protocol
handler into workqueue context. After the change, both paths taking
ptype_lock are always in process contexts, thus, the deadlock should
never occur.
Signed-off-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
net/tipc/eth_media.c | 16 +++++++++++++++-
1 file changed, 15 insertions(+), 1 deletion(-)
diff --git a/net/tipc/eth_media.c b/net/tipc/eth_media.c
index 0f312c2..2132c1e 100644
--- a/net/tipc/eth_media.c
+++ b/net/tipc/eth_media.c
@@ -46,12 +46,14 @@
* @bearer: ptr to associated "generic" bearer structure
* @dev: ptr to associated Ethernet network device
* @tipc_packet_type: used in binding TIPC to Ethernet driver
+ * @setup: work item used when enabling bearer
* @cleanup: work item used when disabling bearer
*/
struct eth_bearer {
struct tipc_bearer *bearer;
struct net_device *dev;
struct packet_type tipc_packet_type;
+ struct work_struct setup;
struct work_struct cleanup;
};
@@ -143,6 +145,17 @@ static int recv_msg(struct sk_buff *buf, struct net_device *dev,
}
/**
+ * setup_bearer - setup association between Ethernet bearer and interface
+ */
+static void setup_bearer(struct work_struct *work)
+{
+ struct eth_bearer *eb_ptr =
+ container_of(work, struct eth_bearer, setup);
+
+ dev_add_pack(&eb_ptr->tipc_packet_type);
+}
+
+/**
* enable_bearer - attach TIPC bearer to an Ethernet interface
*/
static int enable_bearer(struct tipc_bearer *tb_ptr)
@@ -182,7 +195,8 @@ static int enable_bearer(struct tipc_bearer *tb_ptr)
eb_ptr->tipc_packet_type.func = recv_msg;
eb_ptr->tipc_packet_type.af_packet_priv = eb_ptr;
INIT_LIST_HEAD(&(eb_ptr->tipc_packet_type.list));
- dev_add_pack(&eb_ptr->tipc_packet_type);
+ INIT_WORK(&eb_ptr->setup, setup_bearer);
+ schedule_work(&eb_ptr->setup);
/* Associate TIPC bearer with Ethernet bearer */
eb_ptr->bearer = tb_ptr;
--
1.7.11.1
^ permalink raw reply related
* [RFC PATCH net-next 0/9] tipc: misc updates for 3.7
From: Paul Gortmaker @ 2012-08-16 22:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Jon Maloy, Ying Xue, Paul Gortmaker
This series gets some more largely trivial things out of
the way. Most interesting are:
1) fix lockdep splat from bearer init by pushing the setup
off to schedule_work.
2) simplification of configuration by removal of a couple of
tuning knobs which used to have low default values.
The remainder are largely innocuous, I think. I did wonder
if there was an alternate/better way to handle the splat though.
I've done my own local testing of this series on today's net-next
commit 2ea214929d601 ("Merge branch 'for-davem' ... wireless-next")
I'll wait a couple of days to allow for any possible feedback and
change requests, and then send a pull request after that.
Thanks,
Paul.
---
Ying Xue (9):
tipc: optimize the initialization of network device notifier
tipc: fix lockdep warning during bearer initialization
tipc: remove pointless name sanity check and tipc_alphabet array
tipc: manually inline single use media_name_valid routine
tipc: change tipc_net_start routine return value type
tipc: convert tipc_nametbl_size type from variable to macro
tipc: add __read_mostly annotations to several global variables
tipc: eliminate configuration for maximum number of name subscriptions
tipc: eliminate configuration for maximum number of name publications
include/linux/tipc_config.h | 8 ++++----
net/tipc/bearer.c | 21 ++------------------
net/tipc/config.c | 48 +++++----------------------------------------
net/tipc/core.c | 22 +++++++--------------
net/tipc/core.h | 18 ++++++++---------
net/tipc/eth_media.c | 29 +++++++++++++++++++++++----
net/tipc/handler.c | 2 +-
net/tipc/link.c | 4 +---
net/tipc/name_table.c | 14 ++++++-------
net/tipc/net.c | 3 +--
net/tipc/net.h | 2 +-
net/tipc/subscr.c | 4 ++--
12 files changed, 64 insertions(+), 111 deletions(-)
--
1.7.11.1
^ permalink raw reply
* [PATCH net-next 3/9] tipc: remove pointless name sanity check and tipc_alphabet array
From: Paul Gortmaker @ 2012-08-16 22:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Jon Maloy, Ying Xue, Paul Gortmaker
In-Reply-To: <1345154954-12526-1-git-send-email-paul.gortmaker@windriver.com>
From: Ying Xue <ying.xue@windriver.com>
There is no real reason to check whether all letters in the given
media name and network interface name are within the character set
defined in tipc_alphabet array. Even if we eliminate the checking,
the rest of checking conditions in tipc_enable_bearer() can ensure
we do not enable an invalid or illegal bearer.
Signed-off-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
net/tipc/bearer.c | 6 ++----
net/tipc/core.c | 3 ---
net/tipc/core.h | 2 --
net/tipc/link.c | 4 +---
4 files changed, 3 insertions(+), 12 deletions(-)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 09e7124..6b2faa5 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -60,7 +60,7 @@ static int media_name_valid(const char *name)
len = strlen(name);
if ((len + 1) > TIPC_MAX_MEDIA_NAME)
return 0;
- return strspn(name, tipc_alphabet) == len;
+ return 1;
}
/**
@@ -206,9 +206,7 @@ static int bearer_name_validate(const char *name,
/* validate component parts of bearer name */
if ((media_len <= 1) || (media_len > TIPC_MAX_MEDIA_NAME) ||
- (if_len <= 1) || (if_len > TIPC_MAX_IF_NAME) ||
- (strspn(media_name, tipc_alphabet) != (media_len - 1)) ||
- (strspn(if_name, tipc_alphabet) != (if_len - 1)))
+ (if_len <= 1) || (if_len > TIPC_MAX_IF_NAME))
return 0;
/* return bearer name components, if necessary */
diff --git a/net/tipc/core.c b/net/tipc/core.c
index 6586eac..c261a5d 100644
--- a/net/tipc/core.c
+++ b/net/tipc/core.c
@@ -50,9 +50,6 @@
/* global variables used by multiple sub-systems within TIPC */
int tipc_random;
-const char tipc_alphabet[] =
- "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789_.";
-
/* configurable TIPC parameters */
u32 tipc_own_addr;
int tipc_max_ports;
diff --git a/net/tipc/core.h b/net/tipc/core.h
index fd42e10..e4e46cd 100644
--- a/net/tipc/core.h
+++ b/net/tipc/core.h
@@ -85,8 +85,6 @@ extern int tipc_remote_management;
* Other global variables
*/
extern int tipc_random;
-extern const char tipc_alphabet[];
-
/*
* Routines available to privileged subsystems
diff --git a/net/tipc/link.c b/net/tipc/link.c
index 1c1e615..a79c755 100644
--- a/net/tipc/link.c
+++ b/net/tipc/link.c
@@ -210,9 +210,7 @@ static int link_name_validate(const char *name,
(z_local > 255) || (c_local > 4095) || (n_local > 4095) ||
(z_peer > 255) || (c_peer > 4095) || (n_peer > 4095) ||
(if_local_len <= 1) || (if_local_len > TIPC_MAX_IF_NAME) ||
- (if_peer_len <= 1) || (if_peer_len > TIPC_MAX_IF_NAME) ||
- (strspn(if_local, tipc_alphabet) != (if_local_len - 1)) ||
- (strspn(if_peer, tipc_alphabet) != (if_peer_len - 1)))
+ (if_peer_len <= 1) || (if_peer_len > TIPC_MAX_IF_NAME))
return 0;
/* return link name components, if necessary */
--
1.7.11.1
^ permalink raw reply related
* [PATCH net-next 4/9] tipc: manually inline single use media_name_valid routine
From: Paul Gortmaker @ 2012-08-16 22:09 UTC (permalink / raw)
To: David Miller; +Cc: netdev, Jon Maloy, Ying Xue, Paul Gortmaker
In-Reply-To: <1345154954-12526-1-git-send-email-paul.gortmaker@windriver.com>
From: Ying Xue <ying.xue@windriver.com>
After eliminating the mechanism which checks whether all letters
in media name string are within a given character set, the
media_name_valid routine becomes trivial. It is also only
used once, so it is unnecessary to keep it as a separate function.
Signed-off-by: Ying Xue <ying.xue@windriver.com>
Signed-off-by: Jon Maloy <jon.maloy@ericsson.com>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
net/tipc/bearer.c | 17 +----------------
1 file changed, 1 insertion(+), 16 deletions(-)
diff --git a/net/tipc/bearer.c b/net/tipc/bearer.c
index 6b2faa5..4ec5c80 100644
--- a/net/tipc/bearer.c
+++ b/net/tipc/bearer.c
@@ -49,21 +49,6 @@ struct tipc_bearer tipc_bearers[MAX_BEARERS];
static void bearer_disable(struct tipc_bearer *b_ptr);
/**
- * media_name_valid - validate media name
- *
- * Returns 1 if media name is valid, otherwise 0.
- */
-static int media_name_valid(const char *name)
-{
- u32 len;
-
- len = strlen(name);
- if ((len + 1) > TIPC_MAX_MEDIA_NAME)
- return 0;
- return 1;
-}
-
-/**
* tipc_media_find - locates specified media object by name
*/
struct tipc_media *tipc_media_find(const char *name)
@@ -102,7 +87,7 @@ int tipc_register_media(struct tipc_media *m_ptr)
write_lock_bh(&tipc_net_lock);
- if (!media_name_valid(m_ptr->name))
+ if ((strlen(m_ptr->name) + 1) > TIPC_MAX_MEDIA_NAME)
goto exit;
if ((m_ptr->bcast_addr.media_id != m_ptr->type_id) ||
!m_ptr->bcast_addr.broadcast)
--
1.7.11.1
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox