* [PATCH 0/7] Fix sti drivers whcih mix reg address spaces
From: Peter Griffin @ 2014-11-19 8:27 UTC (permalink / raw)
To: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
srinivas.kandagatla-Re5JQEeQqe8AvxtiuMwx3w,
maxime.coquelin-qxv4g6HH51o, patrice.chotard-qxv4g6HH51o,
peppe.cavallaro-qxv4g6HH51o, kishon-l0cyMroinI0, arnd-r2nGTMty4D4
Cc: peter.griffin-QSEj5FYQhm4dnm+yROfE0A,
lee.jones-QSEj5FYQhm4dnm+yROfE0A, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, alexandre.torgue-qxv4g6HH51o
Hi,
Following on from Arnds comments about the picophy driver here
https://lkml.org/lkml/2014/11/13/161, this series fixes the
remaining upstreamed drivers for STI, which are mixing address spaces
in the reg property. We do this in a way similar to the keystone
and bcm7445 platforms, by having sysconfig phandle/ offset pair (
where only one register is required). Or phandle / integer array
where multiple offsets in the same bank are needed).
This series breaks DT compatability! But the platform support
is WIP and only being used by the few developers who are upstreaming
support for it. I've made each change to the driver / dt doc / dt
file as a single atomic commit so the kernel will remain bisectable.
This series then also enables the picophy driver, and adds back in
the ehci/ohci dt nodes for stih410 which make use of the picophy.
regards,
Peter.
Peter Griffin (7):
phy: phy-stih407-usb: Pass sysconfig register offsets via syscfg
property.
phy: miphy365x: Pass sysconfig register offsets via syscfg dt
property.
ARM: STi: DT: STiH407: Add usb2 picophy dt nodes
ARM: STi: DT: STiH410: Add usb2 picophy dt nodes
ARM: STi: DT: STiH410: Add DT nodes for the ehci and ohci usb
controllers.
ARM: multi_v7_defconfig: Enable stih407 usb picophy
stmmac: dwmac-sti: Pass sysconfig register offset via syscon dt
property.
.../devicetree/bindings/net/sti-dwmac.txt | 14 ++---
.../devicetree/bindings/phy/phy-miphy365x.txt | 15 ++---
.../devicetree/bindings/phy/phy-stih407-usb.txt | 10 +---
arch/arm/boot/dts/stih407-family.dtsi | 9 +++
arch/arm/boot/dts/stih410.dtsi | 70 ++++++++++++++++++++++
arch/arm/boot/dts/stih415.dtsi | 12 ++--
arch/arm/boot/dts/stih416.dtsi | 22 +++----
arch/arm/configs/multi_v7_defconfig | 1 +
drivers/net/ethernet/stmicro/stmmac/dwmac-sti.c | 13 ++--
drivers/phy/phy-miphy365x.c | 29 ++++-----
drivers/phy/phy-stih407-usb.c | 25 ++++----
11 files changed, 143 insertions(+), 77 deletions(-)
--
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [BNX2] A Netdev Watchdog with kernel stable 3.4
From: Rui Xiang @ 2014-11-19 8:21 UTC (permalink / raw)
To: Michael Chan, sony.chacko; +Cc: netdev
In-Reply-To: <1416380319.6396.15.camel@LTIRV-MCHAN1.corp.ad.broadcom.com>
Thank you for your comments and attention, Michael.
And welcome Sony's advise. :)
On 2014/11/19 14:58, Michael Chan wrote:
> Copying the current maintainer Sony. The PCI command register looks
> strange. Please see below.
>
> On Wed, 2014-11-19 at 14:28 +0800, Rui Xiang wrote:
>> ping...
>>
>> On 2014/11/17 20:42, Rui Xiang wrote:
>>> Hi Michael,
>>>
>>> On a system that was running stable 3.4.87, I got the below stack.
>>> That was a NETDEV WATCHDOG. And we could also see watchdog timeouts with the
>>> BNX2. (After the stack, an oops occurred while running ifconfig. I think it
>>> would be related to this timeout.)
>>>
>>> Otherwises, the bnx2_dump_state and bnx2_dump_mcp_state have printed the states.
>>> Through these states info, can we got the real situation of NIC1.
>>> Or can we see what resulted the WATCHDOG, a bnx2 device fault or other reasons.
>>>
>>> Thanks.
>>>
>>>
>>> *The stack*:
>>>
>>> WARNING: at /usr/src/packages/BUILD/kernel-default-3.4.87/linux-3.4/net/sched/sch_generic.c:256 dev_watchdog+0x256/0x260()
>>> NETDEV WATCHDOG: NIC1 (bnx2): transmit queue 3 timed out
>>> Modules linked in: smb3_failover(O) smb2(O) smb(O) smb_manager(O) nfs(O) nfs_acl(O) nfsd(O) lockd(O) nal(O) auth_rpcgss(O)
>>> scsi_dh_hp_sw scsi_dh_alua scsi_dh_emc scsi_dh_rdac scsi_dh scsi_mod [last unloaded: ipmi_msghandler]
>>> Pid: 0, comm: swapper/0 Tainted: P W O 3.4.87-default #1
>>> Call Trace:
>>> <IRQ> [<ffffffff8103fcea>] warn_slowpath_common+0x7a/0xb0
>>> [<ffffffff8103fdc1>] warn_slowpath_fmt+0x41/0x50
>>> [<ffffffff81047749>] ? raise_softirq_irqoff+0x9/0x30
>>> [<ffffffff813ae0f6>] dev_watchdog+0x256/0x260
>>> [<ffffffff813adea0>] ? dev_deactivate_queue.constprop.30+0x70/0x70
>>> [<ffffffff8104edc7>] run_timer_softirq+0x147/0x340
>>> [<ffffffff810470d8>] __do_softirq+0xc8/0x1e0
>>> [<ffffffff8109250f>] ? tick_program_event+0x1f/0x30
>>> [<ffffffff81460a6c>] call_softirq+0x1c/0x30
>>> [<ffffffff8100417d>] do_softirq+0x9d/0xd0
>>> [<ffffffff810474a5>] irq_exit+0xb5/0xc0
>>> [<ffffffff81021b49>] smp_apic_timer_interrupt+0x69/0xa0
>>> [<ffffffff8146006f>] apic_timer_interrupt+0x6f/0x80
>>> <EOI> [<ffffffff81457bdd>] ? retint_restore_args+0x13/0x13
>>> [<ffffffff81360149>] ? poll_idle+0x49/0x90
>>> [<ffffffff8136011f>] ? poll_idle+0x1f/0x90
>>> [<ffffffff8135fcc9>] cpuidle_enter+0x19/0x20
>>> [<ffffffff813602f2>] cpuidle_idle_call+0xa2/0x250
>>> [<ffffffff8100b08f>] cpu_idle+0x6f/0xe0
>>> [<ffffffff81915960>] ? rawsock_init+0x12/0x12
>>> [<ffffffff814331c9>] rest_init+0x6d/0x74
>>> [<ffffffff818d3be5>] start_kernel+0x3a2/0x3af
>>> [<ffffffff818d3642>] ? repair_env_string+0x5e/0x5e
>>> [<ffffffff818d332a>] x86_64_start_reservations+0x131/0x135
>>> [<ffffffff818d342e>] x86_64_start_kernel+0x100/0x10f
>>> ---[ end trace 497e24e681e0c02d ]---
>>> bnx2 0000:05:00.1: NIC1: DEBUG: intr_sem[0] PCI_CMD[00100002]
>
> The memory bit in PCI_CMD is set, but the bus master bit is not set.
> DMA won't work if the bus master bit is not set. What was happening
> before the timeout? Was it working fine for a while and it suddenly
> stopped?
>
>From the dmesg, I think it should be stopped suddenly. Because before this timeout, It seems
that nic1 can handle packages naturally.
[ 1882.809865] IPv4: martian source 93.93.255.255 from 93.93.41.1, on dev NIC1
[ 1882.809871] ll header: 00000000: ff ff ff ff ff ff 04 f9 38 85 c7 a6 08 00 ........8.....
After about 10 seconds, the timeout happens. And during the 10 seconds, there is no other exception.
[ 1892.991286] NETDEV WATCHDOG: NIC1 (bnx2): transmit queue 3 timed out
So from your experience, which pathes could result in the bus master bit not set, or an hardware error?
Thanks.
>>> bnx2 0000:05:00.1: NIC1: DEBUG: PCI_PM[19002008] PCI_MISC_CFG[92000088]
>>> bnx2 0000:05:00.1: NIC1: DEBUG: EMAC_TX_STATUS[00000008] EMAC_RX_STATUS[00000000]
>>> bnx2 0000:05:00.1: NIC1: DEBUG: RPM_MGMT_PKT_CTRL[40000088]
>>> bnx2 0000:05:00.1: NIC1: DEBUG: HC_STATS_INTERRUPT_STATUS[01ff0000]
>>> bnx2 0000:05:00.1: NIC1: DEBUG: PBA[00000000]
>>> bnx2 0000:05:00.1: NIC1: <--- start MCP states dump --->
>>> bnx2 0000:05:00.1: NIC1: DEBUG: MCP_STATE_P0[0003e10e] MCP_STATE_P1[0003e10e]
>>> bnx2 0000:05:00.1: NIC1: DEBUG: MCP mode[0000b800] state[80008000] evt_mask[00000500]
>>> bnx2 0000:05:00.1: NIC1: DEBUG: pc[08008f60] pc[0800d21c] instr[00051080]
>>> bnx2 0000:05:00.1: NIC1: DEBUG: shmem states:
>>> bnx2 0000:05:00.1: NIC1: DEBUG: drv_mb[01030003] fw_mb[00000003] link_status[0000006f] drv_pulse_mb[0000073d]
>>> bnx2 0000:05:00.1: NIC1: DEBUG: dev_info_signature[44564907] reset_type[01005254] condition[0003e10e]
>>> bnx2 0000:05:00.1: NIC1: DEBUG: 000003cc: 00000000 00000000 00000000 00000000
>>> bnx2 0000:05:00.1: NIC1: DEBUG: 000003dc: 00000000 00000000 00000000 00000000
>>> bnx2 0000:05:00.1: NIC1: DEBUG: 000003ec: 00000000 00000000 00000000 00000000
>>> bnx2 0000:05:00.1: NIC1: DEBUG: 0x3fc[00000000]
>>> bnx2 0000:05:00.1: NIC1: <--- end MCP states dump --->
>>>
>>
>>
>
>
>
> .
>
^ permalink raw reply
* Re: [ovs-dev] [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
From: Pravin Shelar @ 2014-11-19 8:11 UTC (permalink / raw)
To: Joe Stringer; +Cc: dev@openvswitch.org, netdev, LKML
In-Reply-To: <CAOftzPgYBBFTfNZunf4oGSmECzjOKF0=dnQ9EkTDFsDi+9jfNw@mail.gmail.com>
On Tue, Nov 18, 2014 at 11:25 PM, Joe Stringer <joestringer@nicira.com> wrote:
> On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:
>>
>> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <joestringer@nicira.com>
>> wrote:
>> > When userspace doesn't provide a mask, OVS datapath generates a fully
>> > unwildcarded mask for the flow. This is done by taking a copy of the
>> > flow key, then iterating across its attributes, setting all values to
>> > 0xff. This works for most attributes, as the length of the netlink
>> > attribute typically matches the length of the value. However, IPv6
>> > labels only use the lower 20 bits of the field. This patch makes a
>> > special case to handle this.
>> >
>> > This fixes the following error seen when installing IPv6 flows without a
>> > mask:
>> >
>> > openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff,
>> > max=fffff)
>> >
>> We should allow exact match mask here rather than generating
>> wildcarded mask. So that ovs can catch invalid ipv6.label.
>
>
> I don't quite follow, I thought this was exact-match? (The existing function
> sets all bits to 1)
>
With 0xffffffff value we can exact match on all ipv6.lable bits.
> In this case, userspace has not specified a mask, but the kernel complains
> about a mask that is too wide (because it generated a mask that's too wide).
> Do you have an alternative fix in mind?
We can avoid the sanity check ipv6.lable for mask key.
^ permalink raw reply
* Re: [patch net-next v3 8/9] net: move vlan pop/push functions into common code
From: Pravin Shelar @ 2014-11-19 8:06 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
In-Reply-To: <1416346664-9290-9-git-send-email-jiri@resnulli.us>
On Tue, Nov 18, 2014 at 1:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> So it can be used from out of openvswitch code.
> Did couple of cosmetic changes on the way, namely variable naming and
> adding support for 8021AD proto.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
>
> v2->v3
> - used previously introduced __vlan_insert_tag helper
> - used skb_push/pull to get skb->data into needed point
> - fixed skb->mac_len computation in skb_vlan_push pointed out by Pravin
> v1->v2:
> - adjusted to fix recent ovs changes
> - included change to use make_writable suggested by Eric
>
> include/linux/skbuff.h | 2 +
> net/core/skbuff.c | 95 +++++++++++++++++++++++++++++++++++++++++++++++
> net/openvswitch/actions.c | 88 ++++++-------------------------------------
> 3 files changed, 109 insertions(+), 76 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index e045516..78c299f 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -2679,6 +2679,8 @@ unsigned int skb_gso_transport_seglen(const struct sk_buff *skb);
> struct sk_buff *skb_segment(struct sk_buff *skb, netdev_features_t features);
> struct sk_buff *skb_vlan_untag(struct sk_buff *skb);
> int skb_ensure_writable(struct sk_buff *skb, int write_len);
> +int skb_vlan_pop(struct sk_buff *skb);
> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci);
>
> struct skb_checksum_ops {
> __wsum (*update)(const void *mem, int len, __wsum wsum);
> diff --git a/net/core/skbuff.c b/net/core/skbuff.c
> index d11bbe0..2f72e62 100644
> --- a/net/core/skbuff.c
> +++ b/net/core/skbuff.c
> @@ -4163,6 +4163,101 @@ int skb_ensure_writable(struct sk_buff *skb, int write_len)
> }
> EXPORT_SYMBOL(skb_ensure_writable);
>
> +/* remove VLAN header from packet and update csum accordingly. */
> +static int __skb_vlan_pop(struct sk_buff *skb, u16 *vlan_tci)
> +{
> + struct vlan_hdr *vhdr;
> + unsigned int offset = skb->data - skb_mac_header(skb);
> + int err;
> +
> + __skb_push(skb, offset);
> + err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
> + if (unlikely(err))
> + goto pull;
> +
> + skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
> +
> + vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> + *vlan_tci = ntohs(vhdr->h_vlan_TCI);
> +
> + memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> + __skb_pull(skb, VLAN_HLEN);
> +
> + vlan_set_encap_proto(skb, vhdr);
> + skb->mac_header += VLAN_HLEN;
> +
> + if (skb_network_offset(skb) < ETH_HLEN)
> + skb_set_network_header(skb, ETH_HLEN);
> +
> + skb_reset_mac_len(skb);
skb_reset_mac_len() sets length according to ethernet and network
offsets, but mpls expects mac-length to be offset to mpls header (ref.
skb_mpls_header()). Therefore rather than reset we need to subtract
VLAN_HLEN from mac_len. Same goes for vlan-push(), we can add
VLAN_HLEN.
> +pull:
> + __skb_pull(skb, offset);
> +
> + return err;
> +}
> +
> +int skb_vlan_pop(struct sk_buff *skb)
> +{
> + u16 vlan_tci;
> + __be16 vlan_proto;
> + int err;
> +
> + if (likely(vlan_tx_tag_present(skb))) {
> + skb->vlan_tci = 0;
> + } else {
> + if (unlikely((skb->protocol != htons(ETH_P_8021Q) &&
> + skb->protocol != htons(ETH_P_8021AD)) ||
> + skb->len < VLAN_ETH_HLEN))
> + return 0;
> +
> + err = __skb_vlan_pop(skb, &vlan_tci);
> + if (err)
> + return err;
> + }
> + /* move next vlan tag to hw accel tag */
> + if (likely((skb->protocol != htons(ETH_P_8021Q) &&
> + skb->protocol != htons(ETH_P_8021AD)) ||
> + skb->len < VLAN_ETH_HLEN))
> + return 0;
> +
> + vlan_proto = skb->protocol;
> + err = __skb_vlan_pop(skb, &vlan_tci);
> + if (unlikely(err))
> + return err;
> +
> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> + return 0;
> +}
> +EXPORT_SYMBOL(skb_vlan_pop);
> +
> +int skb_vlan_push(struct sk_buff *skb, __be16 vlan_proto, u16 vlan_tci)
> +{
> + if (vlan_tx_tag_present(skb)) {
> + unsigned int offset = skb->data - skb_mac_header(skb);
> + int err;
> +
> + /* __vlan_insert_tag expect skb->data pointing to mac header.
> + * So change skb->data before calling it and change back to
> + * original position later
> + */
> + __skb_push(skb, offset);
> + err = __vlan_insert_tag(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> + if (err)
> + return err;
> + skb->protocol = skb->vlan_proto;
> + skb_reset_mac_len(skb);
> + __skb_pull(skb, offset);
> +
> + if (skb->ip_summed == CHECKSUM_COMPLETE)
> + skb->csum = csum_add(skb->csum, csum_partial(skb->data
> + + (2 * ETH_ALEN), VLAN_HLEN, 0));
> + }
> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> + return 0;
> +}
> +EXPORT_SYMBOL(skb_vlan_push);
> +
> /**
> * alloc_skb_with_frags - allocate skb with page frags
> *
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 7ffa377..ae7e1b2 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -206,93 +206,27 @@ static int set_mpls(struct sk_buff *skb, struct sw_flow_key *key,
> return 0;
> }
>
> -/* remove VLAN header from packet and update csum accordingly. */
> -static int __pop_vlan_tci(struct sk_buff *skb, __be16 *current_tci)
> -{
> - struct vlan_hdr *vhdr;
> - int err;
> -
> - err = skb_ensure_writable(skb, VLAN_ETH_HLEN);
> - if (unlikely(err))
> - return err;
> -
> - skb_postpull_rcsum(skb, skb->data + (2 * ETH_ALEN), VLAN_HLEN);
> -
> - vhdr = (struct vlan_hdr *)(skb->data + ETH_HLEN);
> - *current_tci = vhdr->h_vlan_TCI;
> -
> - memmove(skb->data + VLAN_HLEN, skb->data, 2 * ETH_ALEN);
> - __skb_pull(skb, VLAN_HLEN);
> -
> - vlan_set_encap_proto(skb, vhdr);
> - skb->mac_header += VLAN_HLEN;
> -
> - if (skb_network_offset(skb) < ETH_HLEN)
> - skb_set_network_header(skb, ETH_HLEN);
> -
> - /* Update mac_len for subsequent MPLS actions */
> - skb_reset_mac_len(skb);
> - return 0;
> -}
> -
> static int pop_vlan(struct sk_buff *skb, struct sw_flow_key *key)
> {
> - __be16 tci;
> int err;
>
> - if (likely(vlan_tx_tag_present(skb))) {
> - skb->vlan_tci = 0;
> - } else {
> - if (unlikely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN))
> - return 0;
> -
> - err = __pop_vlan_tci(skb, &tci);
> - if (err)
> - return err;
> - }
> - /* move next vlan tag to hw accel tag */
> - if (likely(skb->protocol != htons(ETH_P_8021Q) ||
> - skb->len < VLAN_ETH_HLEN)) {
> + err = skb_vlan_pop(skb);
> + if (vlan_tx_tag_present(skb))
> + invalidate_flow_key(key);
> + else
> key->eth.tci = 0;
> - return 0;
> - }
> -
> - invalidate_flow_key(key);
> - err = __pop_vlan_tci(skb, &tci);
> - if (unlikely(err))
> - return err;
> -
> - __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q), ntohs(tci));
> - return 0;
> + return err;
> }
>
> static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
> const struct ovs_action_push_vlan *vlan)
> {
> - if (unlikely(vlan_tx_tag_present(skb))) {
> - u16 current_tag;
> -
> - /* push down current VLAN tag */
> - current_tag = vlan_tx_tag_get(skb);
> -
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - current_tag);
> - if (!skb)
> - return -ENOMEM;
> - /* Update mac_len for subsequent MPLS actions */
> - skb->mac_len += VLAN_HLEN;
> -
> - if (skb->ip_summed == CHECKSUM_COMPLETE)
> - skb->csum = csum_add(skb->csum, csum_partial(skb->data
> - + (2 * ETH_ALEN), VLAN_HLEN, 0));
> -
> + if (vlan_tx_tag_present(skb))
> invalidate_flow_key(key);
> - } else {
> + else
> key->eth.tci = vlan->vlan_tci;
> - }
> - __vlan_hwaccel_put_tag(skb, vlan->vlan_tpid, ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
> - return 0;
> + return skb_vlan_push(skb, vlan->vlan_tpid,
> + ntohs(vlan->vlan_tci) & ~VLAN_TAG_PRESENT);
> }
>
> static int set_eth_addr(struct sk_buff *skb, struct sw_flow_key *key,
> @@ -858,8 +792,10 @@ static int do_execute_actions(struct datapath *dp, struct sk_buff *skb,
>
> case OVS_ACTION_ATTR_PUSH_VLAN:
> err = push_vlan(skb, key, nla_data(a));
> - if (unlikely(err)) /* skb already freed. */
> + if (unlikely(err)) {
> + dev_kfree_skb_any(skb);
> return err;
> + }
There is no need to check for error. Just break is sufficient. Error
checking is done after switch case.
> break;
>
> case OVS_ACTION_ATTR_POP_VLAN:
> --
> 1.9.3
>
^ permalink raw reply
* Re: [patch net-next v3 6/9] vlan: introduce __vlan_insert_tag helper which does not free skb
From: Pravin Shelar @ 2014-11-19 8:05 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
In-Reply-To: <1416346664-9290-7-git-send-email-jiri@resnulli.us>
On Tue, Nov 18, 2014 at 1:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> There's a need for helper which inserts vlan tag but does not free the
> skb in case of an error.
>
> Suggested-by: Pravin Shelar <pshelar@nicira.com>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
> ---
> include/linux/if_vlan.h | 45 ++++++++++++++++++++++++++++++++++-----------
> 1 file changed, 34 insertions(+), 11 deletions(-)
>
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 291e670..515a35e 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -282,28 +282,24 @@ static inline bool vlan_hw_offload_capable(netdev_features_t features,
> }
>
> /**
> - * vlan_insert_tag - regular VLAN tag inserting
> + * __vlan_insert_tag - regular VLAN tag inserting
> * @skb: skbuff to tag
> * @vlan_proto: VLAN encapsulation protocol
> * @vlan_tci: VLAN TCI to insert
> *
> * Inserts the VLAN tag into @skb as part of the payload
> - * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
> - *
> - * Following the skb_unshare() example, in case of error, the calling function
> - * doesn't have to worry about freeing the original skb.
> + * Returns error if skb_cow_head failes.
> *
> * Does not change skb->protocol so this function can be used during receive.
> */
> -static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
> - __be16 vlan_proto, u16 vlan_tci)
> +static inline int __vlan_insert_tag(struct sk_buff *skb,
> + __be16 vlan_proto, u16 vlan_tci)
> {
> struct vlan_ethhdr *veth;
>
> - if (skb_cow_head(skb, VLAN_HLEN) < 0) {
> - dev_kfree_skb_any(skb);
> - return NULL;
> - }
> + if (skb_cow_head(skb, VLAN_HLEN) < 0)
> + return -ENOMEM;
> +
> veth = (struct vlan_ethhdr *)skb_push(skb, VLAN_HLEN);
>
> /* Move the mac addresses to the beginning of the new header. */
> @@ -316,6 +312,33 @@ static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
> /* now, the TCI */
> veth->h_vlan_TCI = htons(vlan_tci);
>
> + return 0;
> +}
> +
> +/**
> + * vlan_insert_tag - regular VLAN tag inserting
> + * @skb: skbuff to tag
> + * @vlan_proto: VLAN encapsulation protocol
> + * @vlan_tci: VLAN TCI to insert
> + *
> + * Inserts the VLAN tag into @skb as part of the payload
> + * Returns a VLAN tagged skb. If a new skb is created, @skb is freed.
> + *
> + * Following the skb_unshare() example, in case of error, the calling function
> + * doesn't have to worry about freeing the original skb.
> + *
> + * Does not change skb->protocol so this function can be used during receive.
> + */
> +static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
> + __be16 vlan_proto, u16 vlan_tci)
> +{
> + int err;
> +
> + err = __vlan_insert_tag(skb, vlan_proto, vlan_tci);
> + if (err) {
> + dev_kfree_skb_any(skb);
> + return NULL;
> + }
> return skb;
> }
>
> --
> 1.9.3
>
^ permalink raw reply
* Re: [patch net-next v3 5/9] vlan: introduce *vlan_hwaccel_push_inside helpers
From: Pravin Shelar @ 2014-11-19 8:05 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
In-Reply-To: <1416346664-9290-6-git-send-email-jiri@resnulli.us>
On Tue, Nov 18, 2014 at 1:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Use them to push skb->vlan_tci into the payload and avoid code
> duplication.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> ---
> drivers/net/vxlan.c | 22 ++++++----------------
> include/linux/if_vlan.h | 34 ++++++++++++++++++++++++++++++++++
> net/core/dev.c | 8 ++------
> net/core/netpoll.c | 4 +---
> net/ipv4/geneve.c | 11 +++--------
> net/openvswitch/datapath.c | 6 +-----
> net/openvswitch/vport-gre.c | 12 ++++--------
> 7 files changed, 51 insertions(+), 46 deletions(-)
>
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index bb8fbab..64d45fa 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1599,14 +1599,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
> if (unlikely(err))
> return err;
>
> - if (vlan_tx_tag_present(skb)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (WARN_ON(!skb))
> - return -ENOMEM;
> -
> - skb->vlan_tci = 0;
> - }
> + skb = vlan_hwaccel_push_inside(skb);
> + if (WARN_ON(!skb))
> + return -ENOMEM;
>
> vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> vxh->vx_flags = htonl(VXLAN_FLAGS);
> @@ -1643,14 +1638,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
> if (unlikely(err))
> return err;
>
> - if (vlan_tx_tag_present(skb)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (WARN_ON(!skb))
> - return -ENOMEM;
> -
> - skb->vlan_tci = 0;
> - }
> + skb = vlan_hwaccel_push_inside(skb);
> + if (WARN_ON(!skb))
> + return -ENOMEM;
>
> vxh = (struct vxlanhdr *) __skb_push(skb, sizeof(*vxh));
> vxh->vx_flags = htonl(VXLAN_FLAGS);
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 46e4a15..291e670 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -341,6 +341,40 @@ static inline struct sk_buff *vlan_insert_tag_set_proto(struct sk_buff *skb,
> return skb;
> }
>
> +/*
> + * __vlan_hwaccel_push_inside - pushes vlan tag to the payload
> + * @skb: skbuff to tag
> + *
> + * Pushes the VLAN tag from @skb->vlan_tci inside to the payload.
> + *
> + * Following the skb_unshare() example, in case of error, the calling function
> + * doesn't have to worry about freeing the original skb.
> + */
> +static inline struct sk_buff *__vlan_hwaccel_push_inside(struct sk_buff *skb)
> +{
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> + if (likely(skb))
> + skb->vlan_tci = 0;
> + return skb;
> +}
> +/*
> + * vlan_hwaccel_push_inside - pushes vlan tag to the payload
> + * @skb: skbuff to tag
> + *
> + * Checks is tag is present in @skb->vlan_tci and if it is, it pushes the
> + * VLAN tag from @skb->vlan_tci inside to the payload.
> + *
> + * Following the skb_unshare() example, in case of error, the calling function
> + * doesn't have to worry about freeing the original skb.
> + */
> +static inline struct sk_buff *vlan_hwaccel_push_inside(struct sk_buff *skb)
> +{
> + if (vlan_tx_tag_present(skb))
> + skb = __vlan_hwaccel_push_inside(skb);
> + return skb;
> +}
> +
> /**
> * __vlan_hwaccel_put_tag - hardware accelerated VLAN inserting
> * @skb: skbuff to tag
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 3611e60..ac48362 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2644,12 +2644,8 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
> netdev_features_t features)
> {
> if (vlan_tx_tag_present(skb) &&
> - !vlan_hw_offload_capable(features, skb->vlan_proto)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (skb)
> - skb->vlan_tci = 0;
> - }
> + !vlan_hw_offload_capable(features, skb->vlan_proto))
> + skb = __vlan_hwaccel_push_inside(skb);
> return skb;
> }
>
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index 65d3723..e0ad5d1 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -79,8 +79,7 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
>
> if (vlan_tx_tag_present(skb) &&
> !vlan_hw_offload_capable(features, skb->vlan_proto)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> + skb = __vlan_hwaccel_push_inside(skb);
> if (unlikely(!skb)) {
> /* This is actually a packet drop, but we
> * don't want the code that calls this
> @@ -88,7 +87,6 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
> */
> goto out;
> }
> - skb->vlan_tci = 0;
> }
>
> status = netdev_start_xmit(skb, dev, txq, false);
> diff --git a/net/ipv4/geneve.c b/net/ipv4/geneve.c
> index fd430a6..a457232 100644
> --- a/net/ipv4/geneve.c
> +++ b/net/ipv4/geneve.c
> @@ -131,14 +131,9 @@ int geneve_xmit_skb(struct geneve_sock *gs, struct rtable *rt,
> if (unlikely(err))
> return err;
>
> - if (vlan_tx_tag_present(skb)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (unlikely(!skb)
> - return -ENOMEM;
> -
> - skb->vlan_tci = 0;
> - }
> + skb = vlan_hwaccel_push_inside(skb);
> + if (unlikely(!skb))
> + return -ENOMEM;
>
> gnvh = (struct genevehdr *)__skb_push(skb, sizeof(*gnvh) + opt_len);
> geneve_build_header(gnvh, tun_flags, vni, opt_len, opt);
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index c63e60e..0cb9d4f 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -425,12 +425,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
> if (!nskb)
> return -ENOMEM;
>
> - nskb = vlan_insert_tag_set_proto(nskb, nskb->vlan_proto,
> - vlan_tx_tag_get(nskb));
> - if (!nskb)
> - return -ENOMEM;
> + nskb = __vlan_hwaccel_push_inside(nskb);
>
> - nskb->vlan_tci = 0;
Need to check for returned nskb for NULL case.
> skb = nskb;
> }
>
> diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
> index 777cd8c..6b69df5 100644
> --- a/net/openvswitch/vport-gre.c
> +++ b/net/openvswitch/vport-gre.c
> @@ -175,14 +175,10 @@ static int gre_tnl_send(struct vport *vport, struct sk_buff *skb)
> goto err_free_rt;
> }
>
> - if (vlan_tx_tag_present(skb)) {
> - skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> - if (unlikely(!skb) {
> - err = -ENOMEM;
> - goto err_free_rt;
> - }
> - skb->vlan_tci = 0;
> + skb = vlan_hwaccel_push_inside(skb);
> + if (unlikely(!skb)) {
> + err = -ENOMEM;
> + goto err_free_rt;
> }
>
> /* Push Tunnel header. */
> --
> 1.9.3
>
^ permalink raw reply
* Re: [patch net-next v3 4/9] vlan: rename __vlan_put_tag to vlan_insert_tag_set_proto
From: Pravin Shelar @ 2014-11-19 8:05 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
In-Reply-To: <1416346664-9290-5-git-send-email-jiri@resnulli.us>
On Tue, Nov 18, 2014 at 1:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Name fits better. Plus there's going to be introduced
> __vlan_insert_tag later on.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
> ---
> drivers/net/bonding/bond_main.c | 4 ++--
> drivers/net/ethernet/emulex/benet/be_main.c | 6 ++++--
> drivers/net/vxlan.c | 12 ++++++------
> include/linux/if_vlan.h | 8 +++++---
> net/bridge/br_vlan.c | 4 ++--
> net/core/dev.c | 4 ++--
> net/core/netpoll.c | 4 ++--
> net/ipv4/geneve.c | 11 +++++------
> net/openvswitch/actions.c | 4 +++-
> net/openvswitch/datapath.c | 3 ++-
> net/openvswitch/vport-gre.c | 6 +++---
> 11 files changed, 36 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
> index e26c682..c1d7da4 100644
> --- a/drivers/net/bonding/bond_main.c
> +++ b/drivers/net/bonding/bond_main.c
> @@ -2146,8 +2146,8 @@ static void bond_arp_send(struct net_device *slave_dev, int arp_op,
>
> netdev_dbg(slave_dev, "inner tag: proto %X vid %X\n",
> ntohs(outer_tag->vlan_proto), tags->vlan_id);
> - skb = __vlan_put_tag(skb, tags->vlan_proto,
> - tags->vlan_id);
> + skb = vlan_insert_tag_set_proto(skb, tags->vlan_proto,
> + tags->vlan_id);
> if (!skb) {
> net_err_ratelimited("failed to insert inner VLAN tag\n");
> return;
> diff --git a/drivers/net/ethernet/emulex/benet/be_main.c b/drivers/net/ethernet/emulex/benet/be_main.c
> index 54160cc..d02fbc7 100644
> --- a/drivers/net/ethernet/emulex/benet/be_main.c
> +++ b/drivers/net/ethernet/emulex/benet/be_main.c
> @@ -887,7 +887,8 @@ static struct sk_buff *be_insert_vlan_in_pkt(struct be_adapter *adapter,
> }
>
> if (vlan_tag) {
> - skb = __vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_tag);
> + skb = vlan_insert_tag_set_proto(skb, htons(ETH_P_8021Q),
> + vlan_tag);
> if (unlikely(!skb))
> return skb;
> skb->vlan_tci = 0;
> @@ -896,7 +897,8 @@ static struct sk_buff *be_insert_vlan_in_pkt(struct be_adapter *adapter,
> /* Insert the outer VLAN, if any */
> if (adapter->qnq_vid) {
> vlan_tag = adapter->qnq_vid;
> - skb = __vlan_put_tag(skb, htons(ETH_P_8021Q), vlan_tag);
> + skb = vlan_insert_tag_set_proto(skb, htons(ETH_P_8021Q),
> + vlan_tag);
> if (unlikely(!skb))
> return skb;
> if (skip_hw_vlan)
> diff --git a/drivers/net/vxlan.c b/drivers/net/vxlan.c
> index 23b1e8c..bb8fbab 100644
> --- a/drivers/net/vxlan.c
> +++ b/drivers/net/vxlan.c
> @@ -1600,9 +1600,9 @@ static int vxlan6_xmit_skb(struct vxlan_sock *vs,
> return err;
>
> if (vlan_tx_tag_present(skb)) {
> - if (WARN_ON(!__vlan_put_tag(skb,
> - skb->vlan_proto,
> - vlan_tx_tag_get(skb))))
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> + if (WARN_ON(!skb))
> return -ENOMEM;
>
> skb->vlan_tci = 0;
> @@ -1644,9 +1644,9 @@ int vxlan_xmit_skb(struct vxlan_sock *vs,
> return err;
>
> if (vlan_tx_tag_present(skb)) {
> - if (WARN_ON(!__vlan_put_tag(skb,
> - skb->vlan_proto,
> - vlan_tx_tag_get(skb))))
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> + if (WARN_ON(!skb))
> return -ENOMEM;
>
> skb->vlan_tci = 0;
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index 75b70a5..46e4a15 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -320,8 +320,9 @@ static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
> }
>
> /**
> - * __vlan_put_tag - regular VLAN tag inserting
> + * vlan_insert_tag_set_proto - regular VLAN tag inserting
> * @skb: skbuff to tag
> + * @vlan_proto: VLAN encapsulation protocol
> * @vlan_tci: VLAN TCI to insert
> *
> * Inserts the VLAN tag into @skb as part of the payload
> @@ -330,8 +331,9 @@ static inline struct sk_buff *vlan_insert_tag(struct sk_buff *skb,
> * Following the skb_unshare() example, in case of error, the calling function
> * doesn't have to worry about freeing the original skb.
> */
> -static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb,
> - __be16 vlan_proto, u16 vlan_tci)
> +static inline struct sk_buff *vlan_insert_tag_set_proto(struct sk_buff *skb,
> + __be16 vlan_proto,
> + u16 vlan_tci)
> {
> skb = vlan_insert_tag(skb, vlan_proto, vlan_tci);
> if (skb)
> diff --git a/net/bridge/br_vlan.c b/net/bridge/br_vlan.c
> index 150048f..97b8ddf 100644
> --- a/net/bridge/br_vlan.c
> +++ b/net/bridge/br_vlan.c
> @@ -199,8 +199,8 @@ bool br_allowed_ingress(struct net_bridge *br, struct net_port_vlans *v,
> if (skb->vlan_proto != proto) {
> /* Protocol-mismatch, empty out vlan_tci for new tag */
> skb_push(skb, ETH_HLEN);
> - skb = __vlan_put_tag(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> if (unlikely(!skb))
> return false;
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index 1ab168e..3611e60 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -2645,8 +2645,8 @@ static struct sk_buff *validate_xmit_vlan(struct sk_buff *skb,
> {
> if (vlan_tx_tag_present(skb) &&
> !vlan_hw_offload_capable(features, skb->vlan_proto)) {
> - skb = __vlan_put_tag(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> if (skb)
> skb->vlan_tci = 0;
> }
> diff --git a/net/core/netpoll.c b/net/core/netpoll.c
> index e6645b4..65d3723 100644
> --- a/net/core/netpoll.c
> +++ b/net/core/netpoll.c
> @@ -79,8 +79,8 @@ static int netpoll_start_xmit(struct sk_buff *skb, struct net_device *dev,
>
> if (vlan_tx_tag_present(skb) &&
> !vlan_hw_offload_capable(features, skb->vlan_proto)) {
> - skb = __vlan_put_tag(skb, skb->vlan_proto,
> - vlan_tx_tag_get(skb));
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> if (unlikely(!skb)) {
> /* This is actually a packet drop, but we
> * don't want the code that calls this
> diff --git a/net/ipv4/geneve.c b/net/ipv4/geneve.c
> index 31802af..fd430a6 100644
> --- a/net/ipv4/geneve.c
> +++ b/net/ipv4/geneve.c
> @@ -132,12 +132,11 @@ int geneve_xmit_skb(struct geneve_sock *gs, struct rtable *rt,
> return err;
>
> if (vlan_tx_tag_present(skb)) {
> - if (unlikely(!__vlan_put_tag(skb,
> - skb->vlan_proto,
> - vlan_tx_tag_get(skb)))) {
> - err = -ENOMEM;
> - return err;
> - }
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> + if (unlikely(!skb)
> + return -ENOMEM;
> +
> skb->vlan_tci = 0;
> }
>
> diff --git a/net/openvswitch/actions.c b/net/openvswitch/actions.c
> index 749a301..426b913 100644
> --- a/net/openvswitch/actions.c
> +++ b/net/openvswitch/actions.c
> @@ -287,7 +287,9 @@ static int push_vlan(struct sk_buff *skb, struct sw_flow_key *key,
> /* push down current VLAN tag */
> current_tag = vlan_tx_tag_get(skb);
>
> - if (!__vlan_put_tag(skb, skb->vlan_proto, current_tag))
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + current_tag);
> + if (!skb)
> return -ENOMEM;
> /* Update mac_len for subsequent MPLS actions */
> skb->mac_len += VLAN_HLEN;
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index ab141d4..c63e60e 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -425,7 +425,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
> if (!nskb)
> return -ENOMEM;
>
> - nskb = __vlan_put_tag(nskb, nskb->vlan_proto, vlan_tx_tag_get(nskb));
> + nskb = vlan_insert_tag_set_proto(nskb, nskb->vlan_proto,
> + vlan_tx_tag_get(nskb));
> if (!nskb)
> return -ENOMEM;
>
> diff --git a/net/openvswitch/vport-gre.c b/net/openvswitch/vport-gre.c
> index 8e61a5c..777cd8c 100644
> --- a/net/openvswitch/vport-gre.c
> +++ b/net/openvswitch/vport-gre.c
> @@ -176,9 +176,9 @@ static int gre_tnl_send(struct vport *vport, struct sk_buff *skb)
> }
>
> if (vlan_tx_tag_present(skb)) {
> - if (unlikely(!__vlan_put_tag(skb,
> - skb->vlan_proto,
> - vlan_tx_tag_get(skb)))) {
> + skb = vlan_insert_tag_set_proto(skb, skb->vlan_proto,
> + vlan_tx_tag_get(skb));
> + if (unlikely(!skb) {
> err = -ENOMEM;
> goto err_free_rt;
> }
> --
> 1.9.3
>
^ permalink raw reply
* Re: [patch net-next v3 2/9] vlan: make __vlan_hwaccel_put_tag return void
From: Pravin Shelar @ 2014-11-19 8:05 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, David Miller, Jamal Hadi Salim, Tom Herbert, Eric Dumazet,
Willem de Bruijn, Daniel Borkmann, mst, fw, Paul.Durrant,
Thomas Graf, Cong Wang
In-Reply-To: <1416346664-9290-3-git-send-email-jiri@resnulli.us>
On Tue, Nov 18, 2014 at 1:37 PM, Jiri Pirko <jiri@resnulli.us> wrote:
> Always returns the same skb it gets, so change to void.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
Acked-by: Pravin B Shelar <pshelar@nicira.com>
> ---
> drivers/scsi/fcoe/fcoe.c | 6 ++----
> include/linux/if_vlan.h | 9 ++++-----
> net/8021q/vlan_dev.c | 2 +-
> 3 files changed, 7 insertions(+), 10 deletions(-)
>
> diff --git a/drivers/scsi/fcoe/fcoe.c b/drivers/scsi/fcoe/fcoe.c
> index 4a8ac7d..73a8cc4 100644
> --- a/drivers/scsi/fcoe/fcoe.c
> +++ b/drivers/scsi/fcoe/fcoe.c
> @@ -1669,10 +1669,8 @@ static int fcoe_xmit(struct fc_lport *lport, struct fc_frame *fp)
> fcoe->realdev->features & NETIF_F_HW_VLAN_CTAG_TX) {
> /* must set skb->dev before calling vlan_put_tag */
> skb->dev = fcoe->realdev;
> - skb = __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
> - vlan_dev_vlan_id(fcoe->netdev));
> - if (!skb)
> - return -ENOMEM;
> + __vlan_hwaccel_put_tag(skb, htons(ETH_P_8021Q),
> + vlan_dev_vlan_id(fcoe->netdev));
> } else
> skb->dev = fcoe->netdev;
>
> diff --git a/include/linux/if_vlan.h b/include/linux/if_vlan.h
> index d69f057..1b5dbc2 100644
> --- a/include/linux/if_vlan.h
> +++ b/include/linux/if_vlan.h
> @@ -347,13 +347,11 @@ static inline struct sk_buff *__vlan_put_tag(struct sk_buff *skb,
> *
> * Puts the VLAN TCI in @skb->vlan_tci and lets the device do the rest
> */
> -static inline struct sk_buff *__vlan_hwaccel_put_tag(struct sk_buff *skb,
> - __be16 vlan_proto,
> - u16 vlan_tci)
> +static inline void __vlan_hwaccel_put_tag(struct sk_buff *skb,
> + __be16 vlan_proto, u16 vlan_tci)
> {
> skb->vlan_proto = vlan_proto;
> skb->vlan_tci = VLAN_TAG_PRESENT | vlan_tci;
> - return skb;
> }
>
> /**
> @@ -368,7 +366,8 @@ static inline struct sk_buff *vlan_put_tag(struct sk_buff *skb,
> __be16 vlan_proto, u16 vlan_tci)
> {
> if (vlan_hw_offload_capable(skb->dev->features, vlan_proto)) {
> - return __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> + __vlan_hwaccel_put_tag(skb, vlan_proto, vlan_tci);
> + return skb;
> } else {
> return __vlan_put_tag(skb, vlan_proto, vlan_tci);
> }
> diff --git a/net/8021q/vlan_dev.c b/net/8021q/vlan_dev.c
> index 0d441ec..d6524b2 100644
> --- a/net/8021q/vlan_dev.c
> +++ b/net/8021q/vlan_dev.c
> @@ -150,7 +150,7 @@ static netdev_tx_t vlan_dev_hard_start_xmit(struct sk_buff *skb,
> u16 vlan_tci;
> vlan_tci = vlan->vlan_id;
> vlan_tci |= vlan_dev_get_egress_qos_mask(dev, skb->priority);
> - skb = __vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);
> + __vlan_hwaccel_put_tag(skb, vlan->vlan_proto, vlan_tci);
> }
>
> skb->dev = vlan->real_dev;
> --
> 1.9.3
>
^ permalink raw reply
* Re: Device Tree Binding for Marvell DSA Switch on imx28 board over Mdio Interface
From: Oliver Graute @ 2014-11-19 7:49 UTC (permalink / raw)
To: Florian Fainelli; +Cc: Andrew Lunn, netdev@vger.kernel.org, buytenh
In-Reply-To: <546B8E92.7080000@gmail.com>
On Tue, Nov 18, 2014 at 7:23 PM, Florian Fainelli <f.fainelli@gmail.com> wrote:
> On 11/18/2014 12:30 AM, Oliver Graute wrote:
>>> Hi Oliver
>>>
>>> How do you have the strapping pins on the switch set? They determine
>>> what address on the mdio bus the chip responds to.
>>
>> On the circuit diagram the PIN 54 (P5_IND1/P5ID1) is set to
>> "Configuration Address: 0101"
>> P5_MODE[3:0]=0111 = Single RMII MAC Mode (100Mbps FD with 50 MHz clock input)
>> PIN 59 R1_LED/NO_CPU Configuration: CPU is attached SMI address is 0x10 to 0x1F
>>
>> But what is the mdio address of the whole switch? or can I only
>> address individual phy ports?
>
> You should specify in the Device Tree the switch pseudo-PHY address,
> typically 16 for Marvell switches. You can still access the individual
> ports' PHY addresses using address 0 through N.
>
How do I do that exactly? do you have an example Device Tree Snippet?
Best regards,
Oliver
^ permalink raw reply
* Re: [PATCH] [net]ip_tunnel: the lack of vti_link_ops' dellink() cause kernel panic
From: lucien xin @ 2014-11-19 7:47 UTC (permalink / raw)
To: Nicolas Dichtel; +Cc: network dev, Steffen Klassert
In-Reply-To: <546B8093.4080507@6wind.com>
>> ---
>> net/ipv4/ip_vti.c | 1 +
>> 1 file changed, 1 insertion(+)
>>
>> diff --git a/net/ipv4/ip_vti.c b/net/ipv4/ip_vti.c
>> index 3e86101..1a7e979 100644
>> --- a/net/ipv4/ip_vti.c
>> +++ b/net/ipv4/ip_vti.c
>> @@ -528,6 +528,7 @@ static struct rtnl_link_ops vti_link_ops __read_mostly
>> = {
>> .validate = vti_tunnel_validate,
>> .newlink = vti_newlink,
>> .changelink = vti_changelink,
>> + .dellink = ip_tunnel_dellink,
>
> Nitpicking: other lines into this struct uses tabs to align the '=', but
> the one you add uses spaces.
>
>
okay, thanks, I will correct it.
^ permalink raw reply
* Re: [PATCH] [net]ip_tunnel: the lack of vti_link_ops' dellink() cause kernel panic
From: lucien xin @ 2014-11-19 7:44 UTC (permalink / raw)
To: Cong Wang; +Cc: Nicolas Dichtel, network dev, Steffen Klassert
In-Reply-To: <CAHA+R7NjO+bDhN5QX95d8V0VqvbacxCzoHWT5NpDaDn3YCtCeg@mail.gmail.com>
On Wed, Nov 19, 2014 at 7:25 AM, Cong Wang <cwang@twopensource.com> wrote:
> On Tue, Nov 18, 2014 at 9:23 AM, Nicolas Dichtel
> <nicolas.dichtel@6wind.com> wrote:
>>
>> A quick look at the ipv6 side seems to show that there is the same problem.
>> Can
>> you provide the IPv6 patch too?
>>
>
> IPv6 doesn't use ip_tunnel library, so needs to provide its own implementation:
>
> diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
> index ec84d03..3ae094b 100644
> --- a/net/ipv6/ip6_vti.c
> +++ b/net/ipv6/ip6_vti.c
> @@ -911,6 +911,16 @@ static int vti6_newlink(struct net *src_net,
> struct net_device *dev,
> return vti6_tnl_create2(dev);
> }
>
> +static void vti6_dellink(struct net_device *dev, struct list_head *head)
> +{
> + struct net *net = dev_net(dev);
> + struct vti6_net *ip6n = net_generic(net, vti6_net_id);
> +
> + if (dev != ip6n->fb_tnl_dev)
> + unregister_netdevice_queue(dev, head);
> +}
> +
> +
> static int vti6_changelink(struct net_device *dev, struct nlattr *tb[],
> struct nlattr *data[])
> {
> @@ -986,6 +996,7 @@ static struct rtnl_link_ops vti6_link_ops __read_mostly = {
> .setup = vti6_dev_setup,
> .validate = vti6_validate,
> .newlink = vti6_newlink,
> + .dellink = vti6_dellink,
> .changelink = vti6_changelink,
> .get_size = vti6_get_size,
> .fill_info = vti6_fill_info,
the issue do not exist in ip6_vti. cause vti6_init_net() complete the
device ip6_vti0 initialization,
in which dev->rtnl_link_ops never be pointed. so the ip6_vti0's
rtnl_link_ops is null, which lead
to the dellink() will never be invoked in rtnl_dellink():
if (!ops || !ops->dellink)
return -EOPNOTSUPP;
ops->dellink(dev, &list_kill);
unregister_netdevice_many(&list_kill);
instead, "ip link del ip6_vti0" wil return:
RTNETLINK answers: Operation not supported
if this patch for ip6 is to be consistent with ipv4 vti. I suggest we
should also add the below code
to init ip6_vti0's rtnl_link_ops when the ip6_vti0 is inited:
diff --git a/net/ipv6/ip6_vti.c b/net/ipv6/ip6_vti.c
index 31089d1..e68c099 100644
--- a/net/ipv6/ip6_vti.c
+++ b/net/ipv6/ip6_vti.c
@@ -1020,6 +1020,7 @@ static int __net_init vti6_init_net(struct net *net)
if (!ip6n->fb_tnl_dev)
goto err_alloc_dev;
dev_net_set(ip6n->fb_tnl_dev, net);
+ ip6n->fb_tnl_dev->rtnl_link_ops = &vti6_link_ops;
err = vti6_fb_tnl_dev_init(ip6n->fb_tnl_dev);
if (err < 0)
^ permalink raw reply related
* [PATCH net-next v2] enic: support skb->xmit_more
From: Govindarajulu Varadarajan @ 2014-11-19 7:29 UTC (permalink / raw)
To: davem, netdev; +Cc: ssujith, benve, eric.dumazet, Govindarajulu Varadarajan
Check and update posted_index only when skb->xmit_more is 0 or tx queue is full.
v2:
use txq_map instead of skb_get_queue_mapping(skb)
Signed-off-by: Govindarajulu Varadarajan <_govind@gmx.com>
---
drivers/net/ethernet/cisco/enic/enic_main.c | 8 ++++++--
drivers/net/ethernet/cisco/enic/vnic_wq.h | 20 +++++++++++---------
2 files changed, 17 insertions(+), 11 deletions(-)
diff --git a/drivers/net/ethernet/cisco/enic/enic_main.c b/drivers/net/ethernet/cisco/enic/enic_main.c
index 5afe360..b9cda2f 100644
--- a/drivers/net/ethernet/cisco/enic/enic_main.c
+++ b/drivers/net/ethernet/cisco/enic/enic_main.c
@@ -533,6 +533,7 @@ static netdev_tx_t enic_hard_start_xmit(struct sk_buff *skb,
struct vnic_wq *wq;
unsigned long flags;
unsigned int txq_map;
+ struct netdev_queue *txq;
if (skb->len <= 0) {
dev_kfree_skb_any(skb);
@@ -541,6 +542,7 @@ static netdev_tx_t enic_hard_start_xmit(struct sk_buff *skb,
txq_map = skb_get_queue_mapping(skb) % enic->wq_count;
wq = &enic->wq[txq_map];
+ txq = netdev_get_tx_queue(netdev, txq_map);
/* Non-TSO sends must fit within ENIC_NON_TSO_MAX_DESC descs,
* which is very likely. In the off chance it's going to take
@@ -558,7 +560,7 @@ static netdev_tx_t enic_hard_start_xmit(struct sk_buff *skb,
if (vnic_wq_desc_avail(wq) <
skb_shinfo(skb)->nr_frags + ENIC_DESC_MAX_SPLITS) {
- netif_tx_stop_queue(netdev_get_tx_queue(netdev, txq_map));
+ netif_tx_stop_queue(txq);
/* This is a hard error, log it */
netdev_err(netdev, "BUG! Tx ring full when queue awake!\n");
spin_unlock_irqrestore(&enic->wq_lock[txq_map], flags);
@@ -568,7 +570,9 @@ static netdev_tx_t enic_hard_start_xmit(struct sk_buff *skb,
enic_queue_wq_skb(enic, wq, skb);
if (vnic_wq_desc_avail(wq) < MAX_SKB_FRAGS + ENIC_DESC_MAX_SPLITS)
- netif_tx_stop_queue(netdev_get_tx_queue(netdev, txq_map));
+ netif_tx_stop_queue(txq);
+ if (!skb->xmit_more || netif_xmit_stopped(txq))
+ vnic_wq_doorbell(wq);
spin_unlock_irqrestore(&enic->wq_lock[txq_map], flags);
diff --git a/drivers/net/ethernet/cisco/enic/vnic_wq.h b/drivers/net/ethernet/cisco/enic/vnic_wq.h
index 2c6c708..816f1ad 100644
--- a/drivers/net/ethernet/cisco/enic/vnic_wq.h
+++ b/drivers/net/ethernet/cisco/enic/vnic_wq.h
@@ -104,6 +104,17 @@ static inline void *vnic_wq_next_desc(struct vnic_wq *wq)
return wq->to_use->desc;
}
+static inline void vnic_wq_doorbell(struct vnic_wq *wq)
+{
+ /* Adding write memory barrier prevents compiler and/or CPU
+ * reordering, thus avoiding descriptor posting before
+ * descriptor is initialized. Otherwise, hardware can read
+ * stale descriptor fields.
+ */
+ wmb();
+ iowrite32(wq->to_use->index, &wq->ctrl->posted_index);
+}
+
static inline void vnic_wq_post(struct vnic_wq *wq,
void *os_buf, dma_addr_t dma_addr,
unsigned int len, int sop, int eop,
@@ -122,15 +133,6 @@ static inline void vnic_wq_post(struct vnic_wq *wq,
buf->wr_id = wrid;
buf = buf->next;
- if (eop) {
- /* Adding write memory barrier prevents compiler and/or CPU
- * reordering, thus avoiding descriptor posting before
- * descriptor is initialized. Otherwise, hardware can read
- * stale descriptor fields.
- */
- wmb();
- iowrite32(buf->index, &wq->ctrl->posted_index);
- }
wq->to_use = buf;
wq->ring.desc_avail -= desc_skip_cnt;
--
2.1.3
^ permalink raw reply related
* Re: [PATCH net] openvswitch: Fix mask generation for IPv6 labels.
From: Joe Stringer @ 2014-11-19 7:25 UTC (permalink / raw)
To: Pravin Shelar; +Cc: dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org, netdev, LKML
In-Reply-To: <CALnjE+qrkw92tZ-wgpGOPtbuMJVVCfo--7w6PAGO6OW4skiGQQ-JsoAwUIsXosN+BqQ9rBEUg@public.gmane.org>
On 18 November 2014 22:09, Pravin Shelar <pshelar@nicira.com> wrote:
> On Tue, Nov 18, 2014 at 10:54 AM, Joe Stringer <joestringer@nicira.com>
> wrote:
> > When userspace doesn't provide a mask, OVS datapath generates a fully
> > unwildcarded mask for the flow. This is done by taking a copy of the
> > flow key, then iterating across its attributes, setting all values to
> > 0xff. This works for most attributes, as the length of the netlink
> > attribute typically matches the length of the value. However, IPv6
> > labels only use the lower 20 bits of the field. This patch makes a
> > special case to handle this.
> >
> > This fixes the following error seen when installing IPv6 flows without a
> mask:
> >
> > openvswitch: netlink: Invalid IPv6 flow label value (value=ffffffff,
> max=fffff)
> >
> We should allow exact match mask here rather than generating
> wildcarded mask. So that ovs can catch invalid ipv6.label.
I don't quite follow, I thought this was exact-match? (The existing
function sets all bits to 1)
In this case, userspace has not specified a mask, but the kernel complains
about a mask that is too wide (because it generated a mask that's too
wide). Do you have an alternative fix in mind?
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply
* [PATCH V2 net] virtio-net: validate features during probe
From: Jason Wang @ 2014-11-19 7:21 UTC (permalink / raw)
To: rusty, mst, virtualization, netdev, linux-kernel
This patch validates feature dependencies during probe and fail the probing
if a dependency is missed. This fixes the issues of hitting BUG()
when qemu fails to advertise features correctly. One example is booting
guest with ctrl_vq=off through qemu.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
Changes from V1:
- Drop VIRTIO_NET_F_*_UFO from the checklist, since it was disabled
---
drivers/net/virtio_net.c | 91 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 91 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ec2a8b4..b16a761 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1673,6 +1673,93 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
};
#endif
+static int virtnet_validate_features(struct virtio_device *dev,
+ unsigned int *table,
+ int table_size,
+ unsigned int feature)
+{
+ int i;
+
+ if (!virtio_has_feature(dev, feature)) {
+ for (i = 0; i < table_size; i++) {
+ unsigned int f = table[i];
+
+ if (virtio_has_feature(dev, f)) {
+ dev_err(&dev->dev,
+ "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
+ f, feature);
+ return -EINVAL;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int virtnet_check_features(struct virtio_device *dev)
+{
+ unsigned int features_for_ctrl_vq[] = {
+ VIRTIO_NET_F_CTRL_RX,
+ VIRTIO_NET_F_CTRL_VLAN,
+ VIRTIO_NET_F_GUEST_ANNOUNCE,
+ VIRTIO_NET_F_MQ,
+ VIRTIO_NET_F_CTRL_MAC_ADDR
+ };
+ unsigned int features_for_guest_csum[] = {
+ VIRTIO_NET_F_GUEST_TSO4,
+ VIRTIO_NET_F_GUEST_TSO6,
+ VIRTIO_NET_F_GUEST_ECN,
+ };
+ unsigned int features_for_host_csum[] = {
+ VIRTIO_NET_F_HOST_TSO4,
+ VIRTIO_NET_F_HOST_TSO6,
+ VIRTIO_NET_F_HOST_ECN,
+ };
+ int err;
+
+ err = virtnet_validate_features(dev, features_for_ctrl_vq,
+ ARRAY_SIZE(features_for_ctrl_vq),
+ VIRTIO_NET_F_CTRL_VQ);
+ if (err)
+ return err;
+
+ err = virtnet_validate_features(dev, features_for_guest_csum,
+ ARRAY_SIZE(features_for_guest_csum),
+ VIRTIO_NET_F_GUEST_CSUM);
+ if (err)
+ return err;
+
+ err = virtnet_validate_features(dev, features_for_host_csum,
+ ARRAY_SIZE(features_for_host_csum),
+ VIRTIO_NET_F_CSUM);
+ if (err)
+ return err;
+
+ if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
+ (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
+ !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
+ dev_err(&dev->dev,
+ "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
+ VIRTIO_NET_F_GUEST_ECN,
+ VIRTIO_NET_F_GUEST_TSO4,
+ VIRTIO_NET_F_GUEST_TSO6);
+ return -EINVAL;
+ }
+
+ if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
+ (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
+ !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
+ dev_err(&dev->dev,
+ "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
+ VIRTIO_NET_F_HOST_ECN,
+ VIRTIO_NET_F_HOST_TSO4,
+ VIRTIO_NET_F_HOST_TSO6);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int virtnet_probe(struct virtio_device *vdev)
{
int i, err;
@@ -1680,6 +1767,10 @@ static int virtnet_probe(struct virtio_device *vdev)
struct virtnet_info *vi;
u16 max_queue_pairs;
+ err = virtnet_check_features(vdev);
+ if (err)
+ return -EINVAL;
+
/* Find if host supports multiqueue virtio_net device */
err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
struct virtio_net_config,
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] [net]ip_tunnel: the lack of vti_link_ops' dellink() cause kernel panic
From: lucien xin @ 2014-11-19 7:10 UTC (permalink / raw)
To: Cong Wang; +Cc: network dev
In-Reply-To: <CAHA+R7N7x5eMKFtyWpOcM_FGJ+9h-K+wLEwzpZya1kiztUo8RQ@mail.gmail.com>
>>
>
> Hmm, your fix is correct, but the description is wrong, we should just skip
> the unregister of fb tunnel device like other tunnels. Without ->dellink()
> the fb device is still removed from the global netdev list, this needs to skip,
> otherwise ip_vti0 will disappear from your system.
yes, you are right. the issue only exist in the fb tunnel device, I will change
the description.
^ permalink raw reply
* Re: [PATCH net] virtio-net: validate features during probe
From: Jason Wang @ 2014-11-19 7:07 UTC (permalink / raw)
To: rusty, mst, virtualization, netdev, linux-kernel
In-Reply-To: <1416378939-28821-1-git-send-email-jasowang@redhat.com>
On 11/19/2014 02:35 PM, Jason Wang wrote:
> This patch validates feature dependencies during probe and fail the probing
> if a dependency is missed. This fixes the issues of hitting BUG()
> when qemu fails to advertise features correctly. One example is booting
> guest with ctrl_vq=off through qemu.
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
> Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
Just notice UFO has been disabled after
3d0ad09412ffe00c9afa201d01effdb6023d09b4 "drivers/net: Disable UFO
through virtio". Will post V2 and drop VIRTIO_NET_F_*_UFO from the
checklist.
^ permalink raw reply
* Re: [PATCH] bonding: clear header_ops when last slave detached (v2)
From: Wengang @ 2014-11-19 7:00 UTC (permalink / raw)
To: Eric Dumazet; +Cc: netdev
In-Reply-To: <1416375565.14060.43.camel@edumazet-glaptop2.roam.corp.google.com>
Hi Eric,
于 2014年11月19日 13:39, Eric Dumazet 写道:
> On Wed, 2014-11-19 at 13:18 +0800, Wengang Wang wrote:
>> When last slave of a bonding master is removed, the bonding then does not work.
>> When packet_snd is called against with a master net_device, it accesses
>> header_ops. In case the header_ops is not valid any longer(say ipoib module
>> unloaded), it will then access an invalid memory address.
>> This patch try to fix this issue by clearing header_ops when last slave
>> detached.
>>
>> Signed-off-by: Wengang Wang <wen.gang.wang@oracle.com>
>> ---
>> drivers/net/bonding/bond_main.c | 1 +
>> 1 file changed, 1 insertion(+)
> It seems you basically ignored my feedback.
>
>
>
> Some code is doing :
>
> [A] if (dev->header_ops) {
> ...
> [B] dev->header_ops->XXX()
>
>
>
> Nothing prevents you doing the clear after [A] , and before [B]
>
Yes, that's true. So the simplest way is move ipoib_header_ops to vmlinux.
thanks for review.
wengang
^ permalink raw reply
* Re: [BNX2] A Netdev Watchdog with kernel stable 3.4
From: Michael Chan @ 2014-11-19 6:58 UTC (permalink / raw)
To: Rui Xiang, sony.chacko; +Cc: netdev
In-Reply-To: <546C388B.2050603@huawei.com>
Copying the current maintainer Sony. The PCI command register looks
strange. Please see below.
On Wed, 2014-11-19 at 14:28 +0800, Rui Xiang wrote:
> ping...
>
> On 2014/11/17 20:42, Rui Xiang wrote:
> > Hi Michael,
> >
> > On a system that was running stable 3.4.87, I got the below stack.
> > That was a NETDEV WATCHDOG. And we could also see watchdog timeouts with the
> > BNX2. (After the stack, an oops occurred while running ifconfig. I think it
> > would be related to this timeout.)
> >
> > Otherwises, the bnx2_dump_state and bnx2_dump_mcp_state have printed the states.
> > Through these states info, can we got the real situation of NIC1.
> > Or can we see what resulted the WATCHDOG, a bnx2 device fault or other reasons.
> >
> > Thanks.
> >
> >
> > *The stack*:
> >
> > WARNING: at /usr/src/packages/BUILD/kernel-default-3.4.87/linux-3.4/net/sched/sch_generic.c:256 dev_watchdog+0x256/0x260()
> > NETDEV WATCHDOG: NIC1 (bnx2): transmit queue 3 timed out
> > Modules linked in: smb3_failover(O) smb2(O) smb(O) smb_manager(O) nfs(O) nfs_acl(O) nfsd(O) lockd(O) nal(O) auth_rpcgss(O)
> > scsi_dh_hp_sw scsi_dh_alua scsi_dh_emc scsi_dh_rdac scsi_dh scsi_mod [last unloaded: ipmi_msghandler]
> > Pid: 0, comm: swapper/0 Tainted: P W O 3.4.87-default #1
> > Call Trace:
> > <IRQ> [<ffffffff8103fcea>] warn_slowpath_common+0x7a/0xb0
> > [<ffffffff8103fdc1>] warn_slowpath_fmt+0x41/0x50
> > [<ffffffff81047749>] ? raise_softirq_irqoff+0x9/0x30
> > [<ffffffff813ae0f6>] dev_watchdog+0x256/0x260
> > [<ffffffff813adea0>] ? dev_deactivate_queue.constprop.30+0x70/0x70
> > [<ffffffff8104edc7>] run_timer_softirq+0x147/0x340
> > [<ffffffff810470d8>] __do_softirq+0xc8/0x1e0
> > [<ffffffff8109250f>] ? tick_program_event+0x1f/0x30
> > [<ffffffff81460a6c>] call_softirq+0x1c/0x30
> > [<ffffffff8100417d>] do_softirq+0x9d/0xd0
> > [<ffffffff810474a5>] irq_exit+0xb5/0xc0
> > [<ffffffff81021b49>] smp_apic_timer_interrupt+0x69/0xa0
> > [<ffffffff8146006f>] apic_timer_interrupt+0x6f/0x80
> > <EOI> [<ffffffff81457bdd>] ? retint_restore_args+0x13/0x13
> > [<ffffffff81360149>] ? poll_idle+0x49/0x90
> > [<ffffffff8136011f>] ? poll_idle+0x1f/0x90
> > [<ffffffff8135fcc9>] cpuidle_enter+0x19/0x20
> > [<ffffffff813602f2>] cpuidle_idle_call+0xa2/0x250
> > [<ffffffff8100b08f>] cpu_idle+0x6f/0xe0
> > [<ffffffff81915960>] ? rawsock_init+0x12/0x12
> > [<ffffffff814331c9>] rest_init+0x6d/0x74
> > [<ffffffff818d3be5>] start_kernel+0x3a2/0x3af
> > [<ffffffff818d3642>] ? repair_env_string+0x5e/0x5e
> > [<ffffffff818d332a>] x86_64_start_reservations+0x131/0x135
> > [<ffffffff818d342e>] x86_64_start_kernel+0x100/0x10f
> > ---[ end trace 497e24e681e0c02d ]---
> > bnx2 0000:05:00.1: NIC1: DEBUG: intr_sem[0] PCI_CMD[00100002]
The memory bit in PCI_CMD is set, but the bus master bit is not set.
DMA won't work if the bus master bit is not set. What was happening
before the timeout? Was it working fine for a while and it suddenly
stopped?
> > bnx2 0000:05:00.1: NIC1: DEBUG: PCI_PM[19002008] PCI_MISC_CFG[92000088]
> > bnx2 0000:05:00.1: NIC1: DEBUG: EMAC_TX_STATUS[00000008] EMAC_RX_STATUS[00000000]
> > bnx2 0000:05:00.1: NIC1: DEBUG: RPM_MGMT_PKT_CTRL[40000088]
> > bnx2 0000:05:00.1: NIC1: DEBUG: HC_STATS_INTERRUPT_STATUS[01ff0000]
> > bnx2 0000:05:00.1: NIC1: DEBUG: PBA[00000000]
> > bnx2 0000:05:00.1: NIC1: <--- start MCP states dump --->
> > bnx2 0000:05:00.1: NIC1: DEBUG: MCP_STATE_P0[0003e10e] MCP_STATE_P1[0003e10e]
> > bnx2 0000:05:00.1: NIC1: DEBUG: MCP mode[0000b800] state[80008000] evt_mask[00000500]
> > bnx2 0000:05:00.1: NIC1: DEBUG: pc[08008f60] pc[0800d21c] instr[00051080]
> > bnx2 0000:05:00.1: NIC1: DEBUG: shmem states:
> > bnx2 0000:05:00.1: NIC1: DEBUG: drv_mb[01030003] fw_mb[00000003] link_status[0000006f] drv_pulse_mb[0000073d]
> > bnx2 0000:05:00.1: NIC1: DEBUG: dev_info_signature[44564907] reset_type[01005254] condition[0003e10e]
> > bnx2 0000:05:00.1: NIC1: DEBUG: 000003cc: 00000000 00000000 00000000 00000000
> > bnx2 0000:05:00.1: NIC1: DEBUG: 000003dc: 00000000 00000000 00000000 00000000
> > bnx2 0000:05:00.1: NIC1: DEBUG: 000003ec: 00000000 00000000 00000000 00000000
> > bnx2 0000:05:00.1: NIC1: DEBUG: 0x3fc[00000000]
> > bnx2 0000:05:00.1: NIC1: <--- end MCP states dump --->
> >
>
>
^ permalink raw reply
* [PATCH net] tcp: fix connect() invalid -EADDRNOTAVAIL error
From: Jon Maxwell @ 2014-11-19 6:37 UTC (permalink / raw)
To: davem
Cc: kuznet, jmorris, yoshfuji, kaber, netdev, linux-kernel, jmaxwell,
Jon Maxwell
The connect() routine returns -EADDRNOTAVAIL without doing a 4
tuple check when the hash buckets were previously allocated by
bind() and all local ports are used.
The bind() routine creates the local port hash buckets in
inet_csk_get_port(). Depending on the socket options it sets
tb->fastreuse and tb->fastreuseport to 0 or 1 in the bucket.
However the __inet_hash_connect() routine initializes the hash
buckets differently and sets these to -1. The end result is
that connect() calling into __inet_hash_connect() will
subsequently ignore the check_established() routine if, here
__inet_hash_connect()
.
.
if (tb->fastreuse >= 0 ||↩
tb->fastreuseport >= 0)↩
goto next_port;
and cycle through all local ports until it returns -EADDRNOTAVAIL.
The 4 tuple check is in check_established() so connect() can fail
unnecessarily.
Prerequisites for this to happen:
1) The local tcp port range must be exhausted.
2) A process must have called bind() followed by connect() for all
local ports.
3) A different process calls connect() only which returns -EADDRNOTAVAIL.
4) The system more than 1 interface configured.
If a system has 2 IP Addresses and all local tcp ports are in use
for connection from IP Address (1). Connecting to the same ports
via IP Address (2) should work based on the 4 tuple rule. But it
fails under this condition.
To fix this make __inet_hash_connect() honour inet_csk_get_port()'s
tb->fastreuse* variables.
Signed-off-by: Jon Maxwell <jmaxwell37@gmail.com>
---
net/ipv4/inet_hashtables.c | 6 ++----
1 file changed, 2 insertions(+), 4 deletions(-)
diff --git a/net/ipv4/inet_hashtables.c b/net/ipv4/inet_hashtables.c
index 9111a4e..b39e89e 100644
--- a/net/ipv4/inet_hashtables.c
+++ b/net/ipv4/inet_hashtables.c
@@ -513,8 +513,8 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
inet_bind_bucket_for_each(tb, &head->chain) {
if (net_eq(ib_net(tb), net) &&
tb->port == port) {
- if (tb->fastreuse >= 0 ||
- tb->fastreuseport >= 0)
+ if (tb->fastreuse > 0 ||
+ tb->fastreuseport > 0)
goto next_port;
WARN_ON(hlist_empty(&tb->owners));
if (!check_established(death_row, sk,
@@ -530,8 +530,6 @@ int __inet_hash_connect(struct inet_timewait_death_row *death_row,
spin_unlock(&head->lock);
break;
}
- tb->fastreuse = -1;
- tb->fastreuseport = -1;
goto ok;
next_port:
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next 3/4] igb: enable internal PPS for the i210.
From: Richard Cochran @ 2014-11-19 6:37 UTC (permalink / raw)
To: netdev
Cc: David Miller, bruce.w.allan, Jacob Keller, Jeff Kirsher,
John Ronciak, Matthew Vick
In-Reply-To: <e5b4ef7c56119e2d86e42f5680b1b82c9ec1a38e.1416265321.git.richardcochran@gmail.com>
On Tue, Nov 18, 2014 at 12:06:24AM +0100, Richard Cochran wrote:
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5386,8 +5386,14 @@ void igb_update_stats(struct igb_adapter *adapter,
> static void igb_tsync_interrupt(struct igb_adapter *adapter)
> {
> struct e1000_hw *hw = &adapter->hw;
> + struct ptp_clock_event event;
> u32 tsicr = rd32(E1000_TSICR);
>
> + if (tsicr & TSINTR_SYS_WRAP) {
> + event.type = PTP_CLOCK_PPS;
> + ptp_clock_event(adapter->ptp_clock, &event);
This causes a BUG for the 82580. When you enable E1000_TSICR_TXTS, it
seems that TSINTR_SYS_WRAP is also automatically enabled. Because the
82580 variant has no pps device enabled, the driver then dereferences
a null pointer.
So we need to make the call to ptp_clock_event() conditional based on
whether pps is enabled. I'll fix this in V2.
> + }
> +
Thanks,
Richard
^ permalink raw reply
* [PATCH net] virtio-net: validate features during probe
From: Jason Wang @ 2014-11-19 6:35 UTC (permalink / raw)
To: rusty, mst, virtualization, netdev, linux-kernel
This patch validates feature dependencies during probe and fail the probing
if a dependency is missed. This fixes the issues of hitting BUG()
when qemu fails to advertise features correctly. One example is booting
guest with ctrl_vq=off through qemu.
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael S. Tsirkin <mst@redhat.com>
Cc: Cornelia Huck <cornelia.huck@de.ibm.com>
Cc: Wanlong Gao <gaowanlong@cn.fujitsu.com>
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
1 file changed, 93 insertions(+)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index ec2a8b4..4a0ad46 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -1673,6 +1673,95 @@ static const struct attribute_group virtio_net_mrg_rx_group = {
};
#endif
+static int virtnet_validate_features(struct virtio_device *dev,
+ unsigned int *table,
+ int table_size,
+ unsigned int feature)
+{
+ int i;
+
+ if (!virtio_has_feature(dev, feature)) {
+ for (i = 0; i < table_size; i++) {
+ unsigned int f = table[i];
+
+ if (virtio_has_feature(dev, f)) {
+ dev_err(&dev->dev,
+ "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x was not",
+ f, feature);
+ return -EINVAL;
+ }
+ }
+ }
+
+ return 0;
+}
+
+static int virtnet_check_features(struct virtio_device *dev)
+{
+ unsigned int features_for_ctrl_vq[] = {
+ VIRTIO_NET_F_CTRL_RX,
+ VIRTIO_NET_F_CTRL_VLAN,
+ VIRTIO_NET_F_GUEST_ANNOUNCE,
+ VIRTIO_NET_F_MQ,
+ VIRTIO_NET_F_CTRL_MAC_ADDR
+ };
+ unsigned int features_for_guest_csum[] = {
+ VIRTIO_NET_F_GUEST_TSO4,
+ VIRTIO_NET_F_GUEST_TSO6,
+ VIRTIO_NET_F_GUEST_ECN,
+ VIRTIO_NET_F_GUEST_UFO,
+ };
+ unsigned int features_for_host_csum[] = {
+ VIRTIO_NET_F_HOST_TSO4,
+ VIRTIO_NET_F_HOST_TSO6,
+ VIRTIO_NET_F_HOST_ECN,
+ VIRTIO_NET_F_HOST_UFO,
+ };
+ int err;
+
+ err = virtnet_validate_features(dev, features_for_ctrl_vq,
+ ARRAY_SIZE(features_for_ctrl_vq),
+ VIRTIO_NET_F_CTRL_VQ);
+ if (err)
+ return err;
+
+ err = virtnet_validate_features(dev, features_for_guest_csum,
+ ARRAY_SIZE(features_for_guest_csum),
+ VIRTIO_NET_F_GUEST_CSUM);
+ if (err)
+ return err;
+
+ err = virtnet_validate_features(dev, features_for_host_csum,
+ ARRAY_SIZE(features_for_host_csum),
+ VIRTIO_NET_F_CSUM);
+ if (err)
+ return err;
+
+ if (virtio_has_feature(dev, VIRTIO_NET_F_GUEST_ECN) &&
+ (!virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO4) ||
+ !virtio_has_feature(dev, VIRTIO_NET_F_GUEST_TSO6))) {
+ dev_err(&dev->dev,
+ "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
+ VIRTIO_NET_F_GUEST_ECN,
+ VIRTIO_NET_F_GUEST_TSO4,
+ VIRTIO_NET_F_GUEST_TSO6);
+ return -EINVAL;
+ }
+
+ if (virtio_has_feature(dev, VIRTIO_NET_F_HOST_ECN) &&
+ (!virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO4) ||
+ !virtio_has_feature(dev, VIRTIO_NET_F_HOST_TSO6))) {
+ dev_err(&dev->dev,
+ "buggy hyperviser: feature 0x%x was advertised but its dependency 0x%x or 0x%x was not",
+ VIRTIO_NET_F_HOST_ECN,
+ VIRTIO_NET_F_HOST_TSO4,
+ VIRTIO_NET_F_HOST_TSO6);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
static int virtnet_probe(struct virtio_device *vdev)
{
int i, err;
@@ -1680,6 +1769,10 @@ static int virtnet_probe(struct virtio_device *vdev)
struct virtnet_info *vi;
u16 max_queue_pairs;
+ err = virtnet_check_features(vdev);
+ if (err)
+ return -EINVAL;
+
/* Find if host supports multiqueue virtio_net device */
err = virtio_cread_feature(vdev, VIRTIO_NET_F_MQ,
struct virtio_net_config,
--
1.9.1
^ permalink raw reply related
* Re: [PATCH net-next 0/4] igb: auxiliary PHC functions for the i210.
From: Richard Cochran @ 2014-11-19 6:31 UTC (permalink / raw)
To: Jeff Kirsher
Cc: netdev, David Miller, bruce.w.allan, Jacob Keller, John Ronciak,
Matthew Vick
In-Reply-To: <1416266118.2983.3.camel@jtkirshe-mobl.jf.intel.com>
On Mon, Nov 17, 2014 at 03:15:18PM -0800, Jeff Kirsher wrote:
> Thanks Richard, I have added your series of igb patches to my queue.
Jeff, please hold off on these. I found a bug in the PPS code. I will
re-submit with a fix.
Thanks,
Richard
^ permalink raw reply
* Re: [BNX2] A Netdev Watchdog with kernel stable 3.4
From: Rui Xiang @ 2014-11-19 6:28 UTC (permalink / raw)
To: Michael Chan; +Cc: netdev
In-Reply-To: <5469ED24.7010008@huawei.com>
ping...
On 2014/11/17 20:42, Rui Xiang wrote:
> Hi Michael,
>
> On a system that was running stable 3.4.87, I got the below stack.
> That was a NETDEV WATCHDOG. And we could also see watchdog timeouts with the
> BNX2. (After the stack, an oops occurred while running ifconfig. I think it
> would be related to this timeout.)
>
> Otherwises, the bnx2_dump_state and bnx2_dump_mcp_state have printed the states.
> Through these states info, can we got the real situation of NIC1.
> Or can we see what resulted the WATCHDOG, a bnx2 device fault or other reasons.
>
> Thanks.
>
>
> *The stack*:
>
> WARNING: at /usr/src/packages/BUILD/kernel-default-3.4.87/linux-3.4/net/sched/sch_generic.c:256 dev_watchdog+0x256/0x260()
> NETDEV WATCHDOG: NIC1 (bnx2): transmit queue 3 timed out
> Modules linked in: smb3_failover(O) smb2(O) smb(O) smb_manager(O) nfs(O) nfs_acl(O) nfsd(O) lockd(O) nal(O) auth_rpcgss(O)
> scsi_dh_hp_sw scsi_dh_alua scsi_dh_emc scsi_dh_rdac scsi_dh scsi_mod [last unloaded: ipmi_msghandler]
> Pid: 0, comm: swapper/0 Tainted: P W O 3.4.87-default #1
> Call Trace:
> <IRQ> [<ffffffff8103fcea>] warn_slowpath_common+0x7a/0xb0
> [<ffffffff8103fdc1>] warn_slowpath_fmt+0x41/0x50
> [<ffffffff81047749>] ? raise_softirq_irqoff+0x9/0x30
> [<ffffffff813ae0f6>] dev_watchdog+0x256/0x260
> [<ffffffff813adea0>] ? dev_deactivate_queue.constprop.30+0x70/0x70
> [<ffffffff8104edc7>] run_timer_softirq+0x147/0x340
> [<ffffffff810470d8>] __do_softirq+0xc8/0x1e0
> [<ffffffff8109250f>] ? tick_program_event+0x1f/0x30
> [<ffffffff81460a6c>] call_softirq+0x1c/0x30
> [<ffffffff8100417d>] do_softirq+0x9d/0xd0
> [<ffffffff810474a5>] irq_exit+0xb5/0xc0
> [<ffffffff81021b49>] smp_apic_timer_interrupt+0x69/0xa0
> [<ffffffff8146006f>] apic_timer_interrupt+0x6f/0x80
> <EOI> [<ffffffff81457bdd>] ? retint_restore_args+0x13/0x13
> [<ffffffff81360149>] ? poll_idle+0x49/0x90
> [<ffffffff8136011f>] ? poll_idle+0x1f/0x90
> [<ffffffff8135fcc9>] cpuidle_enter+0x19/0x20
> [<ffffffff813602f2>] cpuidle_idle_call+0xa2/0x250
> [<ffffffff8100b08f>] cpu_idle+0x6f/0xe0
> [<ffffffff81915960>] ? rawsock_init+0x12/0x12
> [<ffffffff814331c9>] rest_init+0x6d/0x74
> [<ffffffff818d3be5>] start_kernel+0x3a2/0x3af
> [<ffffffff818d3642>] ? repair_env_string+0x5e/0x5e
> [<ffffffff818d332a>] x86_64_start_reservations+0x131/0x135
> [<ffffffff818d342e>] x86_64_start_kernel+0x100/0x10f
> ---[ end trace 497e24e681e0c02d ]---
> bnx2 0000:05:00.1: NIC1: DEBUG: intr_sem[0] PCI_CMD[00100002]
> bnx2 0000:05:00.1: NIC1: DEBUG: PCI_PM[19002008] PCI_MISC_CFG[92000088]
> bnx2 0000:05:00.1: NIC1: DEBUG: EMAC_TX_STATUS[00000008] EMAC_RX_STATUS[00000000]
> bnx2 0000:05:00.1: NIC1: DEBUG: RPM_MGMT_PKT_CTRL[40000088]
> bnx2 0000:05:00.1: NIC1: DEBUG: HC_STATS_INTERRUPT_STATUS[01ff0000]
> bnx2 0000:05:00.1: NIC1: DEBUG: PBA[00000000]
> bnx2 0000:05:00.1: NIC1: <--- start MCP states dump --->
> bnx2 0000:05:00.1: NIC1: DEBUG: MCP_STATE_P0[0003e10e] MCP_STATE_P1[0003e10e]
> bnx2 0000:05:00.1: NIC1: DEBUG: MCP mode[0000b800] state[80008000] evt_mask[00000500]
> bnx2 0000:05:00.1: NIC1: DEBUG: pc[08008f60] pc[0800d21c] instr[00051080]
> bnx2 0000:05:00.1: NIC1: DEBUG: shmem states:
> bnx2 0000:05:00.1: NIC1: DEBUG: drv_mb[01030003] fw_mb[00000003] link_status[0000006f] drv_pulse_mb[0000073d]
> bnx2 0000:05:00.1: NIC1: DEBUG: dev_info_signature[44564907] reset_type[01005254] condition[0003e10e]
> bnx2 0000:05:00.1: NIC1: DEBUG: 000003cc: 00000000 00000000 00000000 00000000
> bnx2 0000:05:00.1: NIC1: DEBUG: 000003dc: 00000000 00000000 00000000 00000000
> bnx2 0000:05:00.1: NIC1: DEBUG: 000003ec: 00000000 00000000 00000000 00000000
> bnx2 0000:05:00.1: NIC1: DEBUG: 0x3fc[00000000]
> bnx2 0000:05:00.1: NIC1: <--- end MCP states dump --->
>
^ permalink raw reply
* Re: [PATCH net] bonding: fix curr_active_slave/carrier with loadbalance arp monitoring
From: Ding Tianhong @ 2014-11-19 6:20 UTC (permalink / raw)
To: Andy Gospodarek, Veaceslav Falico
Cc: Nikolay Aleksandrov, netdev, davem, Jay Vosburgh, Andy Gospodarek
In-Reply-To: <20141118152847.GB2002@gospo.home.greyhouse.net>
On 2014/11/18 23:28, Andy Gospodarek wrote:
> On Tue, Nov 18, 2014 at 03:37:27PM +0100, Veaceslav Falico wrote:
>> On Tue, Nov 18, 2014 at 03:14:44PM +0100, Nikolay Aleksandrov wrote:
>>> Since commit 6fde8f037e60 ("bonding: fix locking in
>>> bond_loadbalance_arp_mon()") we can have a stale bond carrier state and
>>> stale curr_active_slave when using arp monitoring in loadbalance modes. The
>>> reason is that in bond_loadbalance_arp_mon() we can't have
>>> do_failover == true but slave_state_changed == false, whenever do_failover
>>> is true then slave_state_changed is also true. Then the following piece
>> >from bond_loadbalance_arp_mon():
>>> if (slave_state_changed) {
>>> bond_slave_state_change(bond);
>>> if (BOND_MODE(bond) == BOND_MODE_XOR)
>>> bond_update_slave_arr(bond, NULL);
>>> } else if (do_failover) {
>>
>> Ouch, must have been a big PITA to track :).
>
> Agreed!
>
>>
>>> block_netpoll_tx();
>>> bond_select_active_slave(bond);
>>> unblock_netpoll_tx();
>>> }
>>>
>>> will execute only the first branch, always and regardless of do_failover.
>>> Since these two events aren't related in such way, we need to decouple and
>>> consider them separately.
>>>
>>> For example this issue could lead to the following result:
>>> Bonding Mode: load balancing (round-robin)
>>> *MII Status: down*
>>> MII Polling Interval (ms): 0
>>> Up Delay (ms): 0
>>> Down Delay (ms): 0
>>> ARP Polling Interval (ms): 100
>>> ARP IP target/s (n.n.n.n form): 192.168.9.2
>>>
>>> Slave Interface: ens12
>>> *MII Status: up*
>>> Speed: 10000 Mbps
>>> Duplex: full
>>> Link Failure Count: 2
>>> Permanent HW addr: 00:0f:53:01:42:2c
>>> Slave queue ID: 0
>>>
>>> Slave Interface: eth1
>>> *MII Status: up*
>>> Speed: Unknown
>>> Duplex: Unknown
>>> Link Failure Count: 70
>>> Permanent HW addr: 52:54:00:2f:0f:8e
>>> Slave queue ID: 0
>>>
>>> Since some interfaces are up, then the status of the bond should also be
>>> up, but it will never change unless something invokes bond_set_carrier()
>>> (i.e. enslave, bond_select_active_slave etc). Now, if I force the
>>> calling of bond_select_active_slave via for example changing
>>> primary_reselect (it can change in any mode), then the MII status goes to
>>> "up" because it calls bond_select_active_slave() which should've been done
>> >from bond_loadbalance_arp_mon() itself.
>>>
>>> CC: Veaceslav Falico <vfalico@gmail.com>
>>
>> Acked-by: Veaceslav Falico <vfalico@gmail.com>
>
> Acked-by: Andy Gospodarek <gospo@cumulusnetworks.com>
>
Acked-by: Ding Tianhong <dingtianhong@huawei.com>
>>
>>> CC: Jay Vosburgh <j.vosburgh@gmail.com>
>>> CC: Andy Gospodarek <andy@greyhouse.net>
>>> CC: Ding Tianhong <dingtianhong@huawei.com>
>>>
>>> Fixes: 6fde8f037e60 ("bonding: fix locking in bond_loadbalance_arp_mon()")
>>> Signed-off-by: Nikolay Aleksandrov <nikolay@redhat.com>
>>> ---
>>> Note: I left the parent if() the same even though we can shorten it. I think
>>> it's better this way since it shows that any of the two events can cause
>>> it to enter even though currently we can't have do_failover without
>>> slave_state_changed, that may also change in the future.
>>>
>>> drivers/net/bonding/bond_main.c | 3 ++-
>>> 1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/net/bonding/bond_main.c b/drivers/net/bonding/bond_main.c
>>> index c9ac06cfe6b7..a5115fb7cf33 100644
>>> --- a/drivers/net/bonding/bond_main.c
>>> +++ b/drivers/net/bonding/bond_main.c
>>> @@ -2471,7 +2471,8 @@ static void bond_loadbalance_arp_mon(struct work_struct *work)
>>> bond_slave_state_change(bond);
>>> if (BOND_MODE(bond) == BOND_MODE_XOR)
>>> bond_update_slave_arr(bond, NULL);
>>> - } else if (do_failover) {
>>> + }
>>> + if (do_failover) {
>>> block_netpoll_tx();
>>> bond_select_active_slave(bond);
>>> unblock_netpoll_tx();
>>> --
>>> 1.9.3
>>>
>> --
>> To unsubscribe from this list: send the line "unsubscribe netdev" in
>> the body of a message to majordomo@vger.kernel.org
>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
> .
>
^ permalink raw reply
* Re: [PATCH v2 net-next] tcp: make connect() mem charging friendly
From: Yuchung Cheng @ 2014-11-19 6:10 UTC (permalink / raw)
To: Eric Dumazet; +Cc: Denys Fedoryshchenko, David Miller, netdev, Neal Cardwell
In-Reply-To: <1416294380.14060.6.camel@edumazet-glaptop2.roam.corp.google.com>
On Tue, Nov 18, 2014 at 3:06 PM, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> From: Eric Dumazet <edumazet@google.com>
>
> While working on sk_forward_alloc problems reported by Denys
> Fedoryshchenko, we found that tcp connect() (and fastopen) do not call
> sk_wmem_schedule() for SYN packet (and/or SYN/DATA packet), so
> sk_forward_alloc is negative while connect is in progress.
>
> We can fix this by calling regular sk_stream_alloc_skb() both for the
> SYN packet (in tcp_connect()) and the syn_data packet in
> tcp_send_syn_data()
>
> Then, tcp_send_syn_data() can avoid copying syn_data as we simply
> can manipulate syn_data->cb[] to remove SYN flag (and increment seq)
>
> Instead of open coding memcpy_fromiovecend(), simply use this helper.
>
> This leaves in socket write queue clean fast clone skbs.
>
> This was tested against our fastopen packetdrill tests.
>
> Reported-by: Denys Fedoryshchenko <nuclearcat@nuclearcat.com>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
Acked-by: Yuchung Cheng <ycheng@google.com>
Thanks! this simplifies the code a lot.
> ---
> v2: added a kfree_skb(syn_data) if memcpy_fromiovecend() fails,
> as spotted by Yuchung.
>
> net/ipv4/tcp_output.c | 68 ++++++++++++++++------------------------
> 1 file changed, 28 insertions(+), 40 deletions(-)
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index eb73a1dccf56b823a45c0ca034e40dc50fc48068..f5bd4bd3f7e669b3fd48a843d55e7313a30a3409 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -3011,9 +3011,9 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn)
> {
> struct tcp_sock *tp = tcp_sk(sk);
> struct tcp_fastopen_request *fo = tp->fastopen_req;
> - int syn_loss = 0, space, i, err = 0, iovlen = fo->data->msg_iovlen;
> - struct sk_buff *syn_data = NULL, *data;
> + int syn_loss = 0, space, err = 0;
> unsigned long last_syn_loss = 0;
> + struct sk_buff *syn_data;
>
> tp->rx_opt.mss_clamp = tp->advmss; /* If MSS is not cached */
> tcp_fastopen_cache_get(sk, &tp->rx_opt.mss_clamp, &fo->cookie,
> @@ -3044,48 +3044,40 @@ static int tcp_send_syn_data(struct sock *sk, struct sk_buff *syn)
> /* limit to order-0 allocations */
> space = min_t(size_t, space, SKB_MAX_HEAD(MAX_TCP_HEADER));
>
> - syn_data = skb_copy_expand(syn, MAX_TCP_HEADER, space,
> - sk->sk_allocation);
> - if (syn_data == NULL)
> + syn_data = sk_stream_alloc_skb(sk, space, sk->sk_allocation);
> + if (!syn_data)
> goto fallback;
> + syn_data->ip_summed = CHECKSUM_PARTIAL;
> + memcpy(syn_data->cb, syn->cb, sizeof(syn->cb));
> + if (unlikely(memcpy_fromiovecend(skb_put(syn_data, space),
> + fo->data->msg_iov, 0, space))) {
> + kfree_skb(syn_data);
> + goto fallback;
> + }
>
> - for (i = 0; i < iovlen && syn_data->len < space; ++i) {
> - struct iovec *iov = &fo->data->msg_iov[i];
> - unsigned char __user *from = iov->iov_base;
> - int len = iov->iov_len;
> + /* No more data pending in inet_wait_for_connect() */
> + if (space == fo->size)
> + fo->data = NULL;
> + fo->copied = space;
>
> - if (syn_data->len + len > space)
> - len = space - syn_data->len;
> - else if (i + 1 == iovlen)
> - /* No more data pending in inet_wait_for_connect() */
> - fo->data = NULL;
> + tcp_connect_queue_skb(sk, syn_data);
>
> - if (skb_add_data(syn_data, from, len))
> - goto fallback;
> - }
> + err = tcp_transmit_skb(sk, syn_data, 1, sk->sk_allocation);
>
> - /* Queue a data-only packet after the regular SYN for retransmission */
> - data = pskb_copy(syn_data, sk->sk_allocation);
> - if (data == NULL)
> - goto fallback;
> - TCP_SKB_CB(data)->seq++;
> - TCP_SKB_CB(data)->tcp_flags &= ~TCPHDR_SYN;
> - TCP_SKB_CB(data)->tcp_flags = (TCPHDR_ACK|TCPHDR_PSH);
> - tcp_connect_queue_skb(sk, data);
> - fo->copied = data->len;
> -
> - /* syn_data is about to be sent, we need to take current time stamps
> - * for the packets that are in write queue : SYN packet and DATA
> - */
> - skb_mstamp_get(&syn->skb_mstamp);
> - data->skb_mstamp = syn->skb_mstamp;
> + syn->skb_mstamp = syn_data->skb_mstamp;
>
> - if (tcp_transmit_skb(sk, syn_data, 0, sk->sk_allocation) == 0) {
> + /* Now full SYN+DATA was cloned and sent (or not),
> + * remove the SYN from the original skb (syn_data)
> + * we keep in write queue in case of a retransmit, as we
> + * also have the SYN packet (with no data) in the same queue.
> + */
> + TCP_SKB_CB(syn_data)->seq++;
> + TCP_SKB_CB(syn_data)->tcp_flags = TCPHDR_ACK | TCPHDR_PSH;
> + if (!err) {
> tp->syn_data = (fo->copied > 0);
> NET_INC_STATS(sock_net(sk), LINUX_MIB_TCPORIGDATASENT);
> goto done;
> }
> - syn_data = NULL;
>
> fallback:
> /* Send a regular SYN with Fast Open cookie request option */
> @@ -3094,7 +3086,6 @@ fallback:
> err = tcp_transmit_skb(sk, syn, 1, sk->sk_allocation);
> if (err)
> tp->syn_fastopen = 0;
> - kfree_skb(syn_data);
> done:
> fo->cookie.len = -1; /* Exclude Fast Open option for SYN retries */
> return err;
> @@ -3114,13 +3105,10 @@ int tcp_connect(struct sock *sk)
> return 0;
> }
>
> - buff = alloc_skb_fclone(MAX_TCP_HEADER + 15, sk->sk_allocation);
> - if (unlikely(buff == NULL))
> + buff = sk_stream_alloc_skb(sk, 0, sk->sk_allocation);
> + if (unlikely(!buff))
> return -ENOBUFS;
>
> - /* Reserve space for headers. */
> - skb_reserve(buff, MAX_TCP_HEADER);
> -
> tcp_init_nondata_skb(buff, tp->write_seq++, TCPHDR_SYN);
> tp->retrans_stamp = tcp_time_stamp;
> tcp_connect_queue_skb(sk, buff);
>
>
^ permalink raw reply
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