* [PATCH 1/2] net: team: fix memory leak in team_nl_send_port_list_get
From: Pan Bian @ 2017-04-24 7:04 UTC (permalink / raw)
To: Jiri Pirko, netdev; +Cc: linux-kernel, Pan Bian
In function team_nl_send_port_list_get(), pointer skb keeps the return
value of nlmsg_new(). When the call to genlmsg_put() fails, the memory
is not freed. This will result in a memory leak bug. This patch fixes
it.
Fixes: fbd69cda90e7 ("team: fix memory leak")
Signed-off-by: Pan Bian <bianpan2016@163.com>
---
drivers/net/team/team.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
index f8c81f1..dd3a2e9 100644
--- a/drivers/net/team/team.c
+++ b/drivers/net/team/team.c
@@ -2634,8 +2634,10 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
hdr = genlmsg_put(skb, portid, seq, &team_nl_family, flags | NLM_F_MULTI,
TEAM_CMD_PORT_LIST_GET);
- if (!hdr)
+ if (!hdr) {
+ nlmsg_free(skb);
return -EMSGSIZE;
+ }
if (nla_put_u32(skb, TEAM_ATTR_TEAM_IFINDEX, team->dev->ifindex))
goto nla_put_failure;
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] ipvs: explicitly forbid ipv6 service/dest creation if ipv6 mod is disabled
From: Paolo Abeni @ 2017-04-24 6:50 UTC (permalink / raw)
To: Julian Anastasov
Cc: netfilter-devel, lvs-devel, Wensong Zhang, Simon Horman, netdev
In-Reply-To: <alpine.LFD.2.20.1704221355290.1974@ja.home.ssi.bg>
Hi,
On Sat, 2017-04-22 at 14:16 +0300, Julian Anastasov wrote:
> On Thu, 20 Apr 2017, Paolo Abeni wrote:
>
> > When creating a new ipvs service, ipv6 addresses are always accepted
> > if CONFIG_IP_VS_IPV6 is enabled. On dest creation the address family
> > is not explicitly checked.
> >
> > This allows the user-space to configure ipvs services even if the
> > system is booted with ipv6.disable=1. On specific configuration, ipvs
> > can try to call ipv6 routing code at setup time, causing the kernel to
> > oops due to fib6_rules_ops being NULL.
> >
> > This change addresses the issue adding a check for the ipv6
> > module being enabled while validating ipv6 service operations and
> > adding the same validation for dest operations.
> >
> > According to git history, this issue is apparently present since
> > the introduction of ipv6 support, and the oops can be triggered
> > since commit 09571c7ae30865ad ("IPVS: Add function to determine
> > if IPv6 address is local")
> >
> > Fixes: 09571c7ae30865ad ("IPVS: Add function to determine if IPv6 address is local")
> > Signed-off-by: Paolo Abeni <pabeni@redhat.com>
>
> Looks good to me but I see two places that can benefit
> from such check:
I'm sorry for the lag, I was delayed by other notorious issues ;-)
I'm unable to trigger any crash with the patched kernel.
> - in ip_vs_genl_new_daemon() if we do not want to create IPv6 sockets
> for the sync protocol in make_send_sock() and make_receive_sock().
> Not sure if this can lead to crashes.
This one is, AFAICS, safe, because ip_vs_genl_new_daemon() calls
start_sync_thread(), which tries to create a socket of the specified
address family before doing any real action. Such operation will fail
gracefully, and overall we will get an - expected - error to userspace.
> - in ip_vs_proc_sync_conn() if we do not want backup server to accept
> IPv6 conns because they may be created even when dests are missing.
> We may use retc = 10 there. Not fatal but may eat memory for
> conns that will not be used.
If I read the above correct correctly, even that should be safe, for
the same reason: ipv6 socket creation will fail if ipv6 is disabled.
The problem with the patched code is that it tries to resolve ipv6
addresses that are not created/validated by the kernel.
Cheers,
Paolo
^ permalink raw reply
* Re: [ISSUE: sky2 - rx error] Link stops working under heavy traffic load connected to a mv88e6176
From: Rafa Corvillo @ 2017-04-24 6:45 UTC (permalink / raw)
To: netdev
In-Reply-To: <58F9FD64.80506@aoifes.com>
I resend the mail with the schema fixed. Sorry for the inconvenience.
We are working in an ARMv7 embedded system running kernel 4.9 (LEDE build).
It is an imx6 board with 2 ethernet interfaces. One of them is connected to
a Marvell switch.
The schema of the system is the following:
+-------------------+ eth0
| +--+
| | |
| Embedded system +--+
| |
| ARMv7 |
| | Marvell 88E8057(sky2) +-------------+
| +--+ +--+ +--+ eth1
| | +---------------------+ | | +------+
| +--+ CPU port +--+ mv88e6176 +--+
+------+--+---------+ | |
emulated| | | |
GPIO +--+ +--+ +--+ eth2
MDIO +-----------------------------------+ | | +------+
MDIO +--+ +--+
+-------------+
There is a bridge (br-lan) which includes eth0/eth1/eth2
If I connect the eth1/eth2, the link is up and I can do ping through it.
But, once
I start sending a heavy traffic load the link fails and the kernel sends
the
following messages:
[ 48.557140] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
length 1518
[ 48.564964] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
length 1518
[ 48.572110] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
length 1518
[ 48.579263] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
length 1518
[ 48.586417] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
length 1518
[ 48.593573] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
length 1518
[ 48.600718] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
length 1518
[ 54.877567] net_ratelimit: 6 callbacks suppressed
[ 54.882293] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
length 1518
[ 61.413552] sky2 0000:04:00.0 marvell: rx error, status 0x5f20010
length 1518
I have a modified device-tree of imx6 which includes the mdio and dsa
nodes. This
device-tree works in a kernel 4.1.6, but I know that these parts of the
kernel have
a lot of changes. The changes included for mdio and dsa in the
device-tree are the
following (diff arch/arm/boot/dts/imx6qdl-gw53xx.dtsi
arch/arm/boot/dts/imx6qdl-gw53xx-mdio.dtsi):
16a17,18
> can0 = &can1;
> ethernet0 = &fec;
21a24
> sky2 = ð1;
24a28,29
> usdhc2 = &usdhc3;
> mdio-gpio0 = &mdio0;
62a68,125
> mdio0: mdio {
> compatible = "virtual,mdio-gpio";
> #address-cells = <1>;
> #size-cells = <0>;
> /* MDC = gpio-17, MDIO = gpio-20 */
> gpios = <&gpio1 17 1
> &gpio1 20 0>;
> ethernet-phy@0 {
> compatible = "marvell,dsa";
> };
> };
>
> dsa {
> compatible = "marvell,dsa";
> #address-cells = <2>;
> #size-cells = <0>;
>
> interrupts = <10>;
> dsa,ethernet = <ð1>;
> dsa,mii-bus = <&mdio0>;
>
> switch@0 {
> #address-cells = <1>;
> #size-cells = <0>;
> reg = <0 0>; /* MDIO address 0, switch 0 in tree */
>
> port@0 {
> reg = <0>;
> label = "cpu";
> };
>
> port@1 {
> reg = <1>;
> label = "eth1";
> };
>
> port@2 {
> reg = <2>;
> label = "eth2";
> };
>
> port@3 {
> reg = <3>;
> label = "eth3";
> };
>
> port@4 {
> reg = <4>;
> label = "eth4";
> };
> };
> };
>
361a425,430
> &mdio0 {
> pinctrl-names = "default";
> pinctrl-0 = <&pinctrl_mdio>;
> status = "okay";
> };
>
363c432
< imx6qdl-gw53xx {
---
> imx6qdl-gw53xx-mdio {
448a518,524
> >;
> };
>
> pinctrl_mdio: mdiogrp {
> fsl,pins = <
> MX6QDL_PAD_SD1_DAT1__GPIO1_IO17 0x1b0b9
> MX6QDL_PAD_SD1_CLK__GPIO1_IO20 0x1b0b9
Do you know of any possible reason why this could be happening?
Thanks in advance.
Rafa
^ permalink raw reply
* Re: [PATCH v3 07/29] x86: bpf_jit, use ENTRY+ENDPROC
From: Jiri Slaby @ 2017-04-24 6:45 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: mingo, tglx, hpa, x86, jpoimboe, linux-kernel, David S. Miller,
netdev, Daniel Borkmann, Eric Dumazet
In-Reply-To: <20170421193213.GB91454@ast-mbp.thefacebook.com>
On 04/21/2017, 09:32 PM, Alexei Starovoitov wrote:
> On Fri, Apr 21, 2017 at 04:12:43PM +0200, Jiri Slaby wrote:
>> Do not use a custom macro FUNC for starts of the global functions, use
>> ENTRY instead.
>>
>> And while at it, annotate also ends of the functions by ENDPROC.
>>
>> Signed-off-by: Jiri Slaby <jslaby@suse.cz>
>> Cc: "David S. Miller" <davem@davemloft.net>
>> Cc: Alexey Kuznetsov <kuznet@ms2.inr.ac.ru>
>> Cc: James Morris <jmorris@namei.org>
>> Cc: Hideaki YOSHIFUJI <yoshfuji@linux-ipv6.org>
>> Cc: Patrick McHardy <kaber@trash.net>
>> Cc: Thomas Gleixner <tglx@linutronix.de>
>> Cc: Ingo Molnar <mingo@redhat.com>
>> Cc: "H. Peter Anvin" <hpa@zytor.com>
>> Cc: x86@kernel.org
>> Cc: netdev@vger.kernel.org
>> ---
>> arch/x86/net/bpf_jit.S | 32 ++++++++++++++++++--------------
>> 1 file changed, 18 insertions(+), 14 deletions(-)
>>
>> diff --git a/arch/x86/net/bpf_jit.S b/arch/x86/net/bpf_jit.S
>> index f2a7faf4706e..762c29fb8832 100644
>> --- a/arch/x86/net/bpf_jit.S
>> +++ b/arch/x86/net/bpf_jit.S
>> @@ -23,16 +23,12 @@
>> 32 /* space for rbx,r13,r14,r15 */ + \
>> 8 /* space for skb_copy_bits */)
>>
>> -#define FUNC(name) \
>> - .globl name; \
>> - .type name, @function; \
>> - name:
>> -
>> -FUNC(sk_load_word)
>> +ENTRY(sk_load_word)
>> test %esi,%esi
>> js bpf_slow_path_word_neg
>> +ENDPROC(sk_load_word)
>
> this doens't look right.
> It will add alignment nops in critical paths of these pseudo functions.
> I'm also not sure whether it will still work afterwards.
> Was it tested?
> I'd prefer if this code kept as-is.
It cannot stay as-is simply because we want to know where the functions
end to inject debuginfo properly. The code above does not warrant for
any exception.
Executing a nop takes a little and having externally-callable functions
aligned can actually help performance (no, I haven't measured nor tested
the code). But sure, the tool is generic, so I can introduce a local
macros to avoid alignments in the functions:
#define BPF_FUNC_START_LOCAL(name) \
SYM_START(name, SYM_V_LOCAL, SYM_A_NONE)
#define BPF_FUNC_START(name) \
SYM_START(name, SYM_V_GLOBAL, SYM_A_NONE)
#define BPF_FUNC_END(name) SYM_FUNC_END(name)
thanks,
--
js
suse labs
^ permalink raw reply
* Re: [PATCH 1/2] team: fix memory leak
From: Jiri Pirko @ 2017-04-24 5:40 UTC (permalink / raw)
To: Pan Bian; +Cc: netdev, linux-kernel
In-Reply-To: <1492932564-722-1-git-send-email-bianpan2016@163.com>
Sun, Apr 23, 2017 at 09:29:24AM CEST, bianpan2016@163.com wrote:
>In function team_nl_send_port_list_get(), pointer skb keeps the return
>value of nlmsg_new(). When the call to genlmsg_put() fails, the memory
>is not freed. This will result in a memory leak bug. This patch fixes
>it.
>
Looks good. Please adjust subject so the both patches have a specific
one. Also, please add "Fixes" tag (see git log for details).
Also, is is good to say which tree this patches are generated against
("-net")
>Signed-off-by: Pan Bian <bianpan2016@163.com>
>---
> drivers/net/team/team.c | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
>diff --git a/drivers/net/team/team.c b/drivers/net/team/team.c
>index f8c81f1..dd3a2e9 100644
>--- a/drivers/net/team/team.c
>+++ b/drivers/net/team/team.c
>@@ -2634,8 +2634,10 @@ static int team_nl_send_port_list_get(struct team *team, u32 portid, u32 seq,
>
> hdr = genlmsg_put(skb, portid, seq, &team_nl_family, flags | NLM_F_MULTI,
> TEAM_CMD_PORT_LIST_GET);
>- if (!hdr)
>+ if (!hdr) {
>+ nlmsg_free(skb);
> return -EMSGSIZE;
>+ }
>
> if (nla_put_u32(skb, TEAM_ATTR_TEAM_IFINDEX, team->dev->ifindex))
> goto nla_put_failure;
>--
>1.9.1
>
>
^ permalink raw reply
* Regarding patch: vlan: avoid a synchronize_net when parent device goes down
From: prasad padiyar @ 2017-04-24 5:10 UTC (permalink / raw)
To: netdev
Hi,
I had a query regarding the patch in the subject line.
>From the patch pasted below
vlan = vlan_dev_priv(vlandev);
if (!(vlan->flags & VLAN_FLAG_LOOSE_BINDING))
- dev_change_flags(vlandev, flgs & ~IFF_UP);
+ list_add(&vlandev->close_list, &close_list);
+ }
+
+ dev_close_many(&close_list, false);
+
+ list_for_each_entry_safe(vlandev, tmp, &close_list, close_list) {
netif_stacked_transfer_operstate(dev, vlandev);
+ list_del_init(&vlandev->close_list);
}
+ list_del(&close_list);
"
if (!(vlan->flags & VLAN_FLAG_LOOSE_BINDING))
- dev_change_flags(vlandev, flgs & ~IFF_UP);
+ list_add(&vlandev->close_list, &close_list);
+ }
+
+ dev_close_many(&close_list, false);
+
+ list_for_each_entry_safe(vlandev, tmp, &close_list, close_list) {
netif_stacked_transfer_operstate(dev, vlandev);
+ list_del_init(&vlandev->close_list);
}
+ list_del(&close_list);.
i can see that dev_change_flags which was setting the vlan device to
down state in case VLAN_FLAG_LOOSE_BINDING was not enabled.
But setting the operstate when the VLAN real device goes admin down
was quite independent of VLAN_FLAG_LOOSE_BINDING.
Operstate was always getting transferred irrespective of LOOSE_BINDING flag.
But now with this patch, we see that control of transferring the
operstate is based of VLAN_FLAG_LOOSE_BINDING. If
VLAN_FLAG_LOOSE_BINDING is set then oper state is not changed as per
its real iface.
Is this the expected behavior. Does VLAN_FLAG_LOOSE_BINDING with this
patch has to control the operstate as well which was not
the case earlier without this patch. ?
Can someone share your views about this.
Kernel version: 4.4.20
patch link:
https://patchwork.ozlabs.org/patch/447837/
--
Regards,
Prasad
^ permalink raw reply
* Re: [PATCH 1/2] ixgbe: add XDP support for pass and drop actions
From: John Fastabend @ 2017-04-24 4:26 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: jeffrey.t.kirsher, netdev
In-Reply-To: <20170423210506.79168335@laptop>
On 17-04-23 09:05 PM, Jakub Kicinski wrote:
> Hi!
>
> On Sun, 23 Apr 2017 18:31:19 -0700, John Fastabend wrote:
>> +static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
>> +{
>> + int i, frame_size = dev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
>> + struct ixgbe_adapter *adapter = netdev_priv(dev);
>> + struct bpf_prog *old_prog;
>> +
>> + if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
>> + return -EINVAL;
>> +
>> + if (adapter->flags & IXGBE_FLAG_DCB_ENABLED)
>> + return -EINVAL;
>> +
>> + /* verify ixgbe ring attributes are sufficient for XDP */
>> + for (i = 0; i < adapter->num_rx_queues; i++) {
>> + struct ixgbe_ring *ring = adapter->rx_ring[i];
>> +
>> + if (ring_is_rsc_enabled(ring))
>> + return -EINVAL;
>> +
>> + if (frame_size > ixgbe_rx_bufsz(ring))
>> + return -EINVAL;
>> + }
>
> I was just looking through the drivers, working on extended ack
> reporting, trying to bring out the driver XDP error messages out
> directly to iproute2. It seems that multiple drivers are only
> checking that MTU/buffer size is appropriate in the XDP_SETUP
> function, and ignore XDP in case user tries to change MTU later.
> And I think it's the same story with LRO?
>
Agreed we need to harden the drivers to changes post XDP init. I'll submit
a few follow on patches for the ixgbe devices in the morning.
Thanks,
John
^ permalink raw reply
* Re: [PATCH 1/2] ixgbe: add XDP support for pass and drop actions
From: Jakub Kicinski @ 2017-04-24 4:05 UTC (permalink / raw)
To: John Fastabend; +Cc: jeffrey.t.kirsher, netdev
In-Reply-To: <20170424013119.8354.18750.stgit@john-Precision-Tower-5810>
Hi!
On Sun, 23 Apr 2017 18:31:19 -0700, John Fastabend wrote:
> +static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
> +{
> + int i, frame_size = dev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
> + struct ixgbe_adapter *adapter = netdev_priv(dev);
> + struct bpf_prog *old_prog;
> +
> + if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
> + return -EINVAL;
> +
> + if (adapter->flags & IXGBE_FLAG_DCB_ENABLED)
> + return -EINVAL;
> +
> + /* verify ixgbe ring attributes are sufficient for XDP */
> + for (i = 0; i < adapter->num_rx_queues; i++) {
> + struct ixgbe_ring *ring = adapter->rx_ring[i];
> +
> + if (ring_is_rsc_enabled(ring))
> + return -EINVAL;
> +
> + if (frame_size > ixgbe_rx_bufsz(ring))
> + return -EINVAL;
> + }
I was just looking through the drivers, working on extended ack
reporting, trying to bring out the driver XDP error messages out
directly to iproute2. It seems that multiple drivers are only
checking that MTU/buffer size is appropriate in the XDP_SETUP
function, and ignore XDP in case user tries to change MTU later.
And I think it's the same story with LRO?
^ permalink raw reply
* Re: [PATCH 1/1] mt7601u: check return value of alloc_skb
From: Jakub Kicinski @ 2017-04-24 3:49 UTC (permalink / raw)
To: Pan Bian
Cc: Kalle Valo, Matthias Brugger, linux-wireless, netdev,
linux-kernel, linux-arm-kernel, linux-mediatek
In-Reply-To: <1492930823-17249-1-git-send-email-bianpan2016@163.com>
On Sun, 23 Apr 2017 15:00:23 +0800, Pan Bian wrote:
> Function alloc_skb() will return a NULL pointer if there is no enough
> memory. However, in function mt7601u_mcu_msg_alloc(), its return value
> is not validated before it is used. This patch fixes it.
>
> Signed-off-by: Pan Bian <bianpan2016@163.com>
Acked-by: Jakub Kicinski <kubakici@wp.pl>
Thanks!
^ permalink raw reply
* Re: [PATCH RFC (resend) net-next 0/6] virtio-net: Add support for virtio-net header extensions
From: Jason Wang @ 2017-04-24 3:22 UTC (permalink / raw)
To: vyasevic, Vladislav Yasevich, netdev
Cc: virtio-dev, mst, maxime.coquelin, virtualization
In-Reply-To: <022c8af3-e7e7-5d35-5152-cb12e90359ef@redhat.com>
On 2017年04月21日 21:08, Vlad Yasevich wrote:
> On 04/21/2017 12:05 AM, Jason Wang wrote:
>> On 2017年04月20日 23:34, Vlad Yasevich wrote:
>>> On 04/17/2017 11:01 PM, Jason Wang wrote:
>>>> On 2017年04月16日 00:38, Vladislav Yasevich wrote:
>>>>> Curreclty virtion net header is fixed size and adding things to it is rather
>>>>> difficult to do. This series attempt to add the infrastructure as well as some
>>>>> extensions that try to resolve some deficiencies we currently have.
>>>>>
>>>>> First, vnet header only has space for 16 flags. This may not be enough
>>>>> in the future. The extensions will provide space for 32 possbile extension
>>>>> flags and 32 possible extensions. These flags will be carried in the
>>>>> first pseudo extension header, the presense of which will be determined by
>>>>> the flag in the virtio net header.
>>>>>
>>>>> The extensions themselves will immidiately follow the extension header itself.
>>>>> They will be added to the packet in the same order as they appear in the
>>>>> extension flags. No padding is placed between the extensions and any
>>>>> extensions negotiated, but not used need by a given packet will convert to
>>>>> trailing padding.
>>>> Do we need a explicit padding (e.g an extension) which could be controlled by each side?
>>> I don't think so. The size of the vnet header is set based on the extensions negotiated.
>>> The one part I am not crazy about is that in the case of packet not using any extensions,
>>> the data is still placed after the entire vnet header, which essentially adds a lot
>>> of padding. However, that's really no different then if we simply grew the vnet header.
>>>
>>> The other thing I've tried before is putting extensions into their own sg buffer, but that
>>> made it slower.h
>> Yes.
>>
>>>>> For example:
>>>>> | vnet mrg hdr | ext hdr | ext 1 | ext 2 | ext 5 | .. pad .. | packet data |
>>>> Just some rough thoughts:
>>>>
>>>> - Is this better to use TLV instead of bitmap here? One advantage of TLV is that the
>>>> length is not limited by the length of bitmap.
>>> but the disadvantage is that we add at least 4 bytes per extension of just TL data. That
>>> makes this thing even longer.
>> Yes, and it looks like the length is still limited by e.g the length of T.
> Not only that, but it is also limited by the skb->cb as a whole. So adding putting
> extensions into a TLV style means we have less extensions for now, until we get rid of
> skb->cb usage.
>
>>>> - For 1.1, do we really want something like vnet header? AFAIK, it was not used by modern
>>>> NICs, is this better to pack all meta-data into descriptor itself? This may need a some
>>>> changes in tun/macvtap, but looks more PCIE friendly.
>>> That would really be ideal and I've looked at this. There are small issues of exposing
>>> the 'net metadata' of the descriptor to taps so they can be filled in. The alternative
>>> is to use a different control structure for tap->qemu|vhost channel (that can be
>>> implementation specific) and have qemu|vhost populate the 'net metadata' of the descriptor.
>> Yes, this needs some thought. For vhost, things looks a little bit easier, we can probably
>> use msg_control.
>>
> We can use msg_control in qemu as well, can't we?
AFAIK, it needs some changes since we don't export socket to userspace.
> It really is a question of who is doing
> the work and the number of copies.
>
> I can take a closer look of how it would look if we extend the descriptor with type
> specific data. I don't know if other users of virtio would benefit from it?
Not sure, but we can have a common descriptor header followed by device
specific meta data. This probably need some prototype benchmarking to
see the benefits first.
Thanks
^ permalink raw reply
* [PATCH 2/2] ixgbe: add support for XDP_TX action
From: John Fastabend @ 2017-04-24 1:31 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev
In-Reply-To: <20170424013004.8354.34893.stgit@john-Precision-Tower-5810>
From: John Fastabend <john.fastabend@gmail.com>
Add support for XDP_TX action.
A couple design choices were made here. First I use a new ring
pointer structure xdp_ring[] in the adapter struct instead of
pushing the newly allocated xdp TX rings into the tx_ring[]
structure. This means we have to duplicate loops around rings
in places we want to initialize both TX rings and XDP rings.
But by making it explicit it is obvious when we are using XDP
rings and when we are using TX rings. Further we don't have
to do ring arithmatic which is error prone. As a proof point
for doing this my first patches used only a single ring structure
and introduced bugs in FCoE code and macvlan code paths.
Second I am aware this is not the most optimized version of
this code possible. I want to get baseline support in using
the most readable format possible and then once this series
is included I will optimize the TX path in another series
of patches.
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
---
0 files changed
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index cb14813..e8cd449 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -235,7 +235,11 @@ struct vf_macvlans {
struct ixgbe_tx_buffer {
union ixgbe_adv_tx_desc *next_to_watch;
unsigned long time_stamp;
- struct sk_buff *skb;
+ union {
+ struct sk_buff *skb;
+ /* XDP uses address ptr on irq_clean */
+ void *data;
+ };
unsigned int bytecount;
unsigned short gso_segs;
__be16 protocol;
@@ -288,6 +292,7 @@ enum ixgbe_ring_state_t {
__IXGBE_TX_XPS_INIT_DONE,
__IXGBE_TX_DETECT_HANG,
__IXGBE_HANG_CHECK_ARMED,
+ __IXGBE_TX_XDP_RING,
};
#define ring_uses_build_skb(ring) \
@@ -314,6 +319,12 @@ struct ixgbe_fwd_adapter {
set_bit(__IXGBE_RX_RSC_ENABLED, &(ring)->state)
#define clear_ring_rsc_enabled(ring) \
clear_bit(__IXGBE_RX_RSC_ENABLED, &(ring)->state)
+#define ring_is_xdp(ring) \
+ test_bit(__IXGBE_TX_XDP_RING, &(ring)->state)
+#define set_ring_xdp(ring) \
+ set_bit(__IXGBE_TX_XDP_RING, &(ring)->state)
+#define clear_ring_xdp(ring) \
+ clear_bit(__IXGBE_TX_XDP_RING, &(ring)->state)
struct ixgbe_ring {
struct ixgbe_ring *next; /* pointer to next ring in q_vector */
struct ixgbe_q_vector *q_vector; /* backpointer to host q_vector */
@@ -380,6 +391,7 @@ enum ixgbe_ring_f_enum {
#define IXGBE_MAX_FCOE_INDICES 8
#define MAX_RX_QUEUES (IXGBE_MAX_FDIR_INDICES + 1)
#define MAX_TX_QUEUES (IXGBE_MAX_FDIR_INDICES + 1)
+#define MAX_XDP_QUEUES (IXGBE_MAX_FDIR_INDICES + 1)
#define IXGBE_MAX_L2A_QUEUES 4
#define IXGBE_BAD_L2A_QUEUE 3
#define IXGBE_MAX_MACVLANS 31
@@ -623,6 +635,10 @@ struct ixgbe_adapter {
__be16 vxlan_port;
__be16 geneve_port;
+ /* XDP */
+ int num_xdp_queues;
+ struct ixgbe_ring *xdp_ring[MAX_XDP_QUEUES];
+
/* TX */
struct ixgbe_ring *tx_ring[MAX_TX_QUEUES] ____cacheline_aligned_in_smp;
@@ -669,6 +685,7 @@ struct ixgbe_adapter {
u64 tx_busy;
unsigned int tx_ring_count;
+ unsigned int xdp_ring_count;
unsigned int rx_ring_count;
u32 link_speed;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 79a126d..b0fd2f5 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1071,15 +1071,19 @@ static int ixgbe_set_ringparam(struct net_device *netdev,
if (!netif_running(adapter->netdev)) {
for (i = 0; i < adapter->num_tx_queues; i++)
adapter->tx_ring[i]->count = new_tx_count;
+ for (i = 0; i < adapter->num_xdp_queues; i++)
+ adapter->xdp_ring[i]->count = new_tx_count;
for (i = 0; i < adapter->num_rx_queues; i++)
adapter->rx_ring[i]->count = new_rx_count;
adapter->tx_ring_count = new_tx_count;
+ adapter->xdp_ring_count = new_tx_count;
adapter->rx_ring_count = new_rx_count;
goto clear_reset;
}
/* allocate temporary buffer to store rings in */
i = max_t(int, adapter->num_tx_queues, adapter->num_rx_queues);
+ i = max_t(int, i, adapter->num_xdp_queues);
temp_ring = vmalloc(i * sizeof(struct ixgbe_ring));
if (!temp_ring) {
@@ -1111,12 +1115,33 @@ static int ixgbe_set_ringparam(struct net_device *netdev,
}
}
+ for (i = 0; i < adapter->num_xdp_queues; i++) {
+ memcpy(&temp_ring[i], adapter->xdp_ring[i],
+ sizeof(struct ixgbe_ring));
+
+ temp_ring[i].count = new_tx_count;
+ err = ixgbe_setup_tx_resources(&temp_ring[i]);
+ if (err) {
+ while (i) {
+ i--;
+ ixgbe_free_tx_resources(&temp_ring[i]);
+ }
+ goto err_setup;
+ }
+ }
+
for (i = 0; i < adapter->num_tx_queues; i++) {
ixgbe_free_tx_resources(adapter->tx_ring[i]);
memcpy(adapter->tx_ring[i], &temp_ring[i],
sizeof(struct ixgbe_ring));
}
+ for (i = 0; i < adapter->num_xdp_queues; i++) {
+ ixgbe_free_tx_resources(adapter->xdp_ring[i]);
+
+ memcpy(adapter->xdp_ring[i], &temp_ring[i],
+ sizeof(struct ixgbe_ring));
+ }
adapter->tx_ring_count = new_tx_count;
}
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
index 1b8be7d..b45fdc9 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_lib.c
@@ -267,12 +267,14 @@ static bool ixgbe_cache_ring_sriov(struct ixgbe_adapter *adapter)
**/
static bool ixgbe_cache_ring_rss(struct ixgbe_adapter *adapter)
{
- int i;
+ int i, reg_idx;
for (i = 0; i < adapter->num_rx_queues; i++)
adapter->rx_ring[i]->reg_idx = i;
- for (i = 0; i < adapter->num_tx_queues; i++)
- adapter->tx_ring[i]->reg_idx = i;
+ for (i = 0, reg_idx = 0; i < adapter->num_tx_queues; i++, reg_idx++)
+ adapter->tx_ring[i]->reg_idx = reg_idx;
+ for (i = 0; i < adapter->num_xdp_queues; i++, reg_idx++)
+ adapter->xdp_ring[i]->reg_idx = reg_idx;
return true;
}
@@ -308,6 +310,11 @@ static void ixgbe_cache_ring_register(struct ixgbe_adapter *adapter)
ixgbe_cache_ring_rss(adapter);
}
+static int ixgbe_xdp_queues(struct ixgbe_adapter *adapter)
+{
+ return adapter->xdp_prog ? nr_cpu_ids : 0;
+}
+
#define IXGBE_RSS_64Q_MASK 0x3F
#define IXGBE_RSS_16Q_MASK 0xF
#define IXGBE_RSS_8Q_MASK 0x7
@@ -382,6 +389,7 @@ static bool ixgbe_set_dcb_sriov_queues(struct ixgbe_adapter *adapter)
adapter->num_rx_queues_per_pool = tcs;
adapter->num_tx_queues = vmdq_i * tcs;
+ adapter->num_xdp_queues = 0;
adapter->num_rx_queues = vmdq_i * tcs;
#ifdef IXGBE_FCOE
@@ -479,6 +487,7 @@ static bool ixgbe_set_dcb_queues(struct ixgbe_adapter *adapter)
netdev_set_tc_queue(dev, i, rss_i, rss_i * i);
adapter->num_tx_queues = rss_i * tcs;
+ adapter->num_xdp_queues = 0;
adapter->num_rx_queues = rss_i * tcs;
return true;
@@ -549,6 +558,7 @@ static bool ixgbe_set_sriov_queues(struct ixgbe_adapter *adapter)
adapter->num_rx_queues = vmdq_i * rss_i;
adapter->num_tx_queues = vmdq_i * rss_i;
+ adapter->num_xdp_queues = 0;
/* disable ATR as it is not supported when VMDq is enabled */
adapter->flags &= ~IXGBE_FLAG_FDIR_HASH_CAPABLE;
@@ -669,6 +679,7 @@ static bool ixgbe_set_rss_queues(struct ixgbe_adapter *adapter)
#endif /* IXGBE_FCOE */
adapter->num_rx_queues = rss_i;
adapter->num_tx_queues = rss_i;
+ adapter->num_xdp_queues = ixgbe_xdp_queues(adapter);
return true;
}
@@ -689,6 +700,7 @@ static void ixgbe_set_num_queues(struct ixgbe_adapter *adapter)
/* Start with base case */
adapter->num_rx_queues = 1;
adapter->num_tx_queues = 1;
+ adapter->num_xdp_queues = 0;
adapter->num_rx_pools = adapter->num_rx_queues;
adapter->num_rx_queues_per_pool = 1;
@@ -719,8 +731,11 @@ static int ixgbe_acquire_msix_vectors(struct ixgbe_adapter *adapter)
struct ixgbe_hw *hw = &adapter->hw;
int i, vectors, vector_threshold;
- /* We start by asking for one vector per queue pair */
+ /* We start by asking for one vector per queue pair with XDP queues
+ * being stacked with TX queues.
+ */
vectors = max(adapter->num_rx_queues, adapter->num_tx_queues);
+ vectors = max(vectors, adapter->num_xdp_queues);
/* It is easy to be greedy for MSI-X vectors. However, it really
* doesn't do much good if we have a lot more vectors than CPUs. We'll
@@ -800,6 +815,8 @@ static void ixgbe_add_ring(struct ixgbe_ring *ring,
* @v_idx: index of vector in adapter struct
* @txr_count: total number of Tx rings to allocate
* @txr_idx: index of first Tx ring to allocate
+ * @xdp_count: total number of XDP rings to allocate
+ * @xdp_idx: index of first XDP ring to allocate
* @rxr_count: total number of Rx rings to allocate
* @rxr_idx: index of first Rx ring to allocate
*
@@ -808,6 +825,7 @@ static void ixgbe_add_ring(struct ixgbe_ring *ring,
static int ixgbe_alloc_q_vector(struct ixgbe_adapter *adapter,
int v_count, int v_idx,
int txr_count, int txr_idx,
+ int xdp_count, int xdp_idx,
int rxr_count, int rxr_idx)
{
struct ixgbe_q_vector *q_vector;
@@ -817,7 +835,7 @@ static int ixgbe_alloc_q_vector(struct ixgbe_adapter *adapter,
int ring_count, size;
u8 tcs = netdev_get_num_tc(adapter->netdev);
- ring_count = txr_count + rxr_count;
+ ring_count = txr_count + rxr_count + xdp_count;
size = sizeof(struct ixgbe_q_vector) +
(sizeof(struct ixgbe_ring) * ring_count);
@@ -909,6 +927,33 @@ static int ixgbe_alloc_q_vector(struct ixgbe_adapter *adapter,
ring++;
}
+ while (xdp_count) {
+ /* assign generic ring traits */
+ ring->dev = &adapter->pdev->dev;
+ ring->netdev = adapter->netdev;
+
+ /* configure backlink on ring */
+ ring->q_vector = q_vector;
+
+ /* update q_vector Tx values */
+ ixgbe_add_ring(ring, &q_vector->tx);
+
+ /* apply Tx specific ring traits */
+ ring->count = adapter->tx_ring_count;
+ ring->queue_index = xdp_idx;
+ set_ring_xdp(ring);
+
+ /* assign ring to adapter */
+ adapter->xdp_ring[xdp_idx] = ring;
+
+ /* update count and index */
+ xdp_count--;
+ xdp_idx++;
+
+ /* push pointer to next ring */
+ ring++;
+ }
+
while (rxr_count) {
/* assign generic ring traits */
ring->dev = &adapter->pdev->dev;
@@ -1002,17 +1047,18 @@ static int ixgbe_alloc_q_vectors(struct ixgbe_adapter *adapter)
int q_vectors = adapter->num_q_vectors;
int rxr_remaining = adapter->num_rx_queues;
int txr_remaining = adapter->num_tx_queues;
- int rxr_idx = 0, txr_idx = 0, v_idx = 0;
+ int xdp_remaining = adapter->num_xdp_queues;
+ int rxr_idx = 0, txr_idx = 0, xdp_idx = 0, v_idx = 0;
int err;
/* only one q_vector if MSI-X is disabled. */
if (!(adapter->flags & IXGBE_FLAG_MSIX_ENABLED))
q_vectors = 1;
- if (q_vectors >= (rxr_remaining + txr_remaining)) {
+ if (q_vectors >= (rxr_remaining + txr_remaining + xdp_remaining)) {
for (; rxr_remaining; v_idx++) {
err = ixgbe_alloc_q_vector(adapter, q_vectors, v_idx,
- 0, 0, 1, rxr_idx);
+ 0, 0, 0, 0, 1, rxr_idx);
if (err)
goto err_out;
@@ -1026,8 +1072,11 @@ static int ixgbe_alloc_q_vectors(struct ixgbe_adapter *adapter)
for (; v_idx < q_vectors; v_idx++) {
int rqpv = DIV_ROUND_UP(rxr_remaining, q_vectors - v_idx);
int tqpv = DIV_ROUND_UP(txr_remaining, q_vectors - v_idx);
+ int xqpv = DIV_ROUND_UP(xdp_remaining, q_vectors - v_idx);
+
err = ixgbe_alloc_q_vector(adapter, q_vectors, v_idx,
tqpv, txr_idx,
+ xqpv, xdp_idx,
rqpv, rxr_idx);
if (err)
@@ -1036,14 +1085,17 @@ static int ixgbe_alloc_q_vectors(struct ixgbe_adapter *adapter)
/* update counts and index */
rxr_remaining -= rqpv;
txr_remaining -= tqpv;
+ xdp_remaining -= xqpv;
rxr_idx++;
txr_idx++;
+ xdp_idx += xqpv;
}
return 0;
err_out:
adapter->num_tx_queues = 0;
+ adapter->num_xdp_queues = 0;
adapter->num_rx_queues = 0;
adapter->num_q_vectors = 0;
@@ -1066,6 +1118,7 @@ static void ixgbe_free_q_vectors(struct ixgbe_adapter *adapter)
int v_idx = adapter->num_q_vectors;
adapter->num_tx_queues = 0;
+ adapter->num_xdp_queues = 0;
adapter->num_rx_queues = 0;
adapter->num_q_vectors = 0;
@@ -1172,9 +1225,10 @@ int ixgbe_init_interrupt_scheme(struct ixgbe_adapter *adapter)
ixgbe_cache_ring_register(adapter);
- e_dev_info("Multiqueue %s: Rx Queue count = %u, Tx Queue count = %u\n",
+ e_dev_info("Multiqueue %s: Rx Queue count = %u, Tx Queue count = %u XDP Queue count = %u\n",
(adapter->num_rx_queues > 1) ? "Enabled" : "Disabled",
- adapter->num_rx_queues, adapter->num_tx_queues);
+ adapter->num_rx_queues, adapter->num_tx_queues,
+ adapter->num_xdp_queues);
set_bit(__IXGBE_DOWN, &adapter->state);
@@ -1195,6 +1249,7 @@ int ixgbe_init_interrupt_scheme(struct ixgbe_adapter *adapter)
void ixgbe_clear_interrupt_scheme(struct ixgbe_adapter *adapter)
{
adapter->num_tx_queues = 0;
+ adapter->num_xdp_queues = 0;
adapter->num_rx_queues = 0;
ixgbe_free_q_vectors(adapter);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index ebcf9c0..a13d2bf 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -594,6 +594,19 @@ static void ixgbe_regdump(struct ixgbe_hw *hw, struct ixgbe_reg_info *reginfo)
}
+static void ixgbe_print_buffer(struct ixgbe_ring *ring, int n)
+{
+ struct ixgbe_tx_buffer *tx_buffer;
+
+ tx_buffer = &ring->tx_buffer_info[ring->next_to_clean];
+ pr_info(" %5d %5X %5X %016llX %08X %p %016llX\n",
+ n, ring->next_to_use, ring->next_to_clean,
+ (u64)dma_unmap_addr(tx_buffer, dma),
+ dma_unmap_len(tx_buffer, len),
+ tx_buffer->next_to_watch,
+ (u64)tx_buffer->time_stamp);
+}
+
/*
* ixgbe_dump - Print registers, tx-rings and rx-rings
*/
@@ -603,7 +616,7 @@ static void ixgbe_dump(struct ixgbe_adapter *adapter)
struct ixgbe_hw *hw = &adapter->hw;
struct ixgbe_reg_info *reginfo;
int n = 0;
- struct ixgbe_ring *tx_ring;
+ struct ixgbe_ring *ring;
struct ixgbe_tx_buffer *tx_buffer;
union ixgbe_adv_tx_desc *tx_desc;
struct my_u0 { u64 a; u64 b; } *u0;
@@ -643,14 +656,13 @@ static void ixgbe_dump(struct ixgbe_adapter *adapter)
"Queue [NTU] [NTC] [bi(ntc)->dma ]",
"leng", "ntw", "timestamp");
for (n = 0; n < adapter->num_tx_queues; n++) {
- tx_ring = adapter->tx_ring[n];
- tx_buffer = &tx_ring->tx_buffer_info[tx_ring->next_to_clean];
- pr_info(" %5d %5X %5X %016llX %08X %p %016llX\n",
- n, tx_ring->next_to_use, tx_ring->next_to_clean,
- (u64)dma_unmap_addr(tx_buffer, dma),
- dma_unmap_len(tx_buffer, len),
- tx_buffer->next_to_watch,
- (u64)tx_buffer->time_stamp);
+ ring = adapter->tx_ring[n];
+ ixgbe_print_buffer(ring, n);
+ }
+
+ for (n = 0; n < adapter->num_xdp_queues; n++) {
+ ring = adapter->xdp_ring[n];
+ ixgbe_print_buffer(ring, n);
}
/* Print TX Rings */
@@ -695,28 +707,28 @@ static void ixgbe_dump(struct ixgbe_adapter *adapter)
*/
for (n = 0; n < adapter->num_tx_queues; n++) {
- tx_ring = adapter->tx_ring[n];
+ ring = adapter->tx_ring[n];
pr_info("------------------------------------\n");
- pr_info("TX QUEUE INDEX = %d\n", tx_ring->queue_index);
+ pr_info("TX QUEUE INDEX = %d\n", ring->queue_index);
pr_info("------------------------------------\n");
pr_info("%s%s %s %s %s %s\n",
"T [desc] [address 63:0 ] ",
"[PlPOIdStDDt Ln] [bi->dma ] ",
"leng", "ntw", "timestamp", "bi->skb");
- for (i = 0; tx_ring->desc && (i < tx_ring->count); i++) {
- tx_desc = IXGBE_TX_DESC(tx_ring, i);
- tx_buffer = &tx_ring->tx_buffer_info[i];
+ for (i = 0; ring->desc && (i < ring->count); i++) {
+ tx_desc = IXGBE_TX_DESC(ring, i);
+ tx_buffer = &ring->tx_buffer_info[i];
u0 = (struct my_u0 *)tx_desc;
if (dma_unmap_len(tx_buffer, len) > 0) {
const char *ring_desc;
- if (i == tx_ring->next_to_use &&
- i == tx_ring->next_to_clean)
+ if (i == ring->next_to_use &&
+ i == ring->next_to_clean)
ring_desc = " NTC/U";
- else if (i == tx_ring->next_to_use)
+ else if (i == ring->next_to_use)
ring_desc = " NTU";
- else if (i == tx_ring->next_to_clean)
+ else if (i == ring->next_to_clean)
ring_desc = " NTC";
else
ring_desc = "";
@@ -985,6 +997,10 @@ static void ixgbe_update_xoff_rx_lfc(struct ixgbe_adapter *adapter)
for (i = 0; i < adapter->num_tx_queues; i++)
clear_bit(__IXGBE_HANG_CHECK_ARMED,
&adapter->tx_ring[i]->state);
+
+ for (i = 0; i < adapter->num_xdp_queues; i++)
+ clear_bit(__IXGBE_HANG_CHECK_ARMED,
+ &adapter->xdp_ring[i]->state);
}
static void ixgbe_update_xoff_received(struct ixgbe_adapter *adapter)
@@ -1029,6 +1045,14 @@ static void ixgbe_update_xoff_received(struct ixgbe_adapter *adapter)
if (xoff[tc])
clear_bit(__IXGBE_HANG_CHECK_ARMED, &tx_ring->state);
}
+
+ for (i = 0; i < adapter->num_xdp_queues; i++) {
+ struct ixgbe_ring *xdp_ring = adapter->xdp_ring[i];
+
+ tc = xdp_ring->dcb_tc;
+ if (xoff[tc])
+ clear_bit(__IXGBE_HANG_CHECK_ARMED, &xdp_ring->state);
+ }
}
static u64 ixgbe_get_tx_completed(struct ixgbe_ring *ring)
@@ -1180,7 +1204,10 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
total_packets += tx_buffer->gso_segs;
/* free the skb */
- napi_consume_skb(tx_buffer->skb, napi_budget);
+ if (ring_is_xdp(tx_ring))
+ page_frag_free(tx_buffer->data);
+ else
+ napi_consume_skb(tx_buffer->skb, napi_budget);
/* unmap skb header data */
dma_unmap_single(tx_ring->dev,
@@ -1241,7 +1268,7 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
if (check_for_tx_hang(tx_ring) && ixgbe_check_tx_hang(tx_ring)) {
/* schedule immediate reset if we believe we hung */
struct ixgbe_hw *hw = &adapter->hw;
- e_err(drv, "Detected Tx Unit Hang\n"
+ e_err(drv, "Detected Tx Unit Hang %s\n"
" Tx Queue <%d>\n"
" TDH, TDT <%x>, <%x>\n"
" next_to_use <%x>\n"
@@ -1249,13 +1276,16 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
"tx_buffer_info[next_to_clean]\n"
" time_stamp <%lx>\n"
" jiffies <%lx>\n",
+ ring_is_xdp(tx_ring) ? "(XDP)" : "",
tx_ring->queue_index,
IXGBE_READ_REG(hw, IXGBE_TDH(tx_ring->reg_idx)),
IXGBE_READ_REG(hw, IXGBE_TDT(tx_ring->reg_idx)),
tx_ring->next_to_use, i,
tx_ring->tx_buffer_info[i].time_stamp, jiffies);
- netif_stop_subqueue(tx_ring->netdev, tx_ring->queue_index);
+ if (!ring_is_xdp(tx_ring))
+ netif_stop_subqueue(tx_ring->netdev,
+ tx_ring->queue_index);
e_info(probe,
"tx hang %d detected on queue %d, resetting adapter\n",
@@ -1268,6 +1298,9 @@ static bool ixgbe_clean_tx_irq(struct ixgbe_q_vector *q_vector,
return true;
}
+ if (ring_is_xdp(tx_ring))
+ return !!budget;
+
netdev_tx_completed_queue(txring_txq(tx_ring),
total_packets, total_bytes);
@@ -2170,8 +2203,13 @@ static struct sk_buff *ixgbe_build_skb(struct ixgbe_ring *rx_ring,
#define IXGBE_XDP_PASS 0
#define IXGBE_XDP_CONSUMED 1
+#define IXGBE_XDP_TX 2
-static struct sk_buff *ixgbe_run_xdp(struct ixgbe_ring *rx_ring,
+static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter,
+ struct xdp_buff *xdp);
+
+static struct sk_buff *ixgbe_run_xdp(struct ixgbe_adapter *adapter,
+ struct ixgbe_ring *rx_ring,
struct xdp_buff *xdp)
{
int result = IXGBE_XDP_PASS;
@@ -2188,9 +2226,11 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_ring *rx_ring,
switch (act) {
case XDP_PASS:
break;
+ case XDP_TX:
+ result = ixgbe_xmit_xdp_ring(adapter, xdp);
+ break;
default:
bpf_warn_invalid_xdp_action(act);
- case XDP_TX:
case XDP_ABORTED:
trace_xdp_exception(rx_ring->netdev, xdp_prog, act);
/* fallthrough -- handle aborts by dropping packet */
@@ -2203,6 +2243,23 @@ static struct sk_buff *ixgbe_run_xdp(struct ixgbe_ring *rx_ring,
return ERR_PTR(-result);
}
+static void ixgbe_rx_buffer_flip(struct ixgbe_ring *rx_ring,
+ struct ixgbe_rx_buffer *rx_buffer,
+ unsigned int size)
+{
+#if (PAGE_SIZE < 8192)
+ unsigned int truesize = ixgbe_rx_pg_size(rx_ring) / 2;
+
+ rx_buffer->page_offset ^= truesize;
+#else
+ unsigned int truesize = ring_uses_build_skb(rx_ring) ?
+ SKB_DATA_ALIGN(IXGBE_SKB_PAD + size) :
+ SKB_DATA_ALIGN(size);
+
+ rx_buffer->page_offset += truesize;
+#endif
+}
+
/**
* ixgbe_clean_rx_irq - Clean completed descriptors from Rx ring - bounce buf
* @q_vector: structure containing interrupt and ring information
@@ -2221,8 +2278,8 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
const int budget)
{
unsigned int total_rx_bytes = 0, total_rx_packets = 0;
-#ifdef IXGBE_FCOE
struct ixgbe_adapter *adapter = q_vector->adapter;
+#ifdef IXGBE_FCOE
int ddp_bytes;
unsigned int mss = 0;
#endif /* IXGBE_FCOE */
@@ -2262,13 +2319,16 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
ixgbe_rx_offset(rx_ring);
xdp.data_end = xdp.data + size;
- skb = ixgbe_run_xdp(rx_ring, &xdp);
+ skb = ixgbe_run_xdp(adapter, rx_ring, &xdp);
}
if (IS_ERR(skb)) {
+ if (PTR_ERR(skb) == -IXGBE_XDP_TX)
+ ixgbe_rx_buffer_flip(rx_ring, rx_buffer, size);
+ else
+ rx_buffer->pagecnt_bias++;
total_rx_packets++;
total_rx_bytes += size;
- rx_buffer->pagecnt_bias++;
} else if (skb) {
ixgbe_add_rx_frag(rx_ring, rx_buffer, skb, size);
} else if (ring_uses_build_skb(rx_ring)) {
@@ -3438,6 +3498,8 @@ static void ixgbe_configure_tx(struct ixgbe_adapter *adapter)
/* Setup the HW Tx Head and Tail descriptor pointers */
for (i = 0; i < adapter->num_tx_queues; i++)
ixgbe_configure_tx_ring(adapter, adapter->tx_ring[i]);
+ for (i = 0; i < adapter->num_xdp_queues; i++)
+ ixgbe_configure_tx_ring(adapter, adapter->xdp_ring[i]);
}
static void ixgbe_enable_rx_drop(struct ixgbe_adapter *adapter,
@@ -5579,7 +5641,10 @@ static void ixgbe_clean_tx_ring(struct ixgbe_ring *tx_ring)
union ixgbe_adv_tx_desc *eop_desc, *tx_desc;
/* Free all the Tx ring sk_buffs */
- dev_kfree_skb_any(tx_buffer->skb);
+ if (ring_is_xdp(tx_ring))
+ page_frag_free(tx_buffer->data);
+ else
+ dev_kfree_skb_any(tx_buffer->skb);
/* unmap skb header data */
dma_unmap_single(tx_ring->dev,
@@ -5620,7 +5685,8 @@ static void ixgbe_clean_tx_ring(struct ixgbe_ring *tx_ring)
}
/* reset BQL for queue */
- netdev_tx_reset_queue(txring_txq(tx_ring));
+ if (!ring_is_xdp(tx_ring))
+ netdev_tx_reset_queue(txring_txq(tx_ring));
/* reset next_to_use and next_to_clean */
tx_ring->next_to_use = 0;
@@ -5649,6 +5715,8 @@ static void ixgbe_clean_all_tx_rings(struct ixgbe_adapter *adapter)
for (i = 0; i < adapter->num_tx_queues; i++)
ixgbe_clean_tx_ring(adapter->tx_ring[i]);
+ for (i = 0; i < adapter->num_xdp_queues; i++)
+ ixgbe_clean_tx_ring(adapter->xdp_ring[i]);
}
static void ixgbe_fdir_filter_exit(struct ixgbe_adapter *adapter)
@@ -5743,6 +5811,11 @@ void ixgbe_down(struct ixgbe_adapter *adapter)
u8 reg_idx = adapter->tx_ring[i]->reg_idx;
IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
}
+ for (i = 0; i < adapter->num_xdp_queues; i++) {
+ u8 reg_idx = adapter->xdp_ring[i]->reg_idx;
+
+ IXGBE_WRITE_REG(hw, IXGBE_TXDCTL(reg_idx), IXGBE_TXDCTL_SWFLSH);
+ }
/* Disable the Tx DMA engine on 82599 and later MAC */
switch (hw->mac.type) {
@@ -6111,7 +6184,7 @@ int ixgbe_setup_tx_resources(struct ixgbe_ring *tx_ring)
**/
static int ixgbe_setup_all_tx_resources(struct ixgbe_adapter *adapter)
{
- int i, err = 0;
+ int i, j = 0, err = 0;
for (i = 0; i < adapter->num_tx_queues; i++) {
err = ixgbe_setup_tx_resources(adapter->tx_ring[i]);
@@ -6121,10 +6194,20 @@ static int ixgbe_setup_all_tx_resources(struct ixgbe_adapter *adapter)
e_err(probe, "Allocation for Tx Queue %u failed\n", i);
goto err_setup_tx;
}
+ for (j = 0; j < adapter->num_xdp_queues; j++) {
+ err = ixgbe_setup_tx_resources(adapter->xdp_ring[j]);
+ if (!err)
+ continue;
+
+ e_err(probe, "Allocation for Tx Queue %u failed\n", j);
+ goto err_setup_tx;
+ }
return 0;
err_setup_tx:
/* rewind the index freeing the rings as we go */
+ while (j--)
+ ixgbe_free_tx_resources(adapter->xdp_ring[j]);
while (i--)
ixgbe_free_tx_resources(adapter->tx_ring[i]);
return err;
@@ -6255,6 +6338,9 @@ static void ixgbe_free_all_tx_resources(struct ixgbe_adapter *adapter)
for (i = 0; i < adapter->num_tx_queues; i++)
if (adapter->tx_ring[i]->desc)
ixgbe_free_tx_resources(adapter->tx_ring[i]);
+ for (i = 0; i < adapter->num_xdp_queues; i++)
+ if (adapter->xdp_ring[i]->desc)
+ ixgbe_free_tx_resources(adapter->xdp_ring[i]);
}
/**
@@ -6674,6 +6760,14 @@ void ixgbe_update_stats(struct ixgbe_adapter *adapter)
bytes += tx_ring->stats.bytes;
packets += tx_ring->stats.packets;
}
+ for (i = 0; i < adapter->num_xdp_queues; i++) {
+ struct ixgbe_ring *xdp_ring = adapter->xdp_ring[i];
+
+ restart_queue += xdp_ring->tx_stats.restart_queue;
+ tx_busy += xdp_ring->tx_stats.tx_busy;
+ bytes += xdp_ring->stats.bytes;
+ packets += xdp_ring->stats.packets;
+ }
adapter->restart_queue = restart_queue;
adapter->tx_busy = tx_busy;
netdev->stats.tx_bytes = bytes;
@@ -6867,6 +6961,9 @@ static void ixgbe_fdir_reinit_subtask(struct ixgbe_adapter *adapter)
for (i = 0; i < adapter->num_tx_queues; i++)
set_bit(__IXGBE_TX_FDIR_INIT_DONE,
&(adapter->tx_ring[i]->state));
+ for (i = 0; i < adapter->num_xdp_queues; i++)
+ set_bit(__IXGBE_TX_FDIR_INIT_DONE,
+ &adapter->xdp_ring[i]->state);
/* re-enable flow director interrupts */
IXGBE_WRITE_REG(hw, IXGBE_EIMS, IXGBE_EIMS_FLOW_DIR);
} else {
@@ -6900,6 +6997,8 @@ static void ixgbe_check_hang_subtask(struct ixgbe_adapter *adapter)
if (netif_carrier_ok(adapter->netdev)) {
for (i = 0; i < adapter->num_tx_queues; i++)
set_check_for_tx_hang(adapter->tx_ring[i]);
+ for (i = 0; i < adapter->num_xdp_queues; i++)
+ set_check_for_tx_hang(adapter->xdp_ring[i]);
}
if (!(adapter->flags & IXGBE_FLAG_MSIX_ENABLED)) {
@@ -7130,6 +7229,13 @@ static bool ixgbe_ring_tx_pending(struct ixgbe_adapter *adapter)
return true;
}
+ for (i = 0; i < adapter->num_xdp_queues; i++) {
+ struct ixgbe_ring *ring = adapter->xdp_ring[i];
+
+ if (ring->next_to_use != ring->next_to_clean)
+ return true;
+ }
+
return false;
}
@@ -8090,6 +8196,69 @@ static u16 ixgbe_select_queue(struct net_device *dev, struct sk_buff *skb,
#endif
}
+static int ixgbe_xmit_xdp_ring(struct ixgbe_adapter *adapter,
+ struct xdp_buff *xdp)
+{
+ struct ixgbe_ring *ring = adapter->xdp_ring[smp_processor_id()];
+ struct ixgbe_tx_buffer *tx_buffer;
+ union ixgbe_adv_tx_desc *tx_desc;
+ u32 len, cmd_type;
+ dma_addr_t dma;
+ u16 i;
+
+ len = xdp->data_end - xdp->data;
+
+ if (unlikely(!ixgbe_desc_unused(ring)))
+ return IXGBE_XDP_CONSUMED;
+
+ dma = dma_map_single(ring->dev, xdp->data, len, DMA_TO_DEVICE);
+ if (dma_mapping_error(ring->dev, dma))
+ return IXGBE_XDP_CONSUMED;
+
+ /* record the location of the first descriptor for this packet */
+ tx_buffer = &ring->tx_buffer_info[ring->next_to_use];
+ tx_buffer->bytecount = len;
+ tx_buffer->gso_segs = 1;
+ tx_buffer->protocol = 0;
+
+ i = ring->next_to_use;
+ tx_desc = IXGBE_TX_DESC(ring, i);
+
+ dma_unmap_len_set(tx_buffer, len, len);
+ dma_unmap_addr_set(tx_buffer, dma, dma);
+ tx_buffer->data = xdp->data;
+ tx_desc->read.buffer_addr = cpu_to_le64(dma);
+
+ /* put descriptor type bits */
+ cmd_type = IXGBE_ADVTXD_DTYP_DATA |
+ IXGBE_ADVTXD_DCMD_DEXT |
+ IXGBE_ADVTXD_DCMD_IFCS;
+ cmd_type |= len | IXGBE_TXD_CMD;
+ tx_desc->read.cmd_type_len = cpu_to_le32(cmd_type);
+ tx_desc->read.olinfo_status =
+ cpu_to_le32(len << IXGBE_ADVTXD_PAYLEN_SHIFT);
+
+ /* Force memory writes to complete before letting h/w know there
+ * are new descriptors to fetch. (Only applicable for weak-ordered
+ * memory model archs, such as IA-64).
+ *
+ * We also need this memory barrier to make certain all of the
+ * status bits have been updated before next_to_watch is written.
+ */
+ wmb();
+
+ /* set next_to_watch value indicating a packet is present */
+ i++;
+ if (i == ring->count)
+ i = 0;
+
+ tx_buffer->next_to_watch = tx_desc;
+ ring->next_to_use = i;
+
+ writel(i, ring->tail);
+ return IXGBE_XDP_TX;
+}
+
netdev_tx_t ixgbe_xmit_frame_ring(struct sk_buff *skb,
struct ixgbe_adapter *adapter,
struct ixgbe_ring *tx_ring)
@@ -8381,6 +8550,23 @@ static void ixgbe_netpoll(struct net_device *netdev)
#endif
+static void ixgbe_get_ring_stats64(struct rtnl_link_stats64 *stats,
+ struct ixgbe_ring *ring)
+{
+ u64 bytes, packets;
+ unsigned int start;
+
+ if (ring) {
+ do {
+ start = u64_stats_fetch_begin_irq(&ring->syncp);
+ packets = ring->stats.packets;
+ bytes = ring->stats.bytes;
+ } while (u64_stats_fetch_retry_irq(&ring->syncp, start));
+ stats->tx_packets += packets;
+ stats->tx_bytes += bytes;
+ }
+}
+
static void ixgbe_get_stats64(struct net_device *netdev,
struct rtnl_link_stats64 *stats)
{
@@ -8406,18 +8592,13 @@ static void ixgbe_get_stats64(struct net_device *netdev,
for (i = 0; i < adapter->num_tx_queues; i++) {
struct ixgbe_ring *ring = ACCESS_ONCE(adapter->tx_ring[i]);
- u64 bytes, packets;
- unsigned int start;
- if (ring) {
- do {
- start = u64_stats_fetch_begin_irq(&ring->syncp);
- packets = ring->stats.packets;
- bytes = ring->stats.bytes;
- } while (u64_stats_fetch_retry_irq(&ring->syncp, start));
- stats->tx_packets += packets;
- stats->tx_bytes += bytes;
- }
+ ixgbe_get_ring_stats64(stats, ring);
+ }
+ for (i = 0; i < adapter->num_xdp_queues; i++) {
+ struct ixgbe_ring *ring = ACCESS_ONCE(adapter->xdp_ring[i]);
+
+ ixgbe_get_ring_stats64(stats, ring);
}
rcu_read_unlock();
@@ -9559,9 +9740,23 @@ static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
return -EINVAL;
}
+ if (nr_cpu_ids > MAX_XDP_QUEUES)
+ return -ENOMEM;
+
old_prog = xchg(&adapter->xdp_prog, prog);
- for (i = 0; i < adapter->num_rx_queues; i++)
- xchg(&adapter->rx_ring[i]->xdp_prog, adapter->xdp_prog);
+
+ /* If transitioning XDP modes reconfigure rings */
+ if (!!prog != !!old_prog) {
+ int err = ixgbe_setup_tc(dev, netdev_get_num_tc(dev));
+
+ if (err) {
+ rcu_assign_pointer(adapter->xdp_prog, old_prog);
+ return -EINVAL;
+ }
+ } else {
+ for (i = 0; i < adapter->num_rx_queues; i++)
+ xchg(&adapter->rx_ring[i]->xdp_prog, adapter->xdp_prog);
+ }
if (old_prog)
bpf_prog_put(old_prog);
@@ -10070,6 +10265,9 @@ static int ixgbe_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
for (i = 0; i < adapter->num_tx_queues; i++)
u64_stats_init(&adapter->tx_ring[i]->syncp);
+ for (i = 0; i < adapter->num_xdp_queues; i++)
+ u64_stats_init(&adapter->xdp_ring[i]->syncp);
+
/* WOL not supported for all devices */
adapter->wol = 0;
hw->eeprom.ops.read(hw, 0x2c, &adapter->eeprom_cap);
^ permalink raw reply related
* [PATCH 1/2] ixgbe: add XDP support for pass and drop actions
From: John Fastabend @ 2017-04-24 1:31 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev
In-Reply-To: <20170424013004.8354.34893.stgit@john-Precision-Tower-5810>
Basic XDP drop support for ixgbe. Uses READ_ONCE/xchg semantics on XDP
programs instead of rcu primitives as suggested by Daniel Borkmann and
Alex Duyck.
Signed-off-by: John Fastabend <john.r.fastabend@intel.com>
Acked-by: Alexander Duyck <alexander.h.duyck@intel.com>
---
0 files changed
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe.h b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
index 656ca8f..cb14813 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe.h
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe.h
@@ -318,6 +318,7 @@ struct ixgbe_ring {
struct ixgbe_ring *next; /* pointer to next ring in q_vector */
struct ixgbe_q_vector *q_vector; /* backpointer to host q_vector */
struct net_device *netdev; /* netdev ring belongs to */
+ struct bpf_prog *xdp_prog;
struct device *dev; /* device for DMA mapping */
struct ixgbe_fwd_adapter *l2_accel_priv;
void *desc; /* descriptor ring memory */
@@ -555,6 +556,7 @@ struct ixgbe_adapter {
unsigned long active_vlans[BITS_TO_LONGS(VLAN_N_VID)];
/* OS defined structs */
struct net_device *netdev;
+ struct bpf_prog *xdp_prog;
struct pci_dev *pdev;
unsigned long state;
@@ -835,7 +837,7 @@ enum ixgbe_boards {
void ixgbe_reinit_locked(struct ixgbe_adapter *adapter);
void ixgbe_reset(struct ixgbe_adapter *adapter);
void ixgbe_set_ethtool_ops(struct net_device *netdev);
-int ixgbe_setup_rx_resources(struct ixgbe_ring *);
+int ixgbe_setup_rx_resources(struct ixgbe_adapter *, struct ixgbe_ring *);
int ixgbe_setup_tx_resources(struct ixgbe_ring *);
void ixgbe_free_rx_resources(struct ixgbe_ring *);
void ixgbe_free_tx_resources(struct ixgbe_ring *);
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
index 59730ed..79a126d 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_ethtool.c
@@ -1128,7 +1128,7 @@ static int ixgbe_set_ringparam(struct net_device *netdev,
sizeof(struct ixgbe_ring));
temp_ring[i].count = new_rx_count;
- err = ixgbe_setup_rx_resources(&temp_ring[i]);
+ err = ixgbe_setup_rx_resources(adapter, &temp_ring[i]);
if (err) {
while (i) {
i--;
@@ -1761,7 +1761,7 @@ static int ixgbe_setup_desc_rings(struct ixgbe_adapter *adapter)
rx_ring->netdev = adapter->netdev;
rx_ring->reg_idx = adapter->rx_ring[0]->reg_idx;
- err = ixgbe_setup_rx_resources(rx_ring);
+ err = ixgbe_setup_rx_resources(adapter, rx_ring);
if (err) {
ret_val = 4;
goto err_nomem;
diff --git a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
index 730742a..ebcf9c0 100644
--- a/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
+++ b/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
@@ -49,6 +49,9 @@
#include <linux/if_macvlan.h>
#include <linux/if_bridge.h>
#include <linux/prefetch.h>
+#include <linux/bpf.h>
+#include <linux/bpf_trace.h>
+#include <linux/atomic.h>
#include <scsi/fc/fc_fcoe.h>
#include <net/udp_tunnel.h>
#include <net/pkt_cls.h>
@@ -1856,6 +1859,10 @@ static void ixgbe_dma_sync_frag(struct ixgbe_ring *rx_ring,
* @rx_desc: pointer to the EOP Rx descriptor
* @skb: pointer to current skb being fixed
*
+ * Check if the skb is valid in the XDP case it will be an error pointer.
+ * Return true in this case to abort processing and advance to next
+ * descriptor.
+ *
* Check for corrupted packet headers caused by senders on the local L2
* embedded NIC switch not setting up their Tx Descriptors right. These
* should be very rare.
@@ -1874,6 +1881,10 @@ static bool ixgbe_cleanup_headers(struct ixgbe_ring *rx_ring,
{
struct net_device *netdev = rx_ring->netdev;
+ /* XDP packets use error pointer so abort at this point */
+ if (IS_ERR(skb))
+ return true;
+
/* verify that the packet does not have any known errors */
if (unlikely(ixgbe_test_staterr(rx_desc,
IXGBE_RXDADV_ERR_FRAME_ERR_MASK) &&
@@ -2049,7 +2060,7 @@ static void ixgbe_put_rx_buffer(struct ixgbe_ring *rx_ring,
/* hand second half of page back to the ring */
ixgbe_reuse_rx_page(rx_ring, rx_buffer);
} else {
- if (IXGBE_CB(skb)->dma == rx_buffer->dma) {
+ if (!IS_ERR(skb) && IXGBE_CB(skb)->dma == rx_buffer->dma) {
/* the page has been released from the ring */
IXGBE_CB(skb)->page_released = true;
} else {
@@ -2070,21 +2081,22 @@ static void ixgbe_put_rx_buffer(struct ixgbe_ring *rx_ring,
static struct sk_buff *ixgbe_construct_skb(struct ixgbe_ring *rx_ring,
struct ixgbe_rx_buffer *rx_buffer,
- union ixgbe_adv_rx_desc *rx_desc,
- unsigned int size)
+ struct xdp_buff *xdp,
+ union ixgbe_adv_rx_desc *rx_desc)
{
- void *va = page_address(rx_buffer->page) + rx_buffer->page_offset;
+ unsigned int size = xdp->data_end - xdp->data;
#if (PAGE_SIZE < 8192)
unsigned int truesize = ixgbe_rx_pg_size(rx_ring) / 2;
#else
- unsigned int truesize = SKB_DATA_ALIGN(size);
+ unsigned int truesize = SKB_DATA_ALIGN(xdp->data_end -
+ xdp->data_hard_start);
#endif
struct sk_buff *skb;
/* prefetch first cache line of first page */
- prefetch(va);
+ prefetch(xdp->data);
#if L1_CACHE_BYTES < 128
- prefetch(va + L1_CACHE_BYTES);
+ prefetch(xdp->data + L1_CACHE_BYTES);
#endif
/* allocate a skb to store the frags */
@@ -2097,7 +2109,7 @@ static struct sk_buff *ixgbe_construct_skb(struct ixgbe_ring *rx_ring,
IXGBE_CB(skb)->dma = rx_buffer->dma;
skb_add_rx_frag(skb, 0, rx_buffer->page,
- rx_buffer->page_offset,
+ xdp->data - page_address(rx_buffer->page),
size, truesize);
#if (PAGE_SIZE < 8192)
rx_buffer->page_offset ^= truesize;
@@ -2105,7 +2117,8 @@ static struct sk_buff *ixgbe_construct_skb(struct ixgbe_ring *rx_ring,
rx_buffer->page_offset += truesize;
#endif
} else {
- memcpy(__skb_put(skb, size), va, ALIGN(size, sizeof(long)));
+ memcpy(__skb_put(skb, size),
+ xdp->data, ALIGN(size, sizeof(long)));
rx_buffer->pagecnt_bias++;
}
@@ -2114,32 +2127,32 @@ static struct sk_buff *ixgbe_construct_skb(struct ixgbe_ring *rx_ring,
static struct sk_buff *ixgbe_build_skb(struct ixgbe_ring *rx_ring,
struct ixgbe_rx_buffer *rx_buffer,
- union ixgbe_adv_rx_desc *rx_desc,
- unsigned int size)
+ struct xdp_buff *xdp,
+ union ixgbe_adv_rx_desc *rx_desc)
{
- void *va = page_address(rx_buffer->page) + rx_buffer->page_offset;
#if (PAGE_SIZE < 8192)
unsigned int truesize = ixgbe_rx_pg_size(rx_ring) / 2;
#else
unsigned int truesize = SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) +
- SKB_DATA_ALIGN(IXGBE_SKB_PAD + size);
+ SKB_DATA_ALIGN(xdp->data_end -
+ xdp->data_hard_start);
#endif
struct sk_buff *skb;
/* prefetch first cache line of first page */
- prefetch(va);
+ prefetch(xdp->data);
#if L1_CACHE_BYTES < 128
- prefetch(va + L1_CACHE_BYTES);
+ prefetch(xdp->data + L1_CACHE_BYTES);
#endif
- /* build an skb around the page buffer */
- skb = build_skb(va - IXGBE_SKB_PAD, truesize);
+ /* build an skb to around the page buffer */
+ skb = build_skb(xdp->data_hard_start, truesize);
if (unlikely(!skb))
return NULL;
/* update pointers within the skb to store the data */
- skb_reserve(skb, IXGBE_SKB_PAD);
- __skb_put(skb, size);
+ skb_reserve(skb, xdp->data - xdp->data_hard_start);
+ __skb_put(skb, xdp->data_end - xdp->data);
/* record DMA address if this is the start of a chain of buffers */
if (!ixgbe_test_staterr(rx_desc, IXGBE_RXD_STAT_EOP))
@@ -2155,6 +2168,41 @@ static struct sk_buff *ixgbe_build_skb(struct ixgbe_ring *rx_ring,
return skb;
}
+#define IXGBE_XDP_PASS 0
+#define IXGBE_XDP_CONSUMED 1
+
+static struct sk_buff *ixgbe_run_xdp(struct ixgbe_ring *rx_ring,
+ struct xdp_buff *xdp)
+{
+ int result = IXGBE_XDP_PASS;
+ struct bpf_prog *xdp_prog;
+ u32 act;
+
+ rcu_read_lock();
+ xdp_prog = READ_ONCE(rx_ring->xdp_prog);
+
+ if (!xdp_prog)
+ goto xdp_out;
+
+ act = bpf_prog_run_xdp(xdp_prog, xdp);
+ switch (act) {
+ case XDP_PASS:
+ break;
+ default:
+ bpf_warn_invalid_xdp_action(act);
+ case XDP_TX:
+ case XDP_ABORTED:
+ trace_xdp_exception(rx_ring->netdev, xdp_prog, act);
+ /* fallthrough -- handle aborts by dropping packet */
+ case XDP_DROP:
+ result = IXGBE_XDP_CONSUMED;
+ break;
+ }
+xdp_out:
+ rcu_read_unlock();
+ return ERR_PTR(-result);
+}
+
/**
* ixgbe_clean_rx_irq - Clean completed descriptors from Rx ring - bounce buf
* @q_vector: structure containing interrupt and ring information
@@ -2184,6 +2232,7 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
union ixgbe_adv_rx_desc *rx_desc;
struct ixgbe_rx_buffer *rx_buffer;
struct sk_buff *skb;
+ struct xdp_buff xdp;
unsigned int size;
/* return some buffers to hardware, one at a time is too slow */
@@ -2206,14 +2255,29 @@ static int ixgbe_clean_rx_irq(struct ixgbe_q_vector *q_vector,
rx_buffer = ixgbe_get_rx_buffer(rx_ring, rx_desc, &skb, size);
/* retrieve a buffer from the ring */
- if (skb)
+ if (!skb) {
+ xdp.data = page_address(rx_buffer->page) +
+ rx_buffer->page_offset;
+ xdp.data_hard_start = xdp.data -
+ ixgbe_rx_offset(rx_ring);
+ xdp.data_end = xdp.data + size;
+
+ skb = ixgbe_run_xdp(rx_ring, &xdp);
+ }
+
+ if (IS_ERR(skb)) {
+ total_rx_packets++;
+ total_rx_bytes += size;
+ rx_buffer->pagecnt_bias++;
+ } else if (skb) {
ixgbe_add_rx_frag(rx_ring, rx_buffer, skb, size);
- else if (ring_uses_build_skb(rx_ring))
+ } else if (ring_uses_build_skb(rx_ring)) {
skb = ixgbe_build_skb(rx_ring, rx_buffer,
- rx_desc, size);
- else
+ &xdp, rx_desc);
+ } else {
skb = ixgbe_construct_skb(rx_ring, rx_buffer,
- rx_desc, size);
+ &xdp, rx_desc);
+ }
/* exit if we failed to retrieve a buffer */
if (!skb) {
@@ -6072,7 +6136,8 @@ static int ixgbe_setup_all_tx_resources(struct ixgbe_adapter *adapter)
*
* Returns 0 on success, negative on failure
**/
-int ixgbe_setup_rx_resources(struct ixgbe_ring *rx_ring)
+int ixgbe_setup_rx_resources(struct ixgbe_adapter *adapter,
+ struct ixgbe_ring *rx_ring)
{
struct device *dev = rx_ring->dev;
int orig_node = dev_to_node(dev);
@@ -6109,6 +6174,8 @@ int ixgbe_setup_rx_resources(struct ixgbe_ring *rx_ring)
rx_ring->next_to_clean = 0;
rx_ring->next_to_use = 0;
+ rx_ring->xdp_prog = adapter->xdp_prog;
+
return 0;
err:
vfree(rx_ring->rx_buffer_info);
@@ -6132,7 +6199,7 @@ static int ixgbe_setup_all_rx_resources(struct ixgbe_adapter *adapter)
int i, err = 0;
for (i = 0; i < adapter->num_rx_queues; i++) {
- err = ixgbe_setup_rx_resources(adapter->rx_ring[i]);
+ err = ixgbe_setup_rx_resources(adapter, adapter->rx_ring[i]);
if (!err)
continue;
@@ -6200,6 +6267,7 @@ void ixgbe_free_rx_resources(struct ixgbe_ring *rx_ring)
{
ixgbe_clean_rx_ring(rx_ring);
+ rx_ring->xdp_prog = NULL;
vfree(rx_ring->rx_buffer_info);
rx_ring->rx_buffer_info = NULL;
@@ -9468,6 +9536,54 @@ static void ixgbe_fwd_del(struct net_device *pdev, void *priv)
return features;
}
+static int ixgbe_xdp_setup(struct net_device *dev, struct bpf_prog *prog)
+{
+ int i, frame_size = dev->mtu + ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN;
+ struct ixgbe_adapter *adapter = netdev_priv(dev);
+ struct bpf_prog *old_prog;
+
+ if (adapter->flags & IXGBE_FLAG_SRIOV_ENABLED)
+ return -EINVAL;
+
+ if (adapter->flags & IXGBE_FLAG_DCB_ENABLED)
+ return -EINVAL;
+
+ /* verify ixgbe ring attributes are sufficient for XDP */
+ for (i = 0; i < adapter->num_rx_queues; i++) {
+ struct ixgbe_ring *ring = adapter->rx_ring[i];
+
+ if (ring_is_rsc_enabled(ring))
+ return -EINVAL;
+
+ if (frame_size > ixgbe_rx_bufsz(ring))
+ return -EINVAL;
+ }
+
+ old_prog = xchg(&adapter->xdp_prog, prog);
+ for (i = 0; i < adapter->num_rx_queues; i++)
+ xchg(&adapter->rx_ring[i]->xdp_prog, adapter->xdp_prog);
+
+ if (old_prog)
+ bpf_prog_put(old_prog);
+
+ return 0;
+}
+
+static int ixgbe_xdp(struct net_device *dev, struct netdev_xdp *xdp)
+{
+ struct ixgbe_adapter *adapter = netdev_priv(dev);
+
+ switch (xdp->command) {
+ case XDP_SETUP_PROG:
+ return ixgbe_xdp_setup(dev, xdp->prog);
+ case XDP_QUERY_PROG:
+ xdp->prog_attached = !!(adapter->xdp_prog);
+ return 0;
+ default:
+ return -EINVAL;
+ }
+}
+
static const struct net_device_ops ixgbe_netdev_ops = {
.ndo_open = ixgbe_open,
.ndo_stop = ixgbe_close,
@@ -9513,6 +9629,7 @@ static void ixgbe_fwd_del(struct net_device *pdev, void *priv)
.ndo_udp_tunnel_add = ixgbe_add_udp_tunnel_port,
.ndo_udp_tunnel_del = ixgbe_del_udp_tunnel_port,
.ndo_features_check = ixgbe_features_check,
+ .ndo_xdp = ixgbe_xdp,
};
/**
^ permalink raw reply related
* [PATCH 0/2] ixgbe updates
From: John Fastabend @ 2017-04-24 1:30 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev
Hi Jeff, Here are two patch updates for your tree. I merged in Alex's
build fix and Jakub's comments. Lightly tested xdp1 and xdp2.
---
John Fastabend (2):
ixgbe: add XDP support for pass and drop actions
ixgbe: add support for XDP_TX action
0 files changed
^ permalink raw reply
* Re: [PATCH net-next V3 2/2] rtnl: Add support for netdev event attribute to link messages
From: David Ahern @ 2017-04-24 1:07 UTC (permalink / raw)
To: Vladislav Yasevich, netdev; +Cc: Vladislav Yasevich, roopa, Jiri Pirko
In-Reply-To: <1492795881-11914-3-git-send-email-vyasevic@redhat.com>
On 4/21/17 11:31 AM, Vladislav Yasevich wrote:
> @@ -1276,9 +1277,40 @@ static int rtnl_xdp_fill(struct sk_buff *skb, struct net_device *dev)
> return err;
> }
>
> +static int rtnl_fill_link_event(struct sk_buff *skb, unsigned long event)
> +{
> + u32 rtnl_event;
> +
> + switch (event) {
> + case NETDEV_REBOOT:
> + rtnl_event = IFLA_EVENT_REBOOT;
> + break;
> + case NETDEV_FEAT_CHANGE:
> + rtnl_event = IFLA_EVENT_FEAT_CHANGE;
> + break;
> + case NETDEV_BONDING_FAILOVER:
> + rtnl_event = IFLA_EVENT_BONDING_FAILOVER;
> + break;
> + case NETDEV_NOTIFY_PEERS:
> + rtnl_event = IFLA_EVENT_NOTIFY_PEERS;
> + break;
> + case NETDEV_RESEND_IGMP:
> + rtnl_event = IFLA_EVENT_RESEND_IGMP;
> + break;
> + case NETDEV_CHANGEINFODATA:
> + rtnl_event = IFLA_EVENT_CHANGE_INFO_DATA;
> + break;
> + default:
> + return 0;
> + }
> +
> + return nla_put_u32(skb, IFLA_EVENT, rtnl_event);
> +}
> +
I still have doubts about encoding kernel events into a uapi.
For example, NETDEV_CHANGEINFODATA is only for bonds though nothing
about the name suggests it is a bonding notification. This one was added
specifically to notify userspace (d4261e5650004), yet seems to happen
only during a changelink and that already generates a RTM_NEWLINK
message via do_setlink. Since the rtnetlink_event message does not
contain anything "NETDEV_CHANGEINFODATA" related what purpose does it
really serve besides duplicating netlink messages to userspace.
The REBOOT, IGMP, FEAT_CHANGE and BONDING_FAILOVER seem to be unique
messages (code analysis only) which I get for notifying userspace.
NETDEV_NOTIFY_PEERS is not so clear in how often it duplicates other
messages.
^ permalink raw reply
* [PATCH 1/1] rndis_wlan: add return value validation
From: Pan Bian @ 2017-04-24 0:40 UTC (permalink / raw)
To: Jussi Kivilinna, Kalle Valo, linux-wireless, netdev
Cc: linux-kernel, Pan Bian
From: Pan Bian <bianpan2016@163.com>
Function create_singlethread_workqueue() will return a NULL pointer if
there is no enough memory, and its return value should be validated
before using. However, in function rndis_wlan_bind(), its return value
is not checked. This may cause NULL dereference bugs. This patch fixes
it.
Signed-off-by: Pan Bian <bianpan2016@163.com>
---
drivers/net/wireless/rndis_wlan.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/drivers/net/wireless/rndis_wlan.c b/drivers/net/wireless/rndis_wlan.c
index 785334f..92a1bde 100644
--- a/drivers/net/wireless/rndis_wlan.c
+++ b/drivers/net/wireless/rndis_wlan.c
@@ -3427,6 +3427,10 @@ static int rndis_wlan_bind(struct usbnet *usbdev, struct usb_interface *intf)
/* because rndis_command() sleeps we need to use workqueue */
priv->workqueue = create_singlethread_workqueue("rndis_wlan");
+ if (!priv->workqueue) {
+ wiphy_free(wiphy);
+ return -ENOMEM;
+ }
INIT_WORK(&priv->work, rndis_wlan_worker);
INIT_DELAYED_WORK(&priv->dev_poller_work, rndis_device_poller);
INIT_DELAYED_WORK(&priv->scan_work, rndis_get_scan_results);
--
1.9.1
^ permalink raw reply related
* 10985 netdev
From: kelley @ 2017-04-24 0:24 UTC (permalink / raw)
To: netdev
[-- Attachment #1: 024976477871603.zip --]
[-- Type: application/zip, Size: 1559 bytes --]
^ permalink raw reply
* RE:drivers:net:ethernet:adi:bfin_mac: Use FIELD_SIZEOF defined kernel macro
From: Karim Eshapa @ 2017-04-24 0:23 UTC (permalink / raw)
To: davem
Cc: tremyfr, a, fw, clabbe.montjoie, edumazet, jarod,
adi-buildroot-devel, netdev, linux-kernel, Karim Eshapa
In-Reply-To: <1492970524-12607-1-git-send-email-karim.eshapa@gmail.com>
On Sun, 23 Apr 2017 22:56:38 +0200, Geert Uytterhoeven:
>IMHO this makes the code less safe and less future-proof.
>What if the type of info is ever changed?
>There's no safety check to validate that the FIELD_SIZEOF() operates on the
>same data as the strlcpy() destination.
Really make sense :)
>As the kbuild test robot already told you, this doesn't compile.
>Please try to (at least) compile the code before sending patches.
>Thanks!
So sorry for that I built quickly using the wrong compiler and the wrong
target machine with my old .config that's horrible :)
Thanks a lot,
Karim
^ permalink raw reply
* Re: [PATCH v4 net-next] mdio_bus: Issue GPIO RESET to PHYs.
From: Andrew Lunn @ 2017-04-23 23:35 UTC (permalink / raw)
To: Lars-Peter Clausen
Cc: Roger Quadros, davem, Florian Fainelli, tony, nsekhar, jsarha,
netdev, linux-omap, linux-kernel
In-Reply-To: <1c36aab1-3411-f76d-67f7-e3f6d9e9fda5@metafoo.de>
On Fri, Apr 21, 2017 at 03:31:09PM +0200, Lars-Peter Clausen wrote:
> On 04/21/2017 03:15 PM, Roger Quadros wrote:
> > diff --git a/Documentation/devicetree/bindings/net/mdio.txt b/Documentation/devicetree/bindings/net/mdio.txt
> > new file mode 100644
> > index 0000000..4ffbbac
> > --- /dev/null
> > +++ b/Documentation/devicetree/bindings/net/mdio.txt
> > @@ -0,0 +1,33 @@
> > +Common MDIO bus properties.
> > +
> > +These are generic properties that can apply to any MDIO bus.
> > +
> > +Optional properties:
> > +- reset-gpios: List of one or more GPIOs that control the RESET lines
> > + of the PHYs on that MDIO bus.
> > +- reset-delay-us: RESET pulse width in microseconds as per PHY datasheet.
> > +
> > +A list of child nodes, one per device on the bus is expected. These
> > +should follow the generic phy.txt, or a device specific binding document.
> > +
> > +Example :
> > +This example shows these optional properties, plus other properties
> > +required for the TI Davinci MDIO driver.
> > +
> > + davinci_mdio: ethernet@0x5c030000 {
> > + compatible = "ti,davinci_mdio";
> > + reg = <0x5c030000 0x1000>;
> > + #address-cells = <1>;
> > + #size-cells = <0>;
> > +
> > + reset-gpios = <&gpio2 5 GPIO_ACTIVE_LOW>;
> > + reset-delay-us = <2>; /* PHY datasheet states 1us min */
>
> If this is the reset line of the PHY shouldn't it be a property of the PHY
> node rather than of the MDIO controller node (which might have a reset on
> its own)?
> > +
> > + ethphy0: ethernet-phy@1 {
> > + reg = <1>;
> > + };
> > +
> > + ethphy1: ethernet-phy@3 {
> > + reg = <3>;
> > + };
Hi Lars-Peter
We discussed this when the first proposal was made. There are two
cases, to consider.
1) Here, one GPIO line resets all PHYs on the same MDIO bus. In this
example, two PHYs.
2) There is one GPIO line per PHY. That is a separate case, and as you
say, the reset line should probably be considered a PHY property, not
an MDIO property. However, it can be messy, since in order to probe
the MDIO bus, you probably need to take the PHY out of reset.
Anyway, this patch addresses the first case, so should be accepted. If
anybody wants to address the second case, they are free to do so.
Andrew
^ permalink raw reply
* Re: [PATCH RFC] ptr_ring: add ptr_ring_unconsume
From: Michael S. Tsirkin @ 2017-04-23 23:28 UTC (permalink / raw)
To: Jason Wang; +Cc: linux-kernel, netdev
In-Reply-To: <0defb746-3d4b-14b3-1ad7-82842048ebba@redhat.com>
On Tue, Apr 18, 2017 at 11:07:42AM +0800, Jason Wang wrote:
>
>
> On 2017年04月17日 07:19, Michael S. Tsirkin wrote:
> > Applications that consume a batch of entries in one go
> > can benefit from ability to return some of them back
> > into the ring.
> >
> > Add an API for that - assuming there's space. If there's no space
> > naturally we can't do this and have to drop entries, but this implies
> > ring is full so we'd likely drop some anyway.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> >
> > Jason, in my mind the biggest issue with your batching patchset is the
> > backet drops on disconnect. This API will help avoid that in the common
> > case.
>
> Ok, I will rebase the series on top of this. (Though I don't think we care
> the packet loss).
E.g. I care - I often start sending packets to VM before it's
fully booted. Several vhost resets might follow.
> >
> > I would still prefer that we understand what's going on,
>
> I try to reply in another thread, does it make sense?
>
> > and I would
> > like to know what's the smallest batch size that's still helpful,
>
> Yes, I've replied in another thread, the result is:
>
>
> no batching 1.88Mpps
> RX_BATCH=1 1.93Mpps
> RX_BATCH=4 2.11Mpps
> RX_BATCH=16 2.14Mpps
> RX_BATCH=64 2.25Mpps
> RX_BATCH=256 2.18Mpps
Essentially 4 is enough, other stuf looks more like noise
to me. What about 2?
> > but
> > I'm not going to block the patch on these grounds assuming packet drops
> > are fixed.
>
> Thanks a lot.
>
> >
> > Lightly tested - this is on top of consumer batching patches.
> >
> > Thanks!
> >
> > include/linux/ptr_ring.h | 57 ++++++++++++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 57 insertions(+)
> >
> > diff --git a/include/linux/ptr_ring.h b/include/linux/ptr_ring.h
> > index 783e7f5..5fbeab4 100644
> > --- a/include/linux/ptr_ring.h
> > +++ b/include/linux/ptr_ring.h
> > @@ -457,6 +457,63 @@ static inline int ptr_ring_init(struct ptr_ring *r, int size, gfp_t gfp)
> > return 0;
> > }
> > +/*
> > + * Return entries into ring. Destroy entries that don't fit.
> > + *
> > + * Note: this is expected to be a rare slow path operation.
> > + *
> > + * Note: producer lock is nested within consumer lock, so if you
> > + * resize you must make sure all uses nest correctly.
> > + * In particular if you consume ring in interrupt or BH context, you must
> > + * disable interrupts/BH when doing so.
> > + */
> > +static inline void ptr_ring_unconsume(struct ptr_ring *r, void **batch, int n,
> > + void (*destroy)(void *))
> > +{
> > + unsigned long flags;
> > + int head;
> > +
> > + spin_lock_irqsave(&(r)->consumer_lock, flags);
> > + spin_lock(&(r)->producer_lock);
> > +
> > + if (!r->size)
> > + goto done;
> > +
> > + /*
> > + * Clean out buffered entries (for simplicity). This way following code
> > + * can test entries for NULL and if not assume they are valid.
> > + */
> > + head = r->consumer_head - 1;
> > + while (likely(head >= r->consumer_tail))
> > + r->queue[head--] = NULL;
> > + r->consumer_tail = r->consumer_head;
> > +
> > + /*
> > + * Go over entries in batch, start moving head back and copy entries.
> > + * Stop when we run into previously unconsumed entries.
> > + */
> > + while (n--) {
> > + head = r->consumer_head - 1;
> > + if (head < 0)
> > + head = r->size - 1;
> > + if (r->queue[head]) {
> > + /* This batch entry will have to be destroyed. */
> > + ++n;
> > + goto done;
> > + }
> > + r->queue[head] = batch[n];
> > + r->consumer_tail = r->consumer_head = head;
> > + }
> > +
> > +done:
> > + /* Destroy all entries left in the batch. */
> > + while (n--) {
> > + destroy(batch[n]);
> > + }
> > + spin_unlock(&(r)->producer_lock);
> > + spin_unlock_irqrestore(&(r)->consumer_lock, flags);
> > +}
> > +
> > static inline void **__ptr_ring_swap_queue(struct ptr_ring *r, void **queue,
> > int size, gfp_t gfp,
> > void (*destroy)(void *))
^ permalink raw reply
* I find one aspect of the ip link show command confusing and I'd like you to fix it, please
From: Jeff Silverman @ 2017-04-23 22:36 UTC (permalink / raw)
To: netdev
People,
When my NIC is up, but not connected, I see:
root@jeff-desktop:~# ip link show enp3s0
2: enp3s0: <NO-CARRIER,BROADCAST,MULTICAST,UP> mtu 1500 qdisc mq state
DOWN mode DEFAULT group default qlen 1000
link/ether 00:10:18:cc:9c:77 brd ff:ff:ff:ff:ff:ff
root@jeff-desktop:~# ip link show enp3s0
NO-CARRIER makes sense to me - if the wire is unplugged, then the NIC
isn't seeing the humm at the beginning of each packet. That's clear.
Note that even though my link isn't plugged in, ip still notes that
it is up. That's great.
But if I down my NIC, there is no indication that it is DOWN other
than you can't see the UP flag. If somebody was new to linux, they
would not see what's not there.
2: enp3s0: <BROADCAST,MULTICAST> mtu 1500 qdisc mq state DOWN mode
DEFAULT group default qlen 1000
link/ether 00:10:18:cc:9c:77 brd ff:ff:ff:ff:ff:ff
What I would like you to do is modify the ip command so that if the
NIC has been downed by something, then it explicitly says DOWN.
What would be really nice would be if you enumerated all of the flags
that an interface can have, and note if the flag is set or cleared.
But that's more than what I want with this message.
Many thanks,
Jeff
--
Jeff Silverman, linux sysadmin
nine two four twentieth avenue east
Seattle, WA, nine eight one one two -3507
(253) 459-2318
jeffsilverm@gmail.c0m (note the zero!)
http://www.commercialventvac.com
See my portfolio of writings and talks
^ permalink raw reply
* Re: [PATCH 1/1] drivers:net:ethernet:adi:bfin_mac: Use FIELD_SIZEOF defined kernel macro
From: Geert Uytterhoeven @ 2017-04-23 20:56 UTC (permalink / raw)
To: Karim Eshapa
Cc: David S. Miller, Philippe Reynes, Antonio Quartulli,
Florian Westphal, LABBE Corentin, Eric Dumazet, Jarod Wilson,
adi-buildroot-devel@lists.sourceforge.net, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <1492970524-12607-1-git-send-email-karim.eshapa@gmail.com>
Hi Karim,
On Sun, Apr 23, 2017 at 8:02 PM, Karim Eshapa <karim.eshapa@gmail.com> wrote:
> Use FIELD_SIZEOF defined kernel macro kernel.h
>
> Signed-off-by: Karim Eshapa <karim.eshapa@gmail.com>
> ---
> drivers/net/ethernet/adi/bfin_mac.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/ethernet/adi/bfin_mac.c b/drivers/net/ethernet/adi/bfin_mac.c
> index a9ac58c..60346e0 100644
> --- a/drivers/net/ethernet/adi/bfin_mac.c
> +++ b/drivers/net/ethernet/adi/bfin_mac.c
> @@ -452,10 +452,14 @@ static irqreturn_t bfin_mac_wake_interrupt(int irq, void *dev_id)
> static void bfin_mac_ethtool_getdrvinfo(struct net_device *dev,
> struct ethtool_drvinfo *info)
> {
> - strlcpy(info->driver, KBUILD_MODNAME, sizeof(info->driver));
> - strlcpy(info->version, DRV_VERSION, sizeof(info->version));
> - strlcpy(info->fw_version, "N/A", sizeof(info->fw_version));
> - strlcpy(info->bus_info, dev_name(&dev->dev), sizeof(info->bus_info));
> + strlcpy(info->driver, KBUILD_MODNAME, FIELD_SIZEOF(
> + struct ethtool_drvinfo, driver));
IMHO this makes the code less safe and less future-proof.
What if the type of info is ever changed?
There's no safety check to validate that the FIELD_SIZEOF() operates on the
same data as the strlcpy() destination.
> + strlcpy(info->version, DRV_VERSION, FIELD_SIZEOF(
> + struct ethtool_drvinfo, version));
> + strlcpy(info->fw_version, "N/A", FIELD_SIZEOF(
> + struct ethtool_drvinfo, fw_version));
> + strlcpy(info->bus_info, dev_name(&dev->dev), FIELD_SIZEOF(
> + struct ethtool_drvinfo, bus_info));
> }
>
> static void bfin_mac_ethtool_getwol(struct net_device *dev,
> @@ -785,7 +789,7 @@ static int bfin_mac_hwtstamp_get(struct net_device *netdev,
> struct bfin_mac_local *lp = netdev_priv(netdev);
>
> return copy_to_user(ifr->ifr_data, &lp->stamp_cfg,
> - sizeof(lp->stamp_cfg)) ?
> + FILD_SIZEOF(struct bfin_mac_local, stamp_cfg)) ?
As the kbuild test robot already told you, this doesn't compile.
Please try to (at least) compile the code before sending patches.
Thanks!
Gr{oetje,eeting}s,
Geert
^ permalink raw reply
* Re: [PATCH 1/1] openvswitch: check return value of nla_nest_start
From: Pravin Shelar @ 2017-04-23 20:22 UTC (permalink / raw)
To: Pan Bian
Cc: ovs dev, Linux Kernel Network Developers, David S. Miller,
linux-kernel-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1492929782-1112-1-git-send-email-bianpan2016-9Onoh4P/yGk@public.gmane.org>
On Sat, Apr 22, 2017 at 11:43 PM, Pan Bian <bianpan2016-9Onoh4P/yGk@public.gmane.org> wrote:
> Function nla_nest_start() will return a NULL pointer on error, and its
> return value should be validated before it is used. However, in function
> queue_userspace_packet(), its return value is ignored. This may result
> in NULL dereference when calling nla_nest_end(). This patch fixes the
> bug.
>
> Signed-off-by: Pan Bian <bianpan2016-9Onoh4P/yGk@public.gmane.org>
> ---
> net/openvswitch/datapath.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/net/openvswitch/datapath.c b/net/openvswitch/datapath.c
> index 9c62b63..34c0fbd 100644
> --- a/net/openvswitch/datapath.c
> +++ b/net/openvswitch/datapath.c
> @@ -489,7 +489,8 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
> err = ovs_nla_put_tunnel_info(user_skb,
> upcall_info->egress_tun_info);
> BUG_ON(err);
> - nla_nest_end(user_skb, nla);
> + if (nla)
> + nla_nest_end(user_skb, nla);
There is enough memory allocated for the netlink message accounting
for all attributes beforehand. So it is not required to check retuned
'nla' after each attributes addition to the msg.
> }
>
> if (upcall_info->actions_len) {
> @@ -497,7 +498,7 @@ static int queue_userspace_packet(struct datapath *dp, struct sk_buff *skb,
> err = ovs_nla_put_actions(upcall_info->actions,
> upcall_info->actions_len,
> user_skb);
> - if (!err)
> + if (!err && nla)
> nla_nest_end(user_skb, nla);
> else
> nla_nest_cancel(user_skb, nla);
> --
> 1.9.1
>
>
> _______________________________________________
> dev mailing list
> dev-yBygre7rU0TnMu66kgdUjQ@public.gmane.org
> https://mail.openvswitch.org/mailman/listinfo/ovs-dev
^ permalink raw reply
* Re: [PATCH 1/1] drivers:net:ethernet:adi:bfin_mac: Use FIELD_SIZEOF defined kernel macro
From: kbuild test robot @ 2017-04-23 20:14 UTC (permalink / raw)
To: Karim Eshapa
Cc: kbuild-all, davem, tremyfr, a, fw, clabbe.montjoie, edumazet,
jarod, adi-buildroot-devel, netdev, linux-kernel, Karim Eshapa
In-Reply-To: <1492970524-12607-1-git-send-email-karim.eshapa@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 2043 bytes --]
Hi Karim,
[auto build test ERROR on net-next/master]
[also build test ERROR on v4.11-rc7 next-20170421]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Karim-Eshapa/drivers-net-ethernet-adi-bfin_mac-Use-FIELD_SIZEOF-defined-kernel-macro/20170424-022248
config: blackfin-BF518F-EZBRD_defconfig (attached as .config)
compiler: bfin-uclinux-gcc (GCC) 6.2.0
reproduce:
wget https://raw.githubusercontent.com/01org/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=blackfin
All error/warnings (new ones prefixed by >>):
drivers/net/ethernet/adi/bfin_mac.c: In function 'bfin_mac_hwtstamp_get':
>> drivers/net/ethernet/adi/bfin_mac.c:792:8: error: implicit declaration of function 'FILD_SIZEOF' [-Werror=implicit-function-declaration]
FILD_SIZEOF(struct bfin_mac_local, stamp_cfg)) ?
^~~~~~~~~~~
>> drivers/net/ethernet/adi/bfin_mac.c:792:20: error: expected expression before 'struct'
FILD_SIZEOF(struct bfin_mac_local, stamp_cfg)) ?
^~~~~~
>> drivers/net/ethernet/adi/bfin_mac.c:794:1: warning: control reaches end of non-void function [-Wreturn-type]
}
^
cc1: some warnings being treated as errors
vim +/FILD_SIZEOF +792 drivers/net/ethernet/adi/bfin_mac.c
786 static int bfin_mac_hwtstamp_get(struct net_device *netdev,
787 struct ifreq *ifr)
788 {
789 struct bfin_mac_local *lp = netdev_priv(netdev);
790
791 return copy_to_user(ifr->ifr_data, &lp->stamp_cfg,
> 792 FILD_SIZEOF(struct bfin_mac_local, stamp_cfg)) ?
793 -EFAULT : 0;
> 794 }
795
796 static void bfin_tx_hwtstamp(struct net_device *netdev, struct sk_buff *skb)
797 {
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 12151 bytes --]
^ permalink raw reply
* Re: [PATCH net-next] bpf, doc: update list of architectures that do eBPF JIT
From: David Miller @ 2017-04-23 19:57 UTC (permalink / raw)
To: ast; +Cc: daniel, netdev
In-Reply-To: <20170423160100.1155435-1-ast@fb.com>
From: Alexei Starovoitov <ast@fb.com>
Date: Sun, 23 Apr 2017 09:01:00 -0700
> update the list and remove 'in the future' statement,
> since all still alive 64-bit architectures now do eBPF JIT.
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Applied, thanks :)
^ permalink raw reply
* Re: [PATCH] brcm80211: brcmfmac: Ensure that incoming skb's are writable
From: Arend Van Spriel @ 2017-04-23 19:34 UTC (permalink / raw)
To: James Hughes
Cc: netdev, Franky Lin, Hante Meuleman, Kalle Valo, linux-wireless
In-Reply-To: <CAE_XsMKSO=P-h5AyPJy=59zDq=DmLFqBAX8Bqsofey=aAFD5dQ@mail.gmail.com>
On 21-4-2017 11:22, James Hughes wrote:
> On 20 April 2017 at 20:48, Arend van Spriel
> <arend.vanspriel@broadcom.com> wrote:
>> + linux-wireless
>>
>> On 4/20/2017 1:16 PM, James Hughes wrote:
>>>
>>> The driver was adding header information to incoming skb
>>> without ensuring the head was uncloned and hence writable.
>>>
>>> skb_cow_head has been used to ensure they are writable, however,
>>> this required some changes to error handling to ensure that
>>> if skb_cow_head failed it was not ignored.
>>>
>>> This really needs to be reviewed by someone who is more familiar
>>> with this code base to ensure any deallocation of skb's is
>>> still correct.
>>>
>>> Signed-off-by: James Hughes <james.hughes@raspberrypi.org>
>>> ---
>>> .../wireless/broadcom/brcm80211/brcmfmac/bcdc.c | 15 ++++++++--
>>> .../wireless/broadcom/brcm80211/brcmfmac/core.c | 23 +++++-----------
>>> .../broadcom/brcm80211/brcmfmac/fwsignal.c | 32
>>> +++++++++++++++++-----
>>> .../wireless/broadcom/brcm80211/brcmfmac/sdio.c | 7 ++++-
>>> 4 files changed, 51 insertions(+), 26 deletions(-)
>>>
>>
>> [...]
>>
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> index 5eaac13..08272e8 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>> @@ -198,7 +198,7 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct
>>> sk_buff *skb,
>>> int ret;
>>> struct brcmf_if *ifp = netdev_priv(ndev);
>>> struct brcmf_pub *drvr = ifp->drvr;
>>> - struct ethhdr *eh = (struct ethhdr *)(skb->data);
>>> + struct ethhdr *eh;
>>> brcmf_dbg(DATA, "Enter, bsscfgidx=%d\n", ifp->bsscfgidx);
>>> @@ -212,23 +212,14 @@ static netdev_tx_t brcmf_netdev_start_xmit(struct
>>> sk_buff *skb,
>>> }
>>> /* Make sure there's enough room for any header */
>>> - if (skb_headroom(skb) < drvr->hdrlen) {
>>> - struct sk_buff *skb2;
>>> -
>>> - brcmf_dbg(INFO, "%s: insufficient headroom\n",
>>> - brcmf_ifname(ifp));
>>> - drvr->bus_if->tx_realloc++;
>>> - skb2 = skb_realloc_headroom(skb, drvr->hdrlen);
>>> - dev_kfree_skb(skb);
>>> - skb = skb2;
>>> - if (skb == NULL) {
>>> - brcmf_err("%s: skb_realloc_headroom failed\n",
>>> - brcmf_ifname(ifp));
>>> - ret = -ENOMEM;
>>> - goto done;
>>> - }
>>
>>
>> What you are throwing away here is code that assures there is sufficient
>> headroom for protocol and bus layer in the tx path, because that is
>> determined by drvr->hdrlen. This is where the skb is handed to the driver so
>> if you could leave the functionality above *and* assure it is writeable that
>> would be the best solution as there is no need for all the other changes
>> down the tx path.
>
> The skb_cow_head function takes the required headroom as a parameter
> and will ensure that there is enough space, so I don't think this code
> segment is
> required or have I misunderstood what you mean here?
I looked more into what skb_cow_head() and skb_realloc_headroom()
actually do and it seems you are right.
> Is it safe to rely on the _cow_ being done here and not further down
> in the stack?
> Or at least checked further down in the stack. Previous comments from
> another patch
> requested that the _cow_ be done close to the actual addition of the
> header. I presume
> it is unlikely/impossible that the functions that add header
> information down the stack
> will be called without the above being done first?
It is safe. During probe sequence each layer in the driver stack
increments drvr->hdrlen with headroom it needs. So drvr->hdrlen will
indicate the total headroom needed when .start_xmit() is called. And
yes, the .start_xmit() is the single point of entry in the transmit
path. So the other locations where skb_push() is done are safe.
>>
>>> + ret = skb_cow_head(skb, drvr->hdrlen);
>>> + if (ret) {
>>
>>
>> So move the realloc code above here instead of simply freeing the skb.
>>
>>> + dev_kfree_skb_any(skb);
>>> + goto done;
>>> }
>>> + eh = (struct ethhdr *)(skb->data);
>>
>>
>> Now this is actually a separate fix so I would like a separate patch for it.
>>
>
> No problem, but see final paragraph below.
Let me see...
>> I have a RPi3 sitting on my desk so how can I replicate the issue. It was
>> something about broadcast/multicast traffic when using AP mode and a bridge,
>> right?
>>
>> Regards,
>> Arend
>
> See this issue for details on replication.
> https://github.com/raspberrypi/firmware/issues/673
>
> The bridge I use is setup using a similar procedure to that described
> here. https://www.raspberrypi.org/documentation/configuration/wireless/access-point.md
>
> I'm happy to pass over this work to you guys if you think it is
> appropriate, you are the
> experts on the codebase.
Sure. However, I must say you did an excellent job digging into the
issue and root causing it. We always were under the impression that we
could treat the skb as our own when handed to us in .start_xmit() callback.
Regards,
Arend
^ 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