* Re: Strange routing with VRF and 5.2.7+
From: David Ahern @ 2019-09-22 19:23 UTC (permalink / raw)
To: Ben Greear, netdev
In-Reply-To: <7d1de949-5cf0-cb74-6ca3-52315c34a340@candelatech.com>
On 9/20/19 9:57 AM, Ben Greear wrote:
> On 9/10/19 6:08 PM, Ben Greear wrote:
>> On 9/10/19 3:17 PM, Ben Greear wrote:
>>> Today we were testing creating 200 virtual station vdevs on ath9k,
>>> and using
>>> VRF for the routing.
>>
>> Looks like the same issue happens w/out VRF, but there I have oodles
>> of routing
>> rules, so it is an area ripe for failure.
>>
>> Will upgrade to 5.2.14+ and retest, and try 4.20 as well....
>
> Turns out, this was ipsec (strongswan) inserting a rule that pointed to
> a table
> that we then used for a vrf w/out realizing the rule was added.
>
> Stopping strongswan and/or reconfiguring how routing tables are assigned
> resolved the issue.
>
Hi Ben:
Since you are the pioneer with vrf and ipsec, can you add an ipsec
section with some notes to Documentation/networking/vrf.txt?
^ permalink raw reply
* [PATCH AUTOSEL 5.2 079/185] net: lpc-enet: fix printk format strings
From: Sasha Levin @ 2019-09-22 18:47 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Arnd Bergmann, kbuild test robot, Sasha Levin, netdev
In-Reply-To: <20190922184924.32534-1-sashal@kernel.org>
From: Arnd Bergmann <arnd@arndb.de>
[ Upstream commit de6f97b2bace0e2eb6c3a86e124d1e652a587b56 ]
compile-testing this driver on other architectures showed
multiple warnings:
drivers/net/ethernet/nxp/lpc_eth.c: In function 'lpc_eth_drv_probe':
drivers/net/ethernet/nxp/lpc_eth.c:1337:19: warning: format '%d' expects argument of type 'int', but argument 4 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=]
drivers/net/ethernet/nxp/lpc_eth.c:1342:19: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'dma_addr_t {aka long long unsigned int}' [-Wformat=]
Use format strings that work on all architectures.
Link: https://lore.kernel.org/r/20190809144043.476786-10-arnd@arndb.de
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/net/ethernet/nxp/lpc_eth.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/nxp/lpc_eth.c b/drivers/net/ethernet/nxp/lpc_eth.c
index f7e11f1b0426c..b0c8be127bee1 100644
--- a/drivers/net/ethernet/nxp/lpc_eth.c
+++ b/drivers/net/ethernet/nxp/lpc_eth.c
@@ -1344,13 +1344,14 @@ static int lpc_eth_drv_probe(struct platform_device *pdev)
pldat->dma_buff_base_p = dma_handle;
netdev_dbg(ndev, "IO address space :%pR\n", res);
- netdev_dbg(ndev, "IO address size :%d\n", resource_size(res));
+ netdev_dbg(ndev, "IO address size :%zd\n",
+ (size_t)resource_size(res));
netdev_dbg(ndev, "IO address (mapped) :0x%p\n",
pldat->net_base);
netdev_dbg(ndev, "IRQ number :%d\n", ndev->irq);
- netdev_dbg(ndev, "DMA buffer size :%d\n", pldat->dma_buff_size);
- netdev_dbg(ndev, "DMA buffer P address :0x%08x\n",
- pldat->dma_buff_base_p);
+ netdev_dbg(ndev, "DMA buffer size :%zd\n", pldat->dma_buff_size);
+ netdev_dbg(ndev, "DMA buffer P address :%pad\n",
+ &pldat->dma_buff_base_p);
netdev_dbg(ndev, "DMA buffer V address :0x%p\n",
pldat->dma_buff_base_v);
@@ -1397,8 +1398,8 @@ static int lpc_eth_drv_probe(struct platform_device *pdev)
if (ret)
goto err_out_unregister_netdev;
- netdev_info(ndev, "LPC mac at 0x%08x irq %d\n",
- res->start, ndev->irq);
+ netdev_info(ndev, "LPC mac at 0x%08lx irq %d\n",
+ (unsigned long)res->start, ndev->irq);
device_init_wakeup(dev, 1);
device_set_wakeup_enable(dev, 0);
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.3 198/203] zd1211rw: remove false assertion from zd_mac_clear()
From: Sasha Levin @ 2019-09-22 18:43 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Oliver Neukum, syzbot+74c65761783d66a9c97c, Kalle Valo,
Sasha Levin, linux-wireless, netdev
In-Reply-To: <20190922184350.30563-1-sashal@kernel.org>
From: Oliver Neukum <oneukum@suse.com>
[ Upstream commit 7a2eb7367fdea72e448d1a847aa857f6caf8ea2f ]
The function is called before the lock which is asserted was ever used.
Just remove it.
Reported-by: syzbot+74c65761783d66a9c97c@syzkaller.appspotmail.com
Signed-off-by: Oliver Neukum <oneukum@suse.com>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/net/wireless/zydas/zd1211rw/zd_mac.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/wireless/zydas/zd1211rw/zd_mac.c b/drivers/net/wireless/zydas/zd1211rw/zd_mac.c
index da7e63fca9f57..a9999d10ae81f 100644
--- a/drivers/net/wireless/zydas/zd1211rw/zd_mac.c
+++ b/drivers/net/wireless/zydas/zd1211rw/zd_mac.c
@@ -223,7 +223,6 @@ void zd_mac_clear(struct zd_mac *mac)
{
flush_workqueue(zd_workqueue);
zd_chip_clear(&mac->chip);
- lockdep_assert_held(&mac->lock);
ZD_MEMCLEAR(mac, sizeof(struct zd_mac));
}
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.3 189/203] libertas: Add missing sentinel at end of if_usb.c fw_table
From: Sasha Levin @ 2019-09-22 18:43 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Kevin Easton, syzbot+98156c174c5a2cad9f8f, Kalle Valo,
Sasha Levin, libertas-dev, linux-wireless, netdev
In-Reply-To: <20190922184350.30563-1-sashal@kernel.org>
From: Kevin Easton <kevin@guarana.org>
[ Upstream commit 764f3f1ecffc434096e0a2b02f1a6cc964a89df6 ]
This sentinel tells the firmware loading process when to stop.
Reported-and-tested-by: syzbot+98156c174c5a2cad9f8f@syzkaller.appspotmail.com
Signed-off-by: Kevin Easton <kevin@guarana.org>
Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/net/wireless/marvell/libertas/if_usb.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/marvell/libertas/if_usb.c b/drivers/net/wireless/marvell/libertas/if_usb.c
index afac2481909b6..20436a289d5cd 100644
--- a/drivers/net/wireless/marvell/libertas/if_usb.c
+++ b/drivers/net/wireless/marvell/libertas/if_usb.c
@@ -50,7 +50,8 @@ static const struct lbs_fw_table fw_table[] = {
{ MODEL_8388, "libertas/usb8388_v5.bin", NULL },
{ MODEL_8388, "libertas/usb8388.bin", NULL },
{ MODEL_8388, "usb8388.bin", NULL },
- { MODEL_8682, "libertas/usb8682.bin", NULL }
+ { MODEL_8682, "libertas/usb8682.bin", NULL },
+ { 0, NULL, NULL }
};
static const struct usb_device_id if_usb_table[] = {
--
2.20.1
^ permalink raw reply related
* [PATCH AUTOSEL 5.3 088/203] net: lpc-enet: fix printk format strings
From: Sasha Levin @ 2019-09-22 18:41 UTC (permalink / raw)
To: linux-kernel, stable
Cc: Arnd Bergmann, kbuild test robot, Sasha Levin, netdev
In-Reply-To: <20190922184350.30563-1-sashal@kernel.org>
From: Arnd Bergmann <arnd@arndb.de>
[ Upstream commit de6f97b2bace0e2eb6c3a86e124d1e652a587b56 ]
compile-testing this driver on other architectures showed
multiple warnings:
drivers/net/ethernet/nxp/lpc_eth.c: In function 'lpc_eth_drv_probe':
drivers/net/ethernet/nxp/lpc_eth.c:1337:19: warning: format '%d' expects argument of type 'int', but argument 4 has type 'resource_size_t {aka long long unsigned int}' [-Wformat=]
drivers/net/ethernet/nxp/lpc_eth.c:1342:19: warning: format '%x' expects argument of type 'unsigned int', but argument 4 has type 'dma_addr_t {aka long long unsigned int}' [-Wformat=]
Use format strings that work on all architectures.
Link: https://lore.kernel.org/r/20190809144043.476786-10-arnd@arndb.de
Reported-by: kbuild test robot <lkp@intel.com>
Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
drivers/net/ethernet/nxp/lpc_eth.c | 13 +++++++------
1 file changed, 7 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/nxp/lpc_eth.c b/drivers/net/ethernet/nxp/lpc_eth.c
index f7e11f1b0426c..b0c8be127bee1 100644
--- a/drivers/net/ethernet/nxp/lpc_eth.c
+++ b/drivers/net/ethernet/nxp/lpc_eth.c
@@ -1344,13 +1344,14 @@ static int lpc_eth_drv_probe(struct platform_device *pdev)
pldat->dma_buff_base_p = dma_handle;
netdev_dbg(ndev, "IO address space :%pR\n", res);
- netdev_dbg(ndev, "IO address size :%d\n", resource_size(res));
+ netdev_dbg(ndev, "IO address size :%zd\n",
+ (size_t)resource_size(res));
netdev_dbg(ndev, "IO address (mapped) :0x%p\n",
pldat->net_base);
netdev_dbg(ndev, "IRQ number :%d\n", ndev->irq);
- netdev_dbg(ndev, "DMA buffer size :%d\n", pldat->dma_buff_size);
- netdev_dbg(ndev, "DMA buffer P address :0x%08x\n",
- pldat->dma_buff_base_p);
+ netdev_dbg(ndev, "DMA buffer size :%zd\n", pldat->dma_buff_size);
+ netdev_dbg(ndev, "DMA buffer P address :%pad\n",
+ &pldat->dma_buff_base_p);
netdev_dbg(ndev, "DMA buffer V address :0x%p\n",
pldat->dma_buff_base_v);
@@ -1397,8 +1398,8 @@ static int lpc_eth_drv_probe(struct platform_device *pdev)
if (ret)
goto err_out_unregister_netdev;
- netdev_info(ndev, "LPC mac at 0x%08x irq %d\n",
- res->start, ndev->irq);
+ netdev_info(ndev, "LPC mac at 0x%08lx irq %d\n",
+ (unsigned long)res->start, ndev->irq);
device_init_wakeup(dev, 1);
device_set_wakeup_enable(dev, 0);
--
2.20.1
^ permalink raw reply related
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Michael S. Tsirkin @ 2019-09-22 20:35 UTC (permalink / raw)
To: Matt Cover
Cc: davem, ast, daniel, kafai, songliubraving, yhs, Jason Wang,
Eric Dumazet, Stanislav Fomichev, Matthew Cover, mail, pabeni,
Nicolas Dichtel, wangli39, lifei.shirley, tglx, netdev,
linux-kernel, bpf
In-Reply-To: <CAGyo_hqGbFdt1PoDrmo=S5iTO8TwbrbtOJtbvGT1WrFFMLwk-Q@mail.gmail.com>
On Sun, Sep 22, 2019 at 10:43:19AM -0700, Matt Cover wrote:
> On Sun, Sep 22, 2019 at 5:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Fri, Sep 20, 2019 at 11:58:43AM -0700, Matthew Cover wrote:
> > > Treat a negative return from a TUNSETSTEERINGEBPF bpf prog as a signal
> > > to fallback to tun_automq_select_queue() for tx queue selection.
> > >
> > > Compilation of this exact patch was tested.
> > >
> > > For functional testing 3 additional printk()s were added.
> > >
> > > Functional testing results (on 2 txq tap device):
> > >
> > > [Fri Sep 20 18:33:27 2019] ========== tun no prog ==========
> > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> > > [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> > > [Fri Sep 20 18:33:27 2019] ========== tun prog -1 ==========
> > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '-1'
> > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> > > [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> > > [Fri Sep 20 18:33:27 2019] ========== tun prog 0 ==========
> > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '0'
> > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> > > [Fri Sep 20 18:33:27 2019] ========== tun prog 1 ==========
> > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '1'
> > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '1'
> > > [Fri Sep 20 18:33:27 2019] ========== tun prog 2 ==========
> > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '2'
> > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> > >
> > > Signed-off-by: Matthew Cover <matthew.cover@stackpath.com>
> >
> >
> > Could you add a bit more motivation data here?
>
> Thank you for these questions Michael.
>
> I'll plan on adding the below information to the
> commit message and submitting a v2 of this patch
> when net-next reopens. In the meantime, it would
> be very helpful to know if these answers address
> some of your concerns.
>
> > 1. why is this a good idea
>
> This change allows TUNSETSTEERINGEBPF progs to
> do any of the following.
> 1. implement queue selection for a subset of
> traffic (e.g. special queue selection logic
> for ipv4, but return negative and use the
> default automq logic for ipv6)
> 2. determine there isn't sufficient information
> to do proper queue selection; return
> negative and use the default automq logic
> for the unknown
> 3. implement a noop prog (e.g. do
> bpf_trace_printk() then return negative and
> use the default automq logic for everything)
>
> > 2. how do we know existing userspace does not rely on existing behaviour
>
> Prior to this change a negative return from a
> TUNSETSTEERINGEBPF prog would have been cast
> into a u16 and traversed netdev_cap_txqueue().
>
> In most cases netdev_cap_txqueue() would have
> found this value to exceed real_num_tx_queues
> and queue_index would be updated to 0.
>
> It is possible that a TUNSETSTEERINGEBPF prog
> return a negative value which when cast into a
> u16 results in a positive queue_index less than
> real_num_tx_queues. For example, on x86_64, a
> return value of -65535 results in a queue_index
> of 1; which is a valid queue for any multiqueue
> device.
>
> It seems unlikely, however as stated above is
> unfortunately possible, that existing
> TUNSETSTEERINGEBPF programs would choose to
> return a negative value rather than return the
> positive value which holds the same meaning.
>
> It seems more likely that future
> TUNSETSTEERINGEBPF programs would leverage a
> negative return and potentially be loaded into
> a kernel with the old behavior.
OK if we are returning a special
value, shouldn't we limit it? How about a special
value with this meaning?
If we are changing an ABI let's at least make it
extensible.
> > 3. why doesn't userspace need a way to figure out whether it runs on a kernel with and
> > without this patch
>
> There may be some value in exposing this fact
> to the ebpf prog loader. What is the standard
> practice here, a define?
We'll need something at runtime - people move binaries between kernels
without rebuilding then. An ioctl is one option.
A sysfs attribute is another, an ethtool flag yet another.
A combination of these is possible.
And if we are doing this anyway, maybe let userspace select
the new behaviour? This way we can stay compatible with old
userspace...
> >
> >
> > thanks,
> > MST
> >
> > > ---
> > > drivers/net/tun.c | 20 +++++++++++---------
> > > 1 file changed, 11 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > index aab0be4..173d159 100644
> > > --- a/drivers/net/tun.c
> > > +++ b/drivers/net/tun.c
> > > @@ -583,35 +583,37 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > return txq;
> > > }
> > >
> > > -static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > +static int tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > {
> > > struct tun_prog *prog;
> > > u32 numqueues;
> > > - u16 ret = 0;
> > > + int ret = -1;
> > >
> > > numqueues = READ_ONCE(tun->numqueues);
> > > if (!numqueues)
> > > return 0;
> > >
> > > + rcu_read_lock();
> > > prog = rcu_dereference(tun->steering_prog);
> > > if (prog)
> > > ret = bpf_prog_run_clear_cb(prog->prog, skb);
> > > + rcu_read_unlock();
> > >
> > > - return ret % numqueues;
> > > + if (ret >= 0)
> > > + ret %= numqueues;
> > > +
> > > + return ret;
> > > }
> > >
> > > static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
> > > struct net_device *sb_dev)
> > > {
> > > struct tun_struct *tun = netdev_priv(dev);
> > > - u16 ret;
> > > + int ret;
> > >
> > > - rcu_read_lock();
> > > - if (rcu_dereference(tun->steering_prog))
> > > - ret = tun_ebpf_select_queue(tun, skb);
> > > - else
> > > + ret = tun_ebpf_select_queue(tun, skb);
> > > + if (ret < 0)
> > > ret = tun_automq_select_queue(tun, skb);
> > > - rcu_read_unlock();
> > >
> > > return ret;
> > > }
> > > --
> > > 1.8.3.1
^ permalink raw reply
* Re: [PATCH bpf] selftests/bpf: test_progs: fix client/server race in tcp_rtt
From: Andrii Nakryiko @ 2019-09-22 21:03 UTC (permalink / raw)
To: Stanislav Fomichev
Cc: Networking, bpf, David S. Miller, Alexei Starovoitov,
Daniel Borkmann
In-Reply-To: <20190920233019.187498-1-sdf@google.com>
On Sun, Sep 22, 2019 at 12:10 PM Stanislav Fomichev <sdf@google.com> wrote:
>
> This is the same problem I found earlier in test_sockopt_inherit:
> there is a race between server thread doing accept() and client
> thread doing connect(). Let's explicitly synchronize them via
> pthread conditional variable.
>
> Fixes: b55873984dab ("selftests/bpf: test BPF_SOCK_OPS_RTT_CB")
> Signed-off-by: Stanislav Fomichev <sdf@google.com>
> ---
> tools/testing/selftests/bpf/prog_tests/tcp_rtt.c | 16 +++++++++++++++-
> 1 file changed, 15 insertions(+), 1 deletion(-)
>
> diff --git a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
> index fdc0b3614a9e..e64058906bcd 100644
> --- a/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
> +++ b/tools/testing/selftests/bpf/prog_tests/tcp_rtt.c
> @@ -203,6 +203,9 @@ static int start_server(void)
> return fd;
> }
>
> +static pthread_mutex_t server_started_mtx = PTHREAD_MUTEX_INITIALIZER;
> +static pthread_cond_t server_started = PTHREAD_COND_INITIALIZER;
> +
> static void *server_thread(void *arg)
> {
> struct sockaddr_storage addr;
> @@ -215,6 +218,10 @@ static void *server_thread(void *arg)
> return NULL;
> }
>
> + pthread_mutex_lock(&server_started_mtx);
> + pthread_cond_signal(&server_started);
> + pthread_mutex_unlock(&server_started_mtx);
> +
> client_fd = accept(fd, (struct sockaddr *)&addr, &len);
> if (CHECK_FAIL(client_fd < 0)) {
> perror("Failed to accept client");
> @@ -248,7 +255,14 @@ void test_tcp_rtt(void)
> if (CHECK_FAIL(server_fd < 0))
> goto close_cgroup_fd;
>
> - pthread_create(&tid, NULL, server_thread, (void *)&server_fd);
> + if (CHECK_FAIL(pthread_create(&tid, NULL, server_thread,
> + (void *)&server_fd)))
> + goto close_cgroup_fd;
> +
> + pthread_mutex_lock(&server_started_mtx);
> + pthread_cond_wait(&server_started, &server_started_mtx);
> + pthread_mutex_unlock(&server_started_mtx);
If the server fails to listen, then we'll never get a signal, right?
Let's use timedwait instead to avoid test getting stuck forever in
such cases?
> +
> CHECK_FAIL(run_test(cgroup_fd, server_fd));
> close(server_fd);
> close_cgroup_fd:
> --
> 2.23.0.351.gc4317032e6-goog
>
^ permalink raw reply
* Re: [PATCH v2 net] net: ena: Select DIMLIB for ENA_ETHERNET
From: Jakub Kicinski @ 2019-09-22 21:08 UTC (permalink / raw)
To: Mao Wenan
Cc: netanel, saeedb, zorik, davem, netdev, linux-kernel,
kernel-janitors
In-Reply-To: <20190922053808.117965-1-maowenan@huawei.com>
On Sun, 22 Sep 2019 13:38:08 +0800, Mao Wenan wrote:
> If CONFIG_ENA_ETHERNET=y and CONFIG_DIMLIB=n,
> below erros can be found:
> drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_dim_work':
> ena_netdev.c:(.text+0x21cc): undefined reference to `net_dim_get_rx_moderation'
> ena_netdev.c:(.text+0x21cc): relocation truncated to
> fit: R_AARCH64_CALL26 against undefined symbol `net_dim_get_rx_moderation'
> drivers/net/ethernet/amazon/ena/ena_netdev.o: In function `ena_io_poll':
> ena_netdev.c:(.text+0x7bd4): undefined reference to `net_dim'
> ena_netdev.c:(.text+0x7bd4): relocation truncated to fit:
> R_AARCH64_CALL26 against undefined symbol `net_dim'
>
> After commit 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive
> interrupt moderation"), it introduces dim algorithm, which configured by CONFIG_DIMLIB.
> So, this patch is to select DIMLIB for ENA_ETHERNET.
>
> Fixes: 282faf61a053 ("net: ena: switch to dim algorithm for rx adaptive interrupt moderation")
> Signed-off-by: Mao Wenan <maowenan@huawei.com>
Applied, thank you!
^ permalink raw reply
* Re: [PATCH 0/4] Attempt to fix regression with AR8035 speed downgrade
From: Russell King - ARM Linux admin @ 2019-09-22 21:20 UTC (permalink / raw)
To: Florian Fainelli
Cc: Andrew Lunn, Heiner Kallweit, tinywrkb, David S. Miller, netdev
In-Reply-To: <268c0ea0-8b77-23eb-26cf-820cec1343e4@gmail.com>
On Sun, Sep 22, 2019 at 11:02:13AM -0700, Florian Fainelli wrote:
> On 9/22/2019 10:52 AM, Russell King - ARM Linux admin wrote:
> > On Sun, Sep 22, 2019 at 06:53:35PM +0200, Andrew Lunn wrote:
> >> On Sun, Sep 22, 2019 at 11:59:32AM +0100, Russell King - ARM Linux admin wrote:
> >>> Hi,
> >>>
> >>> tinywrkb, please can you test this series to ensure that it fixes
> >>> your problem - the previous version has turned out to be a non-starter
> >>> as it introduces more problems, thanks!
> >>>
> >>> The following series attempts to address an issue spotted by tinywrkb
> >>> with the AR8035 on the Cubox-i2 in a situation where the PHY downgrades
> >>> the negotiated link.
> >>
> >> Hi Russell
> >>
> >> This all looks sensible.
> >>
> >> One things we need to be careful of, is this is for net and so stable.
> >
> > Since the regression was introduced in 5.1, it should be backported
> > to stable trees.
> >
> >> But only some of the patches have fixes-tags. I don't know if we
> >> should add fixes tags to all the patches, just to give back porters a
> >> hint that they are all needed? It won't compile without the patches,
> >> so at least it fails safe.
> >
> > I only put Fixes: tags on patches that are actually fixing something.
> > Quoting submitting-patches.rst:
> >
> > A Fixes: tag indicates that the patch fixes an issue in a previous
> > commit.
> >
> > Since the preceding two patches are just preparing for the fix, and
> > not actually fixing an issue in themselves, it seems wrong to add a
> > Fixes: tag for them. However, mentioning it in the commit message
> > for the patch that does fix the issue is probably worth it. Thanks.
> >
>
> This is not a criticism of your patch series, which is fine.
>
> I believe Andrew's angle is that if you have fixes that rely on
> non-functional changes, then the fixes cannot be back ported as a
> standalone patch set towards specific stable trees.
Right, which makes it pointless adding a Fixes: tag to those changes.
The amount of change in phylib is quite high at the moment, but
thankfully the bug was introduced after the ethtool linkmode mask
conversion. It looks like v5.2.x should be able to cope with all
three patches simply applied there.
However, v5.1.x will require a different pre-requisit patch, which
is probably easier to do as you suggest.
> This means that
> people who do care about such fixes may have to come up with a slightly
> different fix for earlier kernels affected by those bugs, such fixes
> would not rely on patch #2 and #3 in this series and open code
> phy_resolve_aneg() and genphy_read_lpa() within the at803x.c PHY driver.
> --
> Florian
>
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: CONFIG_NET_TC_SKB_EXT
From: Jakub Kicinski @ 2019-09-22 21:47 UTC (permalink / raw)
To: Paul Blakey
Cc: Pravin Shelar, Daniel Borkmann, Vlad Buslov, David Miller,
netdev@vger.kernel.org, Jiri Pirko, Cong Wang, Jamal Hadi Salim,
Simon Horman, Or Gerlitz
In-Reply-To: <1569153104-17875-1-git-send-email-paulb@mellanox.com>
On Sun, 22 Sep 2019 14:51:44 +0300, Paul Blakey wrote:
> The skb extension is currently used for miss path of software offloading OvS rules with recirculation to tc.
> However, we are also preparing patches to support the hardware side of things.
>
> After the userspace OvS patches to support connection tracking, we'll have two users for
> tc multi chain rules, one from those actually using tc and those translated from OvS rules.
>
> With both of these use cases, there is similar issue of 'miss', from hardware to tc and tc to OvS and the skb
> extension will serve to recover from both.
>
> Consider, for example, the following multi chain tc rules:
>
> tc filter add dev1 ... chain 0 flower dst_mac aa:bb:cc:dd:ee:ff action pedit munge ex set mac dst 02:a0:98:4e:8f:d1 pipe action goto chain 1
> tc filter add dev1 ... chain 1 flower ip_dst 1.1.1.1 action mirred egress redirect dev2
> tc filter add dev1 ... chain 1 flower ip_dst 2.2.2.2 action mirred egress redirect dev2
>
> It's possible we offload the first two rules, but fail to offload the third rule, because of some hardware failure (e.g unsupported match or action).
> If a packet with (dst_mac=aa:bb:cc:dd:ee:ff) and (dst ip=2.2.2.2) arrives,
> we execute the goto chain 1 in hardware (and the pedit), and continue in chain 1, where we miss.
>
> Currently we re-start the processing in tc from chain 0, even though we already did part of the processing in hardware.
> The match on the dst_mac will fail as we already modified it, and we won't execute the third rule action.
> In addition, if we did manage to execute any software tc rules, the packet will be counted twice.
>
> We'll re-use this extension to solve this issue that currently exists by letting drivers tell tc on which chain to start the classification.
>
> Regarding the config, we suggest changing the default to N and letting users decide to enable it, see attached patch.
Partial offloads are very hard. Could we possibly take a page out of
routing offload's book and do a all or nothing offload in presence of
multiple chains?
> ------------------------------------------------------------
>
> Subject: [PATCH net-next] net/sched: Set default of CONFIG_NET_TC_SKB_EXT to N
>
> This a new feature, it is prefered that it defaults to N.
>
> Fixes: 95a7233c452a ('net: openvswitch: Set OvS recirc_id from tc chain index')
> Signed-off-by: Paul Blakey <paulb@mellanox.com>
> ---
> net/sched/Kconfig | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/net/sched/Kconfig b/net/sched/Kconfig
> index b3faafe..4bb10b7 100644
> --- a/net/sched/Kconfig
> +++ b/net/sched/Kconfig
> @@ -966,7 +966,6 @@ config NET_IFE_SKBTCINDEX
> config NET_TC_SKB_EXT
> bool "TC recirculation support"
> depends on NET_CLS_ACT
> - default y if NET_CLS_ACT
> select SKB_EXTENSIONS
>
> help
- Linus suggested we hide this option from user and autoselect if
possible.
- As Daniel said all distros will enable this.
- If correctness is really our concern, giving users an option to
select whether they want something to behave correctly seems like
a hack of lowest kind. Perhaps it's time to add a CONFIG for TC
offload in general? That's a meaningful set of functionality.
^ permalink raw reply
* Re: [PATCH 0/5] net: ethernet: stmmac: some fixes and optimization
From: Jakub Kicinski @ 2019-09-22 22:12 UTC (permalink / raw)
To: Christophe Roullier
Cc: robh, davem, joabreu, mark.rutland, mcoquelin.stm32,
alexandre.torgue, peppe.cavallaro, linux-stm32, linux-kernel,
devicetree, linux-arm-kernel, netdev, andrew
In-Reply-To: <20190920053817.13754-1-christophe.roullier@st.com>
On Fri, 20 Sep 2019 07:38:12 +0200, Christophe Roullier wrote:
> Some improvements (manage syscfg as optional clock, update slew rate of
> ETH_MDIO pin, Enable gating of the MAC TX clock during TX low-power mode)
> Fix warning build message when W=1
There seems to be some new features/cleanups (or improvements as
you say) here. Could you explain the negative impact not applying
these changes will have? Patches 1 and 3 in particular.
net-next is now closed [1], and will reopen some time after the merge
window is over. For now we are only expecting fixes for the net tree.
Could you (a) provide stronger motivation these changes are fixes; or
(b) separate the fixes from improvements?
Thank you!
[1] https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html
^ permalink raw reply
* Re: [PATCH] net/core/dev: print rtnl kind as driver name for virtual devices
From: Jakub Kicinski @ 2019-09-22 22:21 UTC (permalink / raw)
To: Konstantin Khlebnikov; +Cc: netdev, David S. Miller
In-Reply-To: <156898175525.7362.16591901912362742168.stgit@buzz>
On Fri, 20 Sep 2019 15:15:55 +0300, Konstantin Khlebnikov wrote:
> Device kind gives more information than only arbitrary device name.
>
> Signed-off-by: Konstantin Khlebnikov <khlebnikov@yandex-team.ru>
Hi Konstantin!
These look like nice improvements, unfortunately the net-next tree is
now closed [1], and will reopen some time after the merge window is
over. Hopefully you can gather feedback now, but I'm dropping the patch
from patchwork, and you'll have to resubmit once Dave opens net-next
again.
[1] https://www.kernel.org/doc/html/latest/networking/netdev-FAQ.html
Same goes for:
ipv6/addrconf: use netdev_info()/netdev_warn()/netdev_dbg() for logging
^ permalink raw reply
* Re: [PATCH net] net: stmmac: selftests: Flow Control test can also run with ASYM Pause
From: Jakub Kicinski @ 2019-09-22 21:56 UTC (permalink / raw)
To: Jose Abreu
Cc: netdev, Joao Pinto, Giuseppe Cavallaro, Alexandre Torgue,
David S. Miller, Maxime Coquelin, linux-stm32, linux-arm-kernel,
linux-kernel
In-Reply-To: <f35fa5a51f52fc1ef17a0a9ecd470e2a6792b3f8.1568887745.git.joabreu@synopsys.com>
On Thu, 19 Sep 2019 12:09:49 +0200, Jose Abreu wrote:
> The Flow Control selftest is also available with ASYM Pause. Lets add
> this check to the test and fix eventual false positive failures.
>
> Fixes: 091810dbded9 ("net: stmmac: Introduce selftests support")
> Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Hi Jose!
Thanks for the patch it looks good, seems like you posted it from
a slightly different email address than was used for signoff:
From: Jose Abreu <Jose.Abreu@synopsys.com>
vs
Signed-off-by: Jose Abreu <joabreu@synopsys.com>
Could you please fix and repost? Automation may get upset otherwise.
^ permalink raw reply
* Re: [PATCH] net: dsa: b53: Use the correct style for SPDX License Identifier
From: Jakub Kicinski @ 2019-09-22 22:26 UTC (permalink / raw)
To: Nishad Kamdar
Cc: Florian Fainelli, Andrew Lunn, Vivien Didelot, David S. Miller,
Greg Kroah-Hartman, Joe Perches, Uwe Kleine-König, netdev,
linux-kernel
In-Reply-To: <20190921133011.GA2994@nishad>
On Sat, 21 Sep 2019 19:00:16 +0530, Nishad Kamdar wrote:
> This patch corrects the SPDX License Identifier style
> in header file for Broadcom BCM53xx managed switch driver.
> For C header files Documentation/process/license-rules.rst
> mandates C-like comments (opposed to C source files where
> C++ style should be used)
>
> Changes made by using a script provided by Joe Perches here:
> https://lkml.org/lkml/2019/2/7/46.
>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
Applied, thank you!
^ permalink raw reply
* Re: [PATCH] net: dsa: Use the correct style for SPDX License Identifier
From: Jakub Kicinski @ 2019-09-22 22:27 UTC (permalink / raw)
To: Nishad Kamdar
Cc: Hauke Mehrtens, Florian Fainelli, Andrew Lunn, Vivien Didelot,
David S. Miller, Greg Kroah-Hartman, Joe Perches,
Uwe Kleine-König, netdev, linux-kernel
In-Reply-To: <20190921134522.GA3575@nishad>
On Sat, 21 Sep 2019 19:15:25 +0530, Nishad Kamdar wrote:
> This patch corrects the SPDX License Identifier style
> in header file for Distributed Switch Architecture drivers.
> For C header files Documentation/process/license-rules.rst
> mandates C-like comments (opposed to C source files where
> C++ style should be used)
>
> Changes made by using a script provided by Joe Perches here:
> https://lkml.org/lkml/2019/2/7/46.
>
> Suggested-by: Joe Perches <joe@perches.com>
> Signed-off-by: Nishad Kamdar <nishadkamdar@gmail.com>
Applied, thank you!
^ permalink raw reply
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Matt Cover @ 2019-09-22 22:30 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: davem, ast, daniel, kafai, songliubraving, yhs, Jason Wang,
Eric Dumazet, Stanislav Fomichev, Matthew Cover, mail, pabeni,
Nicolas Dichtel, wangli39, lifei.shirley, tglx, netdev,
linux-kernel, bpf
In-Reply-To: <20190922162546-mutt-send-email-mst@kernel.org>
On Sun, Sep 22, 2019 at 1:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>
> On Sun, Sep 22, 2019 at 10:43:19AM -0700, Matt Cover wrote:
> > On Sun, Sep 22, 2019 at 5:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Fri, Sep 20, 2019 at 11:58:43AM -0700, Matthew Cover wrote:
> > > > Treat a negative return from a TUNSETSTEERINGEBPF bpf prog as a signal
> > > > to fallback to tun_automq_select_queue() for tx queue selection.
> > > >
> > > > Compilation of this exact patch was tested.
> > > >
> > > > For functional testing 3 additional printk()s were added.
> > > >
> > > > Functional testing results (on 2 txq tap device):
> > > >
> > > > [Fri Sep 20 18:33:27 2019] ========== tun no prog ==========
> > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> > > > [Fri Sep 20 18:33:27 2019] ========== tun prog -1 ==========
> > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '-1'
> > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> > > > [Fri Sep 20 18:33:27 2019] ========== tun prog 0 ==========
> > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '0'
> > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> > > > [Fri Sep 20 18:33:27 2019] ========== tun prog 1 ==========
> > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '1'
> > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '1'
> > > > [Fri Sep 20 18:33:27 2019] ========== tun prog 2 ==========
> > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '2'
> > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> > > >
> > > > Signed-off-by: Matthew Cover <matthew.cover@stackpath.com>
> > >
> > >
> > > Could you add a bit more motivation data here?
> >
> > Thank you for these questions Michael.
> >
> > I'll plan on adding the below information to the
> > commit message and submitting a v2 of this patch
> > when net-next reopens. In the meantime, it would
> > be very helpful to know if these answers address
> > some of your concerns.
> >
> > > 1. why is this a good idea
> >
> > This change allows TUNSETSTEERINGEBPF progs to
> > do any of the following.
> > 1. implement queue selection for a subset of
> > traffic (e.g. special queue selection logic
> > for ipv4, but return negative and use the
> > default automq logic for ipv6)
> > 2. determine there isn't sufficient information
> > to do proper queue selection; return
> > negative and use the default automq logic
> > for the unknown
> > 3. implement a noop prog (e.g. do
> > bpf_trace_printk() then return negative and
> > use the default automq logic for everything)
> >
> > > 2. how do we know existing userspace does not rely on existing behaviour
> >
> > Prior to this change a negative return from a
> > TUNSETSTEERINGEBPF prog would have been cast
> > into a u16 and traversed netdev_cap_txqueue().
> >
> > In most cases netdev_cap_txqueue() would have
> > found this value to exceed real_num_tx_queues
> > and queue_index would be updated to 0.
> >
> > It is possible that a TUNSETSTEERINGEBPF prog
> > return a negative value which when cast into a
> > u16 results in a positive queue_index less than
> > real_num_tx_queues. For example, on x86_64, a
> > return value of -65535 results in a queue_index
> > of 1; which is a valid queue for any multiqueue
> > device.
> >
> > It seems unlikely, however as stated above is
> > unfortunately possible, that existing
> > TUNSETSTEERINGEBPF programs would choose to
> > return a negative value rather than return the
> > positive value which holds the same meaning.
> >
> > It seems more likely that future
> > TUNSETSTEERINGEBPF programs would leverage a
> > negative return and potentially be loaded into
> > a kernel with the old behavior.
>
> OK if we are returning a special
> value, shouldn't we limit it? How about a special
> value with this meaning?
> If we are changing an ABI let's at least make it
> extensible.
>
A special value with this meaning sounds
good to me. I'll plan on adding a define
set to -1 to cause the fallback to automq.
The way I was initially viewing the old
behavior was that returning negative was
undefined; it happened to have the
outcomes I walked through, but not
necessarily by design.
In order to keep the new behavior
extensible, how should we state that a
negative return other than -1 is
undefined and therefore subject to
change. Is something like this
sufficient?
Documentation/networking/tc-actions-env-rules.txt
Additionally, what should the new
behavior implement when a negative other
than -1 is returned? I would like to have
it do the same thing as -1 for now, but
with the understanding that this behavior
is undefined. Does this sound reasonable?
> > > 3. why doesn't userspace need a way to figure out whether it runs on a kernel with and
> > > without this patch
> >
> > There may be some value in exposing this fact
> > to the ebpf prog loader. What is the standard
> > practice here, a define?
>
>
> We'll need something at runtime - people move binaries between kernels
> without rebuilding then. An ioctl is one option.
> A sysfs attribute is another, an ethtool flag yet another.
> A combination of these is possible.
>
> And if we are doing this anyway, maybe let userspace select
> the new behaviour? This way we can stay compatible with old
> userspace...
>
Understood. I'll look into adding an
ioctl to activate the new behavior. And
perhaps a method of checking which is
behavior is currently active (in case we
ever want to change the default, say
after some suitably long transition
period).
> > >
> > >
> > > thanks,
> > > MST
> > >
> > > > ---
> > > > drivers/net/tun.c | 20 +++++++++++---------
> > > > 1 file changed, 11 insertions(+), 9 deletions(-)
> > > >
> > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > > index aab0be4..173d159 100644
> > > > --- a/drivers/net/tun.c
> > > > +++ b/drivers/net/tun.c
> > > > @@ -583,35 +583,37 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > > return txq;
> > > > }
> > > >
> > > > -static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > > +static int tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > > {
> > > > struct tun_prog *prog;
> > > > u32 numqueues;
> > > > - u16 ret = 0;
> > > > + int ret = -1;
> > > >
> > > > numqueues = READ_ONCE(tun->numqueues);
> > > > if (!numqueues)
> > > > return 0;
> > > >
> > > > + rcu_read_lock();
> > > > prog = rcu_dereference(tun->steering_prog);
> > > > if (prog)
> > > > ret = bpf_prog_run_clear_cb(prog->prog, skb);
> > > > + rcu_read_unlock();
> > > >
> > > > - return ret % numqueues;
> > > > + if (ret >= 0)
> > > > + ret %= numqueues;
> > > > +
> > > > + return ret;
> > > > }
> > > >
> > > > static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
> > > > struct net_device *sb_dev)
> > > > {
> > > > struct tun_struct *tun = netdev_priv(dev);
> > > > - u16 ret;
> > > > + int ret;
> > > >
> > > > - rcu_read_lock();
> > > > - if (rcu_dereference(tun->steering_prog))
> > > > - ret = tun_ebpf_select_queue(tun, skb);
> > > > - else
> > > > + ret = tun_ebpf_select_queue(tun, skb);
> > > > + if (ret < 0)
> > > > ret = tun_automq_select_queue(tun, skb);
> > > > - rcu_read_unlock();
> > > >
> > > > return ret;
> > > > }
> > > > --
> > > > 1.8.3.1
^ permalink raw reply
* Re: [PATCH] net: stmmac: Fix page pool size
From: Jakub Kicinski @ 2019-09-22 22:31 UTC (permalink / raw)
To: Thierry Reding
Cc: David S . Miller, Giuseppe Cavallaro, Alexandre Torgue,
Jose Abreu, Florian Fainelli, Jon Hunter, Bitan Biswas, netdev,
linux-tegra
In-Reply-To: <20190920170127.22850-1-thierry.reding@gmail.com>
On Fri, 20 Sep 2019 19:01:27 +0200, Thierry Reding wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> The size of individual pages in the page pool in given by an order. The
> order is the binary logarithm of the number of pages that make up one of
> the pages in the pool. However, the driver currently passes the number
> of pages rather than the order, so it ends up wasting quite a bit of
> memory.
>
> Fix this by taking the binary logarithm and passing that in the order
> field.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
Since this is a fix could we get a Fixes tag pointing to the commit
which introduced the regression?
^ permalink raw reply
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Matt Cover @ 2019-09-22 22:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: davem, ast, daniel, kafai, songliubraving, yhs, Jason Wang,
Eric Dumazet, Stanislav Fomichev, Matthew Cover, mail, pabeni,
Nicolas Dichtel, wangli39, lifei.shirley, tglx, netdev,
linux-kernel, bpf
In-Reply-To: <CAGyo_hr+_oSwVSKSqKTXaouaMK-6b8+NVLTxWmZD3vn07GEGWA@mail.gmail.com>
On Sun, Sep 22, 2019 at 3:30 PM Matt Cover <werekraken@gmail.com> wrote:
>
> On Sun, Sep 22, 2019 at 1:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >
> > On Sun, Sep 22, 2019 at 10:43:19AM -0700, Matt Cover wrote:
> > > On Sun, Sep 22, 2019 at 5:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > >
> > > > On Fri, Sep 20, 2019 at 11:58:43AM -0700, Matthew Cover wrote:
> > > > > Treat a negative return from a TUNSETSTEERINGEBPF bpf prog as a signal
> > > > > to fallback to tun_automq_select_queue() for tx queue selection.
> > > > >
> > > > > Compilation of this exact patch was tested.
> > > > >
> > > > > For functional testing 3 additional printk()s were added.
> > > > >
> > > > > Functional testing results (on 2 txq tap device):
> > > > >
> > > > > [Fri Sep 20 18:33:27 2019] ========== tun no prog ==========
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> > > > > [Fri Sep 20 18:33:27 2019] ========== tun prog -1 ==========
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '-1'
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> > > > > [Fri Sep 20 18:33:27 2019] ========== tun prog 0 ==========
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '0'
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> > > > > [Fri Sep 20 18:33:27 2019] ========== tun prog 1 ==========
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '1'
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '1'
> > > > > [Fri Sep 20 18:33:27 2019] ========== tun prog 2 ==========
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '2'
> > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> > > > >
> > > > > Signed-off-by: Matthew Cover <matthew.cover@stackpath.com>
> > > >
> > > >
> > > > Could you add a bit more motivation data here?
> > >
> > > Thank you for these questions Michael.
> > >
> > > I'll plan on adding the below information to the
> > > commit message and submitting a v2 of this patch
> > > when net-next reopens. In the meantime, it would
> > > be very helpful to know if these answers address
> > > some of your concerns.
> > >
> > > > 1. why is this a good idea
> > >
> > > This change allows TUNSETSTEERINGEBPF progs to
> > > do any of the following.
> > > 1. implement queue selection for a subset of
> > > traffic (e.g. special queue selection logic
> > > for ipv4, but return negative and use the
> > > default automq logic for ipv6)
> > > 2. determine there isn't sufficient information
> > > to do proper queue selection; return
> > > negative and use the default automq logic
> > > for the unknown
> > > 3. implement a noop prog (e.g. do
> > > bpf_trace_printk() then return negative and
> > > use the default automq logic for everything)
> > >
> > > > 2. how do we know existing userspace does not rely on existing behaviour
> > >
> > > Prior to this change a negative return from a
> > > TUNSETSTEERINGEBPF prog would have been cast
> > > into a u16 and traversed netdev_cap_txqueue().
> > >
> > > In most cases netdev_cap_txqueue() would have
> > > found this value to exceed real_num_tx_queues
> > > and queue_index would be updated to 0.
> > >
> > > It is possible that a TUNSETSTEERINGEBPF prog
> > > return a negative value which when cast into a
> > > u16 results in a positive queue_index less than
> > > real_num_tx_queues. For example, on x86_64, a
> > > return value of -65535 results in a queue_index
> > > of 1; which is a valid queue for any multiqueue
> > > device.
> > >
> > > It seems unlikely, however as stated above is
> > > unfortunately possible, that existing
> > > TUNSETSTEERINGEBPF programs would choose to
> > > return a negative value rather than return the
> > > positive value which holds the same meaning.
> > >
> > > It seems more likely that future
> > > TUNSETSTEERINGEBPF programs would leverage a
> > > negative return and potentially be loaded into
> > > a kernel with the old behavior.
> >
> > OK if we are returning a special
> > value, shouldn't we limit it? How about a special
> > value with this meaning?
> > If we are changing an ABI let's at least make it
> > extensible.
> >
>
> A special value with this meaning sounds
> good to me. I'll plan on adding a define
> set to -1 to cause the fallback to automq.
>
> The way I was initially viewing the old
> behavior was that returning negative was
> undefined; it happened to have the
> outcomes I walked through, but not
> necessarily by design.
>
> In order to keep the new behavior
> extensible, how should we state that a
> negative return other than -1 is
> undefined and therefore subject to
> change. Is something like this
> sufficient?
>
> Documentation/networking/tc-actions-env-rules.txt
>
> Additionally, what should the new
> behavior implement when a negative other
> than -1 is returned? I would like to have
> it do the same thing as -1 for now, but
> with the understanding that this behavior
> is undefined. Does this sound reasonable?
>
> > > > 3. why doesn't userspace need a way to figure out whether it runs on a kernel with and
> > > > without this patch
> > >
> > > There may be some value in exposing this fact
> > > to the ebpf prog loader. What is the standard
> > > practice here, a define?
> >
> >
> > We'll need something at runtime - people move binaries between kernels
> > without rebuilding then. An ioctl is one option.
> > A sysfs attribute is another, an ethtool flag yet another.
> > A combination of these is possible.
> >
> > And if we are doing this anyway, maybe let userspace select
> > the new behaviour? This way we can stay compatible with old
> > userspace...
> >
>
> Understood. I'll look into adding an
> ioctl to activate the new behavior. And
> perhaps a method of checking which is
> behavior is currently active (in case we
> ever want to change the default, say
> after some suitably long transition
> period).
>
Unless of course we can simply state via
documentation that any negative return
for which a define doesn't exist is
undefined behavior. In which case,
there is no old vs new behavior and
no need for an ioctl. Simply the
understanding provided by the
documentation.
> > > >
> > > >
> > > > thanks,
> > > > MST
> > > >
> > > > > ---
> > > > > drivers/net/tun.c | 20 +++++++++++---------
> > > > > 1 file changed, 11 insertions(+), 9 deletions(-)
> > > > >
> > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > > > index aab0be4..173d159 100644
> > > > > --- a/drivers/net/tun.c
> > > > > +++ b/drivers/net/tun.c
> > > > > @@ -583,35 +583,37 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > > > return txq;
> > > > > }
> > > > >
> > > > > -static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > > > +static int tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > > > {
> > > > > struct tun_prog *prog;
> > > > > u32 numqueues;
> > > > > - u16 ret = 0;
> > > > > + int ret = -1;
> > > > >
> > > > > numqueues = READ_ONCE(tun->numqueues);
> > > > > if (!numqueues)
> > > > > return 0;
> > > > >
> > > > > + rcu_read_lock();
> > > > > prog = rcu_dereference(tun->steering_prog);
> > > > > if (prog)
> > > > > ret = bpf_prog_run_clear_cb(prog->prog, skb);
> > > > > + rcu_read_unlock();
> > > > >
> > > > > - return ret % numqueues;
> > > > > + if (ret >= 0)
> > > > > + ret %= numqueues;
> > > > > +
> > > > > + return ret;
> > > > > }
> > > > >
> > > > > static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
> > > > > struct net_device *sb_dev)
> > > > > {
> > > > > struct tun_struct *tun = netdev_priv(dev);
> > > > > - u16 ret;
> > > > > + int ret;
> > > > >
> > > > > - rcu_read_lock();
> > > > > - if (rcu_dereference(tun->steering_prog))
> > > > > - ret = tun_ebpf_select_queue(tun, skb);
> > > > > - else
> > > > > + ret = tun_ebpf_select_queue(tun, skb);
> > > > > + if (ret < 0)
> > > > > ret = tun_automq_select_queue(tun, skb);
> > > > > - rcu_read_unlock();
> > > > >
> > > > > return ret;
> > > > > }
> > > > > --
> > > > > 1.8.3.1
^ permalink raw reply
* Re: [PATCH net] ionic: Fix an error code in ionic_lif_alloc()
From: Jakub Kicinski @ 2019-09-22 21:17 UTC (permalink / raw)
To: Dan Carpenter
Cc: Shannon Nelson, Pensando Drivers, David S. Miller, netdev,
kernel-janitors
In-Reply-To: <20190921055926.GA18726@mwanda>
On Sat, 21 Sep 2019 08:59:26 +0300, Dan Carpenter wrote:
> We need to set the error code on this path. Otherwise it probably
> results in a NULL dereference down the line.
>
> Fixes: aa3198819bea ("ionic: Add RSS support")
> Signed-off-by: Dan Carpenter <dan.carpenter@oracle.com>
Applied, thank you!
^ permalink raw reply
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Matt Cover @ 2019-09-23 0:28 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: davem, ast, daniel, kafai, songliubraving, yhs, Jason Wang,
Eric Dumazet, Stanislav Fomichev, Matthew Cover, mail, pabeni,
Nicolas Dichtel, wangli39, lifei.shirley, tglx, netdev,
linux-kernel, bpf
In-Reply-To: <CAGyo_hpCDPmNvTau50XxRVkq1C=Qn7E8cVkE=BZhhiNF6MjqZA@mail.gmail.com>
On Sun, Sep 22, 2019 at 3:46 PM Matt Cover <werekraken@gmail.com> wrote:
>
> On Sun, Sep 22, 2019 at 3:30 PM Matt Cover <werekraken@gmail.com> wrote:
> >
> > On Sun, Sep 22, 2019 at 1:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> > >
> > > On Sun, Sep 22, 2019 at 10:43:19AM -0700, Matt Cover wrote:
> > > > On Sun, Sep 22, 2019 at 5:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> > > > >
> > > > > On Fri, Sep 20, 2019 at 11:58:43AM -0700, Matthew Cover wrote:
> > > > > > Treat a negative return from a TUNSETSTEERINGEBPF bpf prog as a signal
> > > > > > to fallback to tun_automq_select_queue() for tx queue selection.
> > > > > >
> > > > > > Compilation of this exact patch was tested.
> > > > > >
> > > > > > For functional testing 3 additional printk()s were added.
> > > > > >
> > > > > > Functional testing results (on 2 txq tap device):
> > > > > >
> > > > > > [Fri Sep 20 18:33:27 2019] ========== tun no prog ==========
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> > > > > > [Fri Sep 20 18:33:27 2019] ========== tun prog -1 ==========
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '-1'
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> > > > > > [Fri Sep 20 18:33:27 2019] ========== tun prog 0 ==========
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '0'
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> > > > > > [Fri Sep 20 18:33:27 2019] ========== tun prog 1 ==========
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '1'
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '1'
> > > > > > [Fri Sep 20 18:33:27 2019] ========== tun prog 2 ==========
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '2'
> > > > > > [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> > > > > >
> > > > > > Signed-off-by: Matthew Cover <matthew.cover@stackpath.com>
> > > > >
> > > > >
> > > > > Could you add a bit more motivation data here?
> > > >
> > > > Thank you for these questions Michael.
> > > >
> > > > I'll plan on adding the below information to the
> > > > commit message and submitting a v2 of this patch
> > > > when net-next reopens. In the meantime, it would
> > > > be very helpful to know if these answers address
> > > > some of your concerns.
> > > >
> > > > > 1. why is this a good idea
> > > >
> > > > This change allows TUNSETSTEERINGEBPF progs to
> > > > do any of the following.
> > > > 1. implement queue selection for a subset of
> > > > traffic (e.g. special queue selection logic
> > > > for ipv4, but return negative and use the
> > > > default automq logic for ipv6)
> > > > 2. determine there isn't sufficient information
> > > > to do proper queue selection; return
> > > > negative and use the default automq logic
> > > > for the unknown
> > > > 3. implement a noop prog (e.g. do
> > > > bpf_trace_printk() then return negative and
> > > > use the default automq logic for everything)
> > > >
> > > > > 2. how do we know existing userspace does not rely on existing behaviour
> > > >
> > > > Prior to this change a negative return from a
> > > > TUNSETSTEERINGEBPF prog would have been cast
> > > > into a u16 and traversed netdev_cap_txqueue().
> > > >
> > > > In most cases netdev_cap_txqueue() would have
> > > > found this value to exceed real_num_tx_queues
> > > > and queue_index would be updated to 0.
> > > >
> > > > It is possible that a TUNSETSTEERINGEBPF prog
> > > > return a negative value which when cast into a
> > > > u16 results in a positive queue_index less than
> > > > real_num_tx_queues. For example, on x86_64, a
> > > > return value of -65535 results in a queue_index
> > > > of 1; which is a valid queue for any multiqueue
> > > > device.
> > > >
> > > > It seems unlikely, however as stated above is
> > > > unfortunately possible, that existing
> > > > TUNSETSTEERINGEBPF programs would choose to
> > > > return a negative value rather than return the
> > > > positive value which holds the same meaning.
> > > >
> > > > It seems more likely that future
> > > > TUNSETSTEERINGEBPF programs would leverage a
> > > > negative return and potentially be loaded into
> > > > a kernel with the old behavior.
> > >
> > > OK if we are returning a special
> > > value, shouldn't we limit it? How about a special
> > > value with this meaning?
> > > If we are changing an ABI let's at least make it
> > > extensible.
> > >
> >
> > A special value with this meaning sounds
> > good to me. I'll plan on adding a define
> > set to -1 to cause the fallback to automq.
> >
> > The way I was initially viewing the old
> > behavior was that returning negative was
> > undefined; it happened to have the
> > outcomes I walked through, but not
> > necessarily by design.
> >
> > In order to keep the new behavior
> > extensible, how should we state that a
> > negative return other than -1 is
> > undefined and therefore subject to
> > change. Is something like this
> > sufficient?
> >
> > Documentation/networking/tc-actions-env-rules.txt
> >
> > Additionally, what should the new
> > behavior implement when a negative other
> > than -1 is returned? I would like to have
> > it do the same thing as -1 for now, but
> > with the understanding that this behavior
> > is undefined. Does this sound reasonable?
> >
> > > > > 3. why doesn't userspace need a way to figure out whether it runs on a kernel with and
> > > > > without this patch
> > > >
> > > > There may be some value in exposing this fact
> > > > to the ebpf prog loader. What is the standard
> > > > practice here, a define?
> > >
> > >
> > > We'll need something at runtime - people move binaries between kernels
> > > without rebuilding then. An ioctl is one option.
> > > A sysfs attribute is another, an ethtool flag yet another.
> > > A combination of these is possible.
> > >
> > > And if we are doing this anyway, maybe let userspace select
> > > the new behaviour? This way we can stay compatible with old
> > > userspace...
> > >
> >
> > Understood. I'll look into adding an
> > ioctl to activate the new behavior. And
> > perhaps a method of checking which is
> > behavior is currently active (in case we
> > ever want to change the default, say
> > after some suitably long transition
> > period).
> >
>
> Unless of course we can simply state via
> documentation that any negative return
> for which a define doesn't exist is
> undefined behavior. In which case,
> there is no old vs new behavior and
> no need for an ioctl. Simply the
> understanding provided by the
> documentation.
>
On second thought, this again doesn't
solve for runtime determination.
How does this sound as a complete
solution for v2?
1. leave the changes to
tun_ebpf_select_queue() as they are
2. update tun_select_queue() to only
run tun_automq_select_queue() when
ret == TUN_SSE_DO_AUTOMQ
(this will also happen when !prog)
3. add an ioctl or sysfs endpoint which
allows for runtime querying of the
TUNSETSTEERINGEBPF "capabilities"
(if I can keep this more generic than
return value, I will; e.g. perhaps
one day it could be used to indicate
a hookpoint specific bpf helper
function or similar as a capability)
4. add documentation on how to check
"capabilities" and that any
unspecified negative return value
results in undefined behavior
> > > > >
> > > > >
> > > > > thanks,
> > > > > MST
> > > > >
> > > > > > ---
> > > > > > drivers/net/tun.c | 20 +++++++++++---------
> > > > > > 1 file changed, 11 insertions(+), 9 deletions(-)
> > > > > >
> > > > > > diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> > > > > > index aab0be4..173d159 100644
> > > > > > --- a/drivers/net/tun.c
> > > > > > +++ b/drivers/net/tun.c
> > > > > > @@ -583,35 +583,37 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > > > > return txq;
> > > > > > }
> > > > > >
> > > > > > -static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > > > > +static int tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> > > > > > {
> > > > > > struct tun_prog *prog;
> > > > > > u32 numqueues;
> > > > > > - u16 ret = 0;
> > > > > > + int ret = -1;
> > > > > >
> > > > > > numqueues = READ_ONCE(tun->numqueues);
> > > > > > if (!numqueues)
> > > > > > return 0;
> > > > > >
> > > > > > + rcu_read_lock();
> > > > > > prog = rcu_dereference(tun->steering_prog);
> > > > > > if (prog)
> > > > > > ret = bpf_prog_run_clear_cb(prog->prog, skb);
> > > > > > + rcu_read_unlock();
> > > > > >
> > > > > > - return ret % numqueues;
> > > > > > + if (ret >= 0)
> > > > > > + ret %= numqueues;
> > > > > > +
> > > > > > + return ret;
> > > > > > }
> > > > > >
> > > > > > static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
> > > > > > struct net_device *sb_dev)
> > > > > > {
> > > > > > struct tun_struct *tun = netdev_priv(dev);
> > > > > > - u16 ret;
> > > > > > + int ret;
> > > > > >
> > > > > > - rcu_read_lock();
> > > > > > - if (rcu_dereference(tun->steering_prog))
> > > > > > - ret = tun_ebpf_select_queue(tun, skb);
> > > > > > - else
> > > > > > + ret = tun_ebpf_select_queue(tun, skb);
> > > > > > + if (ret < 0)
> > > > > > ret = tun_automq_select_queue(tun, skb);
> > > > > > - rcu_read_unlock();
> > > > > >
> > > > > > return ret;
> > > > > > }
> > > > > > --
> > > > > > 1.8.3.1
^ permalink raw reply
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Jason Wang @ 2019-09-23 0:46 UTC (permalink / raw)
To: Matt Cover, Michael S. Tsirkin
Cc: davem, ast, daniel, kafai, songliubraving, yhs, Eric Dumazet,
Stanislav Fomichev, Matthew Cover, mail, pabeni, Nicolas Dichtel,
wangli39, lifei.shirley, tglx, netdev, linux-kernel, bpf
In-Reply-To: <CAGyo_hqGbFdt1PoDrmo=S5iTO8TwbrbtOJtbvGT1WrFFMLwk-Q@mail.gmail.com>
On 2019/9/23 上午1:43, Matt Cover wrote:
> On Sun, Sep 22, 2019 at 5:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Fri, Sep 20, 2019 at 11:58:43AM -0700, Matthew Cover wrote:
>>> Treat a negative return from a TUNSETSTEERINGEBPF bpf prog as a signal
>>> to fallback to tun_automq_select_queue() for tx queue selection.
>>>
>>> Compilation of this exact patch was tested.
>>>
>>> For functional testing 3 additional printk()s were added.
>>>
>>> Functional testing results (on 2 txq tap device):
>>>
>>> [Fri Sep 20 18:33:27 2019] ========== tun no prog ==========
>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
>>> [Fri Sep 20 18:33:27 2019] ========== tun prog -1 ==========
>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '-1'
>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
>>> [Fri Sep 20 18:33:27 2019] ========== tun prog 0 ==========
>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '0'
>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
>>> [Fri Sep 20 18:33:27 2019] ========== tun prog 1 ==========
>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '1'
>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '1'
>>> [Fri Sep 20 18:33:27 2019] ========== tun prog 2 ==========
>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '2'
>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
>>>
>>> Signed-off-by: Matthew Cover <matthew.cover@stackpath.com>
>>
>> Could you add a bit more motivation data here?
> Thank you for these questions Michael.
>
> I'll plan on adding the below information to the
> commit message and submitting a v2 of this patch
> when net-next reopens. In the meantime, it would
> be very helpful to know if these answers address
> some of your concerns.
>
>> 1. why is this a good idea
> This change allows TUNSETSTEERINGEBPF progs to
> do any of the following.
> 1. implement queue selection for a subset of
> traffic (e.g. special queue selection logic
> for ipv4, but return negative and use the
> default automq logic for ipv6)
Well, using ebpf means it need to take care of all the cases. E.g you
can easily implement the fallback through eBPF as well.
> 2. determine there isn't sufficient information
> to do proper queue selection; return
> negative and use the default automq logic
> for the unknown
Same as above.
> 3. implement a noop prog (e.g. do
> bpf_trace_printk() then return negative and
> use the default automq logic for everything)
ditto.
>
>> 2. how do we know existing userspace does not rely on existing behaviour
> Prior to this change a negative return from a
> TUNSETSTEERINGEBPF prog would have been cast
> into a u16 and traversed netdev_cap_txqueue().
>
> In most cases netdev_cap_txqueue() would have
> found this value to exceed real_num_tx_queues
> and queue_index would be updated to 0.
>
> It is possible that a TUNSETSTEERINGEBPF prog
> return a negative value which when cast into a
> u16 results in a positive queue_index less than
> real_num_tx_queues. For example, on x86_64, a
> return value of -65535 results in a queue_index
> of 1; which is a valid queue for any multiqueue
> device.
>
> It seems unlikely, however as stated above is
> unfortunately possible, that existing
> TUNSETSTEERINGEBPF programs would choose to
> return a negative value rather than return the
> positive value which holds the same meaning.
>
> It seems more likely that future
> TUNSETSTEERINGEBPF programs would leverage a
> negative return and potentially be loaded into
> a kernel with the old behavior.
Yes, eBPF can return probably wrong value, but what kernel did is just
to make sure it doesn't harm anything.
I would rather just drop the packet in this case.
Thanks
>
>> 3. why doesn't userspace need a way to figure out whether it runs on a kernel with and
>> without this patch
> There may be some value in exposing this fact
> to the ebpf prog loader. What is the standard
> practice here, a define?
>
>>
>> thanks,
>> MST
>>
>>> ---
>>> drivers/net/tun.c | 20 +++++++++++---------
>>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>>
>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>> index aab0be4..173d159 100644
>>> --- a/drivers/net/tun.c
>>> +++ b/drivers/net/tun.c
>>> @@ -583,35 +583,37 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb)
>>> return txq;
>>> }
>>>
>>> -static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
>>> +static int tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
>>> {
>>> struct tun_prog *prog;
>>> u32 numqueues;
>>> - u16 ret = 0;
>>> + int ret = -1;
>>>
>>> numqueues = READ_ONCE(tun->numqueues);
>>> if (!numqueues)
>>> return 0;
>>>
>>> + rcu_read_lock();
>>> prog = rcu_dereference(tun->steering_prog);
>>> if (prog)
>>> ret = bpf_prog_run_clear_cb(prog->prog, skb);
>>> + rcu_read_unlock();
>>>
>>> - return ret % numqueues;
>>> + if (ret >= 0)
>>> + ret %= numqueues;
>>> +
>>> + return ret;
>>> }
>>>
>>> static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
>>> struct net_device *sb_dev)
>>> {
>>> struct tun_struct *tun = netdev_priv(dev);
>>> - u16 ret;
>>> + int ret;
>>>
>>> - rcu_read_lock();
>>> - if (rcu_dereference(tun->steering_prog))
>>> - ret = tun_ebpf_select_queue(tun, skb);
>>> - else
>>> + ret = tun_ebpf_select_queue(tun, skb);
>>> + if (ret < 0)
>>> ret = tun_automq_select_queue(tun, skb);
>>> - rcu_read_unlock();
>>>
>>> return ret;
>>> }
>>> --
>>> 1.8.3.1
^ permalink raw reply
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Jason Wang @ 2019-09-23 0:51 UTC (permalink / raw)
To: Matt Cover, Michael S. Tsirkin
Cc: davem, ast, daniel, kafai, songliubraving, yhs, Eric Dumazet,
Stanislav Fomichev, Matthew Cover, mail, pabeni, Nicolas Dichtel,
wangli39, lifei.shirley, tglx, netdev, linux-kernel, bpf
In-Reply-To: <CAGyo_hr+_oSwVSKSqKTXaouaMK-6b8+NVLTxWmZD3vn07GEGWA@mail.gmail.com>
On 2019/9/23 上午6:30, Matt Cover wrote:
> On Sun, Sep 22, 2019 at 1:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
>> On Sun, Sep 22, 2019 at 10:43:19AM -0700, Matt Cover wrote:
>>> On Sun, Sep 22, 2019 at 5:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
>>>> On Fri, Sep 20, 2019 at 11:58:43AM -0700, Matthew Cover wrote:
>>>>> Treat a negative return from a TUNSETSTEERINGEBPF bpf prog as a signal
>>>>> to fallback to tun_automq_select_queue() for tx queue selection.
>>>>>
>>>>> Compilation of this exact patch was tested.
>>>>>
>>>>> For functional testing 3 additional printk()s were added.
>>>>>
>>>>> Functional testing results (on 2 txq tap device):
>>>>>
>>>>> [Fri Sep 20 18:33:27 2019] ========== tun no prog ==========
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
>>>>> [Fri Sep 20 18:33:27 2019] ========== tun prog -1 ==========
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '-1'
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
>>>>> [Fri Sep 20 18:33:27 2019] ========== tun prog 0 ==========
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '0'
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
>>>>> [Fri Sep 20 18:33:27 2019] ========== tun prog 1 ==========
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '1'
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '1'
>>>>> [Fri Sep 20 18:33:27 2019] ========== tun prog 2 ==========
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '2'
>>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
>>>>>
>>>>> Signed-off-by: Matthew Cover <matthew.cover@stackpath.com>
>>>>
>>>> Could you add a bit more motivation data here?
>>> Thank you for these questions Michael.
>>>
>>> I'll plan on adding the below information to the
>>> commit message and submitting a v2 of this patch
>>> when net-next reopens. In the meantime, it would
>>> be very helpful to know if these answers address
>>> some of your concerns.
>>>
>>>> 1. why is this a good idea
>>> This change allows TUNSETSTEERINGEBPF progs to
>>> do any of the following.
>>> 1. implement queue selection for a subset of
>>> traffic (e.g. special queue selection logic
>>> for ipv4, but return negative and use the
>>> default automq logic for ipv6)
>>> 2. determine there isn't sufficient information
>>> to do proper queue selection; return
>>> negative and use the default automq logic
>>> for the unknown
>>> 3. implement a noop prog (e.g. do
>>> bpf_trace_printk() then return negative and
>>> use the default automq logic for everything)
>>>
>>>> 2. how do we know existing userspace does not rely on existing behaviour
>>> Prior to this change a negative return from a
>>> TUNSETSTEERINGEBPF prog would have been cast
>>> into a u16 and traversed netdev_cap_txqueue().
>>>
>>> In most cases netdev_cap_txqueue() would have
>>> found this value to exceed real_num_tx_queues
>>> and queue_index would be updated to 0.
>>>
>>> It is possible that a TUNSETSTEERINGEBPF prog
>>> return a negative value which when cast into a
>>> u16 results in a positive queue_index less than
>>> real_num_tx_queues. For example, on x86_64, a
>>> return value of -65535 results in a queue_index
>>> of 1; which is a valid queue for any multiqueue
>>> device.
>>>
>>> It seems unlikely, however as stated above is
>>> unfortunately possible, that existing
>>> TUNSETSTEERINGEBPF programs would choose to
>>> return a negative value rather than return the
>>> positive value which holds the same meaning.
>>>
>>> It seems more likely that future
>>> TUNSETSTEERINGEBPF programs would leverage a
>>> negative return and potentially be loaded into
>>> a kernel with the old behavior.
>> OK if we are returning a special
>> value, shouldn't we limit it? How about a special
>> value with this meaning?
>> If we are changing an ABI let's at least make it
>> extensible.
>>
> A special value with this meaning sounds
> good to me. I'll plan on adding a define
> set to -1 to cause the fallback to automq.
Can it really return -1?
I see:
static inline u32 bpf_prog_run_clear_cb(const struct bpf_prog *prog,
struct sk_buff *skb)
...
>
> The way I was initially viewing the old
> behavior was that returning negative was
> undefined; it happened to have the
> outcomes I walked through, but not
> necessarily by design.
Having such fallback may bring extra troubles, it requires the eBPF
program know the existence of the behavior which is not a part of kernel
ABI actually. And then some eBPF program may start to rely on that which
is pretty dangerous. Note, one important consideration is to have
macvtap support where does not have any stuffs like automq.
Thanks
>
> In order to keep the new behavior
> extensible, how should we state that a
> negative return other than -1 is
> undefined and therefore subject to
> change. Is something like this
> sufficient?
>
> Documentation/networking/tc-actions-env-rules.txt
>
> Additionally, what should the new
> behavior implement when a negative other
> than -1 is returned? I would like to have
> it do the same thing as -1 for now, but
> with the understanding that this behavior
> is undefined. Does this sound reasonable?
>
>>>> 3. why doesn't userspace need a way to figure out whether it runs on a kernel with and
>>>> without this patch
>>> There may be some value in exposing this fact
>>> to the ebpf prog loader. What is the standard
>>> practice here, a define?
>>
>> We'll need something at runtime - people move binaries between kernels
>> without rebuilding then. An ioctl is one option.
>> A sysfs attribute is another, an ethtool flag yet another.
>> A combination of these is possible.
>>
>> And if we are doing this anyway, maybe let userspace select
>> the new behaviour? This way we can stay compatible with old
>> userspace...
>>
> Understood. I'll look into adding an
> ioctl to activate the new behavior. And
> perhaps a method of checking which is
> behavior is currently active (in case we
> ever want to change the default, say
> after some suitably long transition
> period).
>
>>>>
>>>> thanks,
>>>> MST
>>>>
>>>>> ---
>>>>> drivers/net/tun.c | 20 +++++++++++---------
>>>>> 1 file changed, 11 insertions(+), 9 deletions(-)
>>>>>
>>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
>>>>> index aab0be4..173d159 100644
>>>>> --- a/drivers/net/tun.c
>>>>> +++ b/drivers/net/tun.c
>>>>> @@ -583,35 +583,37 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb)
>>>>> return txq;
>>>>> }
>>>>>
>>>>> -static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
>>>>> +static int tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
>>>>> {
>>>>> struct tun_prog *prog;
>>>>> u32 numqueues;
>>>>> - u16 ret = 0;
>>>>> + int ret = -1;
>>>>>
>>>>> numqueues = READ_ONCE(tun->numqueues);
>>>>> if (!numqueues)
>>>>> return 0;
>>>>>
>>>>> + rcu_read_lock();
>>>>> prog = rcu_dereference(tun->steering_prog);
>>>>> if (prog)
>>>>> ret = bpf_prog_run_clear_cb(prog->prog, skb);
>>>>> + rcu_read_unlock();
>>>>>
>>>>> - return ret % numqueues;
>>>>> + if (ret >= 0)
>>>>> + ret %= numqueues;
>>>>> +
>>>>> + return ret;
>>>>> }
>>>>>
>>>>> static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
>>>>> struct net_device *sb_dev)
>>>>> {
>>>>> struct tun_struct *tun = netdev_priv(dev);
>>>>> - u16 ret;
>>>>> + int ret;
>>>>>
>>>>> - rcu_read_lock();
>>>>> - if (rcu_dereference(tun->steering_prog))
>>>>> - ret = tun_ebpf_select_queue(tun, skb);
>>>>> - else
>>>>> + ret = tun_ebpf_select_queue(tun, skb);
>>>>> + if (ret < 0)
>>>>> ret = tun_automq_select_queue(tun, skb);
>>>>> - rcu_read_unlock();
>>>>>
>>>>> return ret;
>>>>> }
>>>>> --
>>>>> 1.8.3.1
^ permalink raw reply
* Re: [PATCH net] net: openvswitch: fix possible memleak on createvport fails
From: Tonghao Zhang @ 2019-09-23 0:59 UTC (permalink / raw)
To: Hillf Danton
Cc: Pravin Shelar, Greg Rose, Linux Kernel Network Developers,
Taehee Yoo
In-Reply-To: <5d8719a6.1c69fb81.fb123.2f76SMTPIN_ADDED_MISSING@mx.google.com>
On Sun, Sep 22, 2019 at 2:50 PM Hillf Danton <hdanton@sina.com> wrote:
>
> On Sun, 22 Sep 2019 14:42 from Pravin Shelar <pshelar@ovn.org>
>
> >>
>
> >> On Sat, Sep 21, 2019 at 8:48 PM Hillf Danton <hdanton@sina.com> wrote:
>
> >> Was that posted without netdev Cced?
>
> >
>
> > I do not see your patch on netdev patchwork, repost of the patch would
>
> > put it on netdev patchwork.
>
>
>
> I did send it and you did see it, so no fault on your side and my side.
>
> Where went wrong?
>
> A bit baffled.
I did't not find your patch in the linux upstream, so I send my patch.
Please resent your patch and I hope you should add comment on the
vport->dev->priv_destructor = internal_dev_destructor;
To explain why you move the code here, that can help others review the codes.
^ permalink raw reply
* Re: [RFC PATCH V2 0/6] mdev based hardware virtio offloading support
From: Jason Wang @ 2019-09-23 1:06 UTC (permalink / raw)
To: kvm, linux-s390, linux-kernel, dri-devel, intel-gfx,
intel-gvt-dev, kwankhede, alex.williamson, mst, tiwei.bie
Cc: virtualization, netdev, cohuck, maxime.coquelin, cunming.liang,
zhihong.wang, rob.miller, xiao.w.wang, haotian.wang, zhenyuw,
zhi.a.wang, jani.nikula, joonas.lahtinen, rodrigo.vivi, airlied,
daniel, farman, pasic, sebott, oberpar, heiko.carstens, gor,
borntraeger, akrowiak, pmorel, freude, lingshan.zhu, idos,
eperezma, lulu, parav
In-Reply-To: <20190920082050.19352-1-jasowang@redhat.com>
On 2019/9/20 下午4:20, Jason Wang wrote:
> Hi all:
>
> There are hardware that can do virtio datapath offloading while having
> its own control path. This path tries to implement a mdev based
> unified API to support using kernel virtio driver to drive those
> devices. This is done by introducing a new mdev transport for virtio
> (virtio_mdev) and register itself as a new kind of mdev driver. Then
> it provides a unified way for kernel virtio driver to talk with mdev
> device implementation.
>
> Though the series only contain kernel driver support, the goal is to
> make the transport generic enough to support userspace drivers. This
> means vhost-mdev[1] could be built on top as well by resuing the
> transport.
>
> A sample driver is also implemented which simulate a virito-net
> loopback ethernet device on top of vringh + workqueue. This could be
> used as a reference implementation for real hardware driver.
>
> Consider mdev framework only support VFIO device and driver right now,
> this series also extend it to support other types. This is done
> through introducing class id to the device and pairing it with
> id_talbe claimed by the driver. On top, this seris also decouple
> device specific parents ops out of the common ones.
>
> Pktgen test was done with virito-net + mvnet loop back device.
>
> Please review.
CC Parav.
Thanks
>
> Changes from V1:
>
> - rename device id to class id
> - add docs for class id and device specific ops (device_ops)
> - split device_ops into seperate headers
> - drop the mdev_set_dma_ops()
> - use device_ops to implement the transport API, then it's not a part
> of UAPI any more
> - use GFP_ATOMIC in mvnet sample device and other tweaks
> - set_vring_base/get_vring_base support for mvnet device
>
> Jason Wang (6):
> mdev: class id support
> mdev: introduce device specific ops
> mdev: introduce virtio device and its device ops
> virtio: introudce a mdev based transport
> vringh: fix copy direction of vringh_iov_push_kern()
> docs: Sample driver to demonstrate how to implement virtio-mdev
> framework
>
> .../driver-api/vfio-mediated-device.rst | 11 +-
> drivers/gpu/drm/i915/gvt/kvmgt.c | 17 +-
> drivers/s390/cio/vfio_ccw_ops.c | 17 +-
> drivers/s390/crypto/vfio_ap_ops.c | 14 +-
> drivers/vfio/mdev/Kconfig | 7 +
> drivers/vfio/mdev/Makefile | 1 +
> drivers/vfio/mdev/mdev_core.c | 21 +-
> drivers/vfio/mdev/mdev_driver.c | 14 +
> drivers/vfio/mdev/mdev_private.h | 1 +
> drivers/vfio/mdev/vfio_mdev.c | 37 +-
> drivers/vfio/mdev/virtio_mdev.c | 418 +++++++++++
> drivers/vhost/vringh.c | 8 +-
> include/linux/mdev.h | 46 +-
> include/linux/mod_devicetable.h | 8 +
> include/linux/vfio_mdev.h | 50 ++
> include/linux/virtio_mdev.h | 141 ++++
> samples/Kconfig | 7 +
> samples/vfio-mdev/Makefile | 1 +
> samples/vfio-mdev/mbochs.c | 19 +-
> samples/vfio-mdev/mdpy.c | 19 +-
> samples/vfio-mdev/mtty.c | 17 +-
> samples/vfio-mdev/mvnet.c | 688 ++++++++++++++++++
> 22 files changed, 1473 insertions(+), 89 deletions(-)
> create mode 100644 drivers/vfio/mdev/virtio_mdev.c
> create mode 100644 include/linux/vfio_mdev.h
> create mode 100644 include/linux/virtio_mdev.h
> create mode 100644 samples/vfio-mdev/mvnet.c
>
^ permalink raw reply
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Matt Cover @ 2019-09-23 1:15 UTC (permalink / raw)
To: Jason Wang
Cc: Michael S. Tsirkin, davem, ast, daniel, kafai, songliubraving,
yhs, Eric Dumazet, Stanislav Fomichev, Matthew Cover, mail,
pabeni, Nicolas Dichtel, wangli39, lifei.shirley, tglx, netdev,
linux-kernel, bpf
In-Reply-To: <f2e5b3d5-f38c-40e7-dda9-e1ed737a0135@redhat.com>
On Sun, Sep 22, 2019 at 5:51 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2019/9/23 上午6:30, Matt Cover wrote:
> > On Sun, Sep 22, 2019 at 1:36 PM Michael S. Tsirkin <mst@redhat.com> wrote:
> >> On Sun, Sep 22, 2019 at 10:43:19AM -0700, Matt Cover wrote:
> >>> On Sun, Sep 22, 2019 at 5:37 AM Michael S. Tsirkin <mst@redhat.com> wrote:
> >>>> On Fri, Sep 20, 2019 at 11:58:43AM -0700, Matthew Cover wrote:
> >>>>> Treat a negative return from a TUNSETSTEERINGEBPF bpf prog as a signal
> >>>>> to fallback to tun_automq_select_queue() for tx queue selection.
> >>>>>
> >>>>> Compilation of this exact patch was tested.
> >>>>>
> >>>>> For functional testing 3 additional printk()s were added.
> >>>>>
> >>>>> Functional testing results (on 2 txq tap device):
> >>>>>
> >>>>> [Fri Sep 20 18:33:27 2019] ========== tun no prog ==========
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> >>>>> [Fri Sep 20 18:33:27 2019] ========== tun prog -1 ==========
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '-1'
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '-1'
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_automq_select_queue() ran
> >>>>> [Fri Sep 20 18:33:27 2019] ========== tun prog 0 ==========
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '0'
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> >>>>> [Fri Sep 20 18:33:27 2019] ========== tun prog 1 ==========
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '1'
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '1'
> >>>>> [Fri Sep 20 18:33:27 2019] ========== tun prog 2 ==========
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: bpf_prog_run_clear_cb() returned '2'
> >>>>> [Fri Sep 20 18:33:27 2019] tuntap: tun_ebpf_select_queue() returned '0'
> >>>>>
> >>>>> Signed-off-by: Matthew Cover <matthew.cover@stackpath.com>
> >>>>
> >>>> Could you add a bit more motivation data here?
> >>> Thank you for these questions Michael.
> >>>
> >>> I'll plan on adding the below information to the
> >>> commit message and submitting a v2 of this patch
> >>> when net-next reopens. In the meantime, it would
> >>> be very helpful to know if these answers address
> >>> some of your concerns.
> >>>
> >>>> 1. why is this a good idea
> >>> This change allows TUNSETSTEERINGEBPF progs to
> >>> do any of the following.
> >>> 1. implement queue selection for a subset of
> >>> traffic (e.g. special queue selection logic
> >>> for ipv4, but return negative and use the
> >>> default automq logic for ipv6)
> >>> 2. determine there isn't sufficient information
> >>> to do proper queue selection; return
> >>> negative and use the default automq logic
> >>> for the unknown
> >>> 3. implement a noop prog (e.g. do
> >>> bpf_trace_printk() then return negative and
> >>> use the default automq logic for everything)
> >>>
> >>>> 2. how do we know existing userspace does not rely on existing behaviour
> >>> Prior to this change a negative return from a
> >>> TUNSETSTEERINGEBPF prog would have been cast
> >>> into a u16 and traversed netdev_cap_txqueue().
> >>>
> >>> In most cases netdev_cap_txqueue() would have
> >>> found this value to exceed real_num_tx_queues
> >>> and queue_index would be updated to 0.
> >>>
> >>> It is possible that a TUNSETSTEERINGEBPF prog
> >>> return a negative value which when cast into a
> >>> u16 results in a positive queue_index less than
> >>> real_num_tx_queues. For example, on x86_64, a
> >>> return value of -65535 results in a queue_index
> >>> of 1; which is a valid queue for any multiqueue
> >>> device.
> >>>
> >>> It seems unlikely, however as stated above is
> >>> unfortunately possible, that existing
> >>> TUNSETSTEERINGEBPF programs would choose to
> >>> return a negative value rather than return the
> >>> positive value which holds the same meaning.
> >>>
> >>> It seems more likely that future
> >>> TUNSETSTEERINGEBPF programs would leverage a
> >>> negative return and potentially be loaded into
> >>> a kernel with the old behavior.
> >> OK if we are returning a special
> >> value, shouldn't we limit it? How about a special
> >> value with this meaning?
> >> If we are changing an ABI let's at least make it
> >> extensible.
> >>
> > A special value with this meaning sounds
> > good to me. I'll plan on adding a define
> > set to -1 to cause the fallback to automq.
>
>
> Can it really return -1?
>
> I see:
>
> static inline u32 bpf_prog_run_clear_cb(const struct bpf_prog *prog,
> struct sk_buff *skb)
> ...
>
>
> >
> > The way I was initially viewing the old
> > behavior was that returning negative was
> > undefined; it happened to have the
> > outcomes I walked through, but not
> > necessarily by design.
>
>
> Having such fallback may bring extra troubles, it requires the eBPF
> program know the existence of the behavior which is not a part of kernel
> ABI actually. And then some eBPF program may start to rely on that which
> is pretty dangerous. Note, one important consideration is to have
> macvtap support where does not have any stuffs like automq.
>
> Thanks
>
How about we call this TUN_SSE_ABORT
instead of TUN_SSE_DO_AUTOMQ?
TUN_SSE_ABORT could be documented as
falling back to the default queue
selection method in either space
(presumably macvtap has some queue
selection method when there is no prog).
>
> >
> > In order to keep the new behavior
> > extensible, how should we state that a
> > negative return other than -1 is
> > undefined and therefore subject to
> > change. Is something like this
> > sufficient?
> >
> > Documentation/networking/tc-actions-env-rules.txt
> >
> > Additionally, what should the new
> > behavior implement when a negative other
> > than -1 is returned? I would like to have
> > it do the same thing as -1 for now, but
> > with the understanding that this behavior
> > is undefined. Does this sound reasonable?
> >
> >>>> 3. why doesn't userspace need a way to figure out whether it runs on a kernel with and
> >>>> without this patch
> >>> There may be some value in exposing this fact
> >>> to the ebpf prog loader. What is the standard
> >>> practice here, a define?
> >>
> >> We'll need something at runtime - people move binaries between kernels
> >> without rebuilding then. An ioctl is one option.
> >> A sysfs attribute is another, an ethtool flag yet another.
> >> A combination of these is possible.
> >>
> >> And if we are doing this anyway, maybe let userspace select
> >> the new behaviour? This way we can stay compatible with old
> >> userspace...
> >>
> > Understood. I'll look into adding an
> > ioctl to activate the new behavior. And
> > perhaps a method of checking which is
> > behavior is currently active (in case we
> > ever want to change the default, say
> > after some suitably long transition
> > period).
> >
> >>>>
> >>>> thanks,
> >>>> MST
> >>>>
> >>>>> ---
> >>>>> drivers/net/tun.c | 20 +++++++++++---------
> >>>>> 1 file changed, 11 insertions(+), 9 deletions(-)
> >>>>>
> >>>>> diff --git a/drivers/net/tun.c b/drivers/net/tun.c
> >>>>> index aab0be4..173d159 100644
> >>>>> --- a/drivers/net/tun.c
> >>>>> +++ b/drivers/net/tun.c
> >>>>> @@ -583,35 +583,37 @@ static u16 tun_automq_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> >>>>> return txq;
> >>>>> }
> >>>>>
> >>>>> -static u16 tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> >>>>> +static int tun_ebpf_select_queue(struct tun_struct *tun, struct sk_buff *skb)
> >>>>> {
> >>>>> struct tun_prog *prog;
> >>>>> u32 numqueues;
> >>>>> - u16 ret = 0;
> >>>>> + int ret = -1;
> >>>>>
> >>>>> numqueues = READ_ONCE(tun->numqueues);
> >>>>> if (!numqueues)
> >>>>> return 0;
> >>>>>
> >>>>> + rcu_read_lock();
> >>>>> prog = rcu_dereference(tun->steering_prog);
> >>>>> if (prog)
> >>>>> ret = bpf_prog_run_clear_cb(prog->prog, skb);
> >>>>> + rcu_read_unlock();
> >>>>>
> >>>>> - return ret % numqueues;
> >>>>> + if (ret >= 0)
> >>>>> + ret %= numqueues;
> >>>>> +
> >>>>> + return ret;
> >>>>> }
> >>>>>
> >>>>> static u16 tun_select_queue(struct net_device *dev, struct sk_buff *skb,
> >>>>> struct net_device *sb_dev)
> >>>>> {
> >>>>> struct tun_struct *tun = netdev_priv(dev);
> >>>>> - u16 ret;
> >>>>> + int ret;
> >>>>>
> >>>>> - rcu_read_lock();
> >>>>> - if (rcu_dereference(tun->steering_prog))
> >>>>> - ret = tun_ebpf_select_queue(tun, skb);
> >>>>> - else
> >>>>> + ret = tun_ebpf_select_queue(tun, skb);
> >>>>> + if (ret < 0)
> >>>>> ret = tun_automq_select_queue(tun, skb);
> >>>>> - rcu_read_unlock();
> >>>>>
> >>>>> return ret;
> >>>>> }
> >>>>> --
> >>>>> 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