* Re: [PATCH v7 32/46] vhost/net: virtio 1.0 byte swap
From: Cornelia Huck @ 2014-12-01 12:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <1417359787-10138-33-git-send-email-mst@redhat.com>
On Sun, 30 Nov 2014 17:11:54 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/net.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply
* Re: [PATCH v7 33/46] vhost/net: larger header for virtio 1.0
From: Cornelia Huck @ 2014-12-01 12:35 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <1417359787-10138-34-git-send-email-mst@redhat.com>
On Sun, 30 Nov 2014 17:11:59 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> Reviewed-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/vhost/net.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
Reviewed-by: Cornelia Huck <cornelia.huck@de.ibm.com>
^ permalink raw reply
* Re: [PATCH v7 31/46] vhost: virtio 1.0 endian-ness support
From: Michael S. Tsirkin @ 2014-12-01 12:37 UTC (permalink / raw)
To: Cornelia Huck
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <20141201133353.0bbaa2e1.cornelia.huck@de.ibm.com>
On Mon, Dec 01, 2014 at 01:33:53PM +0100, Cornelia Huck wrote:
> On Sun, 30 Nov 2014 17:11:49 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/vhost/vhost.c | 93 +++++++++++++++++++++++++++++++--------------------
> > 1 file changed, 56 insertions(+), 37 deletions(-)
> >
>
> > @@ -1113,18 +1120,19 @@ static int get_indirect(struct vhost_virtqueue *vq,
> > {
> > struct vring_desc desc;
> > unsigned int i = 0, count, found = 0;
> > + u32 len = vhost32_to_cpu(vq, indirect->len);
> > int ret;
> >
> > /* Sanity check */
> > - if (unlikely(indirect->len % sizeof desc)) {
> > + if (unlikely(len % sizeof desc)) {
> > vq_err(vq, "Invalid length in indirect descriptor: "
> > "len 0x%llx not multiple of 0x%zx\n",
> > - (unsigned long long)indirect->len,
> > + (unsigned long long)vhost32_to_cpu(vq, indirect->len),
>
> Can't you use len here?
Not if I want error message to be readable.
> > sizeof desc);
> > return -EINVAL;
> > }
> >
> > - ret = translate_desc(vq, indirect->addr, indirect->len, vq->indirect,
> > + ret = translate_desc(vq, vhost64_to_cpu(vq, indirect->addr), len, vq->indirect,
> > UIO_MAXIOV);
> > if (unlikely(ret < 0)) {
> > vq_err(vq, "Translation failure %d in indirect.\n", ret);
>
>
> > @@ -1404,7 +1422,7 @@ int vhost_add_used_n(struct vhost_virtqueue *vq, struct vring_used_elem *heads,
> >
> > /* Make sure buffer is written before we update index. */
> > smp_wmb();
> > - if (put_user(vq->last_used_idx, &vq->used->idx)) {
> > + if (__put_user(cpu_to_vhost16(vq, vq->last_used_idx), &vq->used->idx)) {
>
> Why s/put_user/__put_user/ - I don't see how endianness conversions
> should have an influence there?
We should generally to __ variants since addresses are pre-validated.
But I agree - should be a separate patch.
>
> > vq_err(vq, "Failed to increment used idx");
> > return -EFAULT;
> > }
>
> > @@ -1449,11 +1468,11 @@ static bool vhost_notify(struct vhost_dev *dev, struct vhost_virtqueue *vq)
> > if (unlikely(!v))
> > return true;
> >
> > - if (get_user(event, vhost_used_event(vq))) {
> > + if (__get_user(event, vhost_used_event(vq))) {
>
> Dito: why the change?
Same. Will split this out, it's unrelated to virtio 1.0.
> > vq_err(vq, "Failed to get used event idx");
> > return true;
> > }
> > - return vring_need_event(event, new, old);
> > + return vring_need_event(vhost16_to_cpu(vq, event), new, old);
> > }
> >
> > /* This actually signals the guest, using eventfd. */
^ permalink raw reply
* Re: [PATCH v7 36/46] vhost/net: suppress compiler warning
From: Cornelia Huck @ 2014-12-01 12:37 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <1417359787-10138-37-git-send-email-mst@redhat.com>
On Sun, 30 Nov 2014 17:12:13 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> len is always initialized since function is called with size > 0.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> drivers/vhost/net.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> index 984242e..54ffbb0 100644
> --- a/drivers/vhost/net.c
> +++ b/drivers/vhost/net.c
> @@ -501,7 +501,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> int headcount = 0;
> unsigned d;
> int r, nlogs = 0;
> - u32 len;
> + u32 uninitialized_var(len);
>
> while (datalen > 0 && headcount < quota) {
> if (unlikely(seg >= UIO_MAXIOV)) {
Want to merge this with the patch introducing the variable and add a
comment there?
^ permalink raw reply
* Re: [PATCH v7 31/46] vhost: virtio 1.0 endian-ness support
From: Cornelia Huck @ 2014-12-01 12:42 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <20141201123701.GB17958@redhat.com>
On Mon, 1 Dec 2014 14:37:01 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Dec 01, 2014 at 01:33:53PM +0100, Cornelia Huck wrote:
> > On Sun, 30 Nov 2014 17:11:49 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > drivers/vhost/vhost.c | 93 +++++++++++++++++++++++++++++++--------------------
> > > 1 file changed, 56 insertions(+), 37 deletions(-)
> > >
> >
> > > @@ -1113,18 +1120,19 @@ static int get_indirect(struct vhost_virtqueue *vq,
> > > {
> > > struct vring_desc desc;
> > > unsigned int i = 0, count, found = 0;
> > > + u32 len = vhost32_to_cpu(vq, indirect->len);
> > > int ret;
> > >
> > > /* Sanity check */
> > > - if (unlikely(indirect->len % sizeof desc)) {
> > > + if (unlikely(len % sizeof desc)) {
> > > vq_err(vq, "Invalid length in indirect descriptor: "
> > > "len 0x%llx not multiple of 0x%zx\n",
> > > - (unsigned long long)indirect->len,
> > > + (unsigned long long)vhost32_to_cpu(vq, indirect->len),
> >
> > Can't you use len here?
>
> Not if I want error message to be readable.
Huh? Both have the same value.
^ permalink raw reply
* HELLO
From: matildawoart @ 2014-12-01 12:47 UTC (permalink / raw)
My Dearest, how are you today? I hope you are fine, I am
Barrister Matilda woart from woart & Associate Chambers. I have something very
important
to discuss with you and you can contact me via my private
email:
Matildawoart@gmail.com
Best Regards,
Honorable Matilda woart solicitor-at-law (CDF, LLM. BL)
Office: 20 Rue du Chenin de Fer
Matilda woart
Liberia.
^ permalink raw reply
* Re: [PATCH v7 31/46] vhost: virtio 1.0 endian-ness support
From: Michael S. Tsirkin @ 2014-12-01 12:49 UTC (permalink / raw)
To: Cornelia Huck
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <20141201134247.55e68161.cornelia.huck@de.ibm.com>
On Mon, Dec 01, 2014 at 01:42:47PM +0100, Cornelia Huck wrote:
> On Mon, 1 Dec 2014 14:37:01 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Dec 01, 2014 at 01:33:53PM +0100, Cornelia Huck wrote:
> > > On Sun, 30 Nov 2014 17:11:49 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > drivers/vhost/vhost.c | 93 +++++++++++++++++++++++++++++++--------------------
> > > > 1 file changed, 56 insertions(+), 37 deletions(-)
> > > >
> > >
> > > > @@ -1113,18 +1120,19 @@ static int get_indirect(struct vhost_virtqueue *vq,
> > > > {
> > > > struct vring_desc desc;
> > > > unsigned int i = 0, count, found = 0;
> > > > + u32 len = vhost32_to_cpu(vq, indirect->len);
> > > > int ret;
> > > >
> > > > /* Sanity check */
> > > > - if (unlikely(indirect->len % sizeof desc)) {
> > > > + if (unlikely(len % sizeof desc)) {
> > > > vq_err(vq, "Invalid length in indirect descriptor: "
> > > > "len 0x%llx not multiple of 0x%zx\n",
> > > > - (unsigned long long)indirect->len,
> > > > + (unsigned long long)vhost32_to_cpu(vq, indirect->len),
> > >
> > > Can't you use len here?
> >
> > Not if I want error message to be readable.
>
> Huh? Both have the same value.
Ah, good point.
^ permalink raw reply
* Re: tun issue after e0b46d0ee9c: tun: Use iovec iterators
From: Marcelo Ricardo Leitner @ 2014-12-01 13:05 UTC (permalink / raw)
To: Herbert Xu; +Cc: netdev
In-Reply-To: <20141130082106.GA9531@gondor.apana.org.au>
On 30-11-2014 06:21, Herbert Xu wrote:
> On Sat, Nov 29, 2014 at 03:08:09AM -0200, Marcelo Ricardo Leitner wrote:
>> On 28-11-2014 21:59, Herbert Xu wrote:
>>> On Fri, Nov 28, 2014 at 05:25:27PM -0200, Marcelo Ricardo Leitner wrote:
>>>>
>>>> I saw there are tun updates on Dave's queue but none seemed to handle this.
>>>>
>>>> I can't use current net-next
>>>> (799d2fff1858004526ad75d66a5dd8a5cce6ad40) on a kvm hypervisor
>>>> because tun got clogged somehow. Bisected down to:
>>>>
>>>> commit e0b46d0ee9c240c7430a47e9b0365674d4a04522
>>>> Author: Herbert Xu <herbert@gondor.apana.org.au>
>>>> Date: Fri Nov 7 21:22:23 2014 +0800
>>>
>>> Does this patch help?
>>
>> Yay, it does! Works for me, thanks Herbert.
>> I didn't test performance, but dhcp could get through.
>>
>> Are you sure about the Fixes tag? Because bisect really pointed to e0b46d0ee9c.
>
> Well according to your report you were having problems with
> tun_get_user. The bug I introduced was in tun_put_user and has
> already been fixed by Jason Wang.
Ahh yes, ok. Thanks
Cheers,
Marcelo
^ permalink raw reply
* Re: [bisected] xfrm: TCP connection initiating PMTU discovery stalls on v3.12+
From: Wolfgang Walter @ 2014-12-01 13:17 UTC (permalink / raw)
To: Thomas Jarosch; +Cc: netdev, Eric Dumazet
In-Reply-To: <1709726.jUgUSQI9sl@pikkukde.a.i2n>
Am Samstag, 29. November 2014, 12:44:07 schrieb Thomas Jarosch:
> Hello,
>
> we're in the process of updating production level machines
> from kernel 3.4.101 to kernel 3.14.25. On one mail server
> we noticed that emails destined for an IPSec tunnel sometimes
> get stuck in the mail queue with TCP timeouts.
>
> To make a long story short: When the VPN connection is initially
> set up or re-newed, the path MTU for the xfrm tunnel is undetermined.
>
> As soon as a TCP client starts to send large packets,
> it triggers path MTU detection. Some middlebox on the
> way to the final server has a lower MTU and sends back
> an "ICMP fragmentation needed" packet as normal.
>
> With the old kernel, the packet size for the TCP connection inside
> the xfrm tunnel gets adjusted and all is fine. With kernel v3.12+,
> the connection stalls completely. Same thing with kernel v3.18-rc6.
We see something similar with real nic (RTL8139). In our case only the first
tcp-connection which triggers PMTU stalls. Later tcp-connections then work
fine.
I will revert that patch and see if that fixes the problem.
>
> We wrote a small tool to mimic postfix's TCP behavior (see attached file).
> In the end it's a normal TCP client sending large packets.
> The server side is just "socat - tcp4-listen:667".
>
> If you run "socket_client" a second time, the path MTU
> for the xfrm tunnel is already known and packets flow normal, too.
>
>
> The "evil" commit in question is this one:
> ---------------------------------------------------------------------
> commit 8f26fb1c1ed81c33f5d87c5936f4d9d1b4118918
> Author: Eric Dumazet <edumazet@google.com>
> Date: Tue Oct 15 12:24:54 2013 -0700
>
> tcp: remove the sk_can_gso() check from tcp_set_skb_tso_segs()
>
> sk_can_gso() should only be used as a hint in tcp_sendmsg() to build GSO
> packets in the first place. (As a performance hint)
>
> Once we have GSO packets in write queue, we can not decide they are no
> longer GSO only because flow now uses a route which doesn't handle
> TSO/GSO.
>
> Core networking stack handles the case very well for us, all we need
> is keeping track of packet counts in MSS terms, regardless of
> segmentation done later (in GSO or hardware)
>
> Right now, if tcp_fragment() splits a GSO packet in two parts,
> @left and @right, and route changed through a non GSO device,
> both @left and @right have pcount set to 1, which is wrong,
> and leads to incorrect packet_count tracking.
>
> This problem was added in commit d5ac99a648 ("[TCP]: skb pcount with MTU
> discovery")
>
> Signed-off-by: Eric Dumazet <edumazet@google.com>
> Signed-off-by: Neal Cardwell <ncardwell@google.com>
> Signed-off-by: Yuchung Cheng <ycheng@google.com>
> Reported-by: Maciej Żenczykowski <maze@google.com>
> Signed-off-by: David S. Miller <davem@davemloft.net>
>
> diff --git a/net/ipv4/tcp_output.c b/net/ipv4/tcp_output.c
> index 8fad1c1..d46f214 100644
> --- a/net/ipv4/tcp_output.c
> +++ b/net/ipv4/tcp_output.c
> @@ -989,8 +989,7 @@ static void tcp_set_skb_tso_segs(const struct sock *sk,
> struct sk_buff *skb, /* Make sure we own this skb before messing
> gso_size/gso_segs */ WARN_ON_ONCE(skb_cloned(skb));
>
> - if (skb->len <= mss_now || !sk_can_gso(sk) ||
> - skb->ip_summed == CHECKSUM_NONE) {
> + if (skb->len <= mss_now || skb->ip_summed == CHECKSUM_NONE) {
> /* Avoid the costly divide in the normal
> * non-TSO case.
> */
> ---------------------------------------------------------------------
>
> When I revert it, even kernel v3.18-rc6 starts working.
> But I doubt this is the root problem, may be just hiding another issue.
>
> --- Sample output of socket_client using vanilla v3.12 kernel ---
> [1417258063 SEND result: 4096, strerror: Success]
> tcp max seg: res: 0, max_seg: 1370
> [1417258063 SEND result: 4096, strerror: Success]
> tcp max seg: res: 0, max_seg: 1370
> [1417258063 SEND result: 4096, strerror: Success]
> tcp max seg: res: 0, max_seg: 1370
> [1417258063 SEND result: 4096, strerror: Success]
> tcp max seg: res: 0, max_seg: 1370
> [1417258063 SEND result: 4096, strerror: Success]
> tcp max seg: res: 0, max_seg: 1338
> [1417258063 SEND result: 4096, strerror: Success]
> tcp max seg: res: 0, max_seg: 1338
> *STUCK*
> --------------------------------------------------------
>
> The "machine" is running on KVM and using "virtio_net" as NIC driver.
> I've played with the ethtool offload settings:
>
> *** eth1 defaults ***
> Offload parameters for eth1:
> rx-checksumming: on
> tx-checksumming: on
> scatter-gather: on
> tcp-segmentation-offload: on
> udp-fragmentation-offload: on
> generic-segmentation-offload: on
> generic-receive-offload: on
> large-receive-offload: off
>
> *** eth1 working (no stalls) using vanilla kernel ***
> Offload parameters for eth1:
> rx-checksumming: on
> tx-checksumming: off <-- the magic switch
> scatter-gather: off
> tcp-segmentation-offload: off
> udp-fragmentation-offload: off
> generic-segmentation-offload: off
> generic-receive-offload: off
> large-receive-offload: off
>
> When I turn "tx-checksumming" back on, it fails again.
> Though that is probably also just a side effect.
>
> I can provide tcpdumps if needed but they are no real help
> since you can just see the kernel stops sending TCP packets.
> (and the outgoing TCP packets are encrypted in ESP packets)
>
>
> Any vague idea what might be the root cause?
>
> I also tried reverting commit 4d53eff48b5f03ce67f4f301d6acca1d2145cb7a
> ("xfrm: Don't queue retransmitted packets if the original is still on the
> host") but that didn't change the situation. In fact it wasn't even
> triggered.
>
> Please CC: comments. Thanks.
>
> Best regards,
> Thomas
Regards,
--
Wolfgang Walter
Studentenwerk München
Anstalt des öffentlichen Rechts
^ permalink raw reply
* Re: [Xen-devel] [PATCH] xen-netfront: Remove BUGs on paged skb data which crosses a page boundary
From: David Vrabel @ 2014-12-01 13:27 UTC (permalink / raw)
To: David Miller, seth.forshee
Cc: zoltan.kiss, eric.dumazet, netdev, linux-kernel, stefan.bader,
david.vrabel, xen-devel, boris.ostrovsky
In-Reply-To: <20141126.122812.223757363894961994.davem@davemloft.net>
On 26/11/14 17:28, David Miller wrote:
> From: Seth Forshee <seth.forshee@canonical.com>
> Date: Tue, 25 Nov 2014 20:28:24 -0600
>
>> These BUGs can be erroneously triggered by frags which refer to
>> tail pages within a compound page. The data in these pages may
>> overrun the hardware page while still being contained within the
>> compound page, but since compound_order() evaluates to 0 for tail
>> pages the assertion fails. The code already iterates through
>> subsequent pages correctly in this scenario, so the BUGs are
>> unnecessary and can be removed.
>>
>> Fixes: f36c374782e4 ("xen/netfront: handle compound page fragments on transmit")
>> Cc: <stable@vger.kernel.org> # 3.7+
>> Signed-off-by: Seth Forshee <seth.forshee@canonical.com>
>
> Can I get some Xen developer reviews?
Sorry for the delay, I've been on holiday.
Reviewed-by: David Vrabel <david.vrabel@citrix.com>
Thanks.
David
^ permalink raw reply
* Re: [Xen-devel] [PATCH] xen-netfront: Fix handling packets on compound pages with skb_linearize
From: David Vrabel @ 2014-12-01 13:36 UTC (permalink / raw)
To: Stefan Bader, Zoltan Kiss, Konrad Rzeszutek Wilk, Boris Ostrovsky,
David Vrabel
Cc: Wei Liu, Ian Campbell, netdev, linux-kernel, Paul Durrant,
xen-devel
In-Reply-To: <547C2CFC.7060908@canonical.com>
On 01/12/14 08:55, Stefan Bader wrote:
> On 11.08.2014 19:32, Zoltan Kiss wrote:
>> There is a long known problem with the netfront/netback interface: if the guest
>> tries to send a packet which constitues more than MAX_SKB_FRAGS + 1 ring slots,
>> it gets dropped. The reason is that netback maps these slots to a frag in the
>> frags array, which is limited by size. Having so many slots can occur since
>> compound pages were introduced, as the ring protocol slice them up into
>> individual (non-compound) page aligned slots. The theoretical worst case
>> scenario looks like this (note, skbs are limited to 64 Kb here):
>> linear buffer: at most PAGE_SIZE - 17 * 2 bytes, overlapping page boundary,
>> using 2 slots
>> first 15 frags: 1 + PAGE_SIZE + 1 bytes long, first and last bytes are at the
>> end and the beginning of a page, therefore they use 3 * 15 = 45 slots
>> last 2 frags: 1 + 1 bytes, overlapping page boundary, 2 * 2 = 4 slots
>> Although I don't think this 51 slots skb can really happen, we need a solution
>> which can deal with every scenario. In real life there is only a few slots
>> overdue, but usually it causes the TCP stream to be blocked, as the retry will
>> most likely have the same buffer layout.
>> This patch solves this problem by linearizing the packet. This is not the
>> fastest way, and it can fail much easier as it tries to allocate a big linear
>> area for the whole packet, but probably easier by an order of magnitude than
>> anything else. Probably this code path is not touched very frequently anyway.
>>
>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>> Cc: Wei Liu <wei.liu2@citrix.com>
>> Cc: Ian Campbell <Ian.Campbell@citrix.com>
>> Cc: Paul Durrant <paul.durrant@citrix.com>
>> Cc: netdev@vger.kernel.org
>> Cc: linux-kernel@vger.kernel.org
>> Cc: xen-devel@lists.xenproject.org
>
> This does not seem to be marked explicitly as stable. Has someone already asked
> David Miller to put it on his stable queue? IMO it qualifies quite well and the
> actual change should be simple to pick/backport.
I think it's a candidate, yes.
Can you expand on the user visible impact of the bug this patch fixes?
I think it results in certain types of traffic not working (because the
domU always generates skb's with the problematic frag layout), but I
can't remember the details.
David
^ permalink raw reply
* Re: [PATCH v7 36/46] vhost/net: suppress compiler warning
From: Michael S. Tsirkin @ 2014-12-01 13:48 UTC (permalink / raw)
To: Cornelia Huck
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <20141201133740.56d7d6d8.cornelia.huck@de.ibm.com>
On Mon, Dec 01, 2014 at 01:37:40PM +0100, Cornelia Huck wrote:
> On Sun, 30 Nov 2014 17:12:13 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > len is always initialized since function is called with size > 0.
> >
> > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > ---
> > drivers/vhost/net.c | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > index 984242e..54ffbb0 100644
> > --- a/drivers/vhost/net.c
> > +++ b/drivers/vhost/net.c
> > @@ -501,7 +501,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > int headcount = 0;
> > unsigned d;
> > int r, nlogs = 0;
> > - u32 len;
> > + u32 uninitialized_var(len);
> >
> > while (datalen > 0 && headcount < quota) {
> > if (unlikely(seg >= UIO_MAXIOV)) {
>
> Want to merge this with the patch introducing the variable and add a
> comment there?
Not really. Warnings in bisect are fine I think.
--
MST
^ permalink raw reply
* Re: [PATCH] mips: bpf: Fix broken BPF_MOD
From: Sergei Shtylyov @ 2014-12-01 13:50 UTC (permalink / raw)
To: Denis Kirjanov, netdev; +Cc: markos.chandras
In-Reply-To: <1417427822-12729-1-git-send-email-kda@linux-powerpc.org>
Hello.
On 12/1/2014 12:57 PM, Denis Kirjanov wrote:
You should CC the 'linux-mips' ML.
> Remove optimize_div() from BPF_MOD | BPF_K case
> since we don't know the dividend and fix the
> emit_mod() by reading the mod operation result from HI register
Isn't this 2 unrelated fixes? They should be in 2 patches, not a single
one in that case.
> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
> ---
> arch/mips/net/bpf_jit.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
WBR, Sergei
^ permalink raw reply
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Thomas Graf @ 2014-12-01 13:52 UTC (permalink / raw)
To: Du, Fan
Cc: 'Jason Wang', netdev@vger.kernel.org, davem@davemloft.net,
fw@strlen.de, dev, mst, jesse, pshelar
In-Reply-To: <5A90DA2E42F8AE43BC4A093BF0678848DED92B@SHSMSX104.ccr.corp.intel.com>
On 11/30/14 at 10:08am, Du, Fan wrote:
> >-----Original Message-----
> >From: Jason Wang [mailto:jasowang@redhat.com]
> >Sent: Friday, November 28, 2014 3:02 PM
> >To: Du, Fan
> >Cc: netdev@vger.kernel.org; davem@davemloft.net; fw@strlen.de; Du, Fan
> >Subject: Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
> >On Fri, Nov 28, 2014 at 2:33 PM, Fan Du <fan.du@intel.com> wrote:
> >> Test scenario: two KVM guests sitting in different hosts communicate
> >> to each other with a vxlan tunnel.
> >>
> >> All interface MTU is default 1500 Bytes, from guest point of view, its
> >> skb gso_size could be as bigger as 1448Bytes, however after guest skb
> >> goes through vxlan encapuslation, individual segments length of a gso
> >> packet could exceed physical NIC MTU 1500, which will be lost at
> >> recevier side.
> >>
> >> So it's possible in virtualized environment, locally created skb len
> >> after encapslation could be bigger than underlayer MTU. In such case,
> >> it's reasonable to do GSO first, then fragment any packet bigger than
> >> MTU as possible.
> >>
> >> +---------------+ TX RX +---------------+
> >> | KVM Guest | -> ... -> | KVM Guest |
> >> +-+-----------+-+ +-+-----------+-+
> >> |Qemu/VirtIO| |Qemu/VirtIO|
> >> +-----------+ +-----------+
> >> | |
> >> v tap0 tap0 v
> >> +-----------+ +-----------+
> >> | ovs bridge| | ovs bridge|
> >> +-----------+ +-----------+
> >> | vxlan vxlan |
> >> v v
> >> +-----------+ +-----------+
> >> | NIC | <------> | NIC |
> >> +-----------+ +-----------+
> >>
> >> Steps to reproduce:
> >> 1. Using kernel builtin openvswitch module to setup ovs bridge.
> >> 2. Runing iperf without -M, communication will stuck.
> >
> >Is this issue specific to ovs or ipv4? Path MTU discovery should help in this case I
> >believe.
>
> Problem here is host stack push local over-sized gso skb down to NIC, and perform GSO there
> without any further ip segmentation.
>
> Reasonable behavior is do gso first at ip level, if gso-ed skb is bigger than MTU && df is set,
> Then push ICMP_DEST_UNREACH/ICMP_FRAG_NEEDED message back to sender to adjust mtu.
Aside from this. I think Virtio should provide a MTU hint to the guest
to adjust MTU in the vNIC to account for both overhead or support for
jumbo frames in the underlay transparently without relying on PMTU or
MSS hints. I remember we talked about this a while ago with at least
Michael but haven't done actual code work on it yet.
> For PMTU to work, that's another issue I will try to address later on.
PMTU discovery was explicitly removed from the OVS datapath. Maybe
Pravin or Jesse can provide some background on that
^ permalink raw reply
* Re: [PATCH] mips: bpf: Fix broken BPF_MOD
From: Denis Kirjanov @ 2014-12-01 13:55 UTC (permalink / raw)
To: Sergei Shtylyov; +Cc: netdev, markos.chandras
In-Reply-To: <547C7240.4010401@cogentembedded.com>
On 12/1/14, Sergei Shtylyov <sergei.shtylyov@cogentembedded.com> wrote:
> Hello.
>
> On 12/1/2014 12:57 PM, Denis Kirjanov wrote:
>
> You should CC the 'linux-mips' ML.
>
>> Remove optimize_div() from BPF_MOD | BPF_K case
>> since we don't know the dividend and fix the
>> emit_mod() by reading the mod operation result from HI register
>
> Isn't this 2 unrelated fixes? They should be in 2 patches, not a single
>
> one in that case.
>
They do fix the BPD_MOD _single_ case. I don't think that it's a good
reason here to split the fin in 2 patches
>> Signed-off-by: Denis Kirjanov <kda@linux-powerpc.org>
>> ---
>> arch/mips/net/bpf_jit.c | 4 ++--
>> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> WBR, Sergei
>
>
^ permalink raw reply
* Re: [Xen-devel] [PATCH] xen-netfront: Fix handling packets on compound pages with skb_linearize
From: Zoltan Kiss @ 2014-12-01 13:59 UTC (permalink / raw)
To: David Vrabel, Stefan Bader, Zoltan Kiss, Konrad Rzeszutek Wilk,
Boris Ostrovsky
Cc: Wei Liu, Ian Campbell, netdev, linux-kernel, Paul Durrant,
xen-devel
In-Reply-To: <547C6EF6.8020604@citrix.com>
On 01/12/14 13:36, David Vrabel wrote:
> On 01/12/14 08:55, Stefan Bader wrote:
>> On 11.08.2014 19:32, Zoltan Kiss wrote:
>>> There is a long known problem with the netfront/netback interface: if the guest
>>> tries to send a packet which constitues more than MAX_SKB_FRAGS + 1 ring slots,
>>> it gets dropped. The reason is that netback maps these slots to a frag in the
>>> frags array, which is limited by size. Having so many slots can occur since
>>> compound pages were introduced, as the ring protocol slice them up into
>>> individual (non-compound) page aligned slots. The theoretical worst case
>>> scenario looks like this (note, skbs are limited to 64 Kb here):
>>> linear buffer: at most PAGE_SIZE - 17 * 2 bytes, overlapping page boundary,
>>> using 2 slots
>>> first 15 frags: 1 + PAGE_SIZE + 1 bytes long, first and last bytes are at the
>>> end and the beginning of a page, therefore they use 3 * 15 = 45 slots
>>> last 2 frags: 1 + 1 bytes, overlapping page boundary, 2 * 2 = 4 slots
>>> Although I don't think this 51 slots skb can really happen, we need a solution
>>> which can deal with every scenario. In real life there is only a few slots
>>> overdue, but usually it causes the TCP stream to be blocked, as the retry will
>>> most likely have the same buffer layout.
>>> This patch solves this problem by linearizing the packet. This is not the
>>> fastest way, and it can fail much easier as it tries to allocate a big linear
>>> area for the whole packet, but probably easier by an order of magnitude than
>>> anything else. Probably this code path is not touched very frequently anyway.
>>>
>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>> Cc: Ian Campbell <Ian.Campbell@citrix.com>
>>> Cc: Paul Durrant <paul.durrant@citrix.com>
>>> Cc: netdev@vger.kernel.org
>>> Cc: linux-kernel@vger.kernel.org
>>> Cc: xen-devel@lists.xenproject.org
>>
>> This does not seem to be marked explicitly as stable. Has someone already asked
>> David Miller to put it on his stable queue? IMO it qualifies quite well and the
>> actual change should be simple to pick/backport.
>
> I think it's a candidate, yes.
>
> Can you expand on the user visible impact of the bug this patch fixes?
> I think it results in certain types of traffic not working (because the
> domU always generates skb's with the problematic frag layout), but I
> can't remember the details.
Yes, this line in the comment talks about it: "In real life there is
only a few slots overdue, but usually it causes the TCP stream to be
blocked, as the retry will most likely have the same buffer layout."
Maybe we can add what kind of traffic triggered this so far, AFAIK NFS
was one of them, and Stefan had an another use case. But my memories are
blur about this.
Zoli
^ permalink raw reply
* Re: [Xen-devel] [PATCH] xen-netfront: Fix handling packets on compound pages with skb_linearize
From: Stefan Bader @ 2014-12-01 14:13 UTC (permalink / raw)
To: Zoltan Kiss, David Vrabel, Zoltan Kiss, Konrad Rzeszutek Wilk,
Boris Ostrovsky
Cc: Wei Liu, Ian Campbell, netdev, linux-kernel, Paul Durrant,
xen-devel
In-Reply-To: <547C742E.6060801@linaro.org>
[-- Attachment #1: Type: text/plain, Size: 3372 bytes --]
On 01.12.2014 14:59, Zoltan Kiss wrote:
>
>
> On 01/12/14 13:36, David Vrabel wrote:
>> On 01/12/14 08:55, Stefan Bader wrote:
>>> On 11.08.2014 19:32, Zoltan Kiss wrote:
>>>> There is a long known problem with the netfront/netback interface: if the guest
>>>> tries to send a packet which constitues more than MAX_SKB_FRAGS + 1 ring slots,
>>>> it gets dropped. The reason is that netback maps these slots to a frag in the
>>>> frags array, which is limited by size. Having so many slots can occur since
>>>> compound pages were introduced, as the ring protocol slice them up into
>>>> individual (non-compound) page aligned slots. The theoretical worst case
>>>> scenario looks like this (note, skbs are limited to 64 Kb here):
>>>> linear buffer: at most PAGE_SIZE - 17 * 2 bytes, overlapping page boundary,
>>>> using 2 slots
>>>> first 15 frags: 1 + PAGE_SIZE + 1 bytes long, first and last bytes are at the
>>>> end and the beginning of a page, therefore they use 3 * 15 = 45 slots
>>>> last 2 frags: 1 + 1 bytes, overlapping page boundary, 2 * 2 = 4 slots
>>>> Although I don't think this 51 slots skb can really happen, we need a solution
>>>> which can deal with every scenario. In real life there is only a few slots
>>>> overdue, but usually it causes the TCP stream to be blocked, as the retry will
>>>> most likely have the same buffer layout.
>>>> This patch solves this problem by linearizing the packet. This is not the
>>>> fastest way, and it can fail much easier as it tries to allocate a big linear
>>>> area for the whole packet, but probably easier by an order of magnitude than
>>>> anything else. Probably this code path is not touched very frequently anyway.
>>>>
>>>> Signed-off-by: Zoltan Kiss <zoltan.kiss@citrix.com>
>>>> Cc: Wei Liu <wei.liu2@citrix.com>
>>>> Cc: Ian Campbell <Ian.Campbell@citrix.com>
>>>> Cc: Paul Durrant <paul.durrant@citrix.com>
>>>> Cc: netdev@vger.kernel.org
>>>> Cc: linux-kernel@vger.kernel.org
>>>> Cc: xen-devel@lists.xenproject.org
>>>
>>> This does not seem to be marked explicitly as stable. Has someone already asked
>>> David Miller to put it on his stable queue? IMO it qualifies quite well and the
>>> actual change should be simple to pick/backport.
>>
>> I think it's a candidate, yes.
>>
>> Can you expand on the user visible impact of the bug this patch fixes?
>> I think it results in certain types of traffic not working (because the
>> domU always generates skb's with the problematic frag layout), but I
>> can't remember the details.
>
> Yes, this line in the comment talks about it: "In real life there is only a few
> slots overdue, but usually it causes the TCP stream to be blocked, as the retry
> will most likely have the same buffer layout."
> Maybe we can add what kind of traffic triggered this so far, AFAIK NFS was one
> of them, and Stefan had an another use case. But my memories are blur about this.
We had some report about some web-app hitting packet losses. I suspect that also
was streaming something. For a easy trigger we found redis-benchmark (part of
the redis keyserver) with a larger (iirc 1kB) payload would trigger the
fragmentation/exceeding pages to happen. Though I think it did not fail but
showed a performance drop instead (from memory which also suffers from loosing
detail).
-Stefan
>
> Zoli
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply
* Re: [PATCH v7 36/46] vhost/net: suppress compiler warning
From: Cornelia Huck @ 2014-12-01 14:21 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <20141201134839.GB18305@redhat.com>
On Mon, 1 Dec 2014 15:48:39 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Dec 01, 2014 at 01:37:40PM +0100, Cornelia Huck wrote:
> > On Sun, 30 Nov 2014 17:12:13 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > len is always initialized since function is called with size > 0.
> > >
> > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > ---
> > > drivers/vhost/net.c | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > index 984242e..54ffbb0 100644
> > > --- a/drivers/vhost/net.c
> > > +++ b/drivers/vhost/net.c
> > > @@ -501,7 +501,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > > int headcount = 0;
> > > unsigned d;
> > > int r, nlogs = 0;
> > > - u32 len;
> > > + u32 uninitialized_var(len);
> > >
> > > while (datalen > 0 && headcount < quota) {
> > > if (unlikely(seg >= UIO_MAXIOV)) {
> >
> > Want to merge this with the patch introducing the variable and add a
> > comment there?
>
> Not really. Warnings in bisect are fine I think.
I'm not sure what a separate patch buys us, though, as it should be
trivial to merge.
^ permalink raw reply
* Re: [PATCH 1/3] net-PPP: Deletion of unnecessary checks before the function call "kfree"
From: SF Markus Elfring @ 2014-12-01 15:00 UTC (permalink / raw)
To: Sergei Shtylyov, Paul Mackerras, linux-ppp, netdev
Cc: LKML, kernel-janitors, Julia Lawall
In-Reply-To: <547C5CBC.6060607@cogentembedded.com>
>> diff --git a/drivers/net/ppp/ppp_mppe.c b/drivers/net/ppp/ppp_mppe.c
>> index 911b216..7e44212 100644
>> --- a/drivers/net/ppp/ppp_mppe.c
>> +++ b/drivers/net/ppp/ppp_mppe.c
>> @@ -238,8 +238,7 @@ static void *mppe_alloc(unsigned char *options, int optlen)
>> return (void *)state;
>>
>> out_free:
>> - if (state->sha1_digest)
>> - kfree(state->sha1_digest);
>> + kfree(state->sha1_digest);
>
> Please keep this line aligned to the others.
Can it be that the previous source code contained unwanted space
characters at this place?
Do you want indentation fixes as a separate update step?
Regards,
Markus
^ permalink raw reply
* Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
From: Michael S. Tsirkin @ 2014-12-01 15:06 UTC (permalink / raw)
To: Thomas Graf
Cc: dev-yBygre7rU0TnMu66kgdUjQ,
netdev-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
'Jason Wang', Du, Fan,
fw-HFFVJYpyMKqzQB+pC5nmwQ@public.gmane.org,
davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org
In-Reply-To: <20141201135225.GA16814-FZi0V3Vbi30CUdFEqe4BF2D2FQJk+8+b@public.gmane.org>
On Mon, Dec 01, 2014 at 01:52:25PM +0000, Thomas Graf wrote:
> On 11/30/14 at 10:08am, Du, Fan wrote:
> > >-----Original Message-----
> > >From: Jason Wang [mailto:jasowang@redhat.com]
> > >Sent: Friday, November 28, 2014 3:02 PM
> > >To: Du, Fan
> > >Cc: netdev@vger.kernel.org; davem@davemloft.net; fw@strlen.de; Du, Fan
> > >Subject: Re: [PATCH net] gso: do GSO for local skb with size bigger than MTU
> > >On Fri, Nov 28, 2014 at 2:33 PM, Fan Du <fan.du@intel.com> wrote:
> > >> Test scenario: two KVM guests sitting in different hosts communicate
> > >> to each other with a vxlan tunnel.
> > >>
> > >> All interface MTU is default 1500 Bytes, from guest point of view, its
> > >> skb gso_size could be as bigger as 1448Bytes, however after guest skb
> > >> goes through vxlan encapuslation, individual segments length of a gso
> > >> packet could exceed physical NIC MTU 1500, which will be lost at
> > >> recevier side.
> > >>
> > >> So it's possible in virtualized environment, locally created skb len
> > >> after encapslation could be bigger than underlayer MTU. In such case,
> > >> it's reasonable to do GSO first, then fragment any packet bigger than
> > >> MTU as possible.
> > >>
> > >> +---------------+ TX RX +---------------+
> > >> | KVM Guest | -> ... -> | KVM Guest |
> > >> +-+-----------+-+ +-+-----------+-+
> > >> |Qemu/VirtIO| |Qemu/VirtIO|
> > >> +-----------+ +-----------+
> > >> | |
> > >> v tap0 tap0 v
> > >> +-----------+ +-----------+
> > >> | ovs bridge| | ovs bridge|
> > >> +-----------+ +-----------+
> > >> | vxlan vxlan |
> > >> v v
> > >> +-----------+ +-----------+
> > >> | NIC | <------> | NIC |
> > >> +-----------+ +-----------+
> > >>
> > >> Steps to reproduce:
> > >> 1. Using kernel builtin openvswitch module to setup ovs bridge.
> > >> 2. Runing iperf without -M, communication will stuck.
> > >
> > >Is this issue specific to ovs or ipv4? Path MTU discovery should help in this case I
> > >believe.
> >
> > Problem here is host stack push local over-sized gso skb down to NIC, and perform GSO there
> > without any further ip segmentation.
> >
> > Reasonable behavior is do gso first at ip level, if gso-ed skb is bigger than MTU && df is set,
> > Then push ICMP_DEST_UNREACH/ICMP_FRAG_NEEDED message back to sender to adjust mtu.
Sounds about right.
> Aside from this. I think Virtio should provide a MTU hint to the guest
> to adjust MTU in the vNIC to account for both overhead or support for
> jumbo frames in the underlay transparently without relying on PMTU or
> MSS hints. I remember we talked about this a while ago with at least
> Michael but haven't done actual code work on it yet.
This can be an optimization but can't replace fixing PMTU.
In particular this can't help legacy guests.
> > For PMTU to work, that's another issue I will try to address later on.
>
> PMTU discovery was explicitly removed from the OVS datapath. Maybe
> Pravin or Jesse can provide some background on that
PMTU was never working properly for OVS users. There was an ugly hack that kind
of worked part of the time. That was dropped, and good riddance.
Proper PMTU support for all kind of tunneling solutions, e.g. along the
lines you outline above, belongs in host core.
--
MST
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
^ permalink raw reply
* Payment
From: Finance Department @ 2014-12-01 14:35 UTC (permalink / raw)
Dear Recipient,
You have been awarded the sum of 8,000,000.00 (Eight Million Pounds sterling) with reference number 77100146 by office of the ministry of finance UK.Send us your personal details to deliver your funds.
Gloria Peter
^ permalink raw reply
* Re: [PATCH v7 36/46] vhost/net: suppress compiler warning
From: Michael S. Tsirkin @ 2014-12-01 15:12 UTC (permalink / raw)
To: Cornelia Huck
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <20141201152103.0f375544.cornelia.huck@de.ibm.com>
On Mon, Dec 01, 2014 at 03:21:03PM +0100, Cornelia Huck wrote:
> On Mon, 1 Dec 2014 15:48:39 +0200
> "Michael S. Tsirkin" <mst@redhat.com> wrote:
>
> > On Mon, Dec 01, 2014 at 01:37:40PM +0100, Cornelia Huck wrote:
> > > On Sun, 30 Nov 2014 17:12:13 +0200
> > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > >
> > > > len is always initialized since function is called with size > 0.
> > > >
> > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > ---
> > > > drivers/vhost/net.c | 2 +-
> > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > > index 984242e..54ffbb0 100644
> > > > --- a/drivers/vhost/net.c
> > > > +++ b/drivers/vhost/net.c
> > > > @@ -501,7 +501,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > > > int headcount = 0;
> > > > unsigned d;
> > > > int r, nlogs = 0;
> > > > - u32 len;
> > > > + u32 uninitialized_var(len);
> > > >
> > > > while (datalen > 0 && headcount < quota) {
> > > > if (unlikely(seg >= UIO_MAXIOV)) {
> > >
> > > Want to merge this with the patch introducing the variable and add a
> > > comment there?
> >
> > Not really. Warnings in bisect are fine I think.
>
> I'm not sure what a separate patch buys us, though, as it should be
> trivial to merge.
Easier to document the reason if it's split out.
^ permalink raw reply
* Re: [PATCH v7 36/46] vhost/net: suppress compiler warning
From: Cornelia Huck @ 2014-12-01 15:18 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: thuth, kvm, rusty, netdev, linux-kernel, virtualization, dahi,
pbonzini, David Miller
In-Reply-To: <20141201151208.GA18919@redhat.com>
On Mon, 1 Dec 2014 17:12:08 +0200
"Michael S. Tsirkin" <mst@redhat.com> wrote:
> On Mon, Dec 01, 2014 at 03:21:03PM +0100, Cornelia Huck wrote:
> > On Mon, 1 Dec 2014 15:48:39 +0200
> > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> >
> > > On Mon, Dec 01, 2014 at 01:37:40PM +0100, Cornelia Huck wrote:
> > > > On Sun, 30 Nov 2014 17:12:13 +0200
> > > > "Michael S. Tsirkin" <mst@redhat.com> wrote:
> > > >
> > > > > len is always initialized since function is called with size > 0.
> > > > >
> > > > > Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
> > > > > ---
> > > > > drivers/vhost/net.c | 2 +-
> > > > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/vhost/net.c b/drivers/vhost/net.c
> > > > > index 984242e..54ffbb0 100644
> > > > > --- a/drivers/vhost/net.c
> > > > > +++ b/drivers/vhost/net.c
> > > > > @@ -501,7 +501,7 @@ static int get_rx_bufs(struct vhost_virtqueue *vq,
> > > > > int headcount = 0;
> > > > > unsigned d;
> > > > > int r, nlogs = 0;
> > > > > - u32 len;
> > > > > + u32 uninitialized_var(len);
> > > > >
> > > > > while (datalen > 0 && headcount < quota) {
> > > > > if (unlikely(seg >= UIO_MAXIOV)) {
> > > >
> > > > Want to merge this with the patch introducing the variable and add a
> > > > comment there?
> > >
> > > Not really. Warnings in bisect are fine I think.
> >
> > I'm not sure what a separate patch buys us, though, as it should be
> > trivial to merge.
>
> Easier to document the reason if it's split out.
>
That's why I suggested a comment ;)
^ permalink raw reply
* Re: [PATCH 0/7] Fix sti drivers whcih mix reg address spaces
From: Arnd Bergmann @ 2014-12-01 15:36 UTC (permalink / raw)
To: Peter Griffin
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r,
linux-kernel-u79uwXL29TY76Z2rM5mHXA,
srinivas.kandagatla-Re5JQEeQqe8AvxtiuMwx3w,
maxime.coquelin-qxv4g6HH51o, patrice.chotard-qxv4g6HH51o,
peppe.cavallaro-qxv4g6HH51o, kishon-l0cyMroinI0,
lee.jones-QSEj5FYQhm4dnm+yROfE0A, netdev-u79uwXL29TY76Z2rM5mHXA,
devicetree-u79uwXL29TY76Z2rM5mHXA, alexandre.torgue-qxv4g6HH51o
In-Reply-To: <1416385632-5832-1-git-send-email-peter.griffin-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
On Wednesday 19 November 2014 08:27:05 Peter Griffin wrote:
> Hi,
>
> Following on from Arnds comments about the picophy driver here
> https://lkml.org/lkml/2014/11/13/161, this series fixes the
> remaining upstreamed drivers for STI, which are mixing address spaces
> in the reg property. We do this in a way similar to the keystone
> and bcm7445 platforms, by having sysconfig phandle/ offset pair (
> where only one register is required). Or phandle / integer array
> where multiple offsets in the same bank are needed).
>
> This series breaks DT compatability! But the platform support
> is WIP and only being used by the few developers who are upstreaming
> support for it. I've made each change to the driver / dt doc / dt
> file as a single atomic commit so the kernel will remain bisectable.
>
> This series then also enables the picophy driver, and adds back in
> the ehci/ohci dt nodes for stih410 which make use of the picophy.
This all looks very good to me, nice work!
Reviewed-by: Arnd Bergmann <arnd-r2nGTMty4D4@public.gmane.org>
I have one comment for patch 4, but I don't have a good solution for
that, hopefully someone on the devicetree-discuss list has some insight,
otherwise we just keep your version.
Arnd
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH 4/7] ARM: STi: DT: STiH410: Add usb2 picophy dt nodes
From: Arnd Bergmann @ 2014-12-01 15:40 UTC (permalink / raw)
To: linux-arm-kernel
Cc: Peter Griffin, linux-kernel, srinivas.kandagatla, maxime.coquelin,
patrice.chotard, peppe.cavallaro, kishon, netdev, lee.jones,
alexandre.torgue, devicetree
In-Reply-To: <1416385632-5832-5-git-send-email-peter.griffin@linaro.org>
On Wednesday 19 November 2014 08:27:09 Peter Griffin wrote:
> + soc {
> + usb2_picophy1: phy@1 {
> + compatible = "st,stih407-usb2-phy";
> + #phy-cells = <0>;
> + st,syscfg = <&syscfg_core 0xf8 0xf4>;
> + resets = <&softreset STIH407_PICOPHY_SOFTRESET>,
> + <&picophyreset STIH407_PICOPHY0_RESET>;
> + reset-names = "global", "port";
> + };
>
> + usb2_picophy2: phy@2 {
> + compatible = "st,stih407-usb2-phy";
> + #phy-cells = <0>;
> + st,syscfg = <&syscfg_core 0xfc 0xf4>;
> + resets = <&softreset STIH407_PICOPHY_SOFTRESET>,
> + <&picophyreset STIH407_PICOPHY1_RESET>;
> + reset-names = "global", "port";
> + };
> + };
In theory the unit-address (the @1 and @2 part of the name) is supposed to
match the 'reg' property value, but of course that doesn't work any
more with the changed binding. The same problem keeps coming up, so
I wonder if anyone has an idea how this is supposed to be handled properly.
Should we just make up unit-address numbers? I guess a more elaborate
variant would be to have a parent node with #address-cells = <1> and
no ranges, to make up a new address space with arbitrarily assigned
reg values, like
phys {
#address-cells = <1>; /* just counting the nodes */
#size-cells = <0>;
usb2_picophy1: phy@0 {
compatible = "st,stih407-usb2-phy";
reg = <0>;
#phy-cells = <0>;
st,syscfg = <&syscfg_core 0xf8 0xf4>;
};
usb2_picophy2: phy@1 {
compatible = "st,stih407-usb2-phy";
reg = <0>;
#phy-cells = <0>;
st,syscfg = <&syscfg_core 0xf8 0xf4>;
};
}
Should we try to do it like this, or is that overcomplicating
things?
Arnd
^ 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