* Re: [PATCH net-next V1 1/2] ethtool: Support for configurable RSS hash function
From: Eyal perry @ 2014-11-26 20:29 UTC (permalink / raw)
To: David Miller, ben
Cc: amirv, netdev, ogerlitz, yevgenyp, eyalpe, Tom Lendacky,
ariel.elior, prashant, mchan, hariprasad, sathya.perla,
subbu.seetharaman, ajit.khaparde, jeffrey.t.kirsher,
jesse.brandeburg, bruce.w.allan, carolyn.wyborny,
donald.c.skidmore, gregory.v.rose, matthew.vick, john.ronciak,
mitch.a.williams, linux-net-drivers, sshah, sbhatewara,
pv-drivers
In-Reply-To: <20141122.165407.641057904952001007.davem@davemloft.net>
On 11/22/2014 11:54 PM, David Miller wrote:
> From: Amir Vadai <amirv@mellanox.com>
> Date: Thu, 20 Nov 2014 16:26:49 +0200
>
>> + /* We require at least one supported parameter to be changed and no
>> + * change in any of the unsupported parameters
>> + */
>> + if ((!indir && !key) || hfunc != ETH_RSS_HASH_NO_CHANGE)
>> + return -EOPNOTSUPP;
>> +
>
> I know it will make more work for you, but all of these driver
> implementations of this hook should:
>
> 1) Accept hfunc of whatever hash function the chip is using,
> not just ETH_RSS_HASH_NO_CHANGE.
>
> 2) Provide an accurate hfunc value in the ->get() call.
Hello David, Ben, et al,
Before submitting V2, I'd like to consult you regarding the
implementation shown above. I thought of skipping the validity check
which I've described above as "We require at least one supported
parameter...", instead, I think it's better to fail the ->set() call
only in case of unsupported action requested, e.g.:
+ if (hfunc != ETH_RSS_HASH_NO_CHANGE &&
+ hfunc != ETH_RSS_HASH_TOP)
+ return -EOPNOTSUPP;
+ if (indir)
+ /* set indirection table code ... */
+ if (key)
+ /* set hash key code ... */
The drawbacks are the change of previous behavior (only requests for at
least one change were supported), however it seems more reasonable and
makes the code much more readable.
In similar manner, for the ->get() call, remove the validity checks (as
I suggested in V1), and just protect against NULL pointer dereference, e.g:
- if (!indir && !key)
- return -EOPNOTSUPP;
+ if (indir)
+ /* fill in the given indirection table array */
+ if (key)
+ /* fill in the given hash key array */
+ if (hfunc)
+ *hfunc = ETH_RSS_HASH_TOP;
Please advise,
Thanks,
Eyal.
^ permalink raw reply
* Re: Multiple DSA switch on shared MII
From: Florian Fainelli @ 2014-11-26 20:23 UTC (permalink / raw)
To: Rajib Karmakar; +Cc: netdev
In-Reply-To: <CAOi_9k8WySmjm5cdFn1kJQuE7cJ1rDyM9SV2RvbcmbB+UeTVdQ@mail.gmail.com>
On 25/11/14 23:33, Rajib Karmakar wrote:
> Hello Florian,
>
> Thanks for your reply.
>
> On Wed, Nov 26, 2014 at 10:52 AM, Florian Fainelli <f.fainelli@gmail.com> wrote:
>> 2014-11-25 20:52 GMT-08:00 Rajib Karmakar <rajibkit@gmail.com>:
>>> Hello,
>>>
>>> I am developing a DSA driver for Marvell 6172 and need to create 2 dsa
>>> switch chip (one for WAN and one for LAN).
>>
>> This is not typically how it is designed to work, you would register
>> one dsa switch chip with the ports assignment in this data structure.
>> Right now, DSA does not support a dual Ethernet MAC configuration,
>> although you could probably do per-port VLAN membership and create two
>> default VLANs to allow that.
>>
>>>
>>> My device has 2 MACs and one shared mii_bus. I have added two
>>> dsa_platform_data structures and registered them but cannot probe as
>>> dsa_probe tries mdio_register() on the same bus and fails.
>>>
>>> Is it possible to create two DSA switch on same (shared) mii?
>>
>> Not in its current form, and I am not exactly sure how we would support that.
>
> Yes, not with the current DSA implementation, but I can manage to
> solve this by a small (ugly) patch - renamed the slave mii bus as
> "<master_bus->id>:<pd->sw_addr>:<platform_device->id>" instead of
> "<master_bus->id>:<pd->sw_addr>". Though I am not yet sure enough if
> this could have any negative impact or not.
This will register two virtual slave mii buses backed by the same real
mdio bus driver, although that is not necessarily a problem, you want to
make sure that they are not going to poll the same PHYs in the switch
driver.
This is probably the easiest way to achieve what you want, can you just
introduce a check on the "id" being >= 0 (using -1 with platform_devices
is valid when there is just one device in the system).
Thanks!
>
> Comments please.
>
>>
>>>
>>> Regards,
>>> Rajib
>>> --
>>> 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
>>
>>
>>
>> --
>> Florian
>
> ----
> diff -rup org/net/dsa/dsa.c mod/net/dsa/dsa.c
> --- org/net/dsa/dsa.c
> +++ mod/net/dsa/dsa.c
> @@ -68,7 +68,7 @@ dsa_switch_probe(struct mii_bus *bus, in
>
> /* basic switch operations **************************************************/
> static struct dsa_switch *
> -dsa_switch_setup(struct dsa_switch_tree *dst, int index,
> +dsa_switch_setup(struct dsa_switch_tree *dst, int index, int id,
> struct device *parent, struct mii_bus *bus)
> {
> struct dsa_chip_data *pd = dst->pd->chip + index;
> @@ -156,7 +156,7 @@ dsa_switch_setup(struct dsa_switch_tree
> ret = -ENOMEM;
> goto out;
> }
> - dsa_slave_mii_bus_init(ds);
> + dsa_slave_mii_bus_init(ds, id);
>
> ret = mdiobus_register(ds->slave_mii_bus);
> if (ret < 0)
> @@ -349,7 +349,7 @@ static int dsa_probe(struct platform_dev
> continue;
> }
>
> - ds = dsa_switch_setup(dst, i, &pdev->dev, bus);
> + ds = dsa_switch_setup(dst, i, pdev->id, &pdev->dev, bus);
> if (IS_ERR(ds)) {
> printk(KERN_ERR "%s[%d]: couldn't create dsa switch "
> "instance (error %ld)\n", dev->name, i,
> diff -rup org/net/dsa/dsa_priv.h mod/net/dsa/dsa_priv.h
> --- org/net/dsa/dsa_priv.h
> +++ mod/net/dsa/dsa_priv.h
> @@ -163,7 +163,7 @@ void register_switch_driver(struct dsa_s
> void unregister_switch_driver(struct dsa_switch_driver *type);
>
> /* slave.c */
> -void dsa_slave_mii_bus_init(struct dsa_switch *ds);
> +void dsa_slave_mii_bus_init(struct dsa_switch *ds, int id);
> struct net_device *dsa_slave_create(struct dsa_switch *ds,
> struct device *parent,
> int port, char *name);
> diff -rup org/net/dsa/slave.c mod/net/dsa/slave.c
> --- org/net/dsa/slave.c
> +++ mod/net/dsa/slave.c
> @@ -35,14 +35,14 @@ static int dsa_slave_phy_write(struct mi
> return 0;
> }
>
> -void dsa_slave_mii_bus_init(struct dsa_switch *ds)
> +void dsa_slave_mii_bus_init(struct dsa_switch *ds, int id)
> {
> ds->slave_mii_bus->priv = (void *)ds;
> ds->slave_mii_bus->name = "dsa slave smi";
> ds->slave_mii_bus->read = dsa_slave_phy_read;
> ds->slave_mii_bus->write = dsa_slave_phy_write;
> - snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%s:%.2x",
> - ds->master_mii_bus->id, ds->pd->sw_addr);
> + snprintf(ds->slave_mii_bus->id, MII_BUS_ID_SIZE, "%s:%.2x:%.1x",
> + ds->master_mii_bus->id, ds->pd->sw_addr, id);
> ds->slave_mii_bus->parent = &ds->master_mii_bus->dev;
> }
>
^ permalink raw reply
* Re: [PATCH net-next] sky2: Fix crash inside sky2_rx_clean
From: David Miller @ 2014-11-26 20:17 UTC (permalink / raw)
To: mlindner; +Cc: netdev
In-Reply-To: <5475E012.3060607@marvell.com>
From: Mirko Lindner <mlindner@marvell.com>
Date: Wed, 26 Nov 2014 15:13:38 +0100
> If sky2->tx_le = pci_alloc_consistent() or sky2->tx_ring = kcalloc() in
> sky2_alloc_buffers() fails, sky2->rx_ring = kcalloc() will never be called.
> In this error case handling, sky2_rx_clean() is called from within
> sky2_free_buffers().
>
> In sky2_rx_clean() we find the following:
>
> ...
> memset(sky2->rx_le, 0, RX_LE_BYTES);
> ...
>
> This results in a memset using a NULL pointer and will crash the system.
>
> Signed-off-by: Mirko Lindner <mlindner@marvell.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH net-next] ipvlan: fix sparse warnings
From: David Miller @ 2014-11-26 20:13 UTC (permalink / raw)
To: maheshb; +Cc: netdev
In-Reply-To: <1416979483-30610-1-git-send-email-maheshb@google.com>
From: Mahesh Bandewar <maheshb@google.com>
Date: Tue, 25 Nov 2014 21:24:43 -0800
> Fix sparse warnings reported by kbuild robot
>
> drivers/net/ipvlan/ipvlan_main.c:172:13: warning: symbol 'ipvlan_start_xmit' was not declared. Should it be static?
> drivers/net/ipvlan/ipvlan_main.c:256:33: warning: incorrect type in initializer (different address spaces)
> drivers/net/ipvlan/ipvlan_main.c:256:33: expected void const [noderef] <asn:3>*__vpp_verify
> drivers/net/ipvlan/ipvlan_main.c:256:33: got struct ipvl_pcpu_stats *<noident>
> drivers/net/ipvlan/ipvlan_main.c:544:5: warning: symbol 'ipvlan_link_register' was not declared. Should it be static
>
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Mahesh Bandewar <maheshb@google.com>
Applied, can you please fix the build failures that have been
reported? Those are more important. Namely:
====================
Building with the attached random configuration file,
In file included from drivers/net/ipvlan/ipvlan.h:27:0,
from drivers/net/ipvlan/ipvlan_core.c:10:
include/net/gre.h: In function ‘gre_handle_offloads’:
include/net/gre.h:42:2: error: implicit declaration of function
‘iptunnel_handle_offloads’ [-Werror=implicit-function-declaration]
return iptunnel_handle_offloads(skb, csum,
^
include/net/gre.h:42:2: warning: return makes pointer from integer
without a cast [enabled by default]
drivers/net/ipvlan/ipvlan_core.c: In function ‘ipvlan_process_v6_outbound’:
drivers/net/ipvlan/ipvlan_core.c:379:2: error: implicit declaration of function
‘ip6_route_output’ [-Werror=implicit-function-declaration]
dst = ip6_route_output(dev_net(dev), NULL, &fl6);
^
drivers/net/ipvlan/ipvlan_core.c:379:6: warning: assignment makes
pointer from integer without a cast [enabled by default]
dst = ip6_route_output(dev_net(dev), NULL, &fl6);
^
CC [M] drivers/mtd/mtdblock_ro.o
cc1: some warnings being treated as errors
make[3]: *** [drivers/net/ipvlan/ipvlan_core.o] Error 1
====================
You probably need to add a bunch of missing dependencies to
the Kconfig entry for the ipvlan driver.
For one thing it definitely needs "INET" and that explains
the lack of iptunnel_handle_offloads() when including net/gre.h
Likewise for the ipv6 stuff.
^ permalink raw reply
* Re: [PATCH net-next 0/3] net: bcmgenet: EEE support
From: David Miller @ 2014-11-26 20:09 UTC (permalink / raw)
To: f.fainelli; +Cc: netdev, pgynther
In-Reply-To: <1416978996-24808-1-git-send-email-f.fainelli@gmail.com>
From: Florian Fainelli <f.fainelli@gmail.com>
Date: Tue, 25 Nov 2014 21:16:33 -0800
> This patch series add EEE support to the Broadcom GENET driver, first patch
> basically adds register definitions for the hardware, the second patch does the
> actual implementation and hooks the {get,set}_eee ethtool callbacks, the last
> patch adds auto-negotiation restart capability.
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH net] net-timestamp: make tcp_recvmsg call ipv6_recv_error for AF_INET6 socks
From: Willem de Bruijn @ 2014-11-26 20:03 UTC (permalink / raw)
To: Network Development
Cc: David Miller, Eric Dumazet, kuznet, jmorris, yoshfuji,
Patrick McHardy, Willem de Bruijn
In-Reply-To: <1417031582-16480-1-git-send-email-willemb@google.com>
On Wed, Nov 26, 2014 at 2:53 PM, Willem de Bruijn <willemb@google.com> wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> TCP timestamping introduced MSG_ERRQUEUE handling for TCP sockets.
> If the socket is of family AF_INET6, call ipv6_recv_error instead
> of ip_recv_error.
>
> This change is more complex than a single branch due to the loadable
> ipv6 module. It reuses a pre-existing indirect function call from
> ping. The ping code is safe to call, because it is part of the core
> ipv6 module and always present when AF_INET6 sockets are active.
I forgot to add:
Fixes: 4ed2d765 (net-timestamp: TCP timestamping)
> Signed-off-by: Willem de Bruijn <willemb@google.com>
>
> ----
>
> It may also be worthwhile to add WARN_ON_ONCE(sk->family == AF_INET6)
> to ip_recv_error.
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Joe Perches @ 2014-11-26 20:00 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Quentin Lambert, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAADnVQK3iui8cAmhi28=65p2CJbTu45fR98ri8Uh8xLRb=cwnA@mail.gmail.com>
On Wed, 2014-11-26 at 10:34 -0800, Alexei Starovoitov wrote:
> On Wed, Nov 26, 2014 at 10:02 AM, Joe Perches <joe@perches.com> wrote:
> > On Wed, 2014-11-26 at 09:23 -0800, Alexei Starovoitov wrote:
> >> On Wed, Nov 26, 2014 at 8:58 AM, Joe Perches <joe@perches.com> wrote:
> >
> >> > Is there any value in reordering these tests for frequency
> >> > or maybe using | instead of || to avoid multiple jumps?
> >>
> >> probably not. It's not a critical path.
> >> compiler may fuse conditions depending on values anyway.
> >> If it was a critical path, we could have used
> >> (1 << reg) & mask trick.
> >> I picked explicit 'return true' else 'return false' here,
> >> because it felt easier to read. Just a matter of taste.
> >
> > There is a size difference though: (allyesconfig)
> >
> > $ size arch/x86/net/built-in.o*
> > text data bss dec hex filename
> > 12999 1012 4336 18347 47ab arch/x86/net/built-in.o.new
> > 13177 1076 4592 18845 499d arch/x86/net/built-in.o.old
>
> interesting. Compiler obviously thinks that 178 byte increase
> with -O2 is the right trade off. Which I agree with :)
>
> If I think dropping 'inline' and using -Os will give bigger savings...
This was allyesconfig which already uses -Os
Using -O2, there is no difference using inline
or not, but the size delta with the bitmask is
much larger
$ size arch/x86/net/built-in.o* (allyesconfig, but not -Os)
text data bss dec hex filename
13410 820 3624 17854 45be arch/x86/net/built-in.o.new
16130 884 4200 21214 52de arch/x86/net/built-in.o.old
16130 884 4200 21214 52de arch/x86/net/built-in.o.static
^ permalink raw reply
* Re: [PATCH rfc] packet: zerocopy packet_snd
From: Willem de Bruijn @ 2014-11-26 19:59 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Network Development, David Miller, Eric Dumazet, Daniel Borkmann
In-Reply-To: <20141126182445.GA15744@redhat.com>
> The main problem with zero copy ATM is with queueing disciplines
> which might keep the socket around essentially forever.
> The case was described here:
> https://lkml.org/lkml/2014/1/17/105
> and of course this will make it more serious now that
> more applications will be able to do this, so
> chances that an administrator enables this
> are higher.
The denial of service issue raised there, that a single queue can
block an entire virtio-net device, is less problematic in the case of
packet sockets. A socket can run out of sk_wmem_alloc, but a prudent
application can increase the limit or use separate sockets for
separate flows.
> One possible solution is some kind of timer orphaning frags
> for skbs that have been around for too long.
Perhaps this can be approximated without an explicit timer by calling
skb_copy_ubufs on enqueue whenever qlen exceeds a threshold value?
>
>> ---
>> include/linux/skbuff.h | 1 +
>> include/linux/socket.h | 1 +
>> include/uapi/linux/errqueue.h | 1 +
>> net/packet/af_packet.c | 110 ++++++++++++++++++++++++++++++++++++++----
>> 4 files changed, 103 insertions(+), 10 deletions(-)
>>
>> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
>> index 78c299f..8e661d2 100644
>> --- a/include/linux/skbuff.h
>> +++ b/include/linux/skbuff.h
>> @@ -295,6 +295,7 @@ struct ubuf_info {
>> void (*callback)(struct ubuf_info *, bool zerocopy_success);
>> void *ctx;
>> unsigned long desc;
>> + void *callback_priv;
>> };
>>
>> /* This data is invariant across clones and lives at
>> diff --git a/include/linux/socket.h b/include/linux/socket.h
>> index de52228..0a2e0ea 100644
>> --- a/include/linux/socket.h
>> +++ b/include/linux/socket.h
>> @@ -265,6 +265,7 @@ struct ucred {
>> #define MSG_SENDPAGE_NOTLAST 0x20000 /* sendpage() internal : not the last page */
>> #define MSG_EOF MSG_FIN
>>
>> +#define MSG_ZEROCOPY 0x4000000 /* Pin user pages */
>> #define MSG_FASTOPEN 0x20000000 /* Send data in TCP SYN */
>> #define MSG_CMSG_CLOEXEC 0x40000000 /* Set close_on_exec for file
>> descriptor received through
>> diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h
>> index 07bdce1..61bf318 100644
>> --- a/include/uapi/linux/errqueue.h
>> +++ b/include/uapi/linux/errqueue.h
>> @@ -19,6 +19,7 @@ struct sock_extended_err {
>> #define SO_EE_ORIGIN_ICMP6 3
>> #define SO_EE_ORIGIN_TXSTATUS 4
>> #define SO_EE_ORIGIN_TIMESTAMPING SO_EE_ORIGIN_TXSTATUS
>> +#define SO_EE_ORIGIN_UPAGE 5
>>
>> #define SO_EE_OFFENDER(ee) ((struct sockaddr*)((ee)+1))
>>
>> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
>> index 58af5802..367c23a 100644
>> --- a/net/packet/af_packet.c
>> +++ b/net/packet/af_packet.c
>> @@ -2370,28 +2370,112 @@ out:
>>
>> static struct sk_buff *packet_alloc_skb(struct sock *sk, size_t prepad,
>> size_t reserve, size_t len,
>> - size_t linear, int noblock,
>> + size_t linear, int flags,
>> int *err)
>> {
>> struct sk_buff *skb;
>> + size_t data_len;
>>
>> - /* Under a page? Don't bother with paged skb. */
>> - if (prepad + len < PAGE_SIZE || !linear)
>> - linear = len;
>> + if (flags & MSG_ZEROCOPY) {
>> + /* Minimize linear, but respect header lower bound */
>> + linear = min(len, max_t(size_t, linear, MAX_HEADER));
>> + data_len = 0;
>> + } else {
>> + /* Under a page? Don't bother with paged skb. */
>> + if (prepad + len < PAGE_SIZE || !linear)
>> + linear = len;
>> + data_len = len - linear;
>> + }
>>
>> - skb = sock_alloc_send_pskb(sk, prepad + linear, len - linear, noblock,
>> - err, 0);
>> + skb = sock_alloc_send_pskb(sk, prepad + linear, data_len,
>> + flags & MSG_DONTWAIT, err, 0);
>> if (!skb)
>> return NULL;
>>
>> skb_reserve(skb, reserve);
>> skb_put(skb, linear);
>> - skb->data_len = len - linear;
>> - skb->len += len - linear;
>> + skb->data_len = data_len;
>> + skb->len += data_len;
>>
>> return skb;
>> }
>>
>> +/* release zerocopy resources and avoid destructor callback on kfree */
>> +static void packet_snd_zerocopy_free(struct sk_buff *skb)
>> +{
>> + struct ubuf_info *uarg = skb_shinfo(skb)->destructor_arg;
>> +
>> + if (uarg) {
>> + kfree_skb(uarg->callback_priv);
>> + sock_put((struct sock *) uarg->ctx);
>> + kfree(uarg);
>> +
>> + skb_shinfo(skb)->destructor_arg = NULL;
>> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
>> + }
>> +}
>> +
>> +static void packet_snd_zerocopy_callback(struct ubuf_info *uarg,
>> + bool zerocopy_success)
>> +{
>> + struct sk_buff *err_skb;
>> + struct sock *err_sk;
>> + struct sock_exterr_skb *serr;
>> +
>> + err_sk = uarg->ctx;
>> + err_skb = uarg->callback_priv;
>> +
>> + serr = SKB_EXT_ERR(err_skb);
>> + memset(serr, 0, sizeof(*serr));
>> + serr->ee.ee_errno = ENOMSG;
>> + serr->ee.ee_origin = SO_EE_ORIGIN_UPAGE;
>> + serr->ee.ee_data = uarg->desc & 0xFFFFFFFF;
>> + serr->ee.ee_info = ((u64) uarg->desc) >> 32;
>> + if (sock_queue_err_skb(err_sk, err_skb))
>> + kfree_skb(err_skb);
>> +
>> + kfree(uarg);
>> + sock_put(err_sk);
>> +}
>> +
>> +static int packet_zerocopy_sg_from_iovec(struct sk_buff *skb,
>> + struct msghdr *msg)
>> +{
>> + struct ubuf_info *uarg = NULL;
>> + int ret;
>> +
>> + if (iov_pages(msg->msg_iov, 0, msg->msg_iovlen) > MAX_SKB_FRAGS)
>> + return -EMSGSIZE;
>> +
>> + uarg = kzalloc(sizeof(*uarg), GFP_KERNEL);
>> + if (!uarg)
>> + return -ENOMEM;
>> +
>> + uarg->callback_priv = alloc_skb(0, GFP_KERNEL);
>> + if (!uarg->callback_priv) {
>> + kfree(uarg);
>> + return -ENOMEM;
>> + }
>> +
>> + ret = zerocopy_sg_from_iovec(skb, msg->msg_iov, 0, msg->msg_iovlen);
>> + if (ret) {
>> + kfree_skb(uarg->callback_priv);
>> + kfree(uarg);
>> + return -EIO;
>> + }
>> +
>> + sock_hold(skb->sk);
>> + uarg->ctx = skb->sk;
>> + uarg->callback = packet_snd_zerocopy_callback;
>> + uarg->desc = (unsigned long) msg->msg_iov[0].iov_base;
>> +
>> + skb_shinfo(skb)->destructor_arg = uarg;
>> + skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
>> + skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
>> +
>> + return 0;
>> +}
>> +
>> static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>> {
>> struct sock *sk = sock->sk;
>> @@ -2408,6 +2492,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>> unsigned short gso_type = 0;
>> int hlen, tlen;
>> int extra_len = 0;
>> + bool zerocopy = msg->msg_flags & MSG_ZEROCOPY;
>>
>> /*
>> * Get and verify the address.
>> @@ -2501,7 +2586,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>> hlen = LL_RESERVED_SPACE(dev);
>> tlen = dev->needed_tailroom;
>> skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, vnet_hdr.hdr_len,
>> - msg->msg_flags & MSG_DONTWAIT, &err);
>> + msg->msg_flags, &err);
>> if (skb == NULL)
>> goto out_unlock;
>>
>> @@ -2518,7 +2603,11 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>> }
>>
>> /* Returns -EFAULT on error */
>> - err = skb_copy_datagram_from_iovec(skb, offset, msg->msg_iov, 0, len);
>> + if (zerocopy)
>> + err = packet_zerocopy_sg_from_iovec(skb, msg);
>> + else
>> + err = skb_copy_datagram_from_iovec(skb, offset, msg->msg_iov,
>> + 0, len);
>> if (err)
>> goto out_free;
>>
>> @@ -2578,6 +2667,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
>> return len;
>>
>> out_free:
>> + packet_snd_zerocopy_free(skb);
>> kfree_skb(skb);
>> out_unlock:
>> if (dev)
>> --
>> 2.1.0.rc2.206.gedb03e5
^ permalink raw reply
* [PATCH net] net-timestamp: make tcp_recvmsg call ipv6_recv_error for AF_INET6 socks
From: Willem de Bruijn @ 2014-11-26 19:53 UTC (permalink / raw)
To: netdev
Cc: davem, eric.dumazet, kuznet, jmorris, yoshfuji, kaber,
Willem de Bruijn
From: Willem de Bruijn <willemb@google.com>
TCP timestamping introduced MSG_ERRQUEUE handling for TCP sockets.
If the socket is of family AF_INET6, call ipv6_recv_error instead
of ip_recv_error.
This change is more complex than a single branch due to the loadable
ipv6 module. It reuses a pre-existing indirect function call from
ping. The ping code is safe to call, because it is part of the core
ipv6 module and always present when AF_INET6 sockets are active.
Signed-off-by: Willem de Bruijn <willemb@google.com>
----
It may also be worthwhile to add WARN_ON_ONCE(sk->family == AF_INET6)
to ip_recv_error.
---
include/net/inet_common.h | 2 ++
net/ipv4/af_inet.c | 11 +++++++++++
net/ipv4/ping.c | 12 ++----------
net/ipv4/tcp.c | 2 +-
4 files changed, 16 insertions(+), 11 deletions(-)
diff --git a/include/net/inet_common.h b/include/net/inet_common.h
index fe7994c..b2828a0 100644
--- a/include/net/inet_common.h
+++ b/include/net/inet_common.h
@@ -37,6 +37,8 @@ int inet_ioctl(struct socket *sock, unsigned int cmd, unsigned long arg);
int inet_ctl_sock_create(struct sock **sk, unsigned short family,
unsigned short type, unsigned char protocol,
struct net *net);
+int inet_recv_error(struct sock *sk, struct msghdr *msg, int len,
+ int *addr_len);
static inline void inet_ctl_sock_destroy(struct sock *sk)
{
diff --git a/net/ipv4/af_inet.c b/net/ipv4/af_inet.c
index 8b7fe5b..e67da4e 100644
--- a/net/ipv4/af_inet.c
+++ b/net/ipv4/af_inet.c
@@ -1386,6 +1386,17 @@ out:
return pp;
}
+int inet_recv_error(struct sock *sk, struct msghdr *msg, int len, int *addr_len)
+{
+ if (sk->sk_family == AF_INET)
+ return ip_recv_error(sk, msg, len, addr_len);
+#if IS_ENABLED(CONFIG_IPV6)
+ if (sk->sk_family == AF_INET6)
+ return pingv6_ops.ipv6_recv_error(sk, msg, len, addr_len);
+#endif
+ return -EINVAL;
+}
+
static int inet_gro_complete(struct sk_buff *skb, int nhoff)
{
__be16 newlen = htons(skb->len - nhoff);
diff --git a/net/ipv4/ping.c b/net/ipv4/ping.c
index 85a02a7..5d740cc 100644
--- a/net/ipv4/ping.c
+++ b/net/ipv4/ping.c
@@ -855,16 +855,8 @@ int ping_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
if (flags & MSG_OOB)
goto out;
- if (flags & MSG_ERRQUEUE) {
- if (family == AF_INET) {
- return ip_recv_error(sk, msg, len, addr_len);
-#if IS_ENABLED(CONFIG_IPV6)
- } else if (family == AF_INET6) {
- return pingv6_ops.ipv6_recv_error(sk, msg, len,
- addr_len);
-#endif
- }
- }
+ if (flags & MSG_ERRQUEUE)
+ return inet_recv_error(sk, msg, len, addr_len);
skb = skb_recv_datagram(sk, flags, noblock, &err);
if (!skb)
diff --git a/net/ipv4/tcp.c b/net/ipv4/tcp.c
index 39ec0c3..38c2bcb 100644
--- a/net/ipv4/tcp.c
+++ b/net/ipv4/tcp.c
@@ -1598,7 +1598,7 @@ int tcp_recvmsg(struct kiocb *iocb, struct sock *sk, struct msghdr *msg,
u32 urg_hole = 0;
if (unlikely(flags & MSG_ERRQUEUE))
- return ip_recv_error(sk, msg, len, addr_len);
+ return inet_recv_error(sk, msg, len, addr_len);
if (sk_can_busy_loop(sk) && skb_queue_empty(&sk->sk_receive_queue) &&
(sk->sk_state == TCP_ESTABLISHED))
--
2.2.0.rc0.207.ga3a616c
^ permalink raw reply related
* pull request: wireless 2014-11-26
From: John W. Linville @ 2014-11-26 19:47 UTC (permalink / raw)
To: davem; +Cc: linux-wireless, netdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5630 bytes --]
Dave,
Please pull this little batch of fixes intended for the 3.18 stream...
For the iwlwifi one, Emmanuel says:
"Not all the firmware know how to handle the HOT_SPOT_CMD.
Make sure that the firmware will know this command before
sending it. This avoids a firmware crash."
Along with that, Larry sends a pair of rtlwifi fixes to address some
discrepancies from moving drivers out of staging. Larry says:
"These two patches are needed to fix a regression introduced when
driver rtl8821ae was moved from staging to the regular wireless tree."
Please let me know if there are problems!
Thanks,
John
---
The following changes since commit a1d69c60c44134f64945bbf6a6dfda22eaf4a214:
brcmfmac: don't include linux/unaligned/access_ok.h (2014-11-20 14:46:45 -0500)
are available in the git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/linville/wireless.git tags/master-2014-11-25
for you to fetch changes up to 7d63a5f9b25ba6b130da8eb2d32a72b1462d0249:
rtlwifi: Change order in device startup (2014-11-25 14:22:22 -0500)
----------------------------------------------------------------
John W. Linville (1):
Merge tag 'iwlwifi-for-john-2014-11-23' of git://git.kernel.org/.../iwlwifi/iwlwifi-fixes
Larry Finger (2):
rtlwifi: rtl8821ae: Fix 5G detection problem
rtlwifi: Change order in device startup
Luciano Coelho (1):
iwlwifi: mvm: check TLV flag before trying to use hotspot firmware commands
drivers/net/wireless/iwlwifi/iwl-fw.h | 2 ++
drivers/net/wireless/iwlwifi/mvm/mac80211.c | 12 +++++++++---
drivers/net/wireless/rtlwifi/pci.c | 20 ++++++++++----------
drivers/net/wireless/rtlwifi/rtl8821ae/hw.c | 5 +++--
4 files changed, 24 insertions(+), 15 deletions(-)
diff --git a/drivers/net/wireless/iwlwifi/iwl-fw.h b/drivers/net/wireless/iwlwifi/iwl-fw.h
index 4f6e66892acc..b894a84e8393 100644
--- a/drivers/net/wireless/iwlwifi/iwl-fw.h
+++ b/drivers/net/wireless/iwlwifi/iwl-fw.h
@@ -155,6 +155,7 @@ enum iwl_ucode_tlv_api {
* @IWL_UCODE_TLV_CAPA_QUIET_PERIOD_SUPPORT: supports Quiet Period requests
* @IWL_UCODE_TLV_CAPA_DQA_SUPPORT: supports dynamic queue allocation (DQA),
* which also implies support for the scheduler configuration command
+ * @IWL_UCODE_TLV_CAPA_HOTSPOT_SUPPORT: supports Hot Spot Command
*/
enum iwl_ucode_tlv_capa {
IWL_UCODE_TLV_CAPA_D0I3_SUPPORT = BIT(0),
@@ -163,6 +164,7 @@ enum iwl_ucode_tlv_capa {
IWL_UCODE_TLV_CAPA_WFA_TPC_REP_IE_SUPPORT = BIT(10),
IWL_UCODE_TLV_CAPA_QUIET_PERIOD_SUPPORT = BIT(11),
IWL_UCODE_TLV_CAPA_DQA_SUPPORT = BIT(12),
+ IWL_UCODE_TLV_CAPA_HOTSPOT_SUPPORT = BIT(18),
};
/* The default calibrate table size if not specified by firmware file */
diff --git a/drivers/net/wireless/iwlwifi/mvm/mac80211.c b/drivers/net/wireless/iwlwifi/mvm/mac80211.c
index b62405865b25..b6d2683da3a9 100644
--- a/drivers/net/wireless/iwlwifi/mvm/mac80211.c
+++ b/drivers/net/wireless/iwlwifi/mvm/mac80211.c
@@ -2448,9 +2448,15 @@ static int iwl_mvm_roc(struct ieee80211_hw *hw,
switch (vif->type) {
case NL80211_IFTYPE_STATION:
- /* Use aux roc framework (HS20) */
- ret = iwl_mvm_send_aux_roc_cmd(mvm, channel,
- vif, duration);
+ if (mvm->fw->ucode_capa.capa[0] &
+ IWL_UCODE_TLV_CAPA_HOTSPOT_SUPPORT) {
+ /* Use aux roc framework (HS20) */
+ ret = iwl_mvm_send_aux_roc_cmd(mvm, channel,
+ vif, duration);
+ goto out_unlock;
+ }
+ IWL_ERR(mvm, "hotspot not supported\n");
+ ret = -EINVAL;
goto out_unlock;
case NL80211_IFTYPE_P2P_DEVICE:
/* handle below */
diff --git a/drivers/net/wireless/rtlwifi/pci.c b/drivers/net/wireless/rtlwifi/pci.c
index 61f5d36eca6a..846a2e6e34d8 100644
--- a/drivers/net/wireless/rtlwifi/pci.c
+++ b/drivers/net/wireless/rtlwifi/pci.c
@@ -2249,6 +2249,16 @@ int rtl_pci_probe(struct pci_dev *pdev,
/*like read eeprom and so on */
rtlpriv->cfg->ops->read_eeprom_info(hw);
+ if (rtlpriv->cfg->ops->init_sw_vars(hw)) {
+ RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG, "Can't init_sw_vars\n");
+ err = -ENODEV;
+ goto fail3;
+ }
+ rtlpriv->cfg->ops->init_sw_leds(hw);
+
+ /*aspm */
+ rtl_pci_init_aspm(hw);
+
/* Init mac80211 sw */
err = rtl_init_core(hw);
if (err) {
@@ -2264,16 +2274,6 @@ int rtl_pci_probe(struct pci_dev *pdev,
goto fail3;
}
- if (rtlpriv->cfg->ops->init_sw_vars(hw)) {
- RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG, "Can't init_sw_vars\n");
- err = -ENODEV;
- goto fail3;
- }
- rtlpriv->cfg->ops->init_sw_leds(hw);
-
- /*aspm */
- rtl_pci_init_aspm(hw);
-
err = ieee80211_register_hw(hw);
if (err) {
RT_TRACE(rtlpriv, COMP_ERR, DBG_EMERG,
diff --git a/drivers/net/wireless/rtlwifi/rtl8821ae/hw.c b/drivers/net/wireless/rtlwifi/rtl8821ae/hw.c
index 310d3163dc5b..8ec8200002c7 100644
--- a/drivers/net/wireless/rtlwifi/rtl8821ae/hw.c
+++ b/drivers/net/wireless/rtlwifi/rtl8821ae/hw.c
@@ -3672,8 +3672,9 @@ static void rtl8821ae_update_hal_rate_mask(struct ieee80211_hw *hw,
mac->opmode == NL80211_IFTYPE_ADHOC)
macid = sta->aid + 1;
if (wirelessmode == WIRELESS_MODE_N_5G ||
- wirelessmode == WIRELESS_MODE_AC_5G)
- ratr_bitmap = sta->supp_rates[NL80211_BAND_5GHZ];
+ wirelessmode == WIRELESS_MODE_AC_5G ||
+ wirelessmode == WIRELESS_MODE_A)
+ ratr_bitmap = sta->supp_rates[NL80211_BAND_5GHZ] << 4;
else
ratr_bitmap = sta->supp_rates[NL80211_BAND_2GHZ];
--
John W. Linville Someday the world will need a hero, and you
linville@tuxdriver.com might be all we have. Be ready.
[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply related
* Re: [PATCH net-next] bridge: add vlan id to mdb notifications
From: Jonathan Toppins @ 2014-11-26 19:40 UTC (permalink / raw)
To: Stephen Hemminger, roopa; +Cc: vyasevich, netdev, wkok, gospo, sashok
In-Reply-To: <20141126105614.6a42d697@urahara>
On 11/26/14 1:56 PM, Stephen Hemminger wrote:
> On Wed, 26 Nov 2014 05:53:33 -0800
> roopa@cumulusnetworks.com wrote:
>
>> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
>> index da17e45..db061fd 100644
>> --- a/include/uapi/linux/if_bridge.h
>> +++ b/include/uapi/linux/if_bridge.h
>> @@ -185,6 +185,7 @@ struct br_mdb_entry {
>> struct in6_addr ip6;
>> } u;
>> __be16 proto;
>> + __be16 vid;
>> } addr;
>> };
>>
>
> You can't add fields to existing binary API
>
Roopa, maybe a description of what use case this is trying to solve
would better justify the addition to the UAPI?
-Jon
^ permalink raw reply
* Re: [PATCH net-next] sky2: Fix crash inside sky2_rx_clean
From: Stephen Hemminger @ 2014-11-26 18:57 UTC (permalink / raw)
To: Mirko Lindner; +Cc: davem, netdev@vger.kernel.org
In-Reply-To: <5475E012.3060607@marvell.com>
On Wed, 26 Nov 2014 15:13:38 +0100
Mirko Lindner <mlindner@marvell.com> wrote:
> If sky2->tx_le = pci_alloc_consistent() or sky2->tx_ring = kcalloc() in
> sky2_alloc_buffers() fails, sky2->rx_ring = kcalloc() will never be called.
> In this error case handling, sky2_rx_clean() is called from within
> sky2_free_buffers().
>
> In sky2_rx_clean() we find the following:
>
> ...
> memset(sky2->rx_le, 0, RX_LE_BYTES);
> ...
>
> This results in a memset using a NULL pointer and will crash the system.
>
> Signed-off-by: Mirko Lindner <mlindner@marvell.com>
This matches my earlier patch, but this one is just as good
Acked-by: Stephen Hemminger <stephen@networkplumber.org>
^ permalink raw reply
* Re: [PATCH net-next] bridge: add vlan id to mdb notifications
From: Stephen Hemminger @ 2014-11-26 18:56 UTC (permalink / raw)
To: roopa; +Cc: vyasevich, netdev, wkok, gospo, jtoppins, sashok
In-Reply-To: <1417010013-31454-1-git-send-email-roopa@cumulusnetworks.com>
On Wed, 26 Nov 2014 05:53:33 -0800
roopa@cumulusnetworks.com wrote:
> diff --git a/include/uapi/linux/if_bridge.h b/include/uapi/linux/if_bridge.h
> index da17e45..db061fd 100644
> --- a/include/uapi/linux/if_bridge.h
> +++ b/include/uapi/linux/if_bridge.h
> @@ -185,6 +185,7 @@ struct br_mdb_entry {
> struct in6_addr ip6;
> } u;
> __be16 proto;
> + __be16 vid;
> } addr;
> };
>
You can't add fields to existing binary API
^ permalink raw reply
* Re: [PATCH net] r8152: drop the tx packet with invalid length
From: Eric Dumazet @ 2014-11-26 18:44 UTC (permalink / raw)
To: David Miller; +Cc: hayeswang, netdev, nic_swsd, linux-kernel, linux-usb
In-Reply-To: <20141126.120658.498602780456343676.davem@davemloft.net>
On Wed, 2014-11-26 at 12:06 -0500, David Miller wrote:
> From: Eric Dumazet <eric.dumazet@gmail.com>
> Date: Wed, 26 Nov 2014 08:52:28 -0800
>
> > On Wed, 2014-11-26 at 17:56 +0800, Hayes Wang wrote:
> >> Drop the tx packet which is more than the size of agg_buf_sz. When
> >> creating a bridge with the device, we may get the tx packet with
> >> TSO and the length is more than the gso_max_size which is set by
> >> the driver through netif_set_gso_max_size(). Such packets couldn't
> >> be transmitted and should be dropped directly.
> >>
> >> Signed-off-by: Hayes Wang <hayeswang@realtek.com>
> ...
> > Looks like a candidate for ndo_gso_check(), so that we do not drop, but
> > instead segment from netif_needs_gso()/validate_xmit_skb()
>
> You mean have the bridge implement the ndo_gso_check() method right?
No, I meant this particular driver.
Note that netif_skb_features() does only this check :
if (gso_segs > dev->gso_max_segs || gso_segs < dev->gso_min_segs)
features &= ~NETIF_F_GSO_MASK;
Ie not testing gso_max_size
It looks like all these particular tests should be moved on
ndo_gso_check(), to remove code from netif_skb_features()
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Joe Perches @ 2014-11-26 18:41 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Quentin Lambert, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAADnVQK3iui8cAmhi28=65p2CJbTu45fR98ri8Uh8xLRb=cwnA@mail.gmail.com>
On Wed, 2014-11-26 at 10:34 -0800, Alexei Starovoitov wrote:
> On Wed, Nov 26, 2014 at 10:02 AM, Joe Perches <joe@perches.com> wrote:
> > On Wed, 2014-11-26 at 09:23 -0800, Alexei Starovoitov wrote:
> >> On Wed, Nov 26, 2014 at 8:58 AM, Joe Perches <joe@perches.com> wrote:
> >
> >> > Is there any value in reordering these tests for frequency
> >> > or maybe using | instead of || to avoid multiple jumps?
> >>
> >> probably not. It's not a critical path.
> >> compiler may fuse conditions depending on values anyway.
> >> If it was a critical path, we could have used
> >> (1 << reg) & mask trick.
> >> I picked explicit 'return true' else 'return false' here,
> >> because it felt easier to read. Just a matter of taste.
> >
> > There is a size difference though: (allyesconfig)
> >
> > $ size arch/x86/net/built-in.o*
> > text data bss dec hex filename
> > 12999 1012 4336 18347 47ab arch/x86/net/built-in.o.new
> > 13177 1076 4592 18845 499d arch/x86/net/built-in.o.old
>
> interesting. Compiler obviously thinks that 178 byte increase
> with -O2 is the right trade off. Which I agree with :)
498 overall.
> If I think dropping 'inline' and using -Os will give bigger savings...
> but I suspect 'tinification' folks will compile JIT out anyway...
Smaller is generally better/faster in any case.
> thanks for giving it a shot :)
> That's exactly what I had in mind.
> imo it's less readable, but we probably not going
> to mess much with this piece of code anyway.
> Though to be safe in the future, we'd need to
> add BUILD_BUG_ON that largest value (AUX_REG)
> fits in 32bit (or 64bit) and add a comment that
> verifier goes before the JIT and checks that
> insn->src_reg, insn->dst_reg are less than MAX_BPF_REG,
> so argument 'reg' also doesn't trigger too large shift.
> Perfectionists r us. :)
> ... or just leave it as-is ;)
16 registers max anyway as it's stored
in a :4
No worries, it was just playtime anyway.
cheers, Joe
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Alexei Starovoitov @ 2014-11-26 18:34 UTC (permalink / raw)
To: Joe Perches
Cc: Quentin Lambert, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
On Wed, Nov 26, 2014 at 10:02 AM, Joe Perches <joe@perches.com> wrote:
> On Wed, 2014-11-26 at 09:23 -0800, Alexei Starovoitov wrote:
>> On Wed, Nov 26, 2014 at 8:58 AM, Joe Perches <joe@perches.com> wrote:
>
>> > Is there any value in reordering these tests for frequency
>> > or maybe using | instead of || to avoid multiple jumps?
>>
>> probably not. It's not a critical path.
>> compiler may fuse conditions depending on values anyway.
>> If it was a critical path, we could have used
>> (1 << reg) & mask trick.
>> I picked explicit 'return true' else 'return false' here,
>> because it felt easier to read. Just a matter of taste.
>
> There is a size difference though: (allyesconfig)
>
> $ size arch/x86/net/built-in.o*
> text data bss dec hex filename
> 12999 1012 4336 18347 47ab arch/x86/net/built-in.o.new
> 13177 1076 4592 18845 499d arch/x86/net/built-in.o.old
interesting. Compiler obviously thinks that 178 byte increase
with -O2 is the right trade off. Which I agree with :)
If I think dropping 'inline' and using -Os will give bigger savings...
but I suspect 'tinification' folks will compile JIT out anyway...
> ---
> diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
> index 3f62734..09e2cea 100644
> --- a/arch/x86/net/bpf_jit_comp.c
> +++ b/arch/x86/net/bpf_jit_comp.c
> @@ -135,11 +135,11 @@ static const int reg2hex[] = {
> */
> static inline bool is_ereg(u32 reg)
> {
> - if (reg == BPF_REG_5 || reg == AUX_REG ||
> - (reg >= BPF_REG_7 && reg <= BPF_REG_9))
> - return true;
> - else
> - return false;
> + return (1 << reg) & (BIT(BPF_REG_5) |
> + BIT(AUX_REG) |
> + BIT(BPF_REG_7) |
> + BIT(BPF_REG_8) |
> + BIT(BPF_REG_9));
thanks for giving it a shot :)
That's exactly what I had in mind.
imo it's less readable, but we probably not going
to mess much with this piece of code anyway.
Though to be safe in the future, we'd need to
add BUILD_BUG_ON that largest value (AUX_REG)
fits in 32bit (or 64bit) and add a comment that
verifier goes before the JIT and checks that
insn->src_reg, insn->dst_reg are less than MAX_BPF_REG,
so argument 'reg' also doesn't trigger too large shift.
Perfectionists r us. :)
... or just leave it as-is ;)
^ permalink raw reply
* Re: [PATCH rfc] packet: zerocopy packet_snd
From: Michael S. Tsirkin @ 2014-11-26 18:24 UTC (permalink / raw)
To: Willem de Bruijn; +Cc: netdev, davem, eric.dumazet, dborkman
In-Reply-To: <1416602694-7540-1-git-send-email-willemb@google.com>
On Fri, Nov 21, 2014 at 03:44:54PM -0500, Willem de Bruijn wrote:
> From: Willem de Bruijn <willemb@google.com>
>
> Extend the copy avoidance ("zero copy") interface from virtio and tap
> to packet sockets.
>
> Interface (send):
> Pass MSG_ZEROCOPY flag in send to have the kernel link the original
> user pages into its skb_shinfo(skb)->frags.
>
> Interface (recv notification):
> Process the socket error queue. Ancillary data of type
> SO_EE_ORIGIN_UPAGE will return the pointer to the buffer passed in
> send to notify the completion of that send request.
>
> If the process allows the error queue to overflow, notifications
> will be dropped.
>
> Tested by injecting max sized TCP packets on a host with TSO enabled
> and GSO disabled. This passes the full packet to the NIC. Perf shows
> a large drop in cycles spent (perf record -C 4 -a sleep 4)
>
> Sending 10 Kpps without MSG_ZEROCOPY:
>
> Event count (approx.): 1191500371
> 30.63% psock_reinject [kernel.kallsyms] [k] copy_user_enhanced_fast_string
> 7.50% psock_reinject [kernel.kallsyms] [k] __rmqueue
> 6.26% swapper [kernel.kallsyms] [k] intel_idle
> 5.06% psock_reinject [kernel.kallsyms] [k] get_page_from_freelist
> 1.99% psock_reinject [kernel.kallsyms] [k] mlx4_en_xmit
> 1.60% psock_reinject [kernel.kallsyms] [k] __alloc_pages_nodemask
>
> and the same with MSG_ZEROCOPY:
>
> Event count (approx.): 390783801
> 13.96% swapper [kernel.kallsyms] [k] intel_idle
> 3.11% psock_reinject [kernel.kallsyms] [k] mlx4_en_xmit
> 2.75% psock_reinject [kernel.kallsyms] [k] _raw_spin_lock
> 2.67% swapper [kernel.kallsyms] [k] lapic_next_deadline
> 2.33% psock_reinject [kernel.kallsyms] [k] gup_huge_pmd
> 1.90% swapper [kernel.kallsyms] [k] apic_timer_interrupt
>
> The idle is probably due to using usleep to pace the sender, so this
> underreports the effect.
About 15% gain in CPU utilization with huge packets is what I see with
vhost-net too. So that seems reasonable.
> Another test sent packets over loopback to a UDP socket. The test
> executes mostly synchronously in a single thread:
> sendmsg(fd_packet);
> recvmsg(fd_inet);
> recvmsg(fd_packet, .., MSG_ERRQUEUE);
>
> Result from that at various packet sizes:
>
> no-zc zc !ubufs recvmmsg !callback
> 1 573 368 359 439 517
> 10 569 368 361 441 507
> 100 572 299 340 408 478
> 1000 564 300 338 412 486
> 10000 475 247 331 405 479
> 30000 328 177 324 385 450
> 65000 218 117 307 359 417
>
> Legend:
> zc: pass MSG_ZEROCOPY
> !ubufs: (gross hack) comment out skb_orphan_frags
> in __netif_receive_skb_core
> recvmmsg: switch from synchronous completion notification
> handling to batching.
> !callback: (gross hack) disable upcall mechanism completely,
> to expose its cost.
>
> This demonstrates one of the large blocking issues to the practical
> use of this feature in many datapaths: the kernel copies data of
> sk_buffs with field SKBTX_DEV_ZEROCOPY at skb_clone and other points
> to ensure safe reference counting. Follow on work would be needed
> to refine this mechanism and increase the number of datapaths where
> user pages can safely be used. This is one reason why I demonstrate
> with TSO, as opposed to GSO: GSO also has to copy.
>
> Implementation note: returning the pointer is somewhat ugly. An
> alternative is to keep a per-socket counter, increment that for
> each MSG_ZEROCOPY sendmsg() call and return this counter as
> notification.
>
> Signed-off-by: Willem de Bruijn <willemb@google.com>
The main problem with zero copy ATM is with queueing disciplines
which might keep the socket around essentially forever.
The case was described here:
https://lkml.org/lkml/2014/1/17/105
and of course this will make it more serious now that
more applications will be able to do this, so
chances that an administrator enables this
are higher.
One possible solution is some kind of timer orphaning frags
for skbs that have been around for too long.
> ---
> include/linux/skbuff.h | 1 +
> include/linux/socket.h | 1 +
> include/uapi/linux/errqueue.h | 1 +
> net/packet/af_packet.c | 110 ++++++++++++++++++++++++++++++++++++++----
> 4 files changed, 103 insertions(+), 10 deletions(-)
>
> diff --git a/include/linux/skbuff.h b/include/linux/skbuff.h
> index 78c299f..8e661d2 100644
> --- a/include/linux/skbuff.h
> +++ b/include/linux/skbuff.h
> @@ -295,6 +295,7 @@ struct ubuf_info {
> void (*callback)(struct ubuf_info *, bool zerocopy_success);
> void *ctx;
> unsigned long desc;
> + void *callback_priv;
> };
>
> /* This data is invariant across clones and lives at
> diff --git a/include/linux/socket.h b/include/linux/socket.h
> index de52228..0a2e0ea 100644
> --- a/include/linux/socket.h
> +++ b/include/linux/socket.h
> @@ -265,6 +265,7 @@ struct ucred {
> #define MSG_SENDPAGE_NOTLAST 0x20000 /* sendpage() internal : not the last page */
> #define MSG_EOF MSG_FIN
>
> +#define MSG_ZEROCOPY 0x4000000 /* Pin user pages */
> #define MSG_FASTOPEN 0x20000000 /* Send data in TCP SYN */
> #define MSG_CMSG_CLOEXEC 0x40000000 /* Set close_on_exec for file
> descriptor received through
> diff --git a/include/uapi/linux/errqueue.h b/include/uapi/linux/errqueue.h
> index 07bdce1..61bf318 100644
> --- a/include/uapi/linux/errqueue.h
> +++ b/include/uapi/linux/errqueue.h
> @@ -19,6 +19,7 @@ struct sock_extended_err {
> #define SO_EE_ORIGIN_ICMP6 3
> #define SO_EE_ORIGIN_TXSTATUS 4
> #define SO_EE_ORIGIN_TIMESTAMPING SO_EE_ORIGIN_TXSTATUS
> +#define SO_EE_ORIGIN_UPAGE 5
>
> #define SO_EE_OFFENDER(ee) ((struct sockaddr*)((ee)+1))
>
> diff --git a/net/packet/af_packet.c b/net/packet/af_packet.c
> index 58af5802..367c23a 100644
> --- a/net/packet/af_packet.c
> +++ b/net/packet/af_packet.c
> @@ -2370,28 +2370,112 @@ out:
>
> static struct sk_buff *packet_alloc_skb(struct sock *sk, size_t prepad,
> size_t reserve, size_t len,
> - size_t linear, int noblock,
> + size_t linear, int flags,
> int *err)
> {
> struct sk_buff *skb;
> + size_t data_len;
>
> - /* Under a page? Don't bother with paged skb. */
> - if (prepad + len < PAGE_SIZE || !linear)
> - linear = len;
> + if (flags & MSG_ZEROCOPY) {
> + /* Minimize linear, but respect header lower bound */
> + linear = min(len, max_t(size_t, linear, MAX_HEADER));
> + data_len = 0;
> + } else {
> + /* Under a page? Don't bother with paged skb. */
> + if (prepad + len < PAGE_SIZE || !linear)
> + linear = len;
> + data_len = len - linear;
> + }
>
> - skb = sock_alloc_send_pskb(sk, prepad + linear, len - linear, noblock,
> - err, 0);
> + skb = sock_alloc_send_pskb(sk, prepad + linear, data_len,
> + flags & MSG_DONTWAIT, err, 0);
> if (!skb)
> return NULL;
>
> skb_reserve(skb, reserve);
> skb_put(skb, linear);
> - skb->data_len = len - linear;
> - skb->len += len - linear;
> + skb->data_len = data_len;
> + skb->len += data_len;
>
> return skb;
> }
>
> +/* release zerocopy resources and avoid destructor callback on kfree */
> +static void packet_snd_zerocopy_free(struct sk_buff *skb)
> +{
> + struct ubuf_info *uarg = skb_shinfo(skb)->destructor_arg;
> +
> + if (uarg) {
> + kfree_skb(uarg->callback_priv);
> + sock_put((struct sock *) uarg->ctx);
> + kfree(uarg);
> +
> + skb_shinfo(skb)->destructor_arg = NULL;
> + skb_shinfo(skb)->tx_flags &= ~SKBTX_DEV_ZEROCOPY;
> + }
> +}
> +
> +static void packet_snd_zerocopy_callback(struct ubuf_info *uarg,
> + bool zerocopy_success)
> +{
> + struct sk_buff *err_skb;
> + struct sock *err_sk;
> + struct sock_exterr_skb *serr;
> +
> + err_sk = uarg->ctx;
> + err_skb = uarg->callback_priv;
> +
> + serr = SKB_EXT_ERR(err_skb);
> + memset(serr, 0, sizeof(*serr));
> + serr->ee.ee_errno = ENOMSG;
> + serr->ee.ee_origin = SO_EE_ORIGIN_UPAGE;
> + serr->ee.ee_data = uarg->desc & 0xFFFFFFFF;
> + serr->ee.ee_info = ((u64) uarg->desc) >> 32;
> + if (sock_queue_err_skb(err_sk, err_skb))
> + kfree_skb(err_skb);
> +
> + kfree(uarg);
> + sock_put(err_sk);
> +}
> +
> +static int packet_zerocopy_sg_from_iovec(struct sk_buff *skb,
> + struct msghdr *msg)
> +{
> + struct ubuf_info *uarg = NULL;
> + int ret;
> +
> + if (iov_pages(msg->msg_iov, 0, msg->msg_iovlen) > MAX_SKB_FRAGS)
> + return -EMSGSIZE;
> +
> + uarg = kzalloc(sizeof(*uarg), GFP_KERNEL);
> + if (!uarg)
> + return -ENOMEM;
> +
> + uarg->callback_priv = alloc_skb(0, GFP_KERNEL);
> + if (!uarg->callback_priv) {
> + kfree(uarg);
> + return -ENOMEM;
> + }
> +
> + ret = zerocopy_sg_from_iovec(skb, msg->msg_iov, 0, msg->msg_iovlen);
> + if (ret) {
> + kfree_skb(uarg->callback_priv);
> + kfree(uarg);
> + return -EIO;
> + }
> +
> + sock_hold(skb->sk);
> + uarg->ctx = skb->sk;
> + uarg->callback = packet_snd_zerocopy_callback;
> + uarg->desc = (unsigned long) msg->msg_iov[0].iov_base;
> +
> + skb_shinfo(skb)->destructor_arg = uarg;
> + skb_shinfo(skb)->tx_flags |= SKBTX_DEV_ZEROCOPY;
> + skb_shinfo(skb)->tx_flags |= SKBTX_SHARED_FRAG;
> +
> + return 0;
> +}
> +
> static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> {
> struct sock *sk = sock->sk;
> @@ -2408,6 +2492,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> unsigned short gso_type = 0;
> int hlen, tlen;
> int extra_len = 0;
> + bool zerocopy = msg->msg_flags & MSG_ZEROCOPY;
>
> /*
> * Get and verify the address.
> @@ -2501,7 +2586,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> hlen = LL_RESERVED_SPACE(dev);
> tlen = dev->needed_tailroom;
> skb = packet_alloc_skb(sk, hlen + tlen, hlen, len, vnet_hdr.hdr_len,
> - msg->msg_flags & MSG_DONTWAIT, &err);
> + msg->msg_flags, &err);
> if (skb == NULL)
> goto out_unlock;
>
> @@ -2518,7 +2603,11 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> }
>
> /* Returns -EFAULT on error */
> - err = skb_copy_datagram_from_iovec(skb, offset, msg->msg_iov, 0, len);
> + if (zerocopy)
> + err = packet_zerocopy_sg_from_iovec(skb, msg);
> + else
> + err = skb_copy_datagram_from_iovec(skb, offset, msg->msg_iov,
> + 0, len);
> if (err)
> goto out_free;
>
> @@ -2578,6 +2667,7 @@ static int packet_snd(struct socket *sock, struct msghdr *msg, size_t len)
> return len;
>
> out_free:
> + packet_snd_zerocopy_free(skb);
> kfree_skb(skb);
> out_unlock:
> if (dev)
> --
> 2.1.0.rc2.206.gedb03e5
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Joe Perches @ 2014-11-26 18:02 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: Quentin Lambert, David S. Miller, Alexey Kuznetsov, James Morris,
Hideaki YOSHIFUJI, Patrick McHardy, Thomas Gleixner, Ingo Molnar,
H. Peter Anvin, x86, netdev@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <CAADnVQKtYkKtsd9rUFm2yfMoh7F_SRFPVP8hpvSY00KpmB-NNg@mail.gmail.com>
On Wed, 2014-11-26 at 09:23 -0800, Alexei Starovoitov wrote:
> On Wed, Nov 26, 2014 at 8:58 AM, Joe Perches <joe@perches.com> wrote:
> > Is there any value in reordering these tests for frequency
> > or maybe using | instead of || to avoid multiple jumps?
>
> probably not. It's not a critical path.
> compiler may fuse conditions depending on values anyway.
> If it was a critical path, we could have used
> (1 << reg) & mask trick.
> I picked explicit 'return true' else 'return false' here,
> because it felt easier to read. Just a matter of taste.
There is a size difference though: (allyesconfig)
$ size arch/x86/net/built-in.o*
text data bss dec hex filename
12999 1012 4336 18347 47ab arch/x86/net/built-in.o.new
13177 1076 4592 18845 499d arch/x86/net/built-in.o.old
---
diff --git a/arch/x86/net/bpf_jit_comp.c b/arch/x86/net/bpf_jit_comp.c
index 3f62734..09e2cea 100644
--- a/arch/x86/net/bpf_jit_comp.c
+++ b/arch/x86/net/bpf_jit_comp.c
@@ -135,11 +135,11 @@ static const int reg2hex[] = {
*/
static inline bool is_ereg(u32 reg)
{
- if (reg == BPF_REG_5 || reg == AUX_REG ||
- (reg >= BPF_REG_7 && reg <= BPF_REG_9))
- return true;
- else
- return false;
+ return (1 << reg) & (BIT(BPF_REG_5) |
+ BIT(AUX_REG) |
+ BIT(BPF_REG_7) |
+ BIT(BPF_REG_8) |
+ BIT(BPF_REG_9));
}
/* add modifiers if 'reg' maps to x64 registers r8..r15 */
^ permalink raw reply related
* Re: [patch net-next v3 04/17] net: introduce generic switch devices support
From: Jiri Pirko @ 2014-11-26 17:59 UTC (permalink / raw)
To: Jamal Hadi Salim
Cc: Thomas Graf, Scott Feldman, Netdev, David S. Miller,
nhorman@tuxdriver.com, Andy Gospodarek, dborkman@redhat.com,
ogerlitz@mellanox.com, jesse@nicira.com, pshelar@nicira.com,
azhou@nicira.com, ben@decadent.org.uk, stephen@networkplumber.org,
Kirsher, Jeffrey T, vyasevic@redhat.com, Cong Wang,
Fastabend, John R, Eric Dumazet, Florian Fainelli, Roopa Prabhu,
John Linville
In-Reply-To: <54760939.8050805@mojatatu.com>
Wed, Nov 26, 2014 at 06:09:13PM CET, jhs@mojatatu.com wrote:
>On 11/26/14 11:08, Thomas Graf wrote:
>>On 11/26/14 at 06:36am, Jamal Hadi Salim wrote:
>>
>>
>>Jamal,
>>
>>What is irriating in this context is that you are pushing back on
>>Jiri and others while referring to properitary and closed code which
>>you are unwilling or unable to share. I don't see this as being
>>passive aggressive, everybody is treated the same way in this regard.
>>
>
>WTF? I said i have hardware that is not a switch because it doesnt
>do switching. This all started with the name being "switch" which
>I objected to. You ask me to describe hardware and then you come
>back and say I am using that to stop progress?
Stay calm, I'm sure that this is just a misunderstanding.
>Where the hell did i push back on Jiri? Stop going around
>telling people i do. I invest my time and effort reviewing code,
>proposing ideas, posting etc calling meetings. Infact i initiated
>this whole effort to begin with.
I thought I started this :) Anyway, I much appreciate your involvement
in this Jamal with putting the meetings together and stuff, that's for sure.
We need to join forces, not to fight with each other.
>
>There is no point to responding to any of your other comments.
>
>cheers,
>jamal
^ permalink raw reply
* Re: [PATCH] x86: bpf_jit_comp: simplify trivial boolean return
From: Daniel Borkmann @ 2014-11-26 17:55 UTC (permalink / raw)
To: Joe Perches
Cc: Alexei Starovoitov, Quentin Lambert, David S. Miller,
Alexey Kuznetsov, James Morris, Hideaki YOSHIFUJI,
Patrick McHardy, Thomas Gleixner, Ingo Molnar, H. Peter Anvin,
x86, netdev@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <1417021108.19695.5.camel@perches.com>
On 11/26/2014 05:58 PM, Joe Perches wrote:
...
>> imo existing code is fine and I don't think the time spent
>> reviewing such changes is worth it when there is no
>> improvement in readability.
+1
> Is there any value in reordering these tests for frequency
> or maybe using | instead of || to avoid multiple jumps?
No, it's not a fast-path.
^ permalink raw reply
* Re: [PATCH net-next 00/11] standardize TIPC SKB queue operations
From: David Miller @ 2014-11-26 17:31 UTC (permalink / raw)
To: ying.xue
Cc: jon.maloy, Paul.Gortmaker, erik.hugne, richard.alpe, netdev,
tipc-discussion
In-Reply-To: <1416973315-6047-1-git-send-email-ying.xue@windriver.com>
From: Ying Xue <ying.xue@windriver.com>
Date: Wed, 26 Nov 2014 11:41:44 +0800
> Now the following SKB queues are created and maintained within internal
> TIPC stack:
>
> - link transmission queue
> - link deferred queue
> - link receive queue
> - socket outgoing packet chain
> - name table outgoing packet chain
>
> In order to manage above queues, TIPC stack declares a sk_buff pointer
> for each queue to record its head, and directly modifies "prev" and
> "next" SKB pointers of SKB structure when inserting or deleting a SKB
> to or from the queue. As these operations are pretty complex, they
> easily involve fatal mistakes. If these sk_buff pointers are replaced
> with sk_buff_head instances as queue heads and corresponding generic
> SKB list APIs are used to manage them, the entire TIPC code would
> become quite clean and readable. But before make the change, we need
> to clean up below redundant functionalities:
>
> - remove node subscribe infrastructure
> - remove protocol message queue
> - remove retransmission queue
> - clean up process of pushing packets in link layer
Series applied, thanks.
^ permalink raw reply
* Re: [PATCH] stmmac: platform: fix default values of the filter bins setting
From: David Miller @ 2014-11-26 17:29 UTC (permalink / raw)
To: chenhc; +Cc: peppe.cavallaro, vbridgers2013, netdev
In-Reply-To: <1416969486-9047-1-git-send-email-chenhc@lemote.com>
From: Huacai Chen <chenhc@lemote.com>
Date: Wed, 26 Nov 2014 10:38:06 +0800
> The commit 3b57de958e2a brought the support for a different amount of
> the filter bins, but didn't update the platform driver that without
> CONFIG_OF.
>
> Fixes: 3b57de958e2a (net: stmmac: Support devicetree configs for mcast
> and ucast filter entries)
>
> Signed-off-by: Huacai Chen <chenhc@lemote.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH] xen-netfront: Remove BUGs on paged skb data which crosses a page boundary
From: David Miller @ 2014-11-26 17:28 UTC (permalink / raw)
To: seth.forshee
Cc: konrad.wilk, boris.ostrovsky, david.vrabel, zoltan.kiss,
eric.dumazet, stefan.bader, xen-devel, netdev, linux-kernel
In-Reply-To: <1416968904-70874-1-git-send-email-seth.forshee@canonical.com>
From: Seth Forshee <seth.forshee@canonical.com>
Date: Tue, 25 Nov 2014 20:28:24 -0600
> These BUGs can be erroneously triggered by frags which refer to
> tail pages within a compound page. The data in these pages may
> overrun the hardware page while still being contained within the
> compound page, but since compound_order() evaluates to 0 for tail
> pages the assertion fails. The code already iterates through
> subsequent pages correctly in this scenario, so the BUGs are
> unnecessary and can be removed.
>
> Fixes: f36c374782e4 ("xen/netfront: handle compound page fragments on transmit")
> Cc: <stable@vger.kernel.org> # 3.7+
> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
Can I get some Xen developer reviews?
Thanks.
^ permalink raw reply
* Re: [PATCH V3 1/1] ipv6: Remove unnecessary test
From: David Miller @ 2014-11-26 17:27 UTC (permalink / raw)
To: zyjzyj2000; +Cc: netdev, Yanjun.Zhu, honkiko, opurdila, xemul, cwang
In-Reply-To: <1416968758-19641-1-git-send-email-Yanjun.Zhu@windriver.com>
From: Zhu Yanjun <zyjzyj2000@gmail.com>
Date: Wed, 26 Nov 2014 10:25:58 +0800
> The "init_net" test in function addrconf_exit_net is introduced
> in commit 44a6bd29 [Create ipv6 devconf-s for namespaces] to avoid freeing
> init_net. In commit c900a800 [ipv6: fix bad free of addrconf_init_net],
> function addrconf_init_net will allocate memory for every net regardless of
> init_net. In this case, it is unnecessary to make "init_net" test.
>
> CC: Hong Zhiguo <honkiko@gmail.com>
> CC: Octavian Purdila <opurdila@ixiacom.com>
> CC: Pavel Emelyanov <xemul@openvz.org>
> CC: Cong Wang <cwang@twopensource.com>
> Suggested-by: David S. Miller <davem@davemloft.net>
> Signed-off-by: Zhu Yanjun <Yanjun.Zhu@windriver.com>
Applied, thanks.
^ permalink raw reply
* Re: [PATCH v2 01/17] new helper: skb_copy_and_csum_datagram_msg()
From: David Miller @ 2014-11-26 17:27 UTC (permalink / raw)
To: viro; +Cc: netdev, linux-kernel
In-Reply-To: <20141125205911.GA29748@ZenIV.linux.org.uk>
From: Al Viro <viro@ZenIV.linux.org.uk>
Date: Tue, 25 Nov 2014 20:59:11 +0000
> On Tue, Nov 25, 2014 at 02:28:20PM -0500, David Miller wrote:
>>
>> Al, this series looks fine to me, do you have a tree I can pull
>> it from?
>
> Er... Same as the last time?
>
> git://git.kernel.org/pub/scm/linux/kernel/git/viro/vfs.git for-davem
>
> and there's more fun stuff in #iov_iter-net in the same tree, but that's
> the next batch...
Pulled, thanks Al.
^ 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