* Re: INFO: trying to register non-static key in bond_3ad_update_lacp_rate
From: syzbot @ 2019-10-29 4:46 UTC (permalink / raw)
To: a, alex.aring, allison, andrew, andy, ap420073, aroulin, ast,
b.a.t.m.a.n, bridge, cleech, daniel, davem, dcaratti, dsa,
edumazet, f.fainelli, fw, gregkh, gustavo, gvaradar, haiyangz,
idosch, info, ivan.khoronzhuk, j.vosburgh, j, jakub.kicinski, jhs,
jiri, jiri, johan.hedberg, johannes.berg, john.hurley, jwi,
kstewart, kvalo, kys, lariel, linmiaohe, linux-bluetooth,
linux-hams, linux-hyperv, linux-kernel, linux-ppp, linux-wireless,
linux-wpan, liuhangbin, marcel, mareklindner
In-Reply-To: <000000000000fc25a1059602460a@google.com>
syzbot has bisected this bug to:
commit ab92d68fc22f9afab480153bd82a20f6e2533769
Author: Taehee Yoo <ap420073@gmail.com>
Date: Mon Oct 21 18:47:51 2019 +0000
net: core: add generic lockdep keys
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=1467f674e00000
start commit: 60c1769a Add linux-next specific files for 20191028
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=1667f674e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=1267f674e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=cb86688f30db053d
dashboard link: https://syzkaller.appspot.com/bug?extid=0d083911ab18b710da71
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=15381ee0e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=11571570e00000
Reported-by: syzbot+0d083911ab18b710da71@syzkaller.appspotmail.com
Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: INFO: trying to register non-static key in bond_3ad_update_ad_actor_settings
From: syzbot @ 2019-10-29 9:52 UTC (permalink / raw)
To: a, alex.aring, allison, andrew, andy, ap420073, aroulin, ast,
b.a.t.m.a.n, bridge, cleech, daniel, davem, dcaratti, dsa,
edumazet, f.fainelli, fw, gbastien, gregkh, gustavo, haiyangz,
idosch, info, ivan.khoronzhuk, j.vosburgh, j, jakub.kicinski, jhs,
jiri, jiri, johan.hedberg, johannes.berg, john.hurley, jwi,
kstewart, kvalo, kys, linmiaohe, linux-bluetooth, linux-hams,
linux-hyperv, linux-kernel, linux-ppp, linux-wireless, linux-wpan,
liuhangbin, marcel, mareklindner, mcroce
In-Reply-To: <000000000000044a7f0595fbaf2c@google.com>
syzbot has bisected this bug to:
commit ab92d68fc22f9afab480153bd82a20f6e2533769
Author: Taehee Yoo <ap420073@gmail.com>
Date: Mon Oct 21 18:47:51 2019 +0000
net: core: add generic lockdep keys
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=12b54d70e00000
start commit: 60c1769a Add linux-next specific files for 20191028
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=11b54d70e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=16b54d70e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=cb86688f30db053d
dashboard link: https://syzkaller.appspot.com/bug?extid=8da67f407bcba2c72e6e
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=14d43a04e00000
C reproducer: https://syzkaller.appspot.com/x/repro.c?x=16be3b9ce00000
Reported-by: syzbot+8da67f407bcba2c72e6e@syzkaller.appspotmail.com
Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: BUG: MAX_LOCKDEP_KEYS too low!
From: syzbot @ 2019-10-29 14:09 UTC (permalink / raw)
To: a, alex.aring, allison, andrew, andy, ap420073, aroulin, ast,
b.a.t.m.a.n, bridge, cleech, daniel, davem, dcaratti, dsa,
edumazet, f.fainelli, fw, gregkh, gustavo, gvaradar, haiyangz,
idosch, info, ivan.khoronzhuk, j.vosburgh, j, jakub.kicinski, jhs,
jiri, jiri, johan.hedberg, johannes.berg, john.hurley, jwi,
kstewart, kvalo, kys, lariel, linmiaohe, linux-bluetooth,
linux-hams, linux-hyperv, linux-kernel, linux-ppp, linux-wireless,
linux-wpan, liuhangbin, marcel, mareklindner
In-Reply-To: <0000000000009ea5720595dc03a3@google.com>
syzbot has bisected this bug to:
commit ab92d68fc22f9afab480153bd82a20f6e2533769
Author: Taehee Yoo <ap420073@gmail.com>
Date: Mon Oct 21 18:47:51 2019 +0000
net: core: add generic lockdep keys
bisection log: https://syzkaller.appspot.com/x/bisect.txt?x=12e05224e00000
start commit: 60c1769a Add linux-next specific files for 20191028
git tree: linux-next
final crash: https://syzkaller.appspot.com/x/report.txt?x=11e05224e00000
console output: https://syzkaller.appspot.com/x/log.txt?x=16e05224e00000
kernel config: https://syzkaller.appspot.com/x/.config?x=cb86688f30db053d
dashboard link: https://syzkaller.appspot.com/bug?extid=692f39f040c1f415567b
syz repro: https://syzkaller.appspot.com/x/repro.syz?x=10be9ed0e00000
Reported-by: syzbot+692f39f040c1f415567b@syzkaller.appspotmail.com
Fixes: ab92d68fc22f ("net: core: add generic lockdep keys")
For information about bisection process see: https://goo.gl/tpsmEJ#bisection
^ permalink raw reply
* Re: [PATCH net-next 00/14] vsock: add multi-transports support
From: Stefano Garzarella @ 2019-10-29 16:27 UTC (permalink / raw)
To: Stefan Hajnoczi, Adit Ranadive, Vishnu Dasa, Andy king,
Aditya Sarwade, George Zhang, Jorgen Hansen
Cc: netdev, Sasha Levin, linux-hyperv, Stephen Hemminger,
Arnd Bergmann, kvm, Michael S. Tsirkin, Greg Kroah-Hartman,
Dexuan Cui, linux-kernel, virtualization, Haiyang Zhang,
Stefan Hajnoczi, David S. Miller
In-Reply-To: <20191027080146.GA4472@stefanha-x1.localdomain>
On Sun, Oct 27, 2019 at 09:01:46AM +0100, Stefan Hajnoczi wrote:
> On Wed, Oct 23, 2019 at 11:55:40AM +0200, Stefano Garzarella wrote:
> > This series adds the multi-transports support to vsock, following
> > this proposal: https://www.spinics.net/lists/netdev/msg575792.html
> >
> > With the multi-transports support, we can use VSOCK with nested VMs
> > (using also different hypervisors) loading both guest->host and
> > host->guest transports at the same time.
> > Before this series, vmci-transport supported this behavior but only
> > using VMware hypervisor on L0, L1, etc.
> >
> > RFC: https://patchwork.ozlabs.org/cover/1168442/
> > RFC -> v1:
> > - Added R-b/A-b from Dexuan and Stefan
> > - Fixed comments and typos in several patches (Stefan)
> > - Patch 7: changed .notify_buffer_size return to void (Stefan)
> > - Added patch 8 to simplify the API exposed to the transports (Stefan)
> > - Patch 11:
> > + documented VSOCK_TRANSPORT_F_* flags (Stefan)
> > + fixed vsock_assign_transport() when the socket is already assigned
> > + moved features outside of struct vsock_transport, and used as
> > parameter of vsock_core_register() as a preparation of Patch 12
> > - Removed "vsock: add 'transport_hg' to handle g2h\h2g transports" patch
> > - Added patch 12 to register vmci_transport only when VMCI guest/host
> > are active
>
> Has there been feedback from Jorgen or someone else from VMware? A
> Reviewed-by or Acked-by would be nice since this patch series affects
> VMCI AF_VSOCK.
>
Unfortunately not for now, I'm adding to this thread some VMware guys that
reviewed latest vmci patches.
Would be nice to have your feedback for these changes.
Thanks in advance,
Stefano
^ permalink raw reply
* Re: [PATCH net-next 12/14] vsock/vmci: register vmci_transport only when VMCI guest/host are active
From: Stefano Garzarella @ 2019-10-29 16:35 UTC (permalink / raw)
To: Stefan Hajnoczi
Cc: netdev, Sasha Levin, linux-hyperv, Stephen Hemminger,
Arnd Bergmann, kvm, Michael S. Tsirkin, Greg Kroah-Hartman,
Dexuan Cui, linux-kernel, virtualization, Haiyang Zhang,
Stefan Hajnoczi, David S. Miller, Jorgen Hansen
In-Reply-To: <20191027081752.GD4472@stefanha-x1.localdomain>
On Sun, Oct 27, 2019 at 09:17:52AM +0100, Stefan Hajnoczi wrote:
> On Wed, Oct 23, 2019 at 11:55:52AM +0200, Stefano Garzarella wrote:
> > +static int __init vmci_transport_init(void)
> > +{
> > + int features = VSOCK_TRANSPORT_F_DGRAM;
>
> Where is this variable used?
It is introduced in the previous patch "vsock: add multi-transports support",
and it is used in the vsock_core_register(), but since now the
vmci_transport_init() registers the vmci_transport only with DGRAM
feature, I can remove this variable and I can use directly the
VSOCK_TRANSPORT_F_DGRAM.
I'll fix in the v3.
Thanks,
Stefano
^ permalink raw reply
* RE: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2019-10-29 19:17 UTC (permalink / raw)
To: Jakub Kicinski
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <20191028143322.45d81da4@cakuba.hsd1.ca.comcast.net>
> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Monday, October 28, 2019 5:33 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; davem@davemloft.net; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
>
> On Mon, 28 Oct 2019 21:07:04 +0000, Haiyang Zhang wrote:
> > This patch adds support of XDP in native mode for hv_netvsc driver, and
> > transparently sets the XDP program on the associated VF NIC as well.
> >
> > XDP program cannot run with LRO (RSC) enabled, so you need to disable
> LRO
> > before running XDP:
> > ethtool -K eth0 lro off
> >
> > XDP actions not yet supported:
> > XDP_TX, XDP_REDIRECT
>
> I don't think we want to merge support without at least XDP_TX these
> days..
Thanks for your detailed comments --
I'm working on the XDP_TX...
>
> And without the ability to prepend headers this may be the least
> complete initial XDP implementation we've seen :(
The RNDIS packet buffer received by netvsc doesn't have a head room, but I'm
considering copy the packets to the page buffer, with a head room space
reserved for XDP.
>
> > Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
>
> > diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c
> > index d22a36f..688487b 100644
> > --- a/drivers/net/hyperv/netvsc.c
> > +++ b/drivers/net/hyperv/netvsc.c
> > @@ -122,8 +122,10 @@ static void free_netvsc_device(struct rcu_head
> *head)
> > vfree(nvdev->send_buf);
> > kfree(nvdev->send_section_map);
> >
> > - for (i = 0; i < VRSS_CHANNEL_MAX; i++)
> > + for (i = 0; i < VRSS_CHANNEL_MAX; i++) {
> > + xdp_rxq_info_unreg(&nvdev->chan_table[i].xdp_rxq);
> > vfree(nvdev->chan_table[i].mrc.slots);
> > + }
> >
> > kfree(nvdev);
> > }
> > @@ -1370,6 +1372,10 @@ struct netvsc_device *netvsc_device_add(struct
> hv_device *device,
> > nvchan->net_device = net_device;
> > u64_stats_init(&nvchan->tx_stats.syncp);
> > u64_stats_init(&nvchan->rx_stats.syncp);
> > +
> > + xdp_rxq_info_reg(&nvchan->xdp_rxq, ndev, i);
> > + xdp_rxq_info_reg_mem_model(&nvchan->xdp_rxq,
> > + MEM_TYPE_PAGE_SHARED, NULL);
>
> These can fail.
I will add error handling.
>
> > }
> >
> > /* Enable NAPI handler before init callbacks */
> > diff --git a/drivers/net/hyperv/netvsc_bpf.c
> b/drivers/net/hyperv/netvsc_bpf.c
> > new file mode 100644
> > index 0000000..4d235ac
> > --- /dev/null
> > +++ b/drivers/net/hyperv/netvsc_bpf.c
> > @@ -0,0 +1,211 @@
> > +// SPDX-License-Identifier: GPL-2.0-only
> > +/* Copyright (c) 2019, Microsoft Corporation.
> > + *
> > + * Author:
> > + * Haiyang Zhang <haiyangz@microsoft.com>
> > + */
> > +
> > +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> > +
> > +#include <linux/netdevice.h>
> > +#include <linux/etherdevice.h>
> > +#include <linux/ethtool.h>
> > +#include <linux/bpf.h>
> > +#include <linux/bpf_trace.h>
> > +#include <linux/kernel.h>
> > +#include <net/xdp.h>
> > +
> > +#include <linux/mutex.h>
> > +#include <linux/rtnetlink.h>
> > +
> > +#include "hyperv_net.h"
> > +
> > +u32 netvsc_run_xdp(struct net_device *ndev, struct netvsc_channel
> *nvchan,
> > + void **p_pbuf)
> > +{
> > + struct page *page = NULL;
> > + void *data = nvchan->rsc.data[0];
> > + u32 len = nvchan->rsc.len[0];
> > + void *pbuf = data;
> > + struct bpf_prog *prog;
> > + struct xdp_buff xdp;
> > + u32 act = XDP_PASS;
> > +
> > + *p_pbuf = NULL;
> > +
> > + rcu_read_lock();
> > + prog = rcu_dereference(nvchan->bpf_prog);
> > +
> > + if (!prog || nvchan->rsc.cnt > 1)
>
> Can rsc.cnt == 1 not be ensured at setup time? This looks quite
> limiting if random frames could be forced to bypass the filter.
Yes, the setup code already check/ensure LRO is disabled. So rsc.cnt > 1
is NOT expected here. Just an error check. I will change the return value
to XDP_ABORTED for this.
>
> > + goto out;
> > +
> > + /* copy to a new page buffer if data are not within a page */
> > + if (virt_to_page(data) != virt_to_page(data + len - 1)) {
> > + page = alloc_page(GFP_ATOMIC);
> > + if (!page)
> > + goto out;
>
> Returning XDP_PASS on allocation failure seems highly questionable.
I will change the return value to XDP_ABORTED for this too.
>
> > + pbuf = page_address(page);
> > + memcpy(pbuf, nvchan->rsc.data[0], len);
> > +
> > + *p_pbuf = pbuf;
> > + }
> > +
> > + xdp.data_hard_start = pbuf;
> > + xdp.data = xdp.data_hard_start;
>
> This patch also doesn't add any headroom for XDP to prepend data :(
I'm considering to add the headroom to the start of the page.
>
> > + xdp_set_data_meta_invalid(&xdp);
> > + xdp.data_end = xdp.data + len;
> > + xdp.rxq = &nvchan->xdp_rxq;
> > + xdp.handle = 0;
> > +
> > + act = bpf_prog_run_xdp(prog, &xdp);
> > +
> > + switch (act) {
> > + case XDP_PASS:
> > + /* Pass to upper layers */
> > + break;
> > +
> > + case XDP_ABORTED:
> > + trace_xdp_exception(ndev, prog, act);
> > + break;
> > +
> > + case XDP_DROP:
> > + break;
> > +
> > + default:
> > + bpf_warn_invalid_xdp_action(act);
> > + }
> > +
> > +out:
> > + rcu_read_unlock();
> > +
> > + if (page && act != XDP_PASS) {
> > + *p_pbuf = NULL;
> > + __free_page(page);
> > + }
> > +
> > + return act;
> > +}
> > +
> > +unsigned int netvsc_xdp_fraglen(unsigned int len)
> > +{
> > + return SKB_DATA_ALIGN(len) +
> > + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> > +}
> > +
> > +struct bpf_prog *netvsc_xdp_get(struct netvsc_device *nvdev)
> > +{
> > + return rtnl_dereference(nvdev->chan_table[0].bpf_prog);
> > +}
> > +
> > +int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > + struct netvsc_device *nvdev)
> > +{
> > + struct bpf_prog *old_prog;
> > + int frag_max, i;
> > +
> > + old_prog = netvsc_xdp_get(nvdev);
> > +
> > + if (!old_prog && !prog)
> > + return 0;
>
> I think this case is now handled by the core.
Thanks for the reminder. I saw the code in dev_change_xdp_fd(), so the upper layer
doesn't call XDP_SETUP_PROG with old/new prog both NULL.
But this function is also called by other functions in our driver, like netvsc_detach(),
netvsc_remove(), etc. Instead of checking for NULL in each place, I still keep the check inside
netvsc_xdp_set().
>
> > + frag_max = netvsc_xdp_fraglen(dev->mtu + ETH_HLEN);
> > + if (prog && frag_max > PAGE_SIZE) {
> > + netdev_err(dev, "XDP: mtu:%u too large, frag:%u\n",
> > + dev->mtu, frag_max);
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (prog && (dev->features & NETIF_F_LRO)) {
> > + netdev_err(dev, "XDP: not support LRO\n");
>
> Please report this via extack, that way users will see it in the console
> in which they're installing the program.
I will.
>
> > + return -EOPNOTSUPP;
> > + }
> > +
> > + if (prog) {
> > + prog = bpf_prog_add(prog, nvdev->num_chn);
> > + if (IS_ERR(prog))
> > + return PTR_ERR(prog);
> > + }
> > +
> > + for (i = 0; i < nvdev->num_chn; i++)
> > + rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog);
> > +
> > + if (old_prog)
> > + for (i = 0; i < nvdev->num_chn; i++)
> > + bpf_prog_put(old_prog);
> > +
> > + return 0;
> > +}
> > +
> > +int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog
> *prog)
> > +{
> > + struct netdev_bpf xdp;
> > + bpf_op_t ndo_bpf;
> > +
> > + ASSERT_RTNL();
> > +
> > + if (!vf_netdev)
> > + return 0;
> > +
> > + ndo_bpf = vf_netdev->netdev_ops->ndo_bpf;
> > + if (!ndo_bpf)
> > + return 0;
> > +
> > + memset(&xdp, 0, sizeof(xdp));
> > +
> > + xdp.command = XDP_SETUP_PROG;
> > + xdp.prog = prog;
> > +
> > + return ndo_bpf(vf_netdev, &xdp);
>
> IMHO the automatic propagation is not a good idea. Especially if the
> propagation doesn't make the entire installation fail if VF doesn't
> have ndo_bpf.
On Hyperv and Azure hosts, VF is always acting as a slave below netvsc.
And they are both active -- most data packets go to VF, but broadcast,
multicast, and TCP SYN packets go to netvsc synthetic data path. The synthetic
NIC (netvsc) is also a failover NIC when VF is not available.
We ask customers to only use the synthetic NIC directly. So propagation
of XDP setting to VF NIC is desired.
But, I will change the return code to error, so the entire installation fails if VF is
present but unable to set XDP prog.
Thanks,
- Haiyang
^ permalink raw reply
* Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Jakub Kicinski @ 2019-10-29 19:53 UTC (permalink / raw)
To: Haiyang Zhang
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <DM6PR21MB1337547067BE5E52DFE05E20CA610@DM6PR21MB1337.namprd21.prod.outlook.com>
On Tue, 29 Oct 2019 19:17:25 +0000, Haiyang Zhang wrote:
> > > +int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > > + struct netvsc_device *nvdev)
> > > +{
> > > + struct bpf_prog *old_prog;
> > > + int frag_max, i;
> > > +
> > > + old_prog = netvsc_xdp_get(nvdev);
> > > +
> > > + if (!old_prog && !prog)
> > > + return 0;
> >
> > I think this case is now handled by the core.
> Thanks for the reminder. I saw the code in dev_change_xdp_fd(), so the upper layer
> doesn't call XDP_SETUP_PROG with old/new prog both NULL.
> But this function is also called by other functions in our driver, like netvsc_detach(),
> netvsc_remove(), etc. Instead of checking for NULL in each place, I still keep the check inside
> netvsc_xdp_set().
I see. Makes sense on a closer look.
BTW would you do me a favour and reformat this line:
static struct netvsc_device_info *netvsc_devinfo_get
(struct netvsc_device *nvdev)
to look like this:
static
struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device *nvdev)
or
static struct netvsc_device_info *
netvsc_devinfo_get(struct netvsc_device *nvdev)
Otherwise git diff gets confused about which function given chunk
belongs to. (Incorrectly thinking your patch is touching
netvsc_get_channels()). I spent few minutes trying to figure out what's
going on there :)
> >
> > > + return -EOPNOTSUPP;
> > > + }
> > > +
> > > + if (prog) {
> > > + prog = bpf_prog_add(prog, nvdev->num_chn);
> > > + if (IS_ERR(prog))
> > > + return PTR_ERR(prog);
> > > + }
> > > +
> > > + for (i = 0; i < nvdev->num_chn; i++)
> > > + rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog);
> > > +
> > > + if (old_prog)
> > > + for (i = 0; i < nvdev->num_chn; i++)
> > > + bpf_prog_put(old_prog);
> > > +
> > > + return 0;
> > > +}
> > > +
> > > +int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog *prog)
> > > +{
> > > + struct netdev_bpf xdp;
> > > + bpf_op_t ndo_bpf;
> > > +
> > > + ASSERT_RTNL();
> > > +
> > > + if (!vf_netdev)
> > > + return 0;
> > > +
> > > + ndo_bpf = vf_netdev->netdev_ops->ndo_bpf;
> > > + if (!ndo_bpf)
> > > + return 0;
> > > +
> > > + memset(&xdp, 0, sizeof(xdp));
> > > +
> > > + xdp.command = XDP_SETUP_PROG;
> > > + xdp.prog = prog;
> > > +
> > > + return ndo_bpf(vf_netdev, &xdp);
> >
> > IMHO the automatic propagation is not a good idea. Especially if the
> > propagation doesn't make the entire installation fail if VF doesn't
> > have ndo_bpf.
>
> On Hyperv and Azure hosts, VF is always acting as a slave below netvsc.
> And they are both active -- most data packets go to VF, but broadcast,
> multicast, and TCP SYN packets go to netvsc synthetic data path. The synthetic
> NIC (netvsc) is also a failover NIC when VF is not available.
> We ask customers to only use the synthetic NIC directly. So propagation
> of XDP setting to VF NIC is desired.
> But, I will change the return code to error, so the entire installation fails if VF is
> present but unable to set XDP prog.
Okay, if I read the rest of the code correctly you also fail attach
if xdp propagation failed? If that's the case and we return an error
here on missing NDO, then the propagation could be okay.
So the semantics are these:
(a) install on virt - potentially overwrites the existing VF prog;
(b) install on VF is not noticed by virt;
(c) uninstall on virt - clears both virt and VF, regardless what
program was installed on virt;
(d) uninstall on VF does not propagate;
Since you're adding documentation it would perhaps be worth stating
there that touching the program on the VF is not supported/may lead
to breakage, and users should only touch/configure the program on the
virt.
^ permalink raw reply
* RE: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2019-10-29 20:01 UTC (permalink / raw)
To: Jakub Kicinski
Cc: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <20191029125308.78b52511@cakuba.hsd1.ca.comcast.net>
> -----Original Message-----
> From: Jakub Kicinski <jakub.kicinski@netronome.com>
> Sent: Tuesday, October 29, 2019 3:53 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> <vkuznets@redhat.com>; davem@davemloft.net; linux-
> kernel@vger.kernel.org
> Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
>
> On Tue, 29 Oct 2019 19:17:25 +0000, Haiyang Zhang wrote:
> > > > +int netvsc_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> > > > + struct netvsc_device *nvdev)
> > > > +{
> > > > + struct bpf_prog *old_prog;
> > > > + int frag_max, i;
> > > > +
> > > > + old_prog = netvsc_xdp_get(nvdev);
> > > > +
> > > > + if (!old_prog && !prog)
> > > > + return 0;
> > >
> > > I think this case is now handled by the core.
> > Thanks for the reminder. I saw the code in dev_change_xdp_fd(), so the
> upper layer
> > doesn't call XDP_SETUP_PROG with old/new prog both NULL.
> > But this function is also called by other functions in our driver, like
> netvsc_detach(),
> > netvsc_remove(), etc. Instead of checking for NULL in each place, I still
> keep the check inside
> > netvsc_xdp_set().
>
> I see. Makes sense on a closer look.
>
> BTW would you do me a favour and reformat this line:
>
> static struct netvsc_device_info *netvsc_devinfo_get
> (struct netvsc_device *nvdev)
>
> to look like this:
>
> static
> struct netvsc_device_info *netvsc_devinfo_get(struct netvsc_device
> *nvdev)
>
> or
>
> static struct netvsc_device_info *
> netvsc_devinfo_get(struct netvsc_device *nvdev)
>
> Otherwise git diff gets confused about which function given chunk
> belongs to. (Incorrectly thinking your patch is touching
> netvsc_get_channels()). I spent few minutes trying to figure out what's
> going on there :)
I will.
>
> > >
> > > > + return -EOPNOTSUPP;
> > > > + }
> > > > +
> > > > + if (prog) {
> > > > + prog = bpf_prog_add(prog, nvdev->num_chn);
> > > > + if (IS_ERR(prog))
> > > > + return PTR_ERR(prog);
> > > > + }
> > > > +
> > > > + for (i = 0; i < nvdev->num_chn; i++)
> > > > + rcu_assign_pointer(nvdev->chan_table[i].bpf_prog, prog);
> > > > +
> > > > + if (old_prog)
> > > > + for (i = 0; i < nvdev->num_chn; i++)
> > > > + bpf_prog_put(old_prog);
> > > > +
> > > > + return 0;
> > > > +}
> > > > +
> > > > +int netvsc_vf_setxdp(struct net_device *vf_netdev, struct bpf_prog
> *prog)
> > > > +{
> > > > + struct netdev_bpf xdp;
> > > > + bpf_op_t ndo_bpf;
> > > > +
> > > > + ASSERT_RTNL();
> > > > +
> > > > + if (!vf_netdev)
> > > > + return 0;
> > > > +
> > > > + ndo_bpf = vf_netdev->netdev_ops->ndo_bpf;
> > > > + if (!ndo_bpf)
> > > > + return 0;
> > > > +
> > > > + memset(&xdp, 0, sizeof(xdp));
> > > > +
> > > > + xdp.command = XDP_SETUP_PROG;
> > > > + xdp.prog = prog;
> > > > +
> > > > + return ndo_bpf(vf_netdev, &xdp);
> > >
> > > IMHO the automatic propagation is not a good idea. Especially if the
> > > propagation doesn't make the entire installation fail if VF doesn't
> > > have ndo_bpf.
> >
> > On Hyperv and Azure hosts, VF is always acting as a slave below netvsc.
> > And they are both active -- most data packets go to VF, but broadcast,
> > multicast, and TCP SYN packets go to netvsc synthetic data path. The
> synthetic
> > NIC (netvsc) is also a failover NIC when VF is not available.
> > We ask customers to only use the synthetic NIC directly. So propagation
> > of XDP setting to VF NIC is desired.
> > But, I will change the return code to error, so the entire installation fails if
> VF is
> > present but unable to set XDP prog.
>
> Okay, if I read the rest of the code correctly you also fail attach
> if xdp propagation failed? If that's the case and we return an error
> here on missing NDO, then the propagation could be okay.
>
> So the semantics are these:
>
> (a) install on virt - potentially overwrites the existing VF prog;
> (b) install on VF is not noticed by virt;
> (c) uninstall on virt - clears both virt and VF, regardless what
> program was installed on virt;
> (d) uninstall on VF does not propagate;
>
> Since you're adding documentation it would perhaps be worth stating
> there that touching the program on the VF is not supported/may lead
> to breakage, and users should only touch/configure the program on the
> virt.
Sure I will document the recommended way of install xdp prog.
Thanks,
- Haiyang
^ permalink raw reply
* RE: [PATCH 1/1] x86/hyperv: Initialize clockevents earlier in CPU onlining
From: Dexuan Cui @ 2019-10-29 20:33 UTC (permalink / raw)
To: Michael Kelley, linux-kernel@vger.kernel.org, tglx@linutronix.de,
daniel.lezcano@linaro.org, vkuznets, KY Srinivasan,
Stephen Hemminger, sashal@kernel.org, mingo@redhat.com,
bp@alien8.de, hpa@zytor.com, x86@kernel.org, will@kernel.org,
Ganapatrao.Kulkarni@cavium.com, james.morse@arm.com,
steven.price@arm.com, josephl@nvidia.com,
m.szyprowski@samsumg.com, linux-hyperv@vger.kernel.org
In-Reply-To: <1572228459-10550-1-git-send-email-mikelley@microsoft.com>
> From: Michael Kelley <mikelley@microsoft.com>
> Sent: Sunday, October 27, 2019 7:10 PM
> ...
> Signed-off-by: Michael Kelley <mikelley@microsoft.com>
Should we add the 2 lines:
Cc: stable@vger.kernel.org
Fixes: fd1fea6834d0 ("clocksource/drivers: Make Hyper-V clocksource ISA agnostic")
fd1fea6834d0() removes the clockevents_unbind_device() call and this patch adds it back.
> --- a/arch/x86/hyperv/hv_init.c
> +++ b/arch/x86/hyperv/hv_init.c
> @@ -323,8 +323,15 @@ void __init hyperv_init(void)
>
> x86_init.pci.arch_init = hv_pci_init;
>
> + if (hv_stimer_alloc())
> + goto remove_hypercall_page;
> +
The error handling is imperfect here: when I force hv_stimer_alloc() to return
-ENOMEM, I get a panic in hv_apic_eoi_write(). It looks it is because we have cleared
the pointer 'hv_vp_assist_page' to NULL, but hv_apic_eoi_write() is still in-use.
In case hv_stimer_alloc() fails, can we set 'direct_mode_enabled' to false
and go on with the legacy Hyper-V timer or LAPIC timer? If not, maybe
we can use a BUG_ON() to explicitly panic?
> return;
>
> +remove_hypercall_page:
> + hypercall_msr.as_uint64 = 0;
> + wrmsrl(HV_X64_MSR_HYPERCALL, hypercall_msr.as_uint64);
> + hv_hypercall_pg = NULL;
> remove_cpuhp_state:
> cpuhp_remove_state(cpuhp);
> free_vp_assist_page:
> -void hv_stimer_cleanup(unsigned int cpu)
> +static int hv_stimer_cleanup(unsigned int cpu)
> {
> struct clock_event_device *ce;
>
> /* Turn off clockevent device */
> if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> ce = per_cpu_ptr(hv_clock_event, cpu);
> +
> + /*
> + * In the legacy case where Direct Mode is not enabled
> + * (which can only be on x86/64), stimer cleanup happens
> + * relatively early in the CPU offlining process. We
> + * must unbind the stimer-based clockevent device so
> + * that the LAPIC timer can take over until clockevents
> + * are no longer needed in the offlining process. The
> + * unbind should not be done when Direct Mode is enabled
> + * because we may be on an architecture where there are
> + * no other clockevents devices to fallback to.
> + */
> + if (!direct_mode_enabled)
> + clockevents_unbind_device(ce, cpu);
> hv_ce_shutdown(ce);
In the legacy stimer0 mode, IMO this hv_ce_shutdown() is unnecessary,
because "clockevents_unbind_device(ce, cpu)" automatically calls
ce->set_state_shutdown(), if ce is active:
clockevents_unbind
__clockevents_unbind
clockevents_replace
tick_install_replacement
clockevents_exchange_device
clockevents_switch_state(old, CLOCK_EVT_STATE_DETACHED)
__clockevents_switch_state
And, in both modes (legacy mode and direct mode), it looks incorrect to
call hv_ce_shutdown() if the current processid id != 'cpu', because
hv_ce_shutdown() -> hv_init_timer() can only access the current CPU's
MSR. Maybe we should use an IPI to run hv_ce_shutdown() on the target
CPU in direct mode?
> -int hv_stimer_alloc(int sint)
> +int hv_stimer_alloc(void)
> ...
> + ret = cpuhp_setup_state(CPUHP_AP_HYPERV_TIMER_STARTING,
> + "clockevents/hyperv/stimer:starting",
> + hv_stimer_init, hv_stimer_cleanup);
> + if (ret < 0)
> + goto free_stimer0_irq;
> + stimer0_cpuhp = ret;
> }
> + return ret;
stimer0_cpuhp is 0 when the call is successful, so IMO the logic in
hv_stimer_free() is incorrect. Please see below.
> void hv_stimer_free(void)
> {
> - if (direct_mode_enabled && (stimer0_irq != 0)) {
> - hv_remove_stimer0_irq(stimer0_irq);
> - stimer0_irq = 0;
> + if (direct_mode_enabled) {
> + if (stimer0_cpuhp) {
> + cpuhp_remove_state(stimer0_cpuhp);
> + stimer0_cpuhp = 0;
> + }
> + if (stimer0_irq) {
> + hv_remove_stimer0_irq(stimer0_irq);
> + stimer0_irq = 0;
> + }
> }
IMO this should be
if (direct_mode_enabled) {
if (stimer0_cpuhp == 0)
cpuhp_remove_state(CPUHP_AP_HYPERV_TIMER_STARTING);
if (stimer0_irq) {
hv_remove_stimer0_irq(stimer0_irq);
stimer0_irq = 0;
}
}
BTW, the default value of 'stimer0_cpuhp' is 0, which means success.
Should we change the default value to a non-zero value, e.g. -1 ?
> free_percpu(hv_clock_event);
> hv_clock_event = NULL;
> @@ -190,14 +274,11 @@ void hv_stimer_free(void)
> void hv_stimer_global_cleanup(void)
> {
> int cpu;
> - struct clock_event_device *ce;
>
> - if (ms_hyperv.features & HV_MSR_SYNTIMER_AVAILABLE) {
> - for_each_present_cpu(cpu) {
> - ce = per_cpu_ptr(hv_clock_event, cpu);
> - clockevents_unbind_device(ce, cpu);
> - }
> + for_each_present_cpu(cpu) {
> + hv_stimer_cleanup(cpu);
hv_stimer_cleanup() -> hv_ce_shutdown() -> hv_init_timer() can not
access a remote CPU's MSR.
> @@ -2310,7 +2305,6 @@ static void hv_crash_handler(struct pt_regs *regs)
> */
> vmbus_connection.conn_state = DISCONNECTED;
> cpu = smp_processor_id();
> - hv_stimer_cleanup(cpu);
> hv_synic_cleanup(cpu);
> hyperv_cleanup();
Why should we remove the line hv_stimer_cleanup() in the crash handler?
In the crash handler, IMO we'd better disable the timer before we proceed.
Thanks,
-- Dexuan
^ permalink raw reply
* Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Stephen Hemminger @ 2019-10-29 21:59 UTC (permalink / raw)
To: Haiyang Zhang
Cc: Jakub Kicinski, sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <DM6PR21MB1337547067BE5E52DFE05E20CA610@DM6PR21MB1337.namprd21.prod.outlook.com>
On Tue, 29 Oct 2019 19:17:25 +0000
Haiyang Zhang <haiyangz@microsoft.com> wrote:
> > -----Original Message-----
> > From: Jakub Kicinski <jakub.kicinski@netronome.com>
> > Sent: Monday, October 28, 2019 5:33 PM
> > To: Haiyang Zhang <haiyangz@microsoft.com>
> > Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> > netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> > <vkuznets@redhat.com>; davem@davemloft.net; linux-
> > kernel@vger.kernel.org
> > Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
> >
> > On Mon, 28 Oct 2019 21:07:04 +0000, Haiyang Zhang wrote:
> > > This patch adds support of XDP in native mode for hv_netvsc driver, and
> > > transparently sets the XDP program on the associated VF NIC as well.
> > >
> > > XDP program cannot run with LRO (RSC) enabled, so you need to disable
> > LRO
> > > before running XDP:
> > > ethtool -K eth0 lro off
> > >
> > > XDP actions not yet supported:
> > > XDP_TX, XDP_REDIRECT
> >
> > I don't think we want to merge support without at least XDP_TX these
> > days..
> Thanks for your detailed comments --
> I'm working on the XDP_TX...
>
> >
> > And without the ability to prepend headers this may be the least
> > complete initial XDP implementation we've seen :(
> The RNDIS packet buffer received by netvsc doesn't have a head room, but I'm
> considering copy the packets to the page buffer, with a head room space
> reserved for XDP.
There is a small amount of headroom available by reusing the RNDIS
header and packet space. Looks like 40 bytes or so.
^ permalink raw reply
* RE: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
From: Haiyang Zhang @ 2019-10-29 22:08 UTC (permalink / raw)
To: Stephen Hemminger
Cc: Jakub Kicinski, sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org, KY Srinivasan, Stephen Hemminger,
olaf@aepfle.de, vkuznets, davem@davemloft.net,
linux-kernel@vger.kernel.org
In-Reply-To: <20191029145905.414f86c3@hermes.lan>
> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Tuesday, October 29, 2019 5:59 PM
> To: Haiyang Zhang <haiyangz@microsoft.com>
> Cc: Jakub Kicinski <jakub.kicinski@netronome.com>; sashal@kernel.org;
> linux-hyperv@vger.kernel.org; netdev@vger.kernel.org; KY Srinivasan
> <kys@microsoft.com>; Stephen Hemminger <sthemmin@microsoft.com>;
> olaf@aepfle.de; vkuznets <vkuznets@redhat.com>; davem@davemloft.net;
> linux-kernel@vger.kernel.org
> Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
>
> On Tue, 29 Oct 2019 19:17:25 +0000
> Haiyang Zhang <haiyangz@microsoft.com> wrote:
>
> > > -----Original Message-----
> > > From: Jakub Kicinski <jakub.kicinski@netronome.com>
> > > Sent: Monday, October 28, 2019 5:33 PM
> > > To: Haiyang Zhang <haiyangz@microsoft.com>
> > > Cc: sashal@kernel.org; linux-hyperv@vger.kernel.org;
> > > netdev@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Stephen
> > > Hemminger <sthemmin@microsoft.com>; olaf@aepfle.de; vkuznets
> > > <vkuznets@redhat.com>; davem@davemloft.net; linux-
> > > kernel@vger.kernel.org
> > > Subject: Re: [PATCH net-next, 3/4] hv_netvsc: Add XDP support
> > >
> > > On Mon, 28 Oct 2019 21:07:04 +0000, Haiyang Zhang wrote:
> > > > This patch adds support of XDP in native mode for hv_netvsc driver,
> and
> > > > transparently sets the XDP program on the associated VF NIC as well.
> > > >
> > > > XDP program cannot run with LRO (RSC) enabled, so you need to
> disable
> > > LRO
> > > > before running XDP:
> > > > ethtool -K eth0 lro off
> > > >
> > > > XDP actions not yet supported:
> > > > XDP_TX, XDP_REDIRECT
> > >
> > > I don't think we want to merge support without at least XDP_TX these
> > > days..
> > Thanks for your detailed comments --
> > I'm working on the XDP_TX...
> >
> > >
> > > And without the ability to prepend headers this may be the least
> > > complete initial XDP implementation we've seen :(
> > The RNDIS packet buffer received by netvsc doesn't have a head room, but
> I'm
> > considering copy the packets to the page buffer, with a head room space
> > reserved for XDP.
>
>
> There is a small amount of headroom available by reusing the RNDIS
> header and packet space. Looks like 40 bytes or so.
Yes, I thought about the RNDIS header. But is this space sufficient?
I saw some drivers, like virtio_net gives bigger headroom: 256 bytes.
Thanks,
- Haiyang
^ permalink raw reply
* [GIT PULL] Hyper-V commits for 5.4-rc
From: Sasha Levin @ 2019-10-30 11:37 UTC (permalink / raw)
To: Linus Torvalds; +Cc: linux-kernel, linux-hyperv, kys, sthemmin, linux-kernel
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512
The following changes since commit 54ecb8f7028c5eb3d740bb82b0f1d90f2df63c5c:
Linux 5.4-rc1 (2019-09-30 10:35:40 -0700)
are available in the Git repository at:
git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed
for you to fetch changes up to 590c28b9199c99593b879cbb82d7d4be605894ec:
Drivers: hv: vmbus: Fix harmless building warnings without CONFIG_PM_SLEEP (2019-10-28 12:24:53 -0400)
- ----------------------------------------------------------------
- - Fix a leak and improve the handling of the ring buffer in the Hyper-V
HID driver from Dexuan Cui.
- - Fix a (harmless) build warning in vmbus PM code by Dexuan Cui.
- - A fix for a build issue in the Hyper-V IOMMU driver resulting from
enablement on new architectures by Boqun Feng.
- ----------------------------------------------------------------
Boqun Feng (1):
drivers: iommu: hyperv: Make HYPERV_IOMMU only available on x86
Dexuan Cui (2):
HID: hyperv: Use in-place iterator API in the channel callback
Drivers: hv: vmbus: Fix harmless building warnings without CONFIG_PM_SLEEP
drivers/hid/hid-hyperv.c | 56 +++++++++---------------------------------------
drivers/hv/vmbus_drv.c | 6 ++++++
drivers/iommu/Kconfig | 2 +-
3 files changed, 17 insertions(+), 47 deletions(-)
-----BEGIN PGP SIGNATURE-----
iQIzBAEBCgAdFiEE4n5dijQDou9mhzu83qZv95d3LNwFAl25dZ0ACgkQ3qZv95d3
LNwEXg/+LjI3+t8Qw5rd2aaCU2NUEVTQJA1bMOx8HT62lKVhrbPT7Y3XZ+zP2K9P
fE6c3bHjTnSymrAx3XMNO2qAXYocA6fo3ggNVleTSA7zrh21fIR2XV0NVDoDNZ/d
KEU/kJbAMyVADKRlwnKA9+O+bfOQhvP4wDb3r1fKaKQ+HznEw8rrbQ0NgohGRO0i
85diHq3KUkXfGvh1PzkzeXGSuxMlmoM/ZtsjE+8Okj4NU0R3Wp0nd6kJzi2EwkDz
ZGmxLpV5Se+qilA7gUfayhSdYS0UyFO0BX+rONfKPdEOLwxVS81/5wROwtivJ2xJ
gMqfZg78X8yiqEkFEMfVngxoeARzJGcFYiqjl8iOGL57tU875Mz2yZgAx1PH0J4y
nZ86X44A13ZViOYlMwV0JG0h94FRtiab3PeVbLCXNU1ULSBJcppLP4lUxdVgeZyz
F9xS6RY35QaGxyT1/cb+g2qr//TAXPQaJh4vnh5vEx/U3lI86+IyCs0xuaOhKP06
frdoXOTdRmLZoqm7ruoCt4gTJUIGbYvGruisAVifdS0401UxXssIKoh1/1CHbBE6
1/bUofBwNrXtyfbru2VyROxRqb/uxZj+UECQwRqJkkg4JUOglBz08h+O+Rjie79i
PrUjPAwhUAKxQ19Cj59M+r6MlT5U9L/Fp2q/BODk1gu1jC3bRAM=
=UL+8
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [GIT PULL] Hyper-V commits for 5.4-rc
From: Linus Torvalds @ 2019-10-30 13:15 UTC (permalink / raw)
To: Sasha Levin
Cc: linux-kernel, linux-hyperv, kys, Stephen Hemminger,
Linux Kernel Mailing List
In-Reply-To: <20191030113703.266992083E@mail.kernel.org>
On Wed, Oct 30, 2019 at 12:37 PM Sasha Levin <sashal@kernel.org> wrote:
>
> git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed
No, Sasha, I'm not pulling this.
It's completely broken garbage.
You already sent me two of those fixes earlier, and they got pulled in
commit 56c642e2aa1c ("Merge tag 'hyperv-fixes-signed' of
git://git.kernel.org/pub/scm/linux/kernel/git/hyper>") two weeks ago.
Fine - of the three fixes you claim, I could do this pull, and get at
least one of them.
Except YOU HAVE REBASED your branch, so I see the other two fixes that
I already got as duplicates.
WHY?
Stop this. Read the documentation on rebasing, and stop doing this
kind of insane thing.
Linus
^ permalink raw reply
* Re: [GIT PULL] Hyper-V commits for 5.4-rc
From: Sasha Levin @ 2019-10-30 13:31 UTC (permalink / raw)
To: Linus Torvalds
Cc: linux-kernel, linux-hyperv, kys, Stephen Hemminger,
Linux Kernel Mailing List
In-Reply-To: <CAHk-=wgx_pSBtvmQE9zuNB6aoP52z601SG1pQDtrhm9ZMHNPMw@mail.gmail.com>
On Wed, Oct 30, 2019 at 02:15:08PM +0100, Linus Torvalds wrote:
>On Wed, Oct 30, 2019 at 12:37 PM Sasha Levin <sashal@kernel.org> wrote:
>>
>> git://git.kernel.org/pub/scm/linux/kernel/git/hyperv/linux.git tags/hyperv-fixes-signed
>
>No, Sasha, I'm not pulling this.
>
>It's completely broken garbage.
It is, appologies!
>You already sent me two of those fixes earlier, and they got pulled in
>commit 56c642e2aa1c ("Merge tag 'hyperv-fixes-signed' of
>git://git.kernel.org/pub/scm/linux/kernel/git/hyper>") two weeks ago.
>
>Fine - of the three fixes you claim, I could do this pull, and get at
>least one of them.
>
>Except YOU HAVE REBASED your branch, so I see the other two fixes that
>I already got as duplicates.
>
>WHY?
Honestly, I forgot that I sent you those two commits two weeks ago, but
still - you must be asking yourself why the heck would he rebase those,
right?
As I was working on the branch a few days ago I messed up and killed my
-fixes branch accidentally. To fix that I figured I'll just pick up
those 3 fixes again from my mailbox since they're grouped so nicely
over there and I haven't sent them to you yet.
Except that I did. And now they ended up with different IDs.
I then did a merge attempt before sending them to you, expecting that
things would blow up if I messed something up, but it didn't because the
commits themselves are identical, so I haven't noticed it then either.
Anyway, sorry about this, I'll resend it with just the relevant fix.
--
Thanks,
Sasha
^ permalink raw reply
* RE: [PATCH net-next 01/14] vsock/vmci: remove unused VSOCK_DEFAULT_CONNECT_TIMEOUT
From: Jorgen Hansen @ 2019-10-30 14:54 UTC (permalink / raw)
To: 'Stefano Garzarella', netdev@vger.kernel.org
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org
In-Reply-To: <20191023095554.11340-2-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 01/14] vsock/vmci: remove unused
> VSOCK_DEFAULT_CONNECT_TIMEOUT
>
> The VSOCK_DEFAULT_CONNECT_TIMEOUT definition was introduced with
> commit d021c344051af ("VSOCK: Introduce VM Sockets"), but it is never used
> in the net/vmw_vsock/vmci_transport.c.
>
> VSOCK_DEFAULT_CONNECT_TIMEOUT is used and defined in
> net/vmw_vsock/af_vsock.c
>
> Cc: Jorgen Hansen <jhansen@vmware.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> net/vmw_vsock/vmci_transport.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/net/vmw_vsock/vmci_transport.c
> b/net/vmw_vsock/vmci_transport.c index 8c9c4ed90fa7..f8e3131ac480
> 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -78,11 +78,6 @@ static int PROTOCOL_OVERRIDE = -1;
> #define VMCI_TRANSPORT_DEFAULT_QP_SIZE 262144
> #define VMCI_TRANSPORT_DEFAULT_QP_SIZE_MAX 262144
>
> -/* The default peer timeout indicates how long we will wait for a peer
> response
> - * to a control message.
> - */
> -#define VSOCK_DEFAULT_CONNECT_TIMEOUT (2 * HZ)
> -
> /* Helper function to convert from a VMCI error code to a VSock error code.
> */
>
> static s32 vmci_transport_error_to_vsock_error(s32 vmci_error)
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 02/14] vsock: remove vm_sockets_get_local_cid()
From: Jorgen Hansen @ 2019-10-30 14:55 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-3-sgarzare@redhat.com>
> -----Original Message-----
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> To: netdev@vger.kernel.org
> Subject: [PATCH net-next 02/14] vsock: remove vm_sockets_get_local_cid()
>
> vm_sockets_get_local_cid() is only used in virtio_transport_common.c.
> We can replace it calling the virtio_transport_get_ops() and using the
> get_local_cid() callback registered by the transport.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> include/linux/vm_sockets.h | 2 --
> net/vmw_vsock/af_vsock.c | 10 ----------
> net/vmw_vsock/virtio_transport_common.c | 2 +-
> 3 files changed, 1 insertion(+), 13 deletions(-)
>
> diff --git a/include/linux/vm_sockets.h b/include/linux/vm_sockets.h index
> 33f1a2ecd905..7dd899ccb920 100644
> --- a/include/linux/vm_sockets.h
> +++ b/include/linux/vm_sockets.h
> @@ -10,6 +10,4 @@
>
> #include <uapi/linux/vm_sockets.h>
>
> -int vm_sockets_get_local_cid(void);
> -
> #endif /* _VM_SOCKETS_H */
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> 2ab43b2bba31..2f2582fb7fdd 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -129,16 +129,6 @@ static struct proto vsock_proto = { static const struct
> vsock_transport *transport; static DEFINE_MUTEX(vsock_register_mutex);
>
> -/**** EXPORTS ****/
> -
> -/* Get the ID of the local context. This is transport dependent. */
> -
> -int vm_sockets_get_local_cid(void)
> -{
> - return transport->get_local_cid();
> -}
> -EXPORT_SYMBOL_GPL(vm_sockets_get_local_cid);
> -
> /**** UTILS ****/
>
> /* Each bound VSocket is stored in the bind hash table and each connected
> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index d02c9b41a768..b1cd16ed66ea 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -168,7 +168,7 @@ static int virtio_transport_send_pkt_info(struct
> vsock_sock *vsk,
> struct virtio_vsock_pkt *pkt;
> u32 pkt_len = info->pkt_len;
>
> - src_cid = vm_sockets_get_local_cid();
> + src_cid = virtio_transport_get_ops()->transport.get_local_cid();
> src_port = vsk->local_addr.svm_port;
> if (!info->remote_cid) {
> dst_cid = vsk->remote_addr.svm_cid;
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 03/14] vsock: remove include/linux/vm_sockets.h file
From: Jorgen Hansen @ 2019-10-30 14:57 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-4-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 03/14] vsock: remove include/linux/vm_sockets.h
> file
>
> This header file now only includes the "uapi/linux/vm_sockets.h".
> We can include directly it when needed.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> include/linux/vm_sockets.h | 13 -------------
> include/net/af_vsock.h | 2 +-
> include/net/vsock_addr.h | 2 +-
> net/vmw_vsock/vmci_transport_notify.h | 1 -
> 4 files changed, 2 insertions(+), 16 deletions(-) delete mode 100644
> include/linux/vm_sockets.h
>
> diff --git a/include/linux/vm_sockets.h b/include/linux/vm_sockets.h
> deleted file mode 100644 index 7dd899ccb920..000000000000
> --- a/include/linux/vm_sockets.h
> +++ /dev/null
> @@ -1,13 +0,0 @@
> -/* SPDX-License-Identifier: GPL-2.0-only */
> -/*
> - * VMware vSockets Driver
> - *
> - * Copyright (C) 2007-2013 VMware, Inc. All rights reserved.
> - */
> -
> -#ifndef _VM_SOCKETS_H
> -#define _VM_SOCKETS_H
> -
> -#include <uapi/linux/vm_sockets.h>
> -
> -#endif /* _VM_SOCKETS_H */
> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h index
> 80ea0f93d3f7..c660402b10f2 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -10,7 +10,7 @@
>
> #include <linux/kernel.h>
> #include <linux/workqueue.h>
> -#include <linux/vm_sockets.h>
> +#include <uapi/linux/vm_sockets.h>
>
> #include "vsock_addr.h"
>
> diff --git a/include/net/vsock_addr.h b/include/net/vsock_addr.h index
> 57d2db5c4bdf..cf8cc140d68d 100644
> --- a/include/net/vsock_addr.h
> +++ b/include/net/vsock_addr.h
> @@ -8,7 +8,7 @@
> #ifndef _VSOCK_ADDR_H_
> #define _VSOCK_ADDR_H_
>
> -#include <linux/vm_sockets.h>
> +#include <uapi/linux/vm_sockets.h>
>
> void vsock_addr_init(struct sockaddr_vm *addr, u32 cid, u32 port); int
> vsock_addr_validate(const struct sockaddr_vm *addr); diff --git
> a/net/vmw_vsock/vmci_transport_notify.h
> b/net/vmw_vsock/vmci_transport_notify.h
> index 7843f08d4290..a1aa5a998c0e 100644
> --- a/net/vmw_vsock/vmci_transport_notify.h
> +++ b/net/vmw_vsock/vmci_transport_notify.h
> @@ -11,7 +11,6 @@
> #include <linux/types.h>
> #include <linux/vmw_vmci_defs.h>
> #include <linux/vmw_vmci_api.h>
> -#include <linux/vm_sockets.h>
>
> #include "vmci_transport.h"
>
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 04/14] vsock: add 'transport' member in the struct vsock_sock
From: Jorgen Hansen @ 2019-10-30 14:57 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-5-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 04/14] vsock: add 'transport' member in the struct
> vsock_sock
>
> As a preparation to support multiple transports, this patch adds the
> 'transport' member at the 'struct vsock_sock'.
> This new field is initialized during the creation in the
> __vsock_create() function.
>
> This patch also renames the global 'transport' pointer to 'transport_single',
> since for now we're only supporting a single transport registered at run-time.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> include/net/af_vsock.h | 1 +
> net/vmw_vsock/af_vsock.c | 56 +++++++++++++++++++++++++++----------
> ---
> 2 files changed, 39 insertions(+), 18 deletions(-)
>
> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h index
> c660402b10f2..a5e1e134261d 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -27,6 +27,7 @@ extern spinlock_t vsock_table_lock; struct vsock_sock {
> /* sk must be the first member. */
> struct sock sk;
> + const struct vsock_transport *transport;
> struct sockaddr_vm local_addr;
> struct sockaddr_vm remote_addr;
> /* Links for the global tables of bound and connected sockets. */ diff
> --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> 2f2582fb7fdd..c3a14f853eb0 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -126,7 +126,7 @@ static struct proto vsock_proto = {
> */
> #define VSOCK_DEFAULT_CONNECT_TIMEOUT (2 * HZ)
>
> -static const struct vsock_transport *transport;
> +static const struct vsock_transport *transport_single;
> static DEFINE_MUTEX(vsock_register_mutex);
>
> /**** UTILS ****/
> @@ -408,7 +408,9 @@ static bool vsock_is_pending(struct sock *sk)
>
> static int vsock_send_shutdown(struct sock *sk, int mode) {
> - return transport->shutdown(vsock_sk(sk), mode);
> + struct vsock_sock *vsk = vsock_sk(sk);
> +
> + return vsk->transport->shutdown(vsk, mode);
> }
>
> static void vsock_pending_work(struct work_struct *work) @@ -518,7
> +520,7 @@ static int __vsock_bind_stream(struct vsock_sock *vsk, static int
> __vsock_bind_dgram(struct vsock_sock *vsk,
> struct sockaddr_vm *addr)
> {
> - return transport->dgram_bind(vsk, addr);
> + return vsk->transport->dgram_bind(vsk, addr);
> }
>
> static int __vsock_bind(struct sock *sk, struct sockaddr_vm *addr) @@ -
> 536,7 +538,7 @@ static int __vsock_bind(struct sock *sk, struct sockaddr_vm
> *addr)
> * like AF_INET prevents binding to a non-local IP address (in most
> * cases), we only allow binding to the local CID.
> */
> - cid = transport->get_local_cid();
> + cid = vsk->transport->get_local_cid();
> if (addr->svm_cid != cid && addr->svm_cid != VMADDR_CID_ANY)
> return -EADDRNOTAVAIL;
>
> @@ -586,6 +588,7 @@ struct sock *__vsock_create(struct net *net,
> sk->sk_type = type;
>
> vsk = vsock_sk(sk);
> + vsk->transport = transport_single;
> vsock_addr_init(&vsk->local_addr, VMADDR_CID_ANY,
> VMADDR_PORT_ANY);
> vsock_addr_init(&vsk->remote_addr, VMADDR_CID_ANY,
> VMADDR_PORT_ANY);
>
> @@ -616,7 +619,7 @@ struct sock *__vsock_create(struct net *net,
> vsk->connect_timeout =
> VSOCK_DEFAULT_CONNECT_TIMEOUT;
> }
>
> - if (transport->init(vsk, psk) < 0) {
> + if (vsk->transport->init(vsk, psk) < 0) {
> sk_free(sk);
> return NULL;
> }
> @@ -641,7 +644,7 @@ static void __vsock_release(struct sock *sk, int level)
> /* The release call is supposed to use lock_sock_nested()
> * rather than lock_sock(), if a sock lock should be acquired.
> */
> - transport->release(vsk);
> + vsk->transport->release(vsk);
>
> /* When "level" is SINGLE_DEPTH_NESTING, use the nested
> * version to avoid the warning "possible recursive locking
> @@ -670,7 +673,7 @@ static void vsock_sk_destruct(struct sock *sk) {
> struct vsock_sock *vsk = vsock_sk(sk);
>
> - transport->destruct(vsk);
> + vsk->transport->destruct(vsk);
>
> /* When clearing these addresses, there's no need to set the family
> and
> * possibly register the address family with the kernel.
> @@ -694,13 +697,13 @@ static int vsock_queue_rcv_skb(struct sock *sk,
> struct sk_buff *skb)
>
> s64 vsock_stream_has_data(struct vsock_sock *vsk) {
> - return transport->stream_has_data(vsk);
> + return vsk->transport->stream_has_data(vsk);
> }
> EXPORT_SYMBOL_GPL(vsock_stream_has_data);
>
> s64 vsock_stream_has_space(struct vsock_sock *vsk) {
> - return transport->stream_has_space(vsk);
> + return vsk->transport->stream_has_space(vsk);
> }
> EXPORT_SYMBOL_GPL(vsock_stream_has_space);
>
> @@ -869,6 +872,7 @@ static __poll_t vsock_poll(struct file *file, struct
> socket *sock,
> mask |= EPOLLOUT | EPOLLWRNORM |
> EPOLLWRBAND;
>
> } else if (sock->type == SOCK_STREAM) {
> + const struct vsock_transport *transport = vsk->transport;
> lock_sock(sk);
>
> /* Listening sockets that have connections in their accept
> @@ -944,6 +948,7 @@ static int vsock_dgram_sendmsg(struct socket *sock,
> struct msghdr *msg,
> struct sock *sk;
> struct vsock_sock *vsk;
> struct sockaddr_vm *remote_addr;
> + const struct vsock_transport *transport;
>
> if (msg->msg_flags & MSG_OOB)
> return -EOPNOTSUPP;
> @@ -952,6 +957,7 @@ static int vsock_dgram_sendmsg(struct socket *sock,
> struct msghdr *msg,
> err = 0;
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
>
> lock_sock(sk);
>
> @@ -1036,8 +1042,8 @@ static int vsock_dgram_connect(struct socket
> *sock,
> if (err)
> goto out;
>
> - if (!transport->dgram_allow(remote_addr->svm_cid,
> - remote_addr->svm_port)) {
> + if (!vsk->transport->dgram_allow(remote_addr->svm_cid,
> + remote_addr->svm_port)) {
> err = -EINVAL;
> goto out;
> }
> @@ -1053,7 +1059,9 @@ static int vsock_dgram_connect(struct socket
> *sock, static int vsock_dgram_recvmsg(struct socket *sock, struct msghdr
> *msg,
> size_t len, int flags)
> {
> - return transport->dgram_dequeue(vsock_sk(sock->sk), msg, len,
> flags);
> + struct vsock_sock *vsk = vsock_sk(sock->sk);
> +
> + return vsk->transport->dgram_dequeue(vsk, msg, len, flags);
> }
>
> static const struct proto_ops vsock_dgram_ops = { @@ -1079,6 +1087,8 @@
> static const struct proto_ops vsock_dgram_ops = {
>
> static int vsock_transport_cancel_pkt(struct vsock_sock *vsk) {
> + const struct vsock_transport *transport = vsk->transport;
> +
> if (!transport->cancel_pkt)
> return -EOPNOTSUPP;
>
> @@ -1115,6 +1125,7 @@ static int vsock_stream_connect(struct socket
> *sock, struct sockaddr *addr,
> int err;
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> struct sockaddr_vm *remote_addr;
> long timeout;
> DEFINE_WAIT(wait);
> @@ -1122,6 +1133,7 @@ static int vsock_stream_connect(struct socket
> *sock, struct sockaddr *addr,
> err = 0;
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
>
> lock_sock(sk);
>
> @@ -1365,6 +1377,7 @@ static int vsock_stream_setsockopt(struct socket
> *sock,
> int err;
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> u64 val;
>
> if (level != AF_VSOCK)
> @@ -1385,6 +1398,7 @@ static int vsock_stream_setsockopt(struct socket
> *sock,
> err = 0;
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
>
> lock_sock(sk);
>
> @@ -1442,6 +1456,7 @@ static int vsock_stream_getsockopt(struct socket
> *sock,
> int len;
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> u64 val;
>
> if (level != AF_VSOCK)
> @@ -1465,6 +1480,7 @@ static int vsock_stream_getsockopt(struct socket
> *sock,
> err = 0;
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
>
> switch (optname) {
> case SO_VM_SOCKETS_BUFFER_SIZE:
> @@ -1509,6 +1525,7 @@ static int vsock_stream_sendmsg(struct socket
> *sock, struct msghdr *msg, {
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> ssize_t total_written;
> long timeout;
> int err;
> @@ -1517,6 +1534,7 @@ static int vsock_stream_sendmsg(struct socket
> *sock, struct msghdr *msg,
>
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
> total_written = 0;
> err = 0;
>
> @@ -1648,6 +1666,7 @@ vsock_stream_recvmsg(struct socket *sock, struct
> msghdr *msg, size_t len, {
> struct sock *sk;
> struct vsock_sock *vsk;
> + const struct vsock_transport *transport;
> int err;
> size_t target;
> ssize_t copied;
> @@ -1658,6 +1677,7 @@ vsock_stream_recvmsg(struct socket *sock, struct
> msghdr *msg, size_t len,
>
> sk = sock->sk;
> vsk = vsock_sk(sk);
> + transport = vsk->transport;
> err = 0;
>
> lock_sock(sk);
> @@ -1872,7 +1892,7 @@ static long vsock_dev_do_ioctl(struct file *filp,
>
> switch (cmd) {
> case IOCTL_VM_SOCKETS_GET_LOCAL_CID:
> - if (put_user(transport->get_local_cid(), p) != 0)
> + if (put_user(transport_single->get_local_cid(), p) != 0)
> retval = -EFAULT;
> break;
>
> @@ -1919,7 +1939,7 @@ int __vsock_core_init(const struct vsock_transport
> *t, struct module *owner)
> if (err)
> return err;
>
> - if (transport) {
> + if (transport_single) {
> err = -EBUSY;
> goto err_busy;
> }
> @@ -1928,7 +1948,7 @@ int __vsock_core_init(const struct vsock_transport
> *t, struct module *owner)
> * unload while there are open sockets.
> */
> vsock_proto.owner = owner;
> - transport = t;
> + transport_single = t;
>
> vsock_device.minor = MISC_DYNAMIC_MINOR;
> err = misc_register(&vsock_device);
> @@ -1958,7 +1978,7 @@ int __vsock_core_init(const struct vsock_transport
> *t, struct module *owner)
> err_deregister_misc:
> misc_deregister(&vsock_device);
> err_reset_transport:
> - transport = NULL;
> + transport_single = NULL;
> err_busy:
> mutex_unlock(&vsock_register_mutex);
> return err;
> @@ -1975,7 +1995,7 @@ void vsock_core_exit(void)
>
> /* We do not want the assignment below re-ordered. */
> mb();
> - transport = NULL;
> + transport_single = NULL;
>
> mutex_unlock(&vsock_register_mutex);
> }
> @@ -1986,7 +2006,7 @@ const struct vsock_transport
> *vsock_core_get_transport(void)
> /* vsock_register_mutex not taken since only the transport uses this
> * function and only while registered.
> */
> - return transport;
> + return transport_single;
> }
> EXPORT_SYMBOL_GPL(vsock_core_get_transport);
>
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 06/14] vsock: add 'struct vsock_sock *' param to vsock_core_get_transport()
From: Jorgen Hansen @ 2019-10-30 15:01 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-7-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 06/14] vsock: add 'struct vsock_sock *' param to
> vsock_core_get_transport()
>
> Since now the 'struct vsock_sock' object contains a pointer to the transport,
> this patch adds a parameter to the
> vsock_core_get_transport() to return the right transport assigned to the
> socket.
>
> This patch modifies also the virtio_transport_get_ops(), that uses the
> vsock_core_get_transport(), adding the 'struct vsock_sock *' parameter.
>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> RFC -> v1:
> - Removed comment about protecting transport_single (Stefan)
> ---
> include/net/af_vsock.h | 2 +-
> net/vmw_vsock/af_vsock.c | 7 ++-----
> net/vmw_vsock/virtio_transport_common.c | 9 +++++----
> 3 files changed, 8 insertions(+), 10 deletions(-)
>
> diff --git a/include/net/af_vsock.h b/include/net/af_vsock.h index
> a5e1e134261d..2ca67d048de4 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -166,7 +166,7 @@ static inline int vsock_core_init(const struct
> vsock_transport *t) void vsock_core_exit(void);
>
> /* The transport may downcast this to access transport-specific functions */
> -const struct vsock_transport *vsock_core_get_transport(void);
> +const struct vsock_transport *vsock_core_get_transport(struct
> +vsock_sock *vsk);
>
> /**** UTILS ****/
>
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> c3a14f853eb0..eaea159006c8 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -2001,12 +2001,9 @@ void vsock_core_exit(void) }
> EXPORT_SYMBOL_GPL(vsock_core_exit);
>
> -const struct vsock_transport *vsock_core_get_transport(void)
> +const struct vsock_transport *vsock_core_get_transport(struct
> +vsock_sock *vsk)
> {
> - /* vsock_register_mutex not taken since only the transport uses this
> - * function and only while registered.
> - */
> - return transport_single;
> + return vsk->transport;
> }
> EXPORT_SYMBOL_GPL(vsock_core_get_transport);
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index 9763394f7a61..37a1c7e7c7fe 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -29,9 +29,10 @@
> /* Threshold for detecting small packets to copy */ #define
> GOOD_COPY_LEN 128
>
> -static const struct virtio_transport *virtio_transport_get_ops(void)
> +static const struct virtio_transport *
> +virtio_transport_get_ops(struct vsock_sock *vsk)
> {
> - const struct vsock_transport *t = vsock_core_get_transport();
> + const struct vsock_transport *t = vsock_core_get_transport(vsk);
>
> return container_of(t, struct virtio_transport, transport); } @@ -
> 168,7 +169,7 @@ static int virtio_transport_send_pkt_info(struct vsock_sock
> *vsk,
> struct virtio_vsock_pkt *pkt;
> u32 pkt_len = info->pkt_len;
>
> - src_cid = virtio_transport_get_ops()->transport.get_local_cid();
> + src_cid = virtio_transport_get_ops(vsk)->transport.get_local_cid();
> src_port = vsk->local_addr.svm_port;
> if (!info->remote_cid) {
> dst_cid = vsk->remote_addr.svm_cid;
> @@ -201,7 +202,7 @@ static int virtio_transport_send_pkt_info(struct
> vsock_sock *vsk,
>
> virtio_transport_inc_tx_pkt(vvs, pkt);
>
> - return virtio_transport_get_ops()->send_pkt(pkt);
> + return virtio_transport_get_ops(vsk)->send_pkt(pkt);
> }
>
> static bool virtio_transport_inc_rx_pkt(struct virtio_vsock_sock *vvs,
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 07/14] vsock: handle buffer_size sockopts in the core
From: Jorgen Hansen @ 2019-10-30 15:08 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-8-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 07/14] vsock: handle buffer_size sockopts in the
> core
>
> virtio_transport and vmci_transport handle the buffer_size sockopts in a
> very similar way.
>
> In order to support multiple transports, this patch moves this handling in the
> core to allow the user to change the options also if the socket is not yet
> assigned to any transport.
>
> This patch also adds the '.notify_buffer_size' callback in the 'struct
> virtio_transport' in order to inform the transport, when the buffer_size is
> changed by the user. It is also useful to limit the 'buffer_size' requested (e.g.
> virtio transports).
>
> Acked-by: Dexuan Cui <decui@microsoft.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> RFC -> v1:
> - changed .notify_buffer_size return to void (Stefan)
> - documented that .notify_buffer_size is called with sk_lock held (Stefan)
> ---
> drivers/vhost/vsock.c | 7 +-
> include/linux/virtio_vsock.h | 15 +----
> include/net/af_vsock.h | 15 ++---
> net/vmw_vsock/af_vsock.c | 43 ++++++++++---
> net/vmw_vsock/hyperv_transport.c | 36 -----------
> net/vmw_vsock/virtio_transport.c | 8 +--
> net/vmw_vsock/virtio_transport_common.c | 79 ++++-------------------
> net/vmw_vsock/vmci_transport.c | 86 +++----------------------
> net/vmw_vsock/vmci_transport.h | 3 -
> 9 files changed, 65 insertions(+), 227 deletions(-)
>
> diff --git a/drivers/vhost/vsock.c b/drivers/vhost/vsock.c index
> 92ab3852c954..6d7e4f022748 100644
> --- a/drivers/vhost/vsock.c
> +++ b/drivers/vhost/vsock.c
> @@ -418,13 +418,8 @@ static struct virtio_transport vhost_transport = {
> .notify_send_pre_block =
> virtio_transport_notify_send_pre_block,
> .notify_send_pre_enqueue =
> virtio_transport_notify_send_pre_enqueue,
> .notify_send_post_enqueue =
> virtio_transport_notify_send_post_enqueue,
> + .notify_buffer_size = virtio_transport_notify_buffer_size,
>
> - .set_buffer_size = virtio_transport_set_buffer_size,
> - .set_min_buffer_size =
> virtio_transport_set_min_buffer_size,
> - .set_max_buffer_size =
> virtio_transport_set_max_buffer_size,
> - .get_buffer_size = virtio_transport_get_buffer_size,
> - .get_min_buffer_size =
> virtio_transport_get_min_buffer_size,
> - .get_max_buffer_size =
> virtio_transport_get_max_buffer_size,
> },
>
> .send_pkt = vhost_transport_send_pkt,
> diff --git a/include/linux/virtio_vsock.h b/include/linux/virtio_vsock.h index
> 96d8132acbd7..b79befd2a5a4 100644
> --- a/include/linux/virtio_vsock.h
> +++ b/include/linux/virtio_vsock.h
> @@ -7,9 +7,6 @@
> #include <net/sock.h>
> #include <net/af_vsock.h>
>
> -#define VIRTIO_VSOCK_DEFAULT_MIN_BUF_SIZE 128
> -#define VIRTIO_VSOCK_DEFAULT_BUF_SIZE (1024 * 256)
> -#define VIRTIO_VSOCK_DEFAULT_MAX_BUF_SIZE (1024 * 256)
> #define VIRTIO_VSOCK_DEFAULT_RX_BUF_SIZE (1024 * 4)
> #define VIRTIO_VSOCK_MAX_BUF_SIZE 0xFFFFFFFFUL
> #define VIRTIO_VSOCK_MAX_PKT_BUF_SIZE (1024 * 64)
> @@ -25,11 +22,6 @@ enum {
> struct virtio_vsock_sock {
> struct vsock_sock *vsk;
>
> - /* Protected by lock_sock(sk_vsock(trans->vsk)) */
> - u32 buf_size;
> - u32 buf_size_min;
> - u32 buf_size_max;
> -
> spinlock_t tx_lock;
> spinlock_t rx_lock;
>
> @@ -93,12 +85,6 @@ s64 virtio_transport_stream_has_space(struct
> vsock_sock *vsk);
>
> int virtio_transport_do_socket_init(struct vsock_sock *vsk,
> struct vsock_sock *psk);
> -u64 virtio_transport_get_buffer_size(struct vsock_sock *vsk);
> -u64 virtio_transport_get_min_buffer_size(struct vsock_sock *vsk);
> -u64 virtio_transport_get_max_buffer_size(struct vsock_sock *vsk); -void
> virtio_transport_set_buffer_size(struct vsock_sock *vsk, u64 val); -void
> virtio_transport_set_min_buffer_size(struct vsock_sock *vsk, u64 val); -void
> virtio_transport_set_max_buffer_size(struct vsock_sock *vs, u64 val); int
> virtio_transport_notify_poll_in(struct vsock_sock *vsk,
> size_t target,
> @@ -125,6 +111,7 @@ int
> virtio_transport_notify_send_pre_enqueue(struct vsock_sock *vsk,
> struct vsock_transport_send_notify_data *data); int
> virtio_transport_notify_send_post_enqueue(struct vsock_sock *vsk,
> ssize_t written, struct vsock_transport_send_notify_data *data);
> +void virtio_transport_notify_buffer_size(struct vsock_sock *vsk, u64
> +*val);
>
> u64 virtio_transport_stream_rcvhiwat(struct vsock_sock *vsk); bool
> virtio_transport_stream_is_active(struct vsock_sock *vsk); diff --git
> a/include/net/af_vsock.h b/include/net/af_vsock.h index
> 2ca67d048de4..4b5d16840fd4 100644
> --- a/include/net/af_vsock.h
> +++ b/include/net/af_vsock.h
> @@ -65,6 +65,11 @@ struct vsock_sock {
> bool sent_request;
> bool ignore_connecting_rst;
>
> + /* Protected by lock_sock(sk) */
> + u64 buffer_size;
> + u64 buffer_min_size;
> + u64 buffer_max_size;
> +
> /* Private to transport. */
> void *trans;
> };
> @@ -140,18 +145,12 @@ struct vsock_transport {
> struct vsock_transport_send_notify_data *);
> int (*notify_send_post_enqueue)(struct vsock_sock *, ssize_t,
> struct vsock_transport_send_notify_data *);
> + /* sk_lock held by the caller */
> + void (*notify_buffer_size)(struct vsock_sock *, u64 *);
>
> /* Shutdown. */
> int (*shutdown)(struct vsock_sock *, int);
>
> - /* Buffer sizes. */
> - void (*set_buffer_size)(struct vsock_sock *, u64);
> - void (*set_min_buffer_size)(struct vsock_sock *, u64);
> - void (*set_max_buffer_size)(struct vsock_sock *, u64);
> - u64 (*get_buffer_size)(struct vsock_sock *);
> - u64 (*get_min_buffer_size)(struct vsock_sock *);
> - u64 (*get_max_buffer_size)(struct vsock_sock *);
> -
> /* Addressing. */
> u32 (*get_local_cid)(void);
> };
> diff --git a/net/vmw_vsock/af_vsock.c b/net/vmw_vsock/af_vsock.c index
> eaea159006c8..90ac46ea12ef 100644
> --- a/net/vmw_vsock/af_vsock.c
> +++ b/net/vmw_vsock/af_vsock.c
> @@ -126,6 +126,10 @@ static struct proto vsock_proto = {
> */
> #define VSOCK_DEFAULT_CONNECT_TIMEOUT (2 * HZ)
>
> +#define VSOCK_DEFAULT_BUFFER_SIZE (1024 * 256)
> +#define VSOCK_DEFAULT_BUFFER_MAX_SIZE (1024 * 256) #define
> +VSOCK_DEFAULT_BUFFER_MIN_SIZE 128
> +
> static const struct vsock_transport *transport_single; static
> DEFINE_MUTEX(vsock_register_mutex);
>
> @@ -613,10 +617,16 @@ struct sock *__vsock_create(struct net *net,
> vsk->trusted = psk->trusted;
> vsk->owner = get_cred(psk->owner);
> vsk->connect_timeout = psk->connect_timeout;
> + vsk->buffer_size = psk->buffer_size;
> + vsk->buffer_min_size = psk->buffer_min_size;
> + vsk->buffer_max_size = psk->buffer_max_size;
> } else {
> vsk->trusted = capable(CAP_NET_ADMIN);
> vsk->owner = get_current_cred();
> vsk->connect_timeout =
> VSOCK_DEFAULT_CONNECT_TIMEOUT;
> + vsk->buffer_size = VSOCK_DEFAULT_BUFFER_SIZE;
> + vsk->buffer_min_size =
> VSOCK_DEFAULT_BUFFER_MIN_SIZE;
> + vsk->buffer_max_size =
> VSOCK_DEFAULT_BUFFER_MAX_SIZE;
> }
>
> if (vsk->transport->init(vsk, psk) < 0) { @@ -1368,6 +1378,23 @@
> static int vsock_listen(struct socket *sock, int backlog)
> return err;
> }
>
> +static void vsock_update_buffer_size(struct vsock_sock *vsk,
> + const struct vsock_transport *transport,
> + u64 val)
> +{
> + if (val > vsk->buffer_max_size)
> + val = vsk->buffer_max_size;
> +
> + if (val < vsk->buffer_min_size)
> + val = vsk->buffer_min_size;
> +
> + if (val != vsk->buffer_size &&
> + transport && transport->notify_buffer_size)
> + transport->notify_buffer_size(vsk, &val);
> +
> + vsk->buffer_size = val;
> +}
> +
> static int vsock_stream_setsockopt(struct socket *sock,
> int level,
> int optname,
> @@ -1405,17 +1432,19 @@ static int vsock_stream_setsockopt(struct socket
> *sock,
> switch (optname) {
> case SO_VM_SOCKETS_BUFFER_SIZE:
> COPY_IN(val);
> - transport->set_buffer_size(vsk, val);
> + vsock_update_buffer_size(vsk, transport, val);
> break;
>
> case SO_VM_SOCKETS_BUFFER_MAX_SIZE:
> COPY_IN(val);
> - transport->set_max_buffer_size(vsk, val);
> + vsk->buffer_max_size = val;
> + vsock_update_buffer_size(vsk, transport, vsk->buffer_size);
> break;
>
> case SO_VM_SOCKETS_BUFFER_MIN_SIZE:
> COPY_IN(val);
> - transport->set_min_buffer_size(vsk, val);
> + vsk->buffer_min_size = val;
> + vsock_update_buffer_size(vsk, transport, vsk->buffer_size);
> break;
>
> case SO_VM_SOCKETS_CONNECT_TIMEOUT: {
> @@ -1456,7 +1485,6 @@ static int vsock_stream_getsockopt(struct socket
> *sock,
> int len;
> struct sock *sk;
> struct vsock_sock *vsk;
> - const struct vsock_transport *transport;
> u64 val;
>
> if (level != AF_VSOCK)
> @@ -1480,21 +1508,20 @@ static int vsock_stream_getsockopt(struct socket
> *sock,
> err = 0;
> sk = sock->sk;
> vsk = vsock_sk(sk);
> - transport = vsk->transport;
>
> switch (optname) {
> case SO_VM_SOCKETS_BUFFER_SIZE:
> - val = transport->get_buffer_size(vsk);
> + val = vsk->buffer_size;
> COPY_OUT(val);
> break;
>
> case SO_VM_SOCKETS_BUFFER_MAX_SIZE:
> - val = transport->get_max_buffer_size(vsk);
> + val = vsk->buffer_max_size;
> COPY_OUT(val);
> break;
>
> case SO_VM_SOCKETS_BUFFER_MIN_SIZE:
> - val = transport->get_min_buffer_size(vsk);
> + val = vsk->buffer_min_size;
> COPY_OUT(val);
> break;
>
> diff --git a/net/vmw_vsock/hyperv_transport.c
> b/net/vmw_vsock/hyperv_transport.c
> index bef8772116ec..d62297a62ca6 100644
> --- a/net/vmw_vsock/hyperv_transport.c
> +++ b/net/vmw_vsock/hyperv_transport.c
> @@ -845,36 +845,6 @@ int hvs_notify_send_post_enqueue(struct
> vsock_sock *vsk, ssize_t written,
> return 0;
> }
>
> -static void hvs_set_buffer_size(struct vsock_sock *vsk, u64 val) -{
> - /* Ignored. */
> -}
> -
> -static void hvs_set_min_buffer_size(struct vsock_sock *vsk, u64 val) -{
> - /* Ignored. */
> -}
> -
> -static void hvs_set_max_buffer_size(struct vsock_sock *vsk, u64 val) -{
> - /* Ignored. */
> -}
> -
> -static u64 hvs_get_buffer_size(struct vsock_sock *vsk) -{
> - return -ENOPROTOOPT;
> -}
> -
> -static u64 hvs_get_min_buffer_size(struct vsock_sock *vsk) -{
> - return -ENOPROTOOPT;
> -}
> -
> -static u64 hvs_get_max_buffer_size(struct vsock_sock *vsk) -{
> - return -ENOPROTOOPT;
> -}
> -
> static struct vsock_transport hvs_transport = {
> .get_local_cid = hvs_get_local_cid,
>
> @@ -908,12 +878,6 @@ static struct vsock_transport hvs_transport = {
> .notify_send_pre_enqueue = hvs_notify_send_pre_enqueue,
> .notify_send_post_enqueue = hvs_notify_send_post_enqueue,
>
> - .set_buffer_size = hvs_set_buffer_size,
> - .set_min_buffer_size = hvs_set_min_buffer_size,
> - .set_max_buffer_size = hvs_set_max_buffer_size,
> - .get_buffer_size = hvs_get_buffer_size,
> - .get_min_buffer_size = hvs_get_min_buffer_size,
> - .get_max_buffer_size = hvs_get_max_buffer_size,
> };
>
> static int hvs_probe(struct hv_device *hdev, diff --git
> a/net/vmw_vsock/virtio_transport.c b/net/vmw_vsock/virtio_transport.c
> index 3756f0857946..fb1fc7760e8c 100644
> --- a/net/vmw_vsock/virtio_transport.c
> +++ b/net/vmw_vsock/virtio_transport.c
> @@ -494,13 +494,7 @@ static struct virtio_transport virtio_transport = {
> .notify_send_pre_block =
> virtio_transport_notify_send_pre_block,
> .notify_send_pre_enqueue =
> virtio_transport_notify_send_pre_enqueue,
> .notify_send_post_enqueue =
> virtio_transport_notify_send_post_enqueue,
> -
> - .set_buffer_size = virtio_transport_set_buffer_size,
> - .set_min_buffer_size =
> virtio_transport_set_min_buffer_size,
> - .set_max_buffer_size =
> virtio_transport_set_max_buffer_size,
> - .get_buffer_size = virtio_transport_get_buffer_size,
> - .get_min_buffer_size =
> virtio_transport_get_min_buffer_size,
> - .get_max_buffer_size =
> virtio_transport_get_max_buffer_size,
> + .notify_buffer_size = virtio_transport_notify_buffer_size,
> },
>
> .send_pkt = virtio_transport_send_pkt, diff --git
> a/net/vmw_vsock/virtio_transport_common.c
> b/net/vmw_vsock/virtio_transport_common.c
> index 37a1c7e7c7fe..b2a310dfa158 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -456,17 +456,13 @@ int virtio_transport_do_socket_init(struct
> vsock_sock *vsk,
> if (psk) {
> struct virtio_vsock_sock *ptrans = psk->trans;
>
> - vvs->buf_size = ptrans->buf_size;
> - vvs->buf_size_min = ptrans->buf_size_min;
> - vvs->buf_size_max = ptrans->buf_size_max;
> vvs->peer_buf_alloc = ptrans->peer_buf_alloc;
> - } else {
> - vvs->buf_size = VIRTIO_VSOCK_DEFAULT_BUF_SIZE;
> - vvs->buf_size_min =
> VIRTIO_VSOCK_DEFAULT_MIN_BUF_SIZE;
> - vvs->buf_size_max =
> VIRTIO_VSOCK_DEFAULT_MAX_BUF_SIZE;
> }
>
> - vvs->buf_alloc = vvs->buf_size;
> + if (vsk->buffer_size > VIRTIO_VSOCK_MAX_BUF_SIZE)
> + vsk->buffer_size = VIRTIO_VSOCK_MAX_BUF_SIZE;
> +
> + vvs->buf_alloc = vsk->buffer_size;
>
> spin_lock_init(&vvs->rx_lock);
> spin_lock_init(&vvs->tx_lock);
> @@ -476,71 +472,20 @@ int virtio_transport_do_socket_init(struct
> vsock_sock *vsk, } EXPORT_SYMBOL_GPL(virtio_transport_do_socket_init);
>
> -u64 virtio_transport_get_buffer_size(struct vsock_sock *vsk) -{
> - struct virtio_vsock_sock *vvs = vsk->trans;
> -
> - return vvs->buf_size;
> -}
> -EXPORT_SYMBOL_GPL(virtio_transport_get_buffer_size);
> -
> -u64 virtio_transport_get_min_buffer_size(struct vsock_sock *vsk)
> +/* sk_lock held by the caller */
> +void virtio_transport_notify_buffer_size(struct vsock_sock *vsk, u64
> +*val)
> {
> struct virtio_vsock_sock *vvs = vsk->trans;
>
> - return vvs->buf_size_min;
> -}
> -EXPORT_SYMBOL_GPL(virtio_transport_get_min_buffer_size);
> -
> -u64 virtio_transport_get_max_buffer_size(struct vsock_sock *vsk) -{
> - struct virtio_vsock_sock *vvs = vsk->trans;
> -
> - return vvs->buf_size_max;
> -}
> -EXPORT_SYMBOL_GPL(virtio_transport_get_max_buffer_size);
> -
> -void virtio_transport_set_buffer_size(struct vsock_sock *vsk, u64 val) -{
> - struct virtio_vsock_sock *vvs = vsk->trans;
> + if (*val > VIRTIO_VSOCK_MAX_BUF_SIZE)
> + *val = VIRTIO_VSOCK_MAX_BUF_SIZE;
>
> - if (val > VIRTIO_VSOCK_MAX_BUF_SIZE)
> - val = VIRTIO_VSOCK_MAX_BUF_SIZE;
> - if (val < vvs->buf_size_min)
> - vvs->buf_size_min = val;
> - if (val > vvs->buf_size_max)
> - vvs->buf_size_max = val;
> - vvs->buf_size = val;
> - vvs->buf_alloc = val;
> + vvs->buf_alloc = *val;
>
> virtio_transport_send_credit_update(vsk,
> VIRTIO_VSOCK_TYPE_STREAM,
> NULL);
> }
> -EXPORT_SYMBOL_GPL(virtio_transport_set_buffer_size);
> -
> -void virtio_transport_set_min_buffer_size(struct vsock_sock *vsk, u64 val)
> -{
> - struct virtio_vsock_sock *vvs = vsk->trans;
> -
> - if (val > VIRTIO_VSOCK_MAX_BUF_SIZE)
> - val = VIRTIO_VSOCK_MAX_BUF_SIZE;
> - if (val > vvs->buf_size)
> - vvs->buf_size = val;
> - vvs->buf_size_min = val;
> -}
> -EXPORT_SYMBOL_GPL(virtio_transport_set_min_buffer_size);
> -
> -void virtio_transport_set_max_buffer_size(struct vsock_sock *vsk, u64 val)
> -{
> - struct virtio_vsock_sock *vvs = vsk->trans;
> -
> - if (val > VIRTIO_VSOCK_MAX_BUF_SIZE)
> - val = VIRTIO_VSOCK_MAX_BUF_SIZE;
> - if (val < vvs->buf_size)
> - vvs->buf_size = val;
> - vvs->buf_size_max = val;
> -}
> -EXPORT_SYMBOL_GPL(virtio_transport_set_max_buffer_size);
> +EXPORT_SYMBOL_GPL(virtio_transport_notify_buffer_size);
>
> int
> virtio_transport_notify_poll_in(struct vsock_sock *vsk, @@ -632,9 +577,7
> @@ EXPORT_SYMBOL_GPL(virtio_transport_notify_send_post_enqueue);
>
> u64 virtio_transport_stream_rcvhiwat(struct vsock_sock *vsk) {
> - struct virtio_vsock_sock *vvs = vsk->trans;
> -
> - return vvs->buf_size;
> + return vsk->buffer_size;
> }
> EXPORT_SYMBOL_GPL(virtio_transport_stream_rcvhiwat);
While the VMCI transport uses a transport local consumer_size for stream_rcvhiwat,
that consumer_size is always the same as buffer_size (a vmci queue pair allows the
producer and consumer queues to be of different sizes, but vsock doesn't use that).
So we could move the stream_rcvhiwat code to the common code as well, and just
use buffer_size, if that simplifies things.
> diff --git a/net/vmw_vsock/vmci_transport.c
> b/net/vmw_vsock/vmci_transport.c index f8e3131ac480..8290d37b6587
> 100644
> --- a/net/vmw_vsock/vmci_transport.c
> +++ b/net/vmw_vsock/vmci_transport.c
> @@ -74,10 +74,6 @@ static u32 vmci_transport_qp_resumed_sub_id =
> VMCI_INVALID_ID;
>
> static int PROTOCOL_OVERRIDE = -1;
>
> -#define VMCI_TRANSPORT_DEFAULT_QP_SIZE_MIN 128
> -#define VMCI_TRANSPORT_DEFAULT_QP_SIZE 262144
> -#define VMCI_TRANSPORT_DEFAULT_QP_SIZE_MAX 262144
> -
> /* Helper function to convert from a VMCI error code to a VSock error code.
> */
>
> static s32 vmci_transport_error_to_vsock_error(s32 vmci_error) @@ -
> 1025,11 +1021,11 @@ static int vmci_transport_recv_listen(struct sock *sk,
> /* If the proposed size fits within our min/max, accept it. Otherwise
> * propose our own size.
> */
> - if (pkt->u.size >= vmci_trans(vpending)->queue_pair_min_size &&
> - pkt->u.size <= vmci_trans(vpending)->queue_pair_max_size) {
> + if (pkt->u.size >= vpending->buffer_min_size &&
> + pkt->u.size <= vpending->buffer_max_size) {
> qp_size = pkt->u.size;
> } else {
> - qp_size = vmci_trans(vpending)->queue_pair_size;
> + qp_size = vpending->buffer_size;
> }
>
> /* Figure out if we are using old or new requests based on the @@ -
> 1098,7 +1094,7 @@ static int vmci_transport_recv_listen(struct sock *sk,
> pending->sk_state = TCP_SYN_SENT;
> vmci_trans(vpending)->produce_size =
> vmci_trans(vpending)->consume_size = qp_size;
> - vmci_trans(vpending)->queue_pair_size = qp_size;
> + vpending->buffer_size = qp_size;
>
> vmci_trans(vpending)->notify_ops->process_request(pending);
>
> @@ -1392,8 +1388,8 @@ static int
> vmci_transport_recv_connecting_client_negotiate(
> vsk->ignore_connecting_rst = false;
>
> /* Verify that we're OK with the proposed queue pair size */
> - if (pkt->u.size < vmci_trans(vsk)->queue_pair_min_size ||
> - pkt->u.size > vmci_trans(vsk)->queue_pair_max_size) {
> + if (pkt->u.size < vsk->buffer_min_size ||
> + pkt->u.size > vsk->buffer_max_size) {
> err = -EINVAL;
> goto destroy;
> }
> @@ -1498,8 +1494,7 @@
> vmci_transport_recv_connecting_client_invalid(struct sock *sk,
> vsk->sent_request = false;
> vsk->ignore_connecting_rst = true;
>
> - err = vmci_transport_send_conn_request(
> - sk, vmci_trans(vsk)->queue_pair_size);
> + err = vmci_transport_send_conn_request(sk, vsk-
> >buffer_size);
> if (err < 0)
> err = vmci_transport_error_to_vsock_error(err);
> else
> @@ -1583,21 +1578,6 @@ static int vmci_transport_socket_init(struct
> vsock_sock *vsk,
> INIT_LIST_HEAD(&vmci_trans(vsk)->elem);
> vmci_trans(vsk)->sk = &vsk->sk;
> spin_lock_init(&vmci_trans(vsk)->lock);
> - if (psk) {
> - vmci_trans(vsk)->queue_pair_size =
> - vmci_trans(psk)->queue_pair_size;
> - vmci_trans(vsk)->queue_pair_min_size =
> - vmci_trans(psk)->queue_pair_min_size;
> - vmci_trans(vsk)->queue_pair_max_size =
> - vmci_trans(psk)->queue_pair_max_size;
> - } else {
> - vmci_trans(vsk)->queue_pair_size =
> - VMCI_TRANSPORT_DEFAULT_QP_SIZE;
> - vmci_trans(vsk)->queue_pair_min_size =
> - VMCI_TRANSPORT_DEFAULT_QP_SIZE_MIN;
> - vmci_trans(vsk)->queue_pair_max_size =
> - VMCI_TRANSPORT_DEFAULT_QP_SIZE_MAX;
> - }
>
> return 0;
> }
> @@ -1813,8 +1793,7 @@ static int vmci_transport_connect(struct
> vsock_sock *vsk)
>
> if (vmci_transport_old_proto_override(&old_pkt_proto) &&
> old_pkt_proto) {
> - err = vmci_transport_send_conn_request(
> - sk, vmci_trans(vsk)->queue_pair_size);
> + err = vmci_transport_send_conn_request(sk, vsk-
> >buffer_size);
> if (err < 0) {
> sk->sk_state = TCP_CLOSE;
> return err;
> @@ -1822,8 +1801,7 @@ static int vmci_transport_connect(struct
> vsock_sock *vsk)
> } else {
> int supported_proto_versions =
> vmci_transport_new_proto_supported_versions();
> - err = vmci_transport_send_conn_request2(
> - sk, vmci_trans(vsk)->queue_pair_size,
> + err = vmci_transport_send_conn_request2(sk, vsk-
> >buffer_size,
> supported_proto_versions);
> if (err < 0) {
> sk->sk_state = TCP_CLOSE;
> @@ -1876,46 +1854,6 @@ static bool
> vmci_transport_stream_is_active(struct vsock_sock *vsk)
> return !vmci_handle_is_invalid(vmci_trans(vsk)->qp_handle);
> }
>
> -static u64 vmci_transport_get_buffer_size(struct vsock_sock *vsk) -{
> - return vmci_trans(vsk)->queue_pair_size;
> -}
> -
> -static u64 vmci_transport_get_min_buffer_size(struct vsock_sock *vsk) -{
> - return vmci_trans(vsk)->queue_pair_min_size;
> -}
> -
> -static u64 vmci_transport_get_max_buffer_size(struct vsock_sock *vsk) -{
> - return vmci_trans(vsk)->queue_pair_max_size;
> -}
> -
> -static void vmci_transport_set_buffer_size(struct vsock_sock *vsk, u64 val)
> -{
> - if (val < vmci_trans(vsk)->queue_pair_min_size)
> - vmci_trans(vsk)->queue_pair_min_size = val;
> - if (val > vmci_trans(vsk)->queue_pair_max_size)
> - vmci_trans(vsk)->queue_pair_max_size = val;
> - vmci_trans(vsk)->queue_pair_size = val;
> -}
> -
> -static void vmci_transport_set_min_buffer_size(struct vsock_sock *vsk,
> - u64 val)
> -{
> - if (val > vmci_trans(vsk)->queue_pair_size)
> - vmci_trans(vsk)->queue_pair_size = val;
> - vmci_trans(vsk)->queue_pair_min_size = val;
> -}
> -
> -static void vmci_transport_set_max_buffer_size(struct vsock_sock *vsk,
> - u64 val)
> -{
> - if (val < vmci_trans(vsk)->queue_pair_size)
> - vmci_trans(vsk)->queue_pair_size = val;
> - vmci_trans(vsk)->queue_pair_max_size = val;
> -}
> -
> static int vmci_transport_notify_poll_in(
> struct vsock_sock *vsk,
> size_t target,
> @@ -2098,12 +2036,6 @@ static const struct vsock_transport vmci_transport
> = {
> .notify_send_pre_enqueue =
> vmci_transport_notify_send_pre_enqueue,
> .notify_send_post_enqueue =
> vmci_transport_notify_send_post_enqueue,
> .shutdown = vmci_transport_shutdown,
> - .set_buffer_size = vmci_transport_set_buffer_size,
> - .set_min_buffer_size = vmci_transport_set_min_buffer_size,
> - .set_max_buffer_size = vmci_transport_set_max_buffer_size,
> - .get_buffer_size = vmci_transport_get_buffer_size,
> - .get_min_buffer_size = vmci_transport_get_min_buffer_size,
> - .get_max_buffer_size = vmci_transport_get_max_buffer_size,
> .get_local_cid = vmci_transport_get_local_cid, };
>
> diff --git a/net/vmw_vsock/vmci_transport.h
> b/net/vmw_vsock/vmci_transport.h index 1ca1e8640b31..b7b072194282
> 100644
> --- a/net/vmw_vsock/vmci_transport.h
> +++ b/net/vmw_vsock/vmci_transport.h
> @@ -108,9 +108,6 @@ struct vmci_transport {
> struct vmci_qp *qpair;
> u64 produce_size;
> u64 consume_size;
> - u64 queue_pair_size;
> - u64 queue_pair_min_size;
> - u64 queue_pair_max_size;
> u32 detach_sub_id;
> union vmci_transport_notify notify;
> const struct vmci_transport_notify_ops *notify_ops;
> --
> 2.21.0
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 08/14] vsock: add vsock_create_connected() called by transports
From: Jorgen Hansen @ 2019-10-30 15:12 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-9-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 08/14] vsock: add vsock_create_connected() called
> by transports
>
> All transports call __vsock_create() with the same parameters,
> most of them depending on the parent socket. In order to simplify
> the VSOCK core APIs exposed to the transports, this patch adds
> the vsock_create_connected() callable from transports to create
> a new socket when a connection request is received.
> We also unexported the __vsock_create().
>
> Suggested-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> include/net/af_vsock.h | 5 +----
> net/vmw_vsock/af_vsock.c | 20 +++++++++++++-------
> net/vmw_vsock/hyperv_transport.c | 3 +--
> net/vmw_vsock/virtio_transport_common.c | 3 +--
> net/vmw_vsock/vmci_transport.c | 3 +--
> 5 files changed, 17 insertions(+), 17 deletions(-)
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* RE: [PATCH net-next 09/14] vsock: move vsock_insert_unbound() in the vsock_create()
From: Jorgen Hansen @ 2019-10-30 15:12 UTC (permalink / raw)
To: 'Stefano Garzarella'
Cc: Michael S. Tsirkin, kvm@vger.kernel.org, Greg Kroah-Hartman,
Jason Wang, David S. Miller, Dexuan Cui, Haiyang Zhang,
Sasha Levin, linux-kernel@vger.kernel.org, Arnd Bergmann,
Stefan Hajnoczi, linux-hyperv@vger.kernel.org, K. Y. Srinivasan,
Stephen Hemminger, virtualization@lists.linux-foundation.org,
netdev@vger.kernel.org
In-Reply-To: <20191023095554.11340-10-sgarzare@redhat.com>
> From: Stefano Garzarella [mailto:sgarzare@redhat.com]
> Sent: Wednesday, October 23, 2019 11:56 AM
> Subject: [PATCH net-next 09/14] vsock: move vsock_insert_unbound() in the
> vsock_create()
>
> vsock_insert_unbound() was called only when 'sock' parameter of
> __vsock_create() was not null. This only happened when
> __vsock_create() was called by vsock_create().
>
> In order to simplify the multi-transports support, this patch moves
> vsock_insert_unbound() at the end of vsock_create().
>
> Reviewed-by: Dexuan Cui <decui@microsoft.com>
> Reviewed-by: Stefan Hajnoczi <stefanha@redhat.com>
> Signed-off-by: Stefano Garzarella <sgarzare@redhat.com>
> ---
> net/vmw_vsock/af_vsock.c | 13 +++++++++----
Reviewed-by: Jorgen Hansen <jhansen@vmware.com>
^ permalink raw reply
* [PATCH net, 1/2] hv_netvsc: Fix error handling in netvsc_set_features()
From: Haiyang Zhang @ 2019-10-30 15:32 UTC (permalink / raw)
To: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org
Cc: Haiyang Zhang, KY Srinivasan, Stephen Hemminger, olaf@aepfle.de,
vkuznets, davem@davemloft.net, linux-kernel@vger.kernel.org
In-Reply-To: <1572449471-5219-1-git-send-email-haiyangz@microsoft.com>
When an error is returned by rndis_filter_set_offload_params(), we should
still assign the unaffected features to ndev->features. Otherwise, these
features will be missing.
Fixes: d6792a5a0747 ("hv_netvsc: Add handler for LRO setting change")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 39dddcd..734e411 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -1807,8 +1807,10 @@ static int netvsc_set_features(struct net_device *ndev,
ret = rndis_filter_set_offload_params(ndev, nvdev, &offloads);
- if (ret)
+ if (ret) {
features ^= NETIF_F_LRO;
+ ndev->features = features;
+ }
syncvf:
if (!vf_netdev)
--
1.8.3.1
^ permalink raw reply related
* [PATCH net, 2/2] hv_netvsc: Fix error handling in netvsc_attach()
From: Haiyang Zhang @ 2019-10-30 15:32 UTC (permalink / raw)
To: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org
Cc: Haiyang Zhang, KY Srinivasan, Stephen Hemminger, olaf@aepfle.de,
vkuznets, davem@davemloft.net, linux-kernel@vger.kernel.org
In-Reply-To: <1572449471-5219-1-git-send-email-haiyangz@microsoft.com>
If rndis_filter_open() fails, we need to remove the rndis device created
in earlier steps, before returning an error code. Otherwise, the retry of
netvsc_attach() from its callers will fail and hang.
Fixes: 7b2ee50c0cd5 ("hv_netvsc: common detach logic")
Signed-off-by: Haiyang Zhang <haiyangz@microsoft.com>
---
drivers/net/hyperv/netvsc_drv.c | 9 ++++++++-
1 file changed, 8 insertions(+), 1 deletion(-)
diff --git a/drivers/net/hyperv/netvsc_drv.c b/drivers/net/hyperv/netvsc_drv.c
index 734e411..a14fc8e 100644
--- a/drivers/net/hyperv/netvsc_drv.c
+++ b/drivers/net/hyperv/netvsc_drv.c
@@ -982,7 +982,7 @@ static int netvsc_attach(struct net_device *ndev,
if (netif_running(ndev)) {
ret = rndis_filter_open(nvdev);
if (ret)
- return ret;
+ goto err;
rdev = nvdev->extension;
if (!rdev->link_state)
@@ -990,6 +990,13 @@ static int netvsc_attach(struct net_device *ndev,
}
return 0;
+
+err:
+ netif_device_detach(ndev);
+
+ rndis_filter_device_remove(hdev, nvdev);
+
+ return ret;
}
static int netvsc_set_channels(struct net_device *net,
--
1.8.3.1
^ permalink raw reply related
* [PATCH net, 0/2] hv_netvsc: fix error handling in netvsc_attach/set_features
From: Haiyang Zhang @ 2019-10-30 15:32 UTC (permalink / raw)
To: sashal@kernel.org, linux-hyperv@vger.kernel.org,
netdev@vger.kernel.org
Cc: Haiyang Zhang, KY Srinivasan, Stephen Hemminger, olaf@aepfle.de,
vkuznets, davem@davemloft.net, linux-kernel@vger.kernel.org
The error handling code path in these functions are not correct.
This patch set fixes them.
Haiyang Zhang (2):
hv_netvsc: Fix error handling in netvsc_set_features()
hv_netvsc: Fix error handling in netvsc_attach()
drivers/net/hyperv/netvsc_drv.c | 13 +++++++++++--
1 file changed, 11 insertions(+), 2 deletions(-)
--
1.8.3.1
^ 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