* 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
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Matt Cover @ 2019-09-23 1:20 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: <0f4541d9-a405-6185-7e54-112dc9188146@redhat.com>
On Sun, Sep 22, 2019 at 5:46 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> 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.
>
I really think there is value in being
able to implement a scoped special
case while leaving the rest of the
packets in the kernel's hands.
Having to reimplement automq makes
this hookpoint less accessible to
beginners and experienced alike.
>
> > 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.
>
In addition to TUN_SSE_ABORT, we can
add TUN_SSE_DROP. That could be made the
default for any undefined negative
return as well.
> 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
* [PATCH net-next] net: core: dev: replace state xoff flag comparison by netif_xmit_stopped method
From: jcfaracco @ 2019-09-23 1:57 UTC (permalink / raw)
To: davem; +Cc: netdev
From: Julio Faracco <jcfaracco@gmail.com>
Function netif_schedule_queue() has a hardcoded comparison between queue
state and any xoff flag. This comparison does the same thing as method
netif_xmit_stopped(). In terms of code clarity, it is better. See other
methods like: generic_xdp_tx() and dev_direct_xmit().
Signed-off-by: Julio Faracco <jcfaracco@gmail.com>
---
net/core/dev.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/core/dev.c b/net/core/dev.c
index 71b18e8..a8ecf88 100644
--- a/net/core/dev.c
+++ b/net/core/dev.c
@@ -2771,7 +2771,7 @@ static struct dev_kfree_skb_cb *get_kfree_skb_cb(const struct sk_buff *skb)
void netif_schedule_queue(struct netdev_queue *txq)
{
rcu_read_lock();
- if (!(txq->state & QUEUE_STATE_ANY_XOFF)) {
+ if (!netif_xmit_stopped(txq)) {
struct Qdisc *q = rcu_dereference(txq->qdisc);
__netif_schedule(q);
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Jason Wang @ 2019-09-23 2:32 UTC (permalink / raw)
To: Matt Cover
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: <CAGyo_hp-PJUg7GWFK996vqRxn=cCEdE=hcWdYhyf4K-nSU9qYQ@mail.gmail.com>
On 2019/9/23 上午9:20, Matt Cover wrote:
> On Sun, Sep 22, 2019 at 5:46 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> 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.
>>
> I really think there is value in being
> able to implement a scoped special
> case while leaving the rest of the
> packets in the kernel's hands.
This is only work when some fucntion could not be done by eBPF itself
and then we can provide the function through eBPF helpers. But this is
not the case here.
>
> Having to reimplement automq makes
> this hookpoint less accessible to
> beginners and experienced alike.
Note that automq itself is kind of complicated, it's best effort that is
hard to be documented accurately. It has several limitations (e.g flow
caches etc.) that may not work well in some conditions.
It's not hard to implement a user programmable steering policy through
maps which could have much deterministic behavior than automq. The goal
of steering ebpf is to get rid of automq completely not partially rely
on it.
And I don't see how relying on automq can simplify anything.
Thanks
>
>>> 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.
>>
> In addition to TUN_SSE_ABORT, we can
> add TUN_SSE_DROP. That could be made the
> default for any undefined negative
> return as well.
>
>> 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 2:34 UTC (permalink / raw)
To: Matt Cover
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: <CAGyo_hohbFP+=eu3jWL954hrOgqu4upaw6HTH2=1qC9jcENWxQ@mail.gmail.com>
On 2019/9/23 上午9:15, Matt Cover wrote:
> 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).
This looks like a more complex API, we don't want userspace to differ
macvtap from tap too much.
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-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Matt Cover @ 2019-09-23 3:00 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: <df4ee92f-89e4-70a7-2de8-49fa4acfa08e@redhat.com>
On Sun, Sep 22, 2019 at 7:32 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2019/9/23 上午9:20, Matt Cover wrote:
> > On Sun, Sep 22, 2019 at 5:46 PM Jason Wang <jasowang@redhat.com> wrote:
> >>
> >> 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.
> >>
> > I really think there is value in being
> > able to implement a scoped special
> > case while leaving the rest of the
> > packets in the kernel's hands.
>
>
> This is only work when some fucntion could not be done by eBPF itself
> and then we can provide the function through eBPF helpers. But this is
> not the case here.
>
>
> >
> > Having to reimplement automq makes
> > this hookpoint less accessible to
> > beginners and experienced alike.
>
>
> Note that automq itself is kind of complicated, it's best effort that is
> hard to be documented accurately. It has several limitations (e.g flow
> caches etc.) that may not work well in some conditions.
>
> It's not hard to implement a user programmable steering policy through
> maps which could have much deterministic behavior than automq. The goal
> of steering ebpf is to get rid of automq completely not partially rely
> on it.
>
> And I don't see how relying on automq can simplify anything.
>
> Thanks
>
I'm not suggesting that we document automq.
I'm suggesting that we add a return value
which is documented as signaling to the
kernel to implement whatever queue
selection method is used when there is no
ebpf prog attached. That behavior today is
automq.
There is nothing about this return value
which would harder to change the default
queue selection later. The default already
exists today when there is no program
loaded.
>
> >
> >>> 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.
> >>
> > In addition to TUN_SSE_ABORT, we can
> > add TUN_SSE_DROP. That could be made the
> > default for any undefined negative
> > return as well.
> >
> >> 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: Matt Cover @ 2019-09-23 3:18 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: <7d3abb5d-c5a7-9fbd-f82e-88b4bf717a0b@redhat.com>
On Sun, Sep 22, 2019 at 7:34 PM Jason Wang <jasowang@redhat.com> wrote:
>
>
> On 2019/9/23 上午9:15, Matt Cover wrote:
> > 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).
>
>
> This looks like a more complex API, we don't want userspace to differ
> macvtap from tap too much.
>
> Thanks
>
This is barely more complex and provides
similar to what is done in many places.
For xdp, an XDP_PASS enacts what the
kernel would do if there was no bpf prog.
For tc cls in da mode, TC_ACT_OK enacts
what the kernel would do if there was
no bpf prog. For xt_bpf, false enacts
what the kernel would do if there was
no bpf prog (as long as negation
isn't in play in the rule, I believe).
I know that this is somewhat of an
oversimplification and that each of
these also means something else in
the respective hookpoint, but I standby
seeing value in this change.
macvtap must have some default (i.e the
action which it takes when no prog is
loaded), even if that is just use queue
0. We can provide the same TUN_SSE_ABORT
in userspace which does the same thing;
enacts the default when returned. Any
differences left between tap and macvtap
would be in what the default is, not in
these changes. And that difference already
exists today.
>
> >
> >>> 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: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm.
From: Jianyong Wu (Arm Technology China) @ 2019-09-23 3:19 UTC (permalink / raw)
To: Paolo Bonzini, Marc Zyngier, netdev@vger.kernel.org,
yangbo.lu@nxp.com, john.stultz@linaro.org, tglx@linutronix.de,
sean.j.christopherson@intel.com, richardcochran@gmail.com,
Mark Rutland, Will Deacon, Suzuki Poulose
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Steve Capper,
Kaly Xin (Arm Technology China), Justin He (Arm Technology China),
nd, linux-arm-kernel@lists.infradead.org
In-Reply-To: <56a5b885-62c8-c4ef-e2f8-e945c0eb700e@redhat.com>
Hi Marc, Paolo,
> -----Original Message-----
> From: Paolo Bonzini <pbonzini@redhat.com>
> Sent: Thursday, September 19, 2019 8:13 PM
> To: Marc Zyngier <maz@kernel.org>; Jianyong Wu (Arm Technology China)
> <Jianyong.Wu@arm.com>; netdev@vger.kernel.org; yangbo.lu@nxp.com;
> john.stultz@linaro.org; tglx@linutronix.de; sean.j.christopherson@intel.com;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will
> Deacon <Will.Deacon@arm.com>; Suzuki Poulose
> <Suzuki.Poulose@arm.com>
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm.
>
> On 19/09/19 13:39, Marc Zyngier wrote:
> >> I don't think it's ugly but more important, using tk->tkr_mono.clock
> >> is incorrect. See how the x86 code hardcodes &kvm_clock, it's the
> >> same for ARM.
> > Not really. The guest kernel is free to use any clocksource it wishes.
>
> Understood, in fact it's the same on x86.
>
> However, for PTP to work, the cycles value returned by the clocksource must
> match the one returned by the hypercall. So for ARM
> get_device_system_crosststamp must receive the arch timer clocksource, so
> that it will return -ENODEV if the active clocksource is anything else.
>
After day's reflection on this, I'm a little clear about this issue, let me clarify it.
I think there is three principles for this issue:
(1): guest and host can use different clocksouces as their current clocksouce at the same time
and we can't or it's not easy to probe that if they use the same one.
(2): the cycle value and the clocksouce which passed to get_device_system_crosststamp must be match.
(3): ptp_kvm target to increase the time sync precision so we should choose a high rate clocksource as ptp_kvm clocksource.
Base on (1) and (2) we can deduce that the ptp_kvm clocksource should be better a fixed one. So we can test if the cycle and
clocksource is match.
Base on (3) the arch_sys_timer should be chosen, as it's the best clocksource by far for arm.
Thanks
Jianyong Wu
> Paolo
>
> > In some cases, it is actually desirable (like these broken systems
> > that cannot use an in-kernel irqchip...). Maybe it is that on x86 the
> > guest only uses the kvm_clock, but that's a much harder sell on ARM.
> > The fact that ptp_kvm assumes that the clocksource is fixed doesn't
> > seem correct in that case.
^ permalink raw reply
* one question about openvswitch flow_hash
From: Tonghao Zhang @ 2019-09-23 3:55 UTC (permalink / raw)
To: Pravin Shelar; +Cc: Linux Kernel Network Developers, ovs-discuss
Hi Pravin
one question about openvswitch, why we add BUILD_BUG_ON in flow_hash?
It is never be true, right ?
flow_hash
...
/* Make sure number of hash bytes are multiple of u32. */
BUILD_BUG_ON(sizeof(long) % sizeof(u32));
...
^ permalink raw reply
* Re: [PATCH net v3] net/sched: cls_api: Fix nooffloaddevcnt counter when indr block call success
From: wenxu @ 2019-09-23 4:19 UTC (permalink / raw)
To: Or Gerlitz, Pieter Jansen van Vuuren, Oz Shlomo, John Hurley,
Jakub Kicinski
Cc: David Miller, Linux Netdev List, Roi Dayan, Paul Blakey,
Jiri Pirko
In-Reply-To: <CAJ3xEMhQTr=HPsMs-j3_V6XRKHa0Jo7iYVY+R4U8etoEu9R7jw@mail.gmail.com>
Hi John & Jakub
There are some limitations for indirect tc callback work with skip_sw ?
BR
wenxu
On 9/19/2019 8:50 PM, Or Gerlitz wrote:
>
>> successfully bind with a real hw through indr block call, It also add
>> nooffloadcnt counter. This counter will lead the rule add failed in
>> fl_hw_replace_filter-->tc_setup_cb_call with skip_sw flags.
> wait.. indirect tc callbacks are typically used to do hw offloading
> for decap rules (tunnel key unset action) set on SW devices (gretap, vxlan).
>
> However, AFAIK, it's been couple of years since the kernel doesn't support
> skip_sw for such rules. Did we enable it again? when? I am somehow
> far from the details, so copied some folks..
>
> Or.
>
>
>> In the tc_setup_cb_call will check the nooffloaddevcnt and skip_sw flags
>> as following:
>> if (block->nooffloaddevcnt && err_stop)
>> return -EOPNOTSUPP;
>>
>> So with this patch, if the indr block call success, it will not modify
>> the nooffloaddevcnt counter.
>>
>> Fixes: 7f76fa36754b ("net: sched: register callbacks for indirect tc block binds")
>> Signed-off-by: wenxu <wenxu@ucloud.cn>
>> ---
>> v3: rebase to the net
>>
>> net/sched/cls_api.c | 30 +++++++++++++++++-------------
>> 1 file changed, 17 insertions(+), 13 deletions(-)
>>
>> diff --git a/net/sched/cls_api.c b/net/sched/cls_api.c
>> index 32577c2..c980127 100644
>> --- a/net/sched/cls_api.c
>> +++ b/net/sched/cls_api.c
>> @@ -607,11 +607,11 @@ static void tc_indr_block_get_and_ing_cmd(struct net_device *dev,
>> tc_indr_block_ing_cmd(dev, block, cb, cb_priv, command);
>> }
>>
>> -static void tc_indr_block_call(struct tcf_block *block,
>> - struct net_device *dev,
>> - struct tcf_block_ext_info *ei,
>> - enum flow_block_command command,
>> - struct netlink_ext_ack *extack)
>> +static int tc_indr_block_call(struct tcf_block *block,
>> + struct net_device *dev,
>> + struct tcf_block_ext_info *ei,
>> + enum flow_block_command command,
>> + struct netlink_ext_ack *extack)
>> {
>> struct flow_block_offload bo = {
>> .command = command,
>> @@ -621,10 +621,15 @@ static void tc_indr_block_call(struct tcf_block *block,
>> .block_shared = tcf_block_shared(block),
>> .extack = extack,
>> };
>> +
>> INIT_LIST_HEAD(&bo.cb_list);
>>
>> flow_indr_block_call(dev, &bo, command);
>> - tcf_block_setup(block, &bo);
>> +
>> + if (list_empty(&bo.cb_list))
>> + return -EOPNOTSUPP;
>> +
>> + return tcf_block_setup(block, &bo);
>> }
>>
>> static bool tcf_block_offload_in_use(struct tcf_block *block)
>> @@ -681,8 +686,6 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
>> goto no_offload_dev_inc;
>> if (err)
>> goto err_unlock;
>> -
>> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
>> up_write(&block->cb_lock);
>> return 0;
>>
>> @@ -691,9 +694,10 @@ static int tcf_block_offload_bind(struct tcf_block *block, struct Qdisc *q,
>> err = -EOPNOTSUPP;
>> goto err_unlock;
>> }
>> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
>> + if (err)
>> + block->nooffloaddevcnt++;
>> err = 0;
>> - block->nooffloaddevcnt++;
>> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_BIND, extack);
>> err_unlock:
>> up_write(&block->cb_lock);
>> return err;
>> @@ -706,8 +710,6 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
>> int err;
>>
>> down_write(&block->cb_lock);
>> - tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
>> -
>> if (!dev->netdev_ops->ndo_setup_tc)
>> goto no_offload_dev_dec;
>> err = tcf_block_offload_cmd(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
>> @@ -717,7 +719,9 @@ static void tcf_block_offload_unbind(struct tcf_block *block, struct Qdisc *q,
>> return;
>>
>> no_offload_dev_dec:
>> - WARN_ON(block->nooffloaddevcnt-- == 0);
>> + err = tc_indr_block_call(block, dev, ei, FLOW_BLOCK_UNBIND, NULL);
>> + if (err)
>> + WARN_ON(block->nooffloaddevcnt-- == 0);
>> up_write(&block->cb_lock);
>> }
>>
>> --
>> 1.8.3.1
>>
^ permalink raw reply
* RE: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm.
From: Jianyong Wu (Arm Technology China) @ 2019-09-23 4:57 UTC (permalink / raw)
To: Paolo Bonzini, netdev@vger.kernel.org, yangbo.lu@nxp.com,
john.stultz@linaro.org, tglx@linutronix.de,
sean.j.christopherson@intel.com, maz@kernel.org,
richardcochran@gmail.com, Mark Rutland, Will Deacon,
Suzuki Poulose
Cc: linux-kernel@vger.kernel.org, kvm@vger.kernel.org, Steve Capper,
Kaly Xin (Arm Technology China), Justin He (Arm Technology China),
nd, linux-arm-kernel@lists.infradead.org
In-Reply-To: <ef6ab8bd-41ad-88f8-9cfd-dc749ca65310@redhat.com>
Hi Paolo, Marc
> -----Original Message-----
> From: Paolo Bonzini <pbonzini@redhat.com>
> Sent: Thursday, September 19, 2019 7:07 PM
> To: Jianyong Wu (Arm Technology China) <Jianyong.Wu@arm.com>;
> netdev@vger.kernel.org; yangbo.lu@nxp.com; john.stultz@linaro.org;
> tglx@linutronix.de; sean.j.christopherson@intel.com; maz@kernel.org;
> richardcochran@gmail.com; Mark Rutland <Mark.Rutland@arm.com>; Will
> Deacon <Will.Deacon@arm.com>; Suzuki Poulose
> <Suzuki.Poulose@arm.com>
> Cc: linux-kernel@vger.kernel.org; kvm@vger.kernel.org; Steve Capper
> <Steve.Capper@arm.com>; Kaly Xin (Arm Technology China)
> <Kaly.Xin@arm.com>; Justin He (Arm Technology China)
> <Justin.He@arm.com>; nd <nd@arm.com>; linux-arm-
> kernel@lists.infradead.org
> Subject: Re: [RFC PATCH v3 4/6] psci: Add hvc call service for ptp_kvm.
>
> On 19/09/19 11:46, Jianyong Wu (Arm Technology China) wrote:
> >> On 18/09/19 11:57, Jianyong Wu (Arm Technology China) wrote:
> >>> Paolo Bonzini wrote:
> >>>> This is not Y2038-safe. Please use ktime_get_real_ts64 instead,
> >>>> and split the 64-bit seconds value between val[0] and val[1].
> >
> > Val[] should be long not u32 I think, so in arm64 I can avoid that
> > Y2038_safe, but also need rewrite for arm32.
>
> I don't think there's anything inherently wrong with u32 val[], and as you
> notice it lets you reuse code between arm and arm64. It's up to you and
> Marc to decide.
>
To compatible 32-bit, Integrates second value and nanosecond value as a nanosecond value then split it into val[0] and val[1] and split cycle value into val[2] and val[3],
In this way, time will overflow at Y2262.
WDYT?
Thanks
Jianyong Wu
^ permalink raw reply
* Re: [PATCH net-next] tuntap: Fallback to automq on TUNSETSTEERINGEBPF prog negative return
From: Jason Wang @ 2019-09-23 5:08 UTC (permalink / raw)
To: Matt Cover
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: <CAGyo_hrM8TzTp6L77x-_18XZn+NEcDyZXAxnwgNaCgMBLpCMPg@mail.gmail.com>
On 2019/9/23 上午11:00, Matt Cover wrote:
> On Sun, Sep 22, 2019 at 7:32 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2019/9/23 上午9:20, Matt Cover wrote:
>>> On Sun, Sep 22, 2019 at 5:46 PM Jason Wang <jasowang@redhat.com> wrote:
>>>> 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.
>>>>
>>> I really think there is value in being
>>> able to implement a scoped special
>>> case while leaving the rest of the
>>> packets in the kernel's hands.
>>
>> This is only work when some fucntion could not be done by eBPF itself
>> and then we can provide the function through eBPF helpers. But this is
>> not the case here.
>>
>>
>>> Having to reimplement automq makes
>>> this hookpoint less accessible to
>>> beginners and experienced alike.
>>
>> Note that automq itself is kind of complicated, it's best effort that is
>> hard to be documented accurately. It has several limitations (e.g flow
>> caches etc.) that may not work well in some conditions.
>>
>> It's not hard to implement a user programmable steering policy through
>> maps which could have much deterministic behavior than automq. The goal
>> of steering ebpf is to get rid of automq completely not partially rely
>> on it.
>>
>> And I don't see how relying on automq can simplify anything.
>>
>> Thanks
>>
> I'm not suggesting that we document automq.
>
> I'm suggesting that we add a return value
> which is documented as signaling to the
> kernel to implement whatever queue
> selection method is used when there is no
> ebpf prog attached.
Again, this only work when there's something that could not be done
through eBPF. And then we can provide eBPF helper there.
> That behavior today is
> automq.
Automq is not good, e.g tun_ebpf_select_queue() has already provided a
fallback, anything that automq can do better than that?
>
> There is nothing about this return value
> which would harder to change the default
> queue selection later. The default already
> exists today when there is no program
> loaded.
The patch depends on incorrect behavior of tuntap (updating flow caches
when steering prog is set). I think it's wrong to update flow caches
even when steering program is set which leads extra overhead. Will
probably submit a patch to disable that behavior.
Thanks
>
>>>>> 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.
>>>>
>>> In addition to TUN_SSE_ABORT, we can
>>> add TUN_SSE_DROP. That could be made the
>>> default for any undefined negative
>>> return as well.
>>>
>>>> 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 5:15 UTC (permalink / raw)
To: Matt Cover
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: <CAGyo_hondiOXi8GtqZg-YNV3A+COV=5PMHoNKaHbBjnTRTUe9Q@mail.gmail.com>
On 2019/9/23 上午11:18, Matt Cover wrote:
> On Sun, Sep 22, 2019 at 7:34 PM Jason Wang <jasowang@redhat.com> wrote:
>>
>> On 2019/9/23 上午9:15, Matt Cover wrote:
>>> 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).
>>
>> This looks like a more complex API, we don't want userspace to differ
>> macvtap from tap too much.
>>
>> Thanks
>>
> This is barely more complex and provides
> similar to what is done in many places.
> For xdp, an XDP_PASS enacts what the
> kernel would do if there was no bpf prog.
> For tc cls in da mode, TC_ACT_OK enacts
> what the kernel would do if there was
> no bpf prog. For xt_bpf, false enacts
> what the kernel would do if there was
> no bpf prog (as long as negation
> isn't in play in the rule, I believe).
I think this is simply because you can't implement e.g
XDP_PASS/TC_ACT_OK through eBPF itself which is not the case of steering
prog here.
>
> I know that this is somewhat of an
> oversimplification and that each of
> these also means something else in
> the respective hookpoint, but I standby
> seeing value in this change.
>
> macvtap must have some default (i.e the
> action which it takes when no prog is
> loaded), even if that is just use queue
> 0. We can provide the same TUN_SSE_ABORT
> in userspace which does the same thing;
> enacts the default when returned. Any
> differences left between tap and macvtap
> would be in what the default is, not in
> these changes. And that difference already
> exists today.
I think it's better to safe to just drop the packet instead of trying to
workaround it.
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
* [PATCH net] gianfar: Make reset_gfar static
From: YueHaibing @ 2019-09-23 6:16 UTC (permalink / raw)
To: claudiu.manoil, davem, jakub.kicinski; +Cc: netdev, linux-kernel, YueHaibing
Fix sparse warning:
drivers/net/ethernet/freescale/gianfar.c:2070:6:
warning: symbol 'reset_gfar' was not declared. Should it be static?
Reported-by: Hulk Robot <hulkci@huawei.com>
Signed-off-by: YueHaibing <yuehaibing@huawei.com>
---
drivers/net/ethernet/freescale/gianfar.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 24bf7f6..51ad864 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -2067,7 +2067,7 @@ static int gfar_change_mtu(struct net_device *dev, int new_mtu)
return 0;
}
-void reset_gfar(struct net_device *ndev)
+static void reset_gfar(struct net_device *ndev)
{
struct gfar_private *priv = netdev_priv(ndev);
--
2.7.4
^ permalink raw reply related
* Re: [PATCH] dimlib: make DIMLIB a hidden symbol
From: Uwe Kleine-König @ 2019-09-23 6:18 UTC (permalink / raw)
To: Jakub Kicinski; +Cc: Tal Gilboa, Saeed Mahameed, linux-kernel, netdev
In-Reply-To: <20190921200638.5baf40a9@cakuba.netronome.com>
[-- Attachment #1.1: Type: text/plain, Size: 1506 bytes --]
Hello Jakub,
On 9/22/19 5:06 AM, Jakub Kicinski wrote:
> On Fri, 20 Sep 2019 15:31:15 +0200, Uwe Kleine-König wrote:
>> According to Tal Gilboa the only benefit from DIM comes from a driver
>> that uses it. So it doesn't make sense to make this symbol user visible,
>> instead all drivers that use it should select it (as is already the case
>> AFAICT).
>>
>> Signed-off-by: Uwe Kleine-König <uwe@kleine-koenig.org>
>> ---
>> lib/Kconfig | 3 +--
>> 1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/lib/Kconfig b/lib/Kconfig
>> index cc04124ed8f7..9fe8a21fd183 100644
>> --- a/lib/Kconfig
>> +++ b/lib/Kconfig
>> @@ -555,8 +555,7 @@ config SIGNATURE
>> Implementation is done using GnuPG MPI library
>>
>> config DIMLIB
>> - bool "DIM library"
>> - default y
>> + bool
>> help
>> Dynamic Interrupt Moderation library.
>> Implements an algorithm for dynamically change CQ moderation values
>
> Hi Uwe! Looks like in the net tree there is a spelling mistake and
> moderation is spelled "modertion":
>
> https://git.kernel.org/pub/scm/linux/kernel/git/davem/net.git/tree/lib/Kconfig#n562
>
> I'm not seeing any patch to fix that anywhere, is it possible you have
> some local change in your tree?
I sent a patch[1] for that typo, but at that time wasn't aware that
DIMLIB was relevant for net and so didn't Cc the netdev.
Best regards
Uwe
[1]
https://lore.kernel.org/lkml/20190919210314.22110-1-uwe@kleine-koenig.org/
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 488 bytes --]
^ permalink raw reply
* [PATCH net] net: stmmac: selftests: Flow Control test can also run with ASYM Pause
From: Jose Abreu @ 2019-09-23 7:30 UTC (permalink / raw)
To: netdev
Cc: Joao Pinto, Jose Abreu, Jose Abreu, Giuseppe Cavallaro,
Alexandre Torgue, David S. Miller, Maxime Coquelin, linux-stm32,
linux-arm-kernel, linux-kernel
From: Jose Abreu <joabreu@synopsys.com>
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 <Jose.Abreu@synopsys.com>
---
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 9c8d210b2d6a..5f66f6161629 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -670,7 +670,7 @@ static int stmmac_test_flowctrl(struct stmmac_priv *priv)
unsigned int pkt_count;
int i, ret = 0;
- if (!phydev || !phydev->pause)
+ if (!phydev || (!phydev->pause && !phydev->asym_pause))
return -EOPNOTSUPP;
tpriv = kzalloc(sizeof(*tpriv), GFP_KERNEL);
--
2.7.4
^ permalink raw reply related
* RE: [PATCH net] net: stmmac: selftests: Flow Control test can also run with ASYM Pause
From: Jose Abreu @ 2019-09-23 7:32 UTC (permalink / raw)
To: Jakub Kicinski, Jose Abreu
Cc: netdev@vger.kernel.org, Joao Pinto, Giuseppe Cavallaro,
Alexandre Torgue, David S. Miller, Maxime Coquelin,
linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20190922145642.55453135@cakuba.netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Sep/22/2019, 22:56:42 (UTC+00:00)
> 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.
Yeah that's my external vs. internal email messing around. Any of them
works though. I resent it using the external one. Can you please take a
look ?
---
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH 0/5] net: ethernet: stmmac: some fixes and optimization
From: Christophe ROULLIER @ 2019-09-23 7:46 UTC (permalink / raw)
To: Jakub Kicinski
Cc: robh@kernel.org, davem@davemloft.net, joabreu@synopsys.com,
mark.rutland@arm.com, mcoquelin.stm32@gmail.com, Alexandre TORGUE,
Peppe CAVALLARO, linux-stm32@st-md-mailman.stormreply.com,
linux-kernel@vger.kernel.org, devicetree@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, netdev@vger.kernel.org,
andrew@lunn.ch
In-Reply-To: <20190922151257.51173d89@cakuba.netronome.com>
Hi Jakub, all,
It is not urgent, no problem to wait next merge window (release 5.5)
For patch 1 and 3, it is improvement/cleanup because now syscfg clock is
not mandatory (I put code backward compatible).
Regards,
Christophe
On 9/23/19 12:12 AM, Jakub Kicinski wrote:
> 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] net: stmmac: selftests: Flow Control test can also run with ASYM Pause
From: Jose Abreu @ 2019-09-23 7:47 UTC (permalink / raw)
To: Jose Abreu, netdev@vger.kernel.org
Cc: Joao Pinto, Giuseppe Cavallaro, Alexandre Torgue, David S. Miller,
Maxime Coquelin, linux-stm32@st-md-mailman.stormreply.com,
linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
In-Reply-To: <da7e2fb08061b4b89332c0ef014e053f98832894.1569223775.git.Jose.Abreu@synopsys.com>
From: Jose Abreu <Jose.Abreu@synopsys.com>
Date: Sep/23/2019, 08:30:43 (UTC+00:00)
> From: Jose Abreu <joabreu@synopsys.com>
>
> 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 <Jose.Abreu@synopsys.com>
Oops, "From:" and "Signed-off-by:" are still different. I'll amend this
and re-submit. Please disregard this one.
---
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* [PATCH net v2] net: stmmac: selftests: Flow Control test can also run with ASYM Pause
From: Jose Abreu @ 2019-09-23 7:49 UTC (permalink / raw)
To: netdev
Cc: Joao Pinto, Jose Abreu, Giuseppe Cavallaro, Alexandre Torgue,
Jose Abreu, David S. Miller, Maxime Coquelin, linux-stm32,
linux-arm-kernel, linux-kernel
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 <Jose.Abreu@synopsys.com>
---
Cc: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre Torgue <alexandre.torgue@st.com>
Cc: Jose Abreu <joabreu@synopsys.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Maxime Coquelin <mcoquelin.stm32@gmail.com>
Cc: netdev@vger.kernel.org
Cc: linux-stm32@st-md-mailman.stormreply.com
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
index 9c8d210b2d6a..5f66f6161629 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_selftests.c
@@ -670,7 +670,7 @@ static int stmmac_test_flowctrl(struct stmmac_priv *priv)
unsigned int pkt_count;
int i, ret = 0;
- if (!phydev || !phydev->pause)
+ if (!phydev || (!phydev->pause && !phydev->asym_pause))
return -EOPNOTSUPP;
tpriv = kzalloc(sizeof(*tpriv), GFP_KERNEL);
--
2.7.4
^ permalink raw reply related
* [PATCH] vhost: It's better to use size_t for the 3rd parameter of vhost_exceeds_weight()
From: wangxu @ 2019-09-23 7:46 UTC (permalink / raw)
To: jasowang, mst; +Cc: kvm, virtualization, netdev, linux-kernel
From: Wang Xu <wangxu72@huawei.com>
Caller of vhost_exceeds_weight(..., total_len) in drivers/vhost/net.c
usually pass size_t total_len, which may be affected by rx/tx package.
Signed-off-by: Wang Xu <wangxu72@huawei.com>
---
drivers/vhost/vhost.c | 4 ++--
drivers/vhost/vhost.h | 7 ++++---
2 files changed, 6 insertions(+), 5 deletions(-)
diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
index 36ca2cf..159223a 100644
--- a/drivers/vhost/vhost.c
+++ b/drivers/vhost/vhost.c
@@ -412,7 +412,7 @@ static void vhost_dev_free_iovecs(struct vhost_dev *dev)
}
bool vhost_exceeds_weight(struct vhost_virtqueue *vq,
- int pkts, int total_len)
+ int pkts, size_t total_len)
{
struct vhost_dev *dev = vq->dev;
@@ -454,7 +454,7 @@ static size_t vhost_get_desc_size(struct vhost_virtqueue *vq,
void vhost_dev_init(struct vhost_dev *dev,
struct vhost_virtqueue **vqs, int nvqs,
- int iov_limit, int weight, int byte_weight)
+ int iov_limit, int weight, size_t byte_weight)
{
struct vhost_virtqueue *vq;
int i;
diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
index e9ed272..8d80389d 100644
--- a/drivers/vhost/vhost.h
+++ b/drivers/vhost/vhost.h
@@ -172,12 +172,13 @@ struct vhost_dev {
wait_queue_head_t wait;
int iov_limit;
int weight;
- int byte_weight;
+ size_t byte_weight;
};
-bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int total_len);
+bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts,
+ size_t total_len);
void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs,
- int nvqs, int iov_limit, int weight, int byte_weight);
+ int nvqs, int iov_limit, int weight, size_t byte_weight);
long vhost_dev_set_owner(struct vhost_dev *dev);
bool vhost_dev_has_owner(struct vhost_dev *dev);
long vhost_dev_check_owner(struct vhost_dev *);
--
1.8.5.6
^ permalink raw reply related
* Re: [RFC] VSOCK: add support for MSG_PEEK
From: Stefano Garzarella @ 2019-09-23 7:58 UTC (permalink / raw)
To: Matias Ezequiel Vara Larsen
Cc: stefanha, davem, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1569174507-15267-1-git-send-email-matiasevara@gmail.com>
Hi Matias,
thanks for this patch!
Since this patch only concerns virtio_transport,
I'd use the 'vsock/virtio' prefix in the commit title:
"vsock/virtio: add support for MSG_PEEK"
Some comments below:
On Sun, Sep 22, 2019 at 05:48:27PM +0000, Matias Ezequiel Vara Larsen wrote:
> This patch adds support for MSG_PEEK. In such a case, packets are not
> removed from the rx_queue and credit updates are not sent.
>
> Signed-off-by: Matias Ezequiel Vara Larsen <matiasevara@gmail.com>
> ---
> net/vmw_vsock/virtio_transport_common.c | 59 +++++++++++++++++++++++++++++++--
> 1 file changed, 56 insertions(+), 3 deletions(-)
>
> diff --git a/net/vmw_vsock/virtio_transport_common.c b/net/vmw_vsock/virtio_transport_common.c
> index 94cc0fa..830e890 100644
> --- a/net/vmw_vsock/virtio_transport_common.c
> +++ b/net/vmw_vsock/virtio_transport_common.c
> @@ -264,6 +264,59 @@ static int virtio_transport_send_credit_update(struct vsock_sock *vsk,
> }
>
> static ssize_t
> +virtio_transport_stream_do_peek(struct vsock_sock *vsk,
> + struct msghdr *msg,
> + size_t len)
> +{
> + struct virtio_vsock_sock *vvs = vsk->trans;
> + struct virtio_vsock_pkt *pkt;
> + size_t bytes, off = 0, total = 0;
> + int err = -EFAULT;
> +
> + spin_lock_bh(&vvs->rx_lock);
> +
What about using list_for_each_entry() to cycle through the queued packets?
> + if (list_empty(&vvs->rx_queue)) {
> + spin_unlock_bh(&vvs->rx_lock);
> + return 0;
> + }
> +
> + pkt = list_first_entry(&vvs->rx_queue,
> + struct virtio_vsock_pkt, list);
> + do {
pkt->off contains the offset inside the packet where the unread data starts.
So here we should initialize 'off':
off = pkt->off;
Or just use pkt->off later (without increasing it as in the dequeue).
> + bytes = len - total;
> + if (bytes > pkt->len - off)
> + bytes = pkt->len - off;
> +
> + /* sk_lock is held by caller so no one else can dequeue.
> + * Unlock rx_lock since memcpy_to_msg() may sleep.
> + */
> + spin_unlock_bh(&vvs->rx_lock);
> +
> + err = memcpy_to_msg(msg, pkt->buf + off, bytes);
> + if (err)
> + goto out;
> +
> + spin_lock_bh(&vvs->rx_lock);
> +
> + total += bytes;
Using list_for_each_entry(), here we can just do:
(or better, at the beginning of the cycle)
if (total == len)
break;
removing the next part...
> + off += bytes;
> + if (off == pkt->len) {
> + pkt = list_next_entry(pkt, list);
> + off = 0;
> + }
> + } while ((total < len) && !list_is_first(&pkt->list, &vvs->rx_queue));
...until here.
> +
> + spin_unlock_bh(&vvs->rx_lock);
> +
> + return total;
> +
> +out:
> + if (total)
> + err = total;
> + return err;
> +}
> +
> +static ssize_t
> virtio_transport_stream_do_dequeue(struct vsock_sock *vsk,
> struct msghdr *msg,
> size_t len)
> @@ -330,9 +383,9 @@ virtio_transport_stream_dequeue(struct vsock_sock *vsk,
> size_t len, int flags)
> {
> if (flags & MSG_PEEK)
> - return -EOPNOTSUPP;
> -
> - return virtio_transport_stream_do_dequeue(vsk, msg, len);
> + return virtio_transport_stream_do_peek(vsk, msg, len);
> + else
> + return virtio_transport_stream_do_dequeue(vsk, msg, len);
> }
> EXPORT_SYMBOL_GPL(virtio_transport_stream_dequeue);
>
The rest looks good to me!
Thanks,
Stefano
^ permalink raw reply
* Re: [PATCH] vhost: It's better to use size_t for the 3rd parameter of vhost_exceeds_weight()
From: Michael S. Tsirkin @ 2019-09-23 8:07 UTC (permalink / raw)
To: wangxu; +Cc: jasowang, kvm, virtualization, netdev, linux-kernel
In-Reply-To: <1569224801-101248-1-git-send-email-wangxu72@huawei.com>
On Mon, Sep 23, 2019 at 03:46:41PM +0800, wangxu wrote:
> From: Wang Xu <wangxu72@huawei.com>
>
> Caller of vhost_exceeds_weight(..., total_len) in drivers/vhost/net.c
> usually pass size_t total_len, which may be affected by rx/tx package.
>
> Signed-off-by: Wang Xu <wangxu72@huawei.com>
Puts a bit more pressure on the register file ...
why do we care? Is there some way that it can
exceed INT_MAX?
> ---
> drivers/vhost/vhost.c | 4 ++--
> drivers/vhost/vhost.h | 7 ++++---
> 2 files changed, 6 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 36ca2cf..159223a 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -412,7 +412,7 @@ static void vhost_dev_free_iovecs(struct vhost_dev *dev)
> }
>
> bool vhost_exceeds_weight(struct vhost_virtqueue *vq,
> - int pkts, int total_len)
> + int pkts, size_t total_len)
> {
> struct vhost_dev *dev = vq->dev;
>
> @@ -454,7 +454,7 @@ static size_t vhost_get_desc_size(struct vhost_virtqueue *vq,
>
> void vhost_dev_init(struct vhost_dev *dev,
> struct vhost_virtqueue **vqs, int nvqs,
> - int iov_limit, int weight, int byte_weight)
> + int iov_limit, int weight, size_t byte_weight)
> {
> struct vhost_virtqueue *vq;
> int i;
> diff --git a/drivers/vhost/vhost.h b/drivers/vhost/vhost.h
> index e9ed272..8d80389d 100644
> --- a/drivers/vhost/vhost.h
> +++ b/drivers/vhost/vhost.h
> @@ -172,12 +172,13 @@ struct vhost_dev {
> wait_queue_head_t wait;
> int iov_limit;
> int weight;
> - int byte_weight;
> + size_t byte_weight;
> };
>
This just costs extra memory, and value is never large,
so I don't think this matters.
> -bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts, int total_len);
> +bool vhost_exceeds_weight(struct vhost_virtqueue *vq, int pkts,
> + size_t total_len);
> void vhost_dev_init(struct vhost_dev *, struct vhost_virtqueue **vqs,
> - int nvqs, int iov_limit, int weight, int byte_weight);
> + int nvqs, int iov_limit, int weight, size_t byte_weight);
> long vhost_dev_set_owner(struct vhost_dev *dev);
> bool vhost_dev_has_owner(struct vhost_dev *dev);
> long vhost_dev_check_owner(struct vhost_dev *);
> --
> 1.8.5.6
^ permalink raw reply
* RE: [PATCH] net: stmmac: Fix page pool size
From: Jose Abreu @ 2019-09-23 8:09 UTC (permalink / raw)
To: Jakub Kicinski, Thierry Reding
Cc: David S . Miller, Giuseppe Cavallaro, Alexandre Torgue,
Florian Fainelli, Jon Hunter, Bitan Biswas,
netdev@vger.kernel.org, linux-tegra@vger.kernel.org
In-Reply-To: <20190922153132.0c328fe7@cakuba.netronome.com>
From: Jakub Kicinski <jakub.kicinski@netronome.com>
Date: Sep/22/2019, 23:31:32 (UTC+00:00)
> 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?
This would be:
2af6106ae949 ("net: stmmac: Introducing support for Page Pool")
Can you please resubmit Thierry ?
---
Thanks,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH] net: ath: fix missing checks for bmi reads and writes
From: Kalle Valo @ 2019-09-23 8:18 UTC (permalink / raw)
To: Kangjie Lu
Cc: kjlu, pakki001, David S. Miller, ath10k, linux-wireless, netdev,
linux-kernel
In-Reply-To: <20190315051903.10664-1-kjlu@umn.edu>
Kangjie Lu <kjlu@umn.edu> wrote:
> ath10k_bmi_write32 and ath10k_bmi_read32 can fail. The fix
> checks their statuses to avoid potential undefined behaviors.
>
> Signed-off-by: Kangjie Lu <kjlu@umn.edu>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Patch applied to ath-next branch of ath.git, thanks.
8da96730331d ath10k: fix missing checks for bmi reads and writes
--
https://patchwork.kernel.org/patch/10854069/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ 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