* Re: net/usb/ax88179_178a driver broken in linux-3.12
From: Sarah Sharp @ 2013-11-20 16:54 UTC (permalink / raw)
To: David Laight
Cc: David Miller, mlord, eric.dumazet, ming.lei, netdev, linux-usb
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7439@saturn3.aculab.com>
On Wed, Nov 20, 2013 at 09:54:02AM -0000, David Laight wrote:
> > From: Of David Miller
> > > From: "David Laight" <David.Laight@ACULAB.COM>
> > > Date: Tue, 19 Nov 2013 10:04:11 -0000
> >
> > > There is a patch to xhci-ring.c that should fix the SG problem.
> > > http://www.spinics.net/lists/linux-usb/msg97176.html
> > >
> > > I think it should apply to the 3.12 sources.
> >
> > David, please get this into Linus's tree and queued up for -stable
> > as soon as possible, thank you.
>
> What do I have to do to expedite it?
> I thought that Sarah would have to push the patch through.
Yes, I think the patch should come through my tree.
> It might be necessary to limit the number of SG fragments that
> the mass storage (etc) can generate to 63 (from unlimited).
> That should be a 1 line change somewhere.
Greg's USB tree is frozen until after 3.13-rc1. The patch will have to
wait until then, but it will be marked for stable.
I have no objection to the methodology of the patch, but we do need to
figure out how to limit the number of scatter-gather list entries in the
mass storage driver. We're still figuring out the exact limitation we
need to use, and then the patch can be queued in my for-usb-linus branch
for 3.13-rc1.
Note that the patch is a bandaid fix, and will only help bulk endpoints
that are submitting scatter-gather transfers. Interrupt endpoints using
scatter-gather will still run into these issues, but with David's patch,
the URB submission will now fail, rather than allowing the transfer to
be silently corrupted.
The real fix for this will probably be too large for stable, and is
likely to take at least a kernel revision to fix.
Sarah Sharp
^ permalink raw reply
* Re: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Sarah Sharp @ 2013-11-20 16:50 UTC (permalink / raw)
To: David Laight; +Cc: Ben Hutchings, Alan Stern, netdev, linux-usb
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7438@saturn3.aculab.com>
On Wed, Nov 20, 2013 at 09:46:08AM -0000, David Laight wrote:
> > From: Sarah Sharp
> ...
> > (Also, usb-storage aligns the block sizes to 512K, which explains why
> > we've never had an issue with TD fragments with that driver.)
>
> What is a 'block' in that context?
> 512K sounds more like the value that very long transfers get chopped
> up into. With 4k pages that might be 128 fragments.
Sorry, I meant 512-byte boundaries. See Alan's comment in
drivers/usb/storage/scsiglue.c:
/* USB has unusual DMA-alignment requirements: Although the
* starting address of each scatter-gather element doesn't matter,
* the length of each element except the last must be divisible
* by the Bulk maxpacket value. There's currently no way to
* express this by block-layer constraints, so we'll cop out
* and simply require addresses to be aligned at 512-byte
* boundaries. This is okay since most block I/O involves
* hardware sectors that are multiples of 512 bytes in length,
* and since host controllers up through USB 2.0 have maxpacket
* values no larger than 512.
*
* But it doesn't suffice for Wireless USB, where Bulk maxpacket
* values can be as large as 2048. To make that work properly
* will require changes to the block layer.
*/
blk_queue_update_dma_alignment(sdev->request_queue, (512 - 1));
> I'd have thought that the SG list would normally contain references
> to a number of memory pages - so each would be 4k (on x86) aligned.
> My suspicion is that the xhci controller will generate correct USB3
> data provided the link TRB is on a 1k boundary - so such data won't
> be a problem.
If the max burst size is less than four, and the scsi layer hands down
4k chunks, then the driver would still work without any modification for
TD fragments, since MBP would be 4k and there would never be a link TRB
in the middle of an MBP.
However, the driver could be in violation of the spec if the burst size
was greater than 4. I suspect what would happen is the host controller
would read the TD, and do a shorter burst of 4 max-packet-sized 1k
chunks, and then end the burst early. But I'm not a hardware engineer,
and we can't count on how they designed it. I'm just trying to figure
out why usb-storage worked for so many years without running into this
issue.
> If a user program does a direct transfer from the block device
> (and that is done by locking down the user pages) then the buffer
> could have an arbitrary alignment.
Sure. In that case though, limiting the sg_tablesize so that TDs fit
into one ring segment isn't going to help, because the block layer won't
use it. I guess the transfers will just fail, until we can get a better
fix in.
Sarah Sharp
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Alan Stern @ 2013-11-20 16:26 UTC (permalink / raw)
To: David Laight
Cc: Sarah Sharp, Ben Hutchings, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7438-CgBM+Bx2aUAnGFn1LkZF6NBPR1lH4CV8@public.gmane.org>
On Wed, 20 Nov 2013, David Laight wrote:
> > From: Sarah Sharp
> ...
> > (Also, usb-storage aligns the block sizes to 512K, which explains why
> > we've never had an issue with TD fragments with that driver.)
>
> What is a 'block' in that context?
I think Sarah means that usb-storage requires the block layer to align
its data buffers to 512-byte boundaries. (Note: 512 bytes, not 512K.)
Disk I/O naturally tends to be done in units of the page size, anyway,
although raw I/O can involve single sectors.
If a user supplies an unaligned buffer, the block layer will set up a
bounce buffer.
Alan Stern
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 net-next RFC v2 2/2] ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses
From: Hannes Frederic Sowa @ 2013-11-20 16:24 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, kuznet, jmorris, yoshfuji, kaber, thaller, stephen,
vyasevich, dcbw, David.Laight
In-Reply-To: <20131120161838.GB15778@minipsycho.orion>
On Wed, Nov 20, 2013 at 05:18:38PM +0100, Jiri Pirko wrote:
> Wed, Nov 20, 2013 at 05:05:43PM CET, hannes@stressinduktion.org wrote:
> >On Tue, Nov 19, 2013 at 11:43:29AM +0100, Jiri Pirko wrote:
> >> Creating an address with this flag set will result in kernel taking care
> >> of temporary addresses in the same way as if the address was created by
> >> kernel itself (after RA receive). This allows userspace applications
> >> implementing the autoconfiguration (NetworkManager for example) to
> >> implement ipv6 addresses privacy.
> >>
> >> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> >> Signed-off-by: Thomas Haller <thaller@redhat.com>
> >
> >The patch looks good.
> >
> >Reviewed-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
> >
> >The _bh conversion seems not to be strictly necessary, no?
> >ipv6_create_tempaddr is always called with bh disabled.
>
> It is needed. Found out using lock debug config option.
Yes, I can see it now, too. ;)
^ permalink raw reply
* Re: [patch net-next RFC v2 2/2] ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses
From: Jiri Pirko @ 2013-11-20 16:18 UTC (permalink / raw)
To: netdev, davem, kuznet, jmorris, yoshfuji, kaber, thaller, stephen,
vyasevich, dcbw, David.Laight
In-Reply-To: <20131120160543.GV16541@order.stressinduktion.org>
Wed, Nov 20, 2013 at 05:05:43PM CET, hannes@stressinduktion.org wrote:
>On Tue, Nov 19, 2013 at 11:43:29AM +0100, Jiri Pirko wrote:
>> Creating an address with this flag set will result in kernel taking care
>> of temporary addresses in the same way as if the address was created by
>> kernel itself (after RA receive). This allows userspace applications
>> implementing the autoconfiguration (NetworkManager for example) to
>> implement ipv6 addresses privacy.
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> Signed-off-by: Thomas Haller <thaller@redhat.com>
>
>The patch looks good.
>
>Reviewed-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
>
>The _bh conversion seems not to be strictly necessary, no?
>ipv6_create_tempaddr is always called with bh disabled.
It is needed. Found out using lock debug config option.
>
>Thanks,
>
> Hannes
>
^ permalink raw reply
* Re: [PATCH net] virtio-net: fix page refcnt leaking when fail to allocate frag skb
From: Eric Dumazet @ 2013-11-20 16:14 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet
In-Reply-To: <20131120160625.GA21188@redhat.com>
On Wed, 2013-11-20 at 18:06 +0200, Michael S. Tsirkin wrote:
> Hmm some kind of disconnect here.
> I got you rmanagement about bufferbloat.
>
> What I am saying is that maybe we should drop packets more
> aggressively: when we drop one packet of a flow, why not
> drop everything that's queued and is for the same flow?
I really hope your TCP flows use SACK ;)
Please read the rfc 2018 introduction for details.
If a packet is dropped, it doesn't mean following packets _have_ to be
dropped.
^ permalink raw reply
* Re: [PATCH net] virtio-net: fix page refcnt leaking when fail to allocate frag skb
From: Michael S. Tsirkin @ 2013-11-20 16:06 UTC (permalink / raw)
To: Eric Dumazet
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet
In-Reply-To: <1384960593.8604.147.camel@edumazet-glaptop2.roam.corp.google.com>
On Wed, Nov 20, 2013 at 07:16:33AM -0800, Eric Dumazet wrote:
> On Wed, 2013-11-20 at 10:58 +0200, Michael S. Tsirkin wrote:
> > On Tue, Nov 19, 2013 at 02:00:11PM -0800, Eric Dumazet wrote:
> > > On Tue, 2013-11-19 at 23:53 +0200, Michael S. Tsirkin wrote:
> > >
> > > > Which NIC? Virtio? Prior to 2613af0ed18a11d5c566a81f9a6510b73180660a
> > > > it didn't drop packets received from host as far as I can tell.
> > > > virtio is more like a pipe than a real NIC in this respect.
> > >
> > > Prior/after to this patch, you were not posting buffers, so if packets
> > > were received on a physical NIC, you were dropping the packets anyway.
> > >
> > > It makes no difference at all, adding a cushion might make you feel
> > > better, but its really not worth it.
> > >
> > > Under memory stress, it makes better sense to drop a super big GRO
> > > packet (The one needing frag_list extension ...)
> > >
> > > It gives a better signal to the sender to reduce its pressure, and gives
> > > opportunity to free more of your memory.
> > >
> >
> > OK, but in that case one wonders whether we should do more to free memory?
> >
> > E.g. imagine that we dropped a packet of a specific TCP flow
> > because we couldn't allocate a new packet.
> >
> > What happens now is that the old packet is freed as well.
> >
> > So quite likely the next packet in queue will get processed
> > since it will reuse the memory we have just freed.
> >
> > The next packet and the next after it etc all will have to go through
> > the net stack until they get at the socket and are dropped then
> > because we missed a segment. Even worse, GRO gets disabled so the load
> > on receiver goes up instead of down.
> >
> > Sounds like a problem doesn't it?
>
> I see no problem at all. GRO is a hint for high rates (and obviously
> when there is enough memory)
>
> >
> > GRO actually detects it's the same flow and can see packet is
> > out of sequence. Why doesn't it drop the packet then?
> > Alternatively, we could (for example using the pre-allocated skb
> > like I suggested) notify GRO that it should start dropping packets
> > of this flow.
> >
> > What do you think?
> >
>
> I think we disagree a lot on memory management on networking stacks.
>
> We did a lot of work in TCP stack and Qdisc layers to lower memory
> pressure (and bufferbloat), an you seem to try hard to introduce yet
> another layer of buffer bloat in virtio_net.
>
> So add whatever you want to proudly state to your management :
>
> "Look how smart we are : we drop no packets in our layer"
>
Hmm some kind of disconnect here.
I got you rmanagement about bufferbloat.
What I am saying is that maybe we should drop packets more
aggressively: when we drop one packet of a flow, why not
drop everything that's queued and is for the same flow?
^ permalink raw reply
* Re: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Sarah Sharp @ 2013-11-20 16:06 UTC (permalink / raw)
To: David Laight
Cc: Alan Stern, Ben Hutchings, netdev-u79uwXL29TY76Z2rM5mHXA,
linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7437-CgBM+Bx2aUAnGFn1LkZF6NBPR1lH4CV8@public.gmane.org>
On Wed, Nov 20, 2013 at 09:36:11AM -0000, David Laight wrote:
> > From: Alan Stern [mailto:stern-nwvwT67g6+6dFdvTe/nMLpVzexx5G7lz@public.gmane.org]
> > On Tue, 19 Nov 2013, Sarah Sharp wrote:
> >
> > > The xHCI driver can limit the number of sg-list entries through
> > > hcd->self.sg_tablesize. It's currently set to ~0, which is "however
> > > many entries you want. You could set that to the number of TRBs in a
> > > segment (minus one for the link TRB).
> > >
> > > The usb-storage and uas drivers currently use sg_tablesize. Could the
> > > network stack be taught to use sg_tablesize as well?
> >
> > The sg_tablesize you're talking about is a field in struct usb_bus
> > (there's a similar field in struct scsi_host_template). It's not
> > relevant to the network stack, since network interfaces aren't USB host
> > controllers (or SCSI hosts).
>
> Ben said the largest number of fragments from the current network
> stack will be 17, and that none of them will cross 32k boundaries.
> So the network stack won't send down long SG lists.
Ok, so the networking layer should be fine. However, with the current
patch, if the mass storage driver sends down a scatter-gather list
that's bigger than a ring segment, or needs to be split up so it doesn't
cross 64K boundaries, then the URB submission will fail. We don't want
that to happen.
At the very least, we should limit hcd->self.sg_tablesize in
drivers/usb/host/xhci.c to 63 (TRBS_PER_SEGMENT - 1). But we could
still potentially run into the 64K boundary issue in one or maybe all of
those entries. Would it be crazy to limit the number of entries to half
that (31)? It may impact performance, but it ensures that SCSI reads
and writes don't randomly fail. We can always increase the ring segment
size in a later patch.
Sarah Sharp
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" 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 net-next RFC v2 2/2] ipv6 addrconf: introduce IFA_F_MANAGETEMPADDR to tell kernel to manage temporary addresses
From: Hannes Frederic Sowa @ 2013-11-20 16:05 UTC (permalink / raw)
To: Jiri Pirko
Cc: netdev, davem, kuznet, jmorris, yoshfuji, kaber, thaller, stephen,
vyasevich, dcbw, David.Laight
In-Reply-To: <1384857809-6573-3-git-send-email-jiri@resnulli.us>
On Tue, Nov 19, 2013 at 11:43:29AM +0100, Jiri Pirko wrote:
> Creating an address with this flag set will result in kernel taking care
> of temporary addresses in the same way as if the address was created by
> kernel itself (after RA receive). This allows userspace applications
> implementing the autoconfiguration (NetworkManager for example) to
> implement ipv6 addresses privacy.
>
> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
> Signed-off-by: Thomas Haller <thaller@redhat.com>
The patch looks good.
Reviewed-by: Hannes Frederic Sowa <hannes@stressinduktion.org>
The _bh conversion seems not to be strictly necessary, no?
ipv6_create_tempaddr is always called with bh disabled.
Thanks,
Hannes
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: David Laight @ 2013-11-20 15:54 UTC (permalink / raw)
To: Eric Dumazet
Cc: Alan Stern, Sarah Sharp, Ben Hutchings,
netdev-u79uwXL29TY76Z2rM5mHXA, linux-usb-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1384959827.8604.141.camel-XN9IlZ5yJG9HTL0Zs8A6p/gx64E7kk8eUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset="utf-8", Size: 1296 bytes --]
> From: Eric Dumazet [mailto:eric.dumazet@gmail.com]
> On Wed, 2013-11-20 at 09:36 +0000, David Laight wrote:
>
> > Ben said the largest number of fragments from the current network
> > stack will be 17, and that none of them will cross 32k boundaries.
> > So the network stack won't send down long SG lists.
>
> Please note that skb->head itself _might_ cross a 32K or 64K boundary :
>
> skb->head is kmalloc() provided, and SLUB can be tweaked
> (slub_max_order) to use very high order pages.
Worth remembering...
I suspect the number of fragments (inc 64k boundaries) is still limited
because maximum data length is under 64k (for TSO) and the SLUB memory
has to be physically contiguous (it is still limited even if not).
At the moment the usb bulk tx code calculated the exact number of
fragments needed. To do this it has to scan the sg list twice.
It would seem more sensible to generate a quick upper bound
(ie nfrags*2 + len/65536) and maybe calculate the exact number
if this would exceed the maximum number.
OTOH the max allowed could be documented that way.
In any case this is all aside from the bug itself.
David
N‹§²æìr¸›yúèšØb²X¬¶Ç§vØ^–)Þº{.nÇ+‰·¥Š{±ºÆâžØ^n‡r¡ö¦zË\x1aëh™¨èÚ&¢îý»\x05ËÛÔØï¦v¬Îf\x1dp)¹¹br šê+€Ê+zf£¢·hšˆ§~††Ûiÿûàz¹\x1e®w¥¢¸?™¨èÚ&¢)ߢ^[f
^ permalink raw reply
* [PATCH] if_link.h add support for IEEE 802.1 Qbg ratified standard
From: Thomas Richter @ 2013-11-20 15:26 UTC (permalink / raw)
To: netdev; +Cc: davem, Thomas Richter
This patch adds 2 netlink message attributes and structures to
support the IEEE 802.1 Qbg ratified standard VDP protocol.
The changes are needed to communication filter information
such as MAC address and VLAN identifier to the hardware for
virtual machine identification.
Signed-off-by: Thomas Richter <tmricht@linux.vnet.ibm.com>
---
include/uapi/linux/if_link.h | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/include/uapi/linux/if_link.h b/include/uapi/linux/if_link.h
index b78566f..69a2db8 100644
--- a/include/uapi/linux/if_link.h
+++ b/include/uapi/linux/if_link.h
@@ -423,6 +423,8 @@ enum {
IFLA_PORT_HOST_UUID, /* binary UUID */
IFLA_PORT_REQUEST, /* __u8 */
IFLA_PORT_RESPONSE, /* __u16, output only */
+ IFLA_PORT_VSI_TYPE22, /* 802.1Qbg ratified standard VDP */
+ IFLA_PORT_VSI_FILTER, /* 802.1Qbg ratified standard VDP */
__IFLA_PORT_MAX,
};
@@ -463,6 +465,27 @@ struct ifla_port_vsi {
__u8 pad[3];
};
+/*
+ * Filter information data. The array size and structure members carriing
+ * valid data are determined by the members vsi_filter_num and vsi_filter_fmt
+ * structure ifla_port_vsi22 below.
+ */
+struct ifla_port_vsi_filter {
+ __u32 gpid; /* Group Identifier*/
+ __u16 vlanid; /* Vlan identifier and QoS bits */
+ __u8 mac[6]; /* MAC address */
+};
+
+struct ifla_port_vsi22 { /* 802.1 Qbg Ratified standard */
+ __u8 vsi_mgrid[PORT_UUID_MAX]; /* Manager identifier */
+ __u8 vsi_uuid[PORT_UUID_MAX]; /* VSI identifier */
+ __u8 vsi_uuidfmt; /* Format of UUID string */
+ __u8 vsi_type_id[3];
+ __u8 vsi_type_version;
+ __u8 vsi_hints; /* Hint bits */
+ __u8 vsi_filter_fmt; /* Filter information format */
+ __u16 vsi_filter_num; /* # of filter data entries */
+};
/* IPoIB section */
--
1.8.3.1
^ permalink raw reply related
* Re: [PATCH net] virtio-net: fix page refcnt leaking when fail to allocate frag skb
From: Eric Dumazet @ 2013-11-20 15:16 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet
In-Reply-To: <20131120085835.GB19341@redhat.com>
On Wed, 2013-11-20 at 10:58 +0200, Michael S. Tsirkin wrote:
> On Tue, Nov 19, 2013 at 02:00:11PM -0800, Eric Dumazet wrote:
> > On Tue, 2013-11-19 at 23:53 +0200, Michael S. Tsirkin wrote:
> >
> > > Which NIC? Virtio? Prior to 2613af0ed18a11d5c566a81f9a6510b73180660a
> > > it didn't drop packets received from host as far as I can tell.
> > > virtio is more like a pipe than a real NIC in this respect.
> >
> > Prior/after to this patch, you were not posting buffers, so if packets
> > were received on a physical NIC, you were dropping the packets anyway.
> >
> > It makes no difference at all, adding a cushion might make you feel
> > better, but its really not worth it.
> >
> > Under memory stress, it makes better sense to drop a super big GRO
> > packet (The one needing frag_list extension ...)
> >
> > It gives a better signal to the sender to reduce its pressure, and gives
> > opportunity to free more of your memory.
> >
>
> OK, but in that case one wonders whether we should do more to free memory?
>
> E.g. imagine that we dropped a packet of a specific TCP flow
> because we couldn't allocate a new packet.
>
> What happens now is that the old packet is freed as well.
>
> So quite likely the next packet in queue will get processed
> since it will reuse the memory we have just freed.
>
> The next packet and the next after it etc all will have to go through
> the net stack until they get at the socket and are dropped then
> because we missed a segment. Even worse, GRO gets disabled so the load
> on receiver goes up instead of down.
>
> Sounds like a problem doesn't it?
I see no problem at all. GRO is a hint for high rates (and obviously
when there is enough memory)
>
> GRO actually detects it's the same flow and can see packet is
> out of sequence. Why doesn't it drop the packet then?
> Alternatively, we could (for example using the pre-allocated skb
> like I suggested) notify GRO that it should start dropping packets
> of this flow.
>
> What do you think?
>
I think we disagree a lot on memory management on networking stacks.
We did a lot of work in TCP stack and Qdisc layers to lower memory
pressure (and bufferbloat), an you seem to try hard to introduce yet
another layer of buffer bloat in virtio_net.
So add whatever you want to proudly state to your management :
"Look how smart we are : we drop no packets in our layer"
^ permalink raw reply
* Re: [PATCHv3] net: core: Always propagate flag changes to interfaces
From: Stefan Priebe - Profihost AG @ 2013-11-20 15:05 UTC (permalink / raw)
To: vyasevic, netdev; +Cc: vfalico
In-Reply-To: <528CBF22.4050304@redhat.com>
thanks, works fine.
Am 20.11.2013 14:54, schrieb Vlad Yasevich:
> On 11/20/2013 02:21 AM, Stefan Priebe - Profihost AG wrote:
>> Hi,
>>
>> Am 20.11.2013 02:47, schrieb Vlad Yasevich:
>>> The following commit:
>>> b6c40d68ff6498b7f63ddf97cf0aa818d748dee7
>>> net: only invoke dev->change_rx_flags when device is UP
>>>
>>> tried to fix a problem with VLAN devices and promiscuouse flag setting.
>>> The issue was that VLAN device was setting a flag on an interface that
>>> was down, thus resulting in bad promiscuity count.
>>> This commit blocked flag propagation to any device that is currently
>>> down.
>>>
>>> A later commit:
>>> deede2fabe24e00bd7e246eb81cd5767dc6fcfc7
>>> vlan: Don't propagate flag changes on down interfaces
>>>
>>> fixed VLAN code to only propagate flags when the VLAN interface is up,
>>> thus fixing the same issue as above, only localized to VLAN.
>>>
>>> The problem we have now is that if we have create a complex stack
>>> involving multiple software devices like bridges, bonds, and vlans,
>>> then it is possible that the flags would not propagate properly to
>>> the physical devices. A simple examle of the scenario is the
>>> following:
>>>
>>> eth0----> bond0 ----> bridge0 ---> vlan50
>>>
>>> If bond0 or eth0 happen to be down at the time bond0 is added to
>>> the bridge, then eth0 will never have promisc mode set which is
>>> currently required for operation as part of the bridge. As a
>>> result, packets with vlan50 will be dropped by the interface.
>>>
>>> The only 2 devices that implement the special flag handling are
>>> VLAN and DSA and they both have required code to prevent incorrect
>>> flag propagation. As a result we can remove the generic solution
>>> introduced in b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 and leave
>>> it to the individual devices to decide whether they will block
>>> flag propagation or not.
>>>
>>> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
>>> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>>> ---
>>> v2->v3: Removed a strange chunk that modified comments. Not sure where it
>>> came from.
>>>
>>> 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 974143d..da9c5e1 100644
>>> --- a/net/core/dev.c
>>> +++ b/net/core/dev.c
>>> @@ -4991,7 +4991,7 @@ static void dev_change_rx_flags(struct net_device *dev, int flags)
>>> {
>>> const struct net_device_ops *ops = dev->netdev_ops;
>>>
>>> - if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
>>> + if (ops->ndo_change_rx_flags)
>>> ops->ndo_change_rx_flags(dev, flags);
>>> }
>>
>> thanks for this patch - in one of the first posts you send this one:
>>
>> diff --git a/net/core/dev.c b/net/core/dev.c
>> index fc913f4..016857b 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -4525,7 +4525,9 @@ static void dev_change_rx_flags(struct net_device
>> *dev, int flags)
>> {
>> const struct net_device_ops *ops = dev->netdev_ops;
>>
>> - if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
>> + if (((dev->flags & IFF_UP) ||
>> + (dev->flags & (IFF_MASTER | IFF_SLAVE)))
>> + && ops->ndo_change_rx_flags)
>> ops->ndo_change_rx_flags(dev, flags);
>> }
>>
>
> The new one should work just as well. The first one I had you try
> removed the restriction for master and slave devices such as bonds
> and their slaves. As Veaceslav pointed out, we can simply remove all
> restrictions in this code and have the individual drivers do the right
> thing. The 2 effected drivers are VLAN and DSA and they already do the
> right checks before propagating flags.
>
> I ran the same test with the simplified code and it worked for me
> correctly setting promisc mode in the same set-up you had:
>
> (phys dev) ---> bond ----> bridge ---> vlan
>
> -vlad
>
^ permalink raw reply
* RE: [PATCH] usb: xhci: Link TRB must not occur with a USB payload burst.
From: Eric Dumazet @ 2013-11-20 15:03 UTC (permalink / raw)
To: David Laight; +Cc: Alan Stern, Sarah Sharp, Ben Hutchings, netdev, linux-usb
In-Reply-To: <AE90C24D6B3A694183C094C60CF0A2F6026B7437@saturn3.aculab.com>
On Wed, 2013-11-20 at 09:36 +0000, David Laight wrote:
> Ben said the largest number of fragments from the current network
> stack will be 17, and that none of them will cross 32k boundaries.
> So the network stack won't send down long SG lists.
Please note that skb->head itself _might_ cross a 32K or 64K boundary :
skb->head is kmalloc() provided, and SLUB can be tweaked
(slub_max_order) to use very high order pages.
^ permalink raw reply
* Re: Bluetooth: oops in rfcomm_sock_getsockopt_old
From: Johan Hedberg @ 2013-11-20 15:02 UTC (permalink / raw)
To: Jiri Kosina; +Cc: Marcel Holtmann, Gustavo Padovan, netdev, linux-bluetooth
In-Reply-To: <alpine.LNX.2.00.1311201553040.31451@pobox.suse.cz>
Hi Jiri,
On Wed, Nov 20, 2013, Jiri Kosina wrote:
> I am seeing this during boot (when bluetoothd is starting) with current
> Linus' tree (HEAD == b4789b8e6b), after latest netdev/bluetooth merge:
>
> BUG: unable to handle kernel paging request at 000000234df5351a
> IP: [<ffffffffa05e95b9>] rfcomm_sock_getsockopt_old+0x39/0x190 [rfcomm]
> PGD 0
> Oops: 0000 [#1] SMP
> Modules linked in: rfcomm bnep cpufreq_conservative cpufreq_userspace cpufreq_powersa
> CO_vendor_support kvm_intel snd_hda_codec_conexant kvm iwldvm mac80211 btusb bluetooth
> d_hda_intel snd_hda_codec snd_hwdep cfg80211 thinkpad_acpi snd_seq pcspkr snd_pcm i2c_i801 lpc_ich mfd_core rfkill e1000e ehci_pci snd_timer snd_page_alloc snd_seq_device ptp pps_core wmi snd tpm_tis soundcore battery ac tpm tpm_bios acpi_cpufreq autofs4 uhci_hcd ehci_hcd i915 usbcore usb_common drm_kms_helper drm i2c_algo_bit button video edd fan processor ata_generic thermal thermal_sys
> CPU: 0 PID: 1024 Comm: bluetoothd Not tainted 3.11.0-07976-g8d6083f #1
> Hardware name: LENOVO 7470BN2/7470BN2, BIOS 6DET38WW (2.02 ) 12/19/2008
> task: ffff880076c0e000 ti: ffff880036f94000 task.ti: ffff880036f94000
> RIP: 0010:[<ffffffffa05e95b9>] [<ffffffffa05e95b9>] rfcomm_sock_getsockopt_old+0x39/0x190 [rfcomm]
> RSP: 0018:ffff880036f95e78 EFLAGS: 00010246
> RAX: 000000234df53512 RBX: 00007fff5ebe9e9c RCX: 00007fff5ebe9e9c
> RDX: 00007fff5ebe9e98 RSI: 0000000000000003 RDI: ffff8800784eb480
> RBP: ffff880036f95ec8 R08: ffff880076c1e000 R09: 00007fff5ebea148
> R10: 00007fff5ebe9e98 R11: 0000000000000202 R12: ffff880076c1e000
> R13: 0000000000000003 R14: 00007fff5ebe9e9c R15: 00007fff5ebe9e98
> FS: 00007fd5d7655700(0000) GS:ffff88007c200000(0000) knlGS:0000000000000000
> CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
> CR2: 000000234df5351a CR3: 0000000037b06000 CR4: 00000000000007f0
> Stack:
> 0000000000000004 00007fff5ebe9e98 ffff880036f95ea8 ffffffff81582e8f
> ffffffff8148e61b ffff8800784eb480 0000000000000012 0000000000000003
> 00007fff5ebe9e9c 00007fff5ebe9e98 ffff880036f95f28 ffffffffa05e9763
> Call Trace:
> [<ffffffff81582e8f>] ? _raw_spin_unlock_bh+0x3f/0x50
> [<ffffffff8148e61b>] ? release_sock+0x2b/0xa0
> [<ffffffffa05e9763>] rfcomm_sock_getsockopt+0x53/0x190 [rfcomm]
> [<ffffffff8158b737>] ? sysret_check+0x1b/0x56
> [<ffffffff81486933>] SyS_getsockopt+0x73/0xe0
> [<ffffffff8158b712>] system_call_fastpath+0x16/0x1b
> Code: 04 48 89 5d d8 4c 89 6d e8 48 89 cb 4c 89 65 e0 4c 89 75 f0 41 89 f5 4c 89 7d f8 48 89 55 b8 4c 8b 67 20 49 8b 84 24 50 04 00 00 <4c> 8b 78 08 0f 85 27 01 00 00 e8 e8 83 b6 e0 48 89 d8 e8 60 40
> RIP [<ffffffffa05e95b9>] rfcomm_sock_getsockopt_old+0x39/0x190 [rfcomm]
> RSP <ffff880036f95e78>
> CR2: 000000234df5351a
> ---[ end trace d84df5c733bb1019 ]---
>
>
> I have bisected this to 94a86df01. I don't immediately see how this could
> be causing the issue directly, hence sending out this as a heads-up, and
> will continue looking into this eventually.
>
> It seems to be very reliably reproducible, so I expect the bisection to be
> correct (to be verified still).
The issue is already fixed in the bluetooth and wireless trees but
the patch hasn't yet made it to the net or Linus' tree.
Johan
^ permalink raw reply
* Bluetooth: oops in rfcomm_sock_getsockopt_old
From: Jiri Kosina @ 2013-11-20 14:57 UTC (permalink / raw)
To: Marcel Holtmann, Johan Hedberg, Gustavo Padovan
Cc: netdev-u79uwXL29TY76Z2rM5mHXA,
linux-bluetooth-u79uwXL29TY76Z2rM5mHXA
Hi,
I am seeing this during boot (when bluetoothd is starting) with current
Linus' tree (HEAD == b4789b8e6b), after latest netdev/bluetooth merge:
BUG: unable to handle kernel paging request at 000000234df5351a
IP: [<ffffffffa05e95b9>] rfcomm_sock_getsockopt_old+0x39/0x190 [rfcomm]
PGD 0
Oops: 0000 [#1] SMP
Modules linked in: rfcomm bnep cpufreq_conservative cpufreq_userspace cpufreq_powersa
CO_vendor_support kvm_intel snd_hda_codec_conexant kvm iwldvm mac80211 btusb bluetooth
d_hda_intel snd_hda_codec snd_hwdep cfg80211 thinkpad_acpi snd_seq pcspkr snd_pcm i2c_i801 lpc_ich mfd_core rfkill e1000e ehci_pci snd_timer snd_page_alloc snd_seq_device ptp pps_core wmi snd tpm_tis soundcore battery ac tpm tpm_bios acpi_cpufreq autofs4 uhci_hcd ehci_hcd i915 usbcore usb_common drm_kms_helper drm i2c_algo_bit button video edd fan processor ata_generic thermal thermal_sys
CPU: 0 PID: 1024 Comm: bluetoothd Not tainted 3.11.0-07976-g8d6083f #1
Hardware name: LENOVO 7470BN2/7470BN2, BIOS 6DET38WW (2.02 ) 12/19/2008
task: ffff880076c0e000 ti: ffff880036f94000 task.ti: ffff880036f94000
RIP: 0010:[<ffffffffa05e95b9>] [<ffffffffa05e95b9>] rfcomm_sock_getsockopt_old+0x39/0x190 [rfcomm]
RSP: 0018:ffff880036f95e78 EFLAGS: 00010246
RAX: 000000234df53512 RBX: 00007fff5ebe9e9c RCX: 00007fff5ebe9e9c
RDX: 00007fff5ebe9e98 RSI: 0000000000000003 RDI: ffff8800784eb480
RBP: ffff880036f95ec8 R08: ffff880076c1e000 R09: 00007fff5ebea148
R10: 00007fff5ebe9e98 R11: 0000000000000202 R12: ffff880076c1e000
R13: 0000000000000003 R14: 00007fff5ebe9e9c R15: 00007fff5ebe9e98
FS: 00007fd5d7655700(0000) GS:ffff88007c200000(0000) knlGS:0000000000000000
CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033
CR2: 000000234df5351a CR3: 0000000037b06000 CR4: 00000000000007f0
Stack:
0000000000000004 00007fff5ebe9e98 ffff880036f95ea8 ffffffff81582e8f
ffffffff8148e61b ffff8800784eb480 0000000000000012 0000000000000003
00007fff5ebe9e9c 00007fff5ebe9e98 ffff880036f95f28 ffffffffa05e9763
Call Trace:
[<ffffffff81582e8f>] ? _raw_spin_unlock_bh+0x3f/0x50
[<ffffffff8148e61b>] ? release_sock+0x2b/0xa0
[<ffffffffa05e9763>] rfcomm_sock_getsockopt+0x53/0x190 [rfcomm]
[<ffffffff8158b737>] ? sysret_check+0x1b/0x56
[<ffffffff81486933>] SyS_getsockopt+0x73/0xe0
[<ffffffff8158b712>] system_call_fastpath+0x16/0x1b
Code: 04 48 89 5d d8 4c 89 6d e8 48 89 cb 4c 89 65 e0 4c 89 75 f0 41 89 f5 4c 89 7d f8 48 89 55 b8 4c 8b 67 20 49 8b 84 24 50 04 00 00 <4c> 8b 78 08 0f 85 27 01 00 00 e8 e8 83 b6 e0 48 89 d8 e8 60 40
RIP [<ffffffffa05e95b9>] rfcomm_sock_getsockopt_old+0x39/0x190 [rfcomm]
RSP <ffff880036f95e78>
CR2: 000000234df5351a
---[ end trace d84df5c733bb1019 ]---
I have bisected this to 94a86df01. I don't immediately see how this could
be causing the issue directly, hence sending out this as a heads-up, and
will continue looking into this eventually.
It seems to be very reliably reproducible, so I expect the bisection to be
correct (to be verified still).
--
Jiri Kosina
SUSE Labs
^ permalink raw reply
* Re: [PATCH RFC] virtio_net: fix error handling for mergeable buffers
From: Michael S. Tsirkin @ 2013-11-20 14:20 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
In-Reply-To: <125586345.27477452.1384955642143.JavaMail.root@redhat.com>
On Wed, Nov 20, 2013 at 08:54:02AM -0500, Jason Wang wrote:
>
>
> ----- 原始邮件 -----
> > On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> > > When mergeable buffer were used, we only put the first page buf leave the
> > > rest
> > > of buffers in the virt queue. This will cause the driver could not get the
> > > correct head buffer any more. Fix this by dropping the rest of buffers for
> > > this
> > > packet.
> > >
> > > The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> > > (virtio_net: Defer skb allocation in receive path).
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Michael Dalton <mwdalton@google.com>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Cc: Shirley Ma <xma@us.ibm.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > Just to clarify my previous comment: it was not about the
> > idea of adding drop_mergeable_buffer - rather, I think that
> > adding knowledge about mergeable buffers into page_to_skb creates an
> > ugly internal API.
> >
> > Let's move the call to page_to_skb within receive_mergeable instead:
> > it's also nice that int offset = buf - page_address(page) logic
> > is not spread around like it was.
> >
> > Also, it's not nice that we ignore length errors when we drop
> > packets because of OOM.
> >
> > So I came up with the following - it seems to work but I didn't
> > stress test yet.
>
> I've no objection on this. But I've rather like my small and direct patch
> to be applied to -net first. It has lower risk and was much more easier to
> be backported to stable trees. Then we can do the re-factor like this in
> net-next.
It makes the interfaces too messy. We are not in code freeze in -net -
only feature freeze, so no reason to make code like spagetty,
and it's only 25 lines changed as compared to 40.
It's not a huge refactoring.
It's just as easy to backport my patch too.
You just drop the goto in the new code path we added.
Let me show you (untested) - your patch is not smaller.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
commit 9b442fe970d5c71311d4314edef26ee2eb16e7fb
Author: Michael S. Tsirkin <mst@redhat.com>
Date: Wed Nov 20 12:44:14 2013 +0200
virtio_net: fix resource leak on alloc failure
virtio net got confused, started dropping packets.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 9fbdfcd..df4b9d0 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -297,13 +297,22 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
return skb;
}
-static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
+static struct sk_buff *receive_mergeable(struct net_device *dev,
+ struct receive_queue *rq,
+ struct page *page,
+ unsigned int len)
{
- struct skb_vnet_hdr *hdr = skb_vnet_hdr(skb);
- struct page *page;
- int num_buf, i, len;
+ struct skb_vnet_hdr *hdr = page_address(buf);
+ int num_buf = hdr->mhdr.num_buffers;
+ struct sk_buff *skb = page_to_skb(rq, page, len);
+ int i;
+
+ skb = page_to_skb(rq, page, len);
+
+ if (unlikely(!skb))
+ goto err_skb;
+
- num_buf = hdr->mhdr.num_buffers;
while (--num_buf) {
i = skb_shinfo(skb)->nr_frags;
if (i >= MAX_SKB_FRAGS) {
@@ -313,10 +322,10 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
}
page = virtqueue_get_buf(rq->vq, &len);
if (!page) {
- pr_debug("%s: rx error: %d buffers missing\n",
- skb->dev->name, hdr->mhdr.num_buffers);
- skb->dev->stats.rx_length_errors++;
- return -EINVAL;
+ pr_debug("%s: rx error: %d buffers %d missing\n",
+ dev->name, hdr->mhdr.num_buffers, num_buf);
+ dev->stats.rx_length_errors++;
+ goto err_buf;
}
if (len > PAGE_SIZE)
@@ -326,7 +335,25 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *skb)
--rq->num;
}
- return 0;
+ return skb;
+err_skb:
+ put_page(page);
+err_buf:
+ dev->stats.rx_dropped++;
+ dev_kfree_skb(head_skb);
+ while (--num_buf) {
+ buf = virtqueue_get_buf(rq->vq, &len);
+ if (unlikely(!buf)) {
+ pr_debug("%s: rx error: %d buffers missing\n",
+ dev->name, num_buf);
+ dev->stats.rx_length_errors++;
+ break;
+ }
+ page = buf;
+ give_pages(rq, page);
+ --rq->num;
+ }
+ return NULL;
}
static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
@@ -354,17 +381,18 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
skb_trim(skb, len);
} else {
page = buf;
- skb = page_to_skb(rq, page, len);
- if (unlikely(!skb)) {
- dev->stats.rx_dropped++;
- give_pages(rq, page);
- return;
- }
- if (vi->mergeable_rx_bufs)
- if (receive_mergeable(rq, skb)) {
- dev_kfree_skb(skb);
+ if (vi->mergeable_rx_bufs) {
+ skb = receive_mergeable(dev, rq, page, len);
+ if (unlikely(!skb))
+ return;
+ } else {
+ skb = page_to_skb(rq, page, len);
+ if (unlikely(!skb)) {
+ dev->stats.rx_dropped++;
+ give_pages(rq, page);
return;
}
+ }
}
hdr = skb_vnet_hdr(skb);
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [patch net-next RFC v2 1/2] ipv6 addrconf: extend ifa_flags to u32
From: Jiri Pirko @ 2013-11-20 14:08 UTC (permalink / raw)
To: David Miller
Cc: netdev, kuznet, jmorris, yoshfuji, kaber, thaller, stephen,
hannes, vyasevich, dcbw, David.Laight
In-Reply-To: <20131119.161653.1620911822393300977.davem@davemloft.net>
Tue, Nov 19, 2013 at 10:16:53PM CET, davem@davemloft.net wrote:
>From: Jiri Pirko <jiri@resnulli.us>
>Date: Tue, 19 Nov 2013 11:43:28 +0100
>
>> There is no more space in u8 ifa_flags. So do what davem suffested and
>> add another netlink attr called IFA_FLAGS for carry more flags.
>>
>> Signed-off-by: Jiri Pirko <jiri@resnulli.us>
>> Signed-off-by: Thomas Haller <thaller@redhat.com>
>> ---
>> v1->v2:
>> - reordered struct inet6_ifaddr as suggested by David Laight
>> - changed flags type to u32 on couple of places as suggested by Thomas Haller
>> - changed output of if6_seq_show as suggested by Thomas Haller
>
>This looks fine, but I would suggest to extend this to all addressing
>types using ifm->ifa_flags. It is not such a big deal, there are only
>3 or 4.
Okay - I will do that.
>
>DecNET, ipv4, ipv6, and lastly phonet (which just reports "permanent"
>flag on get/dump).
>
>That way tools like iproute2 can just generically provide and
>interpret IFA_FLAGS attribute unconditionally.
Well the check and ->ifa_flags fallback needs to be in iproute2 anyway
in order to maintain compatibility with older kernels.
Thanks for review!
Jiri
^ permalink raw reply
* Re: [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure
From: Jason Wang @ 2013-11-20 13:56 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet
In-Reply-To: <20131120133707.GA8523@redhat.com>
----- 原始邮件 -----
> On Wed, Nov 20, 2013 at 07:08:50AM -0500, Jason Wang wrote:
> >
> >
> > ----- 原始邮件 -----
> > > On Wed, Nov 20, 2013 at 05:07:26PM +0800, Jason Wang wrote:
> > > > We need decrease the rq->num after we can get a buf through
> > > > virtqueue_get_buf() even if we could not allocate frag skb. Otherwise,
> > > > the
> > > > refill routine won't be triggered under heavy memory stress since the
> > > > driver may
> > > > still think there's enough room.
> > > >
> > > > This bug was introduced by commit
> > > > 2613af0ed18a11d5c566a81f9a6510b73180660a
> > > > (virtio_net: migrate mergeable rx buffers to page frag allocators).
> > > >
> > > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > > Cc: Michael Dalton <mwdalton@google.com>
> > > > Cc: Eric Dumazet <edumazet@google.com>
> > > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > >
> > > So let's wrap virtqueue_get_buf to make sure we get it right?
> > >
> >
> > Ok. good idea.
>
> So I did this (below) but the compiler is not smart enough to
> avoid two branches on data path.
> So I'm not sure anymore: with my patch it's pretty clear how
> the code works.
>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> but I don't think we need to apply this.
>
True, another point is we'd better to handle both increasing and decreasing in the
same way. We do not increase it in a helper.
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 11d9cc9..1cc2e43 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -296,6 +296,14 @@ static struct sk_buff *page_to_skb(struct receive_queue
> *rq,
> return skb;
> }
>
> +static void *rq_get_buf(struct receive_queue *rq, unsigned int *len)
> +{
> + void *buf = virtqueue_get_buf(rq->vq, len);
> + if (buf)
> + rq->num--;
> + return buf;
> +}
> +
> static struct sk_buff *receive_mergeable(struct net_device *dev,
> struct receive_queue *rq,
> void *buf,
> @@ -315,7 +323,7 @@ static struct sk_buff *receive_mergeable(struct
> net_device *dev,
> while (--num_buf) {
> int num_skb_frags;
>
> - buf = virtqueue_get_buf(rq->vq, &len);
> + buf = rq_get_buf(rq, &len);
> if (unlikely(!buf)) {
> pr_debug("%s: rx error: %d buffers out of %d missing\n",
> dev->name, num_buf, hdr->mhdr.num_buffers);
> @@ -329,7 +337,6 @@ static struct sk_buff *receive_mergeable(struct
> net_device *dev,
> }
>
> page = virt_to_head_page(buf);
> - --rq->num;
>
> num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> @@ -370,7 +377,7 @@ err_buf:
> dev->stats.rx_dropped++;
> dev_kfree_skb(head_skb);
> while (--num_buf) {
> - buf = virtqueue_get_buf(rq->vq, &len);
> + buf = rq_get_buf(rq, &len);
> if (unlikely(!buf)) {
> pr_debug("%s: rx error: %d buffers missing\n",
> dev->name, num_buf);
> @@ -379,7 +386,6 @@ err_buf:
> }
> page = virt_to_head_page(buf);
> put_page(page);
> - --rq->num;
> }
> return NULL;
> }
> @@ -675,9 +681,8 @@ static int virtnet_poll(struct napi_struct *napi, int
> budget)
>
> again:
> while (received < budget &&
> - (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> + (buf = rq_get_buf(rq, &len)) != NULL) {
> receive_buf(rq, buf, len);
> - --rq->num;
> received++;
> }
>
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCHv3] net: core: Always propagate flag changes to interfaces
From: Vlad Yasevich @ 2013-11-20 13:54 UTC (permalink / raw)
To: Stefan Priebe - Profihost AG, netdev; +Cc: vfalico
In-Reply-To: <528C62F7.2000100@profihost.ag>
On 11/20/2013 02:21 AM, Stefan Priebe - Profihost AG wrote:
> Hi,
>
> Am 20.11.2013 02:47, schrieb Vlad Yasevich:
>> The following commit:
>> b6c40d68ff6498b7f63ddf97cf0aa818d748dee7
>> net: only invoke dev->change_rx_flags when device is UP
>>
>> tried to fix a problem with VLAN devices and promiscuouse flag setting.
>> The issue was that VLAN device was setting a flag on an interface that
>> was down, thus resulting in bad promiscuity count.
>> This commit blocked flag propagation to any device that is currently
>> down.
>>
>> A later commit:
>> deede2fabe24e00bd7e246eb81cd5767dc6fcfc7
>> vlan: Don't propagate flag changes on down interfaces
>>
>> fixed VLAN code to only propagate flags when the VLAN interface is up,
>> thus fixing the same issue as above, only localized to VLAN.
>>
>> The problem we have now is that if we have create a complex stack
>> involving multiple software devices like bridges, bonds, and vlans,
>> then it is possible that the flags would not propagate properly to
>> the physical devices. A simple examle of the scenario is the
>> following:
>>
>> eth0----> bond0 ----> bridge0 ---> vlan50
>>
>> If bond0 or eth0 happen to be down at the time bond0 is added to
>> the bridge, then eth0 will never have promisc mode set which is
>> currently required for operation as part of the bridge. As a
>> result, packets with vlan50 will be dropped by the interface.
>>
>> The only 2 devices that implement the special flag handling are
>> VLAN and DSA and they both have required code to prevent incorrect
>> flag propagation. As a result we can remove the generic solution
>> introduced in b6c40d68ff6498b7f63ddf97cf0aa818d748dee7 and leave
>> it to the individual devices to decide whether they will block
>> flag propagation or not.
>>
>> Reported-by: Stefan Priebe <s.priebe@profihost.ag>
>> Suggested-by: Veaceslav Falico <vfalico@redhat.com>
>> Signed-off-by: Vlad Yasevich <vyasevic@redhat.com>
>> ---
>> v2->v3: Removed a strange chunk that modified comments. Not sure where it
>> came from.
>>
>> 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 974143d..da9c5e1 100644
>> --- a/net/core/dev.c
>> +++ b/net/core/dev.c
>> @@ -4991,7 +4991,7 @@ static void dev_change_rx_flags(struct net_device *dev, int flags)
>> {
>> const struct net_device_ops *ops = dev->netdev_ops;
>>
>> - if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
>> + if (ops->ndo_change_rx_flags)
>> ops->ndo_change_rx_flags(dev, flags);
>> }
>
> thanks for this patch - in one of the first posts you send this one:
>
> diff --git a/net/core/dev.c b/net/core/dev.c
> index fc913f4..016857b 100644
> --- a/net/core/dev.c
> +++ b/net/core/dev.c
> @@ -4525,7 +4525,9 @@ static void dev_change_rx_flags(struct net_device
> *dev, int flags)
> {
> const struct net_device_ops *ops = dev->netdev_ops;
>
> - if ((dev->flags & IFF_UP) && ops->ndo_change_rx_flags)
> + if (((dev->flags & IFF_UP) ||
> + (dev->flags & (IFF_MASTER | IFF_SLAVE)))
> + && ops->ndo_change_rx_flags)
> ops->ndo_change_rx_flags(dev, flags);
> }
>
The new one should work just as well. The first one I had you try
removed the restriction for master and slave devices such as bonds
and their slaves. As Veaceslav pointed out, we can simply remove all
restrictions in this code and have the individual drivers do the right
thing. The 2 effected drivers are VLAN and DSA and they already do the
right checks before propagating flags.
I ran the same test with the simplified code and it worked for me
correctly setting promisc mode in the same set-up you had:
(phys dev) ---> bond ----> bridge ---> vlan
-vlad
^ permalink raw reply
* Re: [PATCH RFC] virtio_net: fix error handling for mergeable buffers
From: Jason Wang @ 2013-11-20 13:54 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
In-Reply-To: <20131120132657.GA8455@redhat.com>
----- 原始邮件 -----
> On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> > When mergeable buffer were used, we only put the first page buf leave the
> > rest
> > of buffers in the virt queue. This will cause the driver could not get the
> > correct head buffer any more. Fix this by dropping the rest of buffers for
> > this
> > packet.
> >
> > The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> > (virtio_net: Defer skb allocation in receive path).
> >
> > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > Cc: Michael S. Tsirkin <mst@redhat.com>
> > Cc: Michael Dalton <mwdalton@google.com>
> > Cc: Eric Dumazet <edumazet@google.com>
> > Cc: Shirley Ma <xma@us.ibm.com>
> > Signed-off-by: Jason Wang <jasowang@redhat.com>
>
> Just to clarify my previous comment: it was not about the
> idea of adding drop_mergeable_buffer - rather, I think that
> adding knowledge about mergeable buffers into page_to_skb creates an
> ugly internal API.
>
> Let's move the call to page_to_skb within receive_mergeable instead:
> it's also nice that int offset = buf - page_address(page) logic
> is not spread around like it was.
>
> Also, it's not nice that we ignore length errors when we drop
> packets because of OOM.
>
> So I came up with the following - it seems to work but I didn't
> stress test yet.
I've no objection on this. But I've rather like my small and direct patch
to be applied to -net first. It has lower risk and was much more easier to
be backported to stable trees. Then we can do the re-factor like this in
net-next.
>
> commit ebffb3fe4335ffe07124e4518e76d6e05844fa18
> Author: Michael S. Tsirkin <mst@redhat.com>
> Date: Wed Nov 20 14:41:29 2013 +0200
>
> virtio_net: fix error handling for mergeable buffers
>
> Eric Dumazet noticed that if we encounter an error
> when processing a mergeable buffer, we don't
> dequeue all of the buffers from this packet,
> the result is almost sure to be loss of networking.
>
> Jason Wang noticed that we also leak a page and that we don't decrement
> the rq buf count, so we won't repost buffers (a resource leak).
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael Dalton <mwdalton@google.com>
> Reported-by: Eric Dumazet <edumazet@google.com>
> Reported-by: Jason Wang <jasowang@redhat.com>
> Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 01f4eb5..42f6a1e 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -296,41 +296,53 @@ static struct sk_buff *page_to_skb(struct receive_queue
> *rq,
> return skb;
> }
>
> -static int receive_mergeable(struct receive_queue *rq, struct sk_buff
> *head_skb)
> +static struct sk_buff *receive_mergeable(struct net_device *dev,
> + struct receive_queue *rq,
> + void *buf,
> + unsigned int len)
> {
> - struct skb_vnet_hdr *hdr = skb_vnet_hdr(head_skb);
> + struct skb_vnet_hdr *hdr = buf;
> + int num_buf = hdr->mhdr.num_buffers;
> + struct page *page = virt_to_head_page(buf);
> + int offset = buf - page_address(page);
> + struct sk_buff *head_skb = page_to_skb(rq, page, offset, len,
> + MAX_PACKET_LEN);
> struct sk_buff *curr_skb = head_skb;
> - char *buf;
> - struct page *page;
> - int num_buf, len, offset;
>
> - num_buf = hdr->mhdr.num_buffers;
> - while (--num_buf) {
> - int num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> + if (unlikely(!curr_skb))
> + goto err_skb;
> +
> + while (--num_buf) {
> + int num_skb_frags;
> +
> buf = virtqueue_get_buf(rq->vq, &len);
> if (unlikely(!buf)) {
> - pr_debug("%s: rx error: %d buffers missing\n",
> - head_skb->dev->name, hdr->mhdr.num_buffers);
> - head_skb->dev->stats.rx_length_errors++;
> - return -EINVAL;
> + pr_debug("%s: rx error: %d buffers out of %d missing\n",
> + dev->name, num_buf, hdr->mhdr.num_buffers);
> + dev->stats.rx_length_errors++;
> + goto err_buf;
> }
> if (unlikely(len > MAX_PACKET_LEN)) {
> pr_debug("%s: rx error: merge buffer too long\n",
> - head_skb->dev->name);
> + dev->name);
> len = MAX_PACKET_LEN;
> }
> +
> + page = virt_to_head_page(buf);
> + --rq->num;
> +
> + num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
> - if (unlikely(!nskb)) {
> - head_skb->dev->stats.rx_dropped++;
> - return -ENOMEM;
> - }
> +
> + if (unlikely(!nskb))
> + goto err_skb;
> if (curr_skb == head_skb)
> skb_shinfo(curr_skb)->frag_list = nskb;
> else
> curr_skb->next = nskb;
> - curr_skb = nskb;
> head_skb->truesize += nskb->truesize;
> + curr_skb = nskb;
> num_skb_frags = 0;
> }
> if (curr_skb != head_skb) {
> @@ -338,8 +350,7 @@ static int receive_mergeable(struct receive_queue *rq,
> struct sk_buff *head_skb)
> head_skb->len += len;
> head_skb->truesize += MAX_PACKET_LEN;
> }
> - page = virt_to_head_page(buf);
> - offset = buf - (char *)page_address(page);
> + offset = buf - page_address(page);
> if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
> put_page(page);
> skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
> @@ -349,9 +360,28 @@ static int receive_mergeable(struct receive_queue *rq,
> struct sk_buff *head_skb)
> offset, len,
> MAX_PACKET_LEN);
> }
> + }
> +
> + return head_skb;
> +
> +err_skb:
> + put_page(page);
> +err_buf:
> + dev->stats.rx_dropped++;
> + dev_kfree_skb(head_skb);
> + while (--num_buf) {
> + buf = virtqueue_get_buf(rq->vq, &len);
> + if (unlikely(!buf)) {
> + pr_debug("%s: rx error: %d buffers missing\n",
> + dev->name, num_buf);
> + dev->stats.rx_length_errors++;
> + break;
> + }
> + page = virt_to_head_page(buf);
> + put_page(page);
> --rq->num;
> }
> - return 0;
> + return NULL;
> }
>
> static void receive_buf(struct receive_queue *rq, void *buf, unsigned int
> len)
> @@ -380,19 +410,9 @@ static void receive_buf(struct receive_queue *rq, void
> *buf, unsigned int len)
> len -= sizeof(struct virtio_net_hdr);
> skb_trim(skb, len);
> } else if (vi->mergeable_rx_bufs) {
> - struct page *page = virt_to_head_page(buf);
> - skb = page_to_skb(rq, page,
> - (char *)buf - (char *)page_address(page),
> - len, MAX_PACKET_LEN);
> - if (unlikely(!skb)) {
> - dev->stats.rx_dropped++;
> - put_page(page);
> + skb = receive_mergeable(dev, rq, buf, len);
> + if (unlikely(!skb))
> return;
> - }
> - if (receive_mergeable(rq, skb)) {
> - dev_kfree_skb(skb);
> - return;
> - }
> } else {
> page = buf;
> skb = page_to_skb(rq, page, 0, len, PAGE_SIZE);
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net 2/3] virtio-net: fix num calculation on frag skb allocation failure
From: Michael S. Tsirkin @ 2013-11-20 13:37 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet
In-Reply-To: <734327384.27426189.1384949330819.JavaMail.root@redhat.com>
On Wed, Nov 20, 2013 at 07:08:50AM -0500, Jason Wang wrote:
>
>
> ----- 原始邮件 -----
> > On Wed, Nov 20, 2013 at 05:07:26PM +0800, Jason Wang wrote:
> > > We need decrease the rq->num after we can get a buf through
> > > virtqueue_get_buf() even if we could not allocate frag skb. Otherwise, the
> > > refill routine won't be triggered under heavy memory stress since the
> > > driver may
> > > still think there's enough room.
> > >
> > > This bug was introduced by commit 2613af0ed18a11d5c566a81f9a6510b73180660a
> > > (virtio_net: migrate mergeable rx buffers to page frag allocators).
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Michael Dalton <mwdalton@google.com>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> >
> > So let's wrap virtqueue_get_buf to make sure we get it right?
> >
>
> Ok. good idea.
So I did this (below) but the compiler is not smart enough to
avoid two branches on data path.
So I'm not sure anymore: with my patch it's pretty clear how
the code works.
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
but I don't think we need to apply this.
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 11d9cc9..1cc2e43 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -296,6 +296,14 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
return skb;
}
+static void *rq_get_buf(struct receive_queue *rq, unsigned int *len)
+{
+ void *buf = virtqueue_get_buf(rq->vq, len);
+ if (buf)
+ rq->num--;
+ return buf;
+}
+
static struct sk_buff *receive_mergeable(struct net_device *dev,
struct receive_queue *rq,
void *buf,
@@ -315,7 +323,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
while (--num_buf) {
int num_skb_frags;
- buf = virtqueue_get_buf(rq->vq, &len);
+ buf = rq_get_buf(rq, &len);
if (unlikely(!buf)) {
pr_debug("%s: rx error: %d buffers out of %d missing\n",
dev->name, num_buf, hdr->mhdr.num_buffers);
@@ -329,7 +337,6 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
}
page = virt_to_head_page(buf);
- --rq->num;
num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
@@ -370,7 +377,7 @@ err_buf:
dev->stats.rx_dropped++;
dev_kfree_skb(head_skb);
while (--num_buf) {
- buf = virtqueue_get_buf(rq->vq, &len);
+ buf = rq_get_buf(rq, &len);
if (unlikely(!buf)) {
pr_debug("%s: rx error: %d buffers missing\n",
dev->name, num_buf);
@@ -379,7 +386,6 @@ err_buf:
}
page = virt_to_head_page(buf);
put_page(page);
- --rq->num;
}
return NULL;
}
@@ -675,9 +681,8 @@ static int virtnet_poll(struct napi_struct *napi, int budget)
again:
while (received < budget &&
- (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
+ (buf = rq_get_buf(rq, &len)) != NULL) {
receive_buf(rq, buf, len);
- --rq->num;
received++;
}
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply related
* Re: [PATCH net 1/3] virtio-net: drop the rest of buffers when we can't allocate skb
From: Michael S. Tsirkin @ 2013-11-20 13:27 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
In-Reply-To: <1146110853.27426099.1384949282266.JavaMail.root@redhat.com>
On Wed, Nov 20, 2013 at 07:08:02AM -0500, Jason Wang wrote:
>
>
> ----- 原始邮件 -----
> > On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> > > When mergeable buffer were used, we only put the first page buf leave the
> > > rest
> > > of buffers in the virt queue. This will cause the driver could not get the
> > > correct head buffer any more. Fix this by dropping the rest of buffers for
> > > this
> > > packet.
> > >
> > > The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> > > (virtio_net: Defer skb allocation in receive path).
> > >
> > > Cc: Rusty Russell <rusty@rustcorp.com.au>
> > > Cc: Michael S. Tsirkin <mst@redhat.com>
> > > Cc: Michael Dalton <mwdalton@google.com>
> > > Cc: Eric Dumazet <edumazet@google.com>
> > > Cc: Shirley Ma <xma@us.ibm.com>
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > This patch was needed for stable
> > > ---
> > > drivers/net/virtio_net.c | 18 +++++++++++++++++-
> > > 1 file changed, 17 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> > > index 7bab4de..24fd502 100644
> > > --- a/drivers/net/virtio_net.c
> > > +++ b/drivers/net/virtio_net.c
> > > @@ -222,6 +222,17 @@ static void skb_xmit_done(struct virtqueue *vq)
> > > netif_wake_subqueue(vi->dev, vq2txq(vq));
> > > }
> > >
> > > +static void drop_mergeable_buffer(struct receive_queue *rq, int num_buf)
> > > +{
> > > + char *buf;
> > > + int len;
> > > +
> > > + while (--num_buf && (buf = virtqueue_get_buf(rq->vq, &len)) != NULL) {
> > > + --rq->num;
> > > + put_page(virt_to_head_page(buf));
> > > + }
> > > +}
> > > +
> >
> > This is the same code we have in receive_mergeable anyway.
> > So let's reuse that.
> >
> >
>
> receive_mergeable() was called after page_to_skb() was called and
> there's lots of conditions check there. I'm not sure how could we
> reuse them.
I posted a patch showing how :)
> > > /* Called from bottom half context */
> > > static struct sk_buff *page_to_skb(struct receive_queue *rq,
> > > struct page *page, unsigned int offset,
> > > @@ -237,8 +248,13 @@ static struct sk_buff *page_to_skb(struct
> > > receive_queue *rq,
> > >
> > > /* copy small packet so we can reuse these pages for small data */
> > > skb = netdev_alloc_skb_ip_align(vi->dev, GOOD_COPY_LEN);
> > > - if (unlikely(!skb))
> > > + if (unlikely(!skb)) {
> > > + if (vi->mergeable_rx_bufs) {
> > > + hdr = (struct skb_vnet_hdr *)p;
> > > + drop_mergeable_buffer(rq, hdr->mhdr.num_buffers);
> > > + }
> > > return NULL;
> > > + }
> > >
> > > hdr = skb_vnet_hdr(skb);
> > >
> > > --
> > > 1.8.3.2
> > --
> > To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> > the body of a message to majordomo@vger.kernel.org
> > More majordomo info at http://vger.kernel.org/majordomo-info.html
> > Please read the FAQ at http://www.tux.org/lkml/
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* [PATCH RFC] virtio_net: fix error handling for mergeable buffers
From: Michael S. Tsirkin @ 2013-11-20 13:26 UTC (permalink / raw)
To: Jason Wang
Cc: Michael Dalton, netdev, linux-kernel, virtualization,
Eric Dumazet, Shirley Ma
In-Reply-To: <1384938447-3775-1-git-send-email-jasowang@redhat.com>
On Wed, Nov 20, 2013 at 05:07:25PM +0800, Jason Wang wrote:
> When mergeable buffer were used, we only put the first page buf leave the rest
> of buffers in the virt queue. This will cause the driver could not get the
> correct head buffer any more. Fix this by dropping the rest of buffers for this
> packet.
>
> The bug was introduced by commit 9ab86bbcf8be755256f0a5e994e0b38af6b4d399
> (virtio_net: Defer skb allocation in receive path).
>
> Cc: Rusty Russell <rusty@rustcorp.com.au>
> Cc: Michael S. Tsirkin <mst@redhat.com>
> Cc: Michael Dalton <mwdalton@google.com>
> Cc: Eric Dumazet <edumazet@google.com>
> Cc: Shirley Ma <xma@us.ibm.com>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Just to clarify my previous comment: it was not about the
idea of adding drop_mergeable_buffer - rather, I think that
adding knowledge about mergeable buffers into page_to_skb creates an
ugly internal API.
Let's move the call to page_to_skb within receive_mergeable instead:
it's also nice that int offset = buf - page_address(page) logic
is not spread around like it was.
Also, it's not nice that we ignore length errors when we drop
packets because of OOM.
So I came up with the following - it seems to work but I didn't
stress test yet.
commit ebffb3fe4335ffe07124e4518e76d6e05844fa18
Author: Michael S. Tsirkin <mst@redhat.com>
Date: Wed Nov 20 14:41:29 2013 +0200
virtio_net: fix error handling for mergeable buffers
Eric Dumazet noticed that if we encounter an error
when processing a mergeable buffer, we don't
dequeue all of the buffers from this packet,
the result is almost sure to be loss of networking.
Jason Wang noticed that we also leak a page and that we don't decrement
the rq buf count, so we won't repost buffers (a resource leak).
Cc: Rusty Russell <rusty@rustcorp.com.au>
Cc: Michael Dalton <mwdalton@google.com>
Reported-by: Eric Dumazet <edumazet@google.com>
Reported-by: Jason Wang <jasowang@redhat.com>
Signed-off-by: Michael S. Tsirkin <mst@redhat.com>
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 01f4eb5..42f6a1e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -296,41 +296,53 @@ static struct sk_buff *page_to_skb(struct receive_queue *rq,
return skb;
}
-static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
+static struct sk_buff *receive_mergeable(struct net_device *dev,
+ struct receive_queue *rq,
+ void *buf,
+ unsigned int len)
{
- struct skb_vnet_hdr *hdr = skb_vnet_hdr(head_skb);
+ struct skb_vnet_hdr *hdr = buf;
+ int num_buf = hdr->mhdr.num_buffers;
+ struct page *page = virt_to_head_page(buf);
+ int offset = buf - page_address(page);
+ struct sk_buff *head_skb = page_to_skb(rq, page, offset, len,
+ MAX_PACKET_LEN);
struct sk_buff *curr_skb = head_skb;
- char *buf;
- struct page *page;
- int num_buf, len, offset;
- num_buf = hdr->mhdr.num_buffers;
- while (--num_buf) {
- int num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
+ if (unlikely(!curr_skb))
+ goto err_skb;
+
+ while (--num_buf) {
+ int num_skb_frags;
+
buf = virtqueue_get_buf(rq->vq, &len);
if (unlikely(!buf)) {
- pr_debug("%s: rx error: %d buffers missing\n",
- head_skb->dev->name, hdr->mhdr.num_buffers);
- head_skb->dev->stats.rx_length_errors++;
- return -EINVAL;
+ pr_debug("%s: rx error: %d buffers out of %d missing\n",
+ dev->name, num_buf, hdr->mhdr.num_buffers);
+ dev->stats.rx_length_errors++;
+ goto err_buf;
}
if (unlikely(len > MAX_PACKET_LEN)) {
pr_debug("%s: rx error: merge buffer too long\n",
- head_skb->dev->name);
+ dev->name);
len = MAX_PACKET_LEN;
}
+
+ page = virt_to_head_page(buf);
+ --rq->num;
+
+ num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
struct sk_buff *nskb = alloc_skb(0, GFP_ATOMIC);
- if (unlikely(!nskb)) {
- head_skb->dev->stats.rx_dropped++;
- return -ENOMEM;
- }
+
+ if (unlikely(!nskb))
+ goto err_skb;
if (curr_skb == head_skb)
skb_shinfo(curr_skb)->frag_list = nskb;
else
curr_skb->next = nskb;
- curr_skb = nskb;
head_skb->truesize += nskb->truesize;
+ curr_skb = nskb;
num_skb_frags = 0;
}
if (curr_skb != head_skb) {
@@ -338,8 +350,7 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
head_skb->len += len;
head_skb->truesize += MAX_PACKET_LEN;
}
- page = virt_to_head_page(buf);
- offset = buf - (char *)page_address(page);
+ offset = buf - page_address(page);
if (skb_can_coalesce(curr_skb, num_skb_frags, page, offset)) {
put_page(page);
skb_coalesce_rx_frag(curr_skb, num_skb_frags - 1,
@@ -349,9 +360,28 @@ static int receive_mergeable(struct receive_queue *rq, struct sk_buff *head_skb)
offset, len,
MAX_PACKET_LEN);
}
+ }
+
+ return head_skb;
+
+err_skb:
+ put_page(page);
+err_buf:
+ dev->stats.rx_dropped++;
+ dev_kfree_skb(head_skb);
+ while (--num_buf) {
+ buf = virtqueue_get_buf(rq->vq, &len);
+ if (unlikely(!buf)) {
+ pr_debug("%s: rx error: %d buffers missing\n",
+ dev->name, num_buf);
+ dev->stats.rx_length_errors++;
+ break;
+ }
+ page = virt_to_head_page(buf);
+ put_page(page);
--rq->num;
}
- return 0;
+ return NULL;
}
static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
@@ -380,19 +410,9 @@ static void receive_buf(struct receive_queue *rq, void *buf, unsigned int len)
len -= sizeof(struct virtio_net_hdr);
skb_trim(skb, len);
} else if (vi->mergeable_rx_bufs) {
- struct page *page = virt_to_head_page(buf);
- skb = page_to_skb(rq, page,
- (char *)buf - (char *)page_address(page),
- len, MAX_PACKET_LEN);
- if (unlikely(!skb)) {
- dev->stats.rx_dropped++;
- put_page(page);
+ skb = receive_mergeable(dev, rq, buf, len);
+ if (unlikely(!skb))
return;
- }
- if (receive_mergeable(rq, skb)) {
- dev_kfree_skb(skb);
- return;
- }
} else {
page = buf;
skb = page_to_skb(rq, page, 0, len, PAGE_SIZE);
^ permalink raw reply related
* Re: [PATCH net v3 1/2] net: sched: tbf: fix calculation of max_size
From: Yang Yingliang @ 2013-11-20 12:50 UTC (permalink / raw)
To: jbrouer; +Cc: brouer, davem, netdev, eric.dumazet, jpirko
In-Reply-To: <20131120110448.50b68d23@redhat.com>
On 2013/11/20 18:04, Jesper Dangaard Brouer wrote:
> On Wed, 20 Nov 2013 10:14:43 +0800
> Yang Yingliang <yangyingliang@huawei.com> wrote:
>> On 2013/11/19 17:38, Jesper Dangaard Brouer wrote:
>>> On Tue, 19 Nov 2013 15:25:38 +0800
>>> Yang Yingliang <yangyingliang@huawei.com> wrote:
>>>
> [...]
>>>> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
>>>> index 68f9859..c194129 100644
>>>> --- a/net/sched/sch_tbf.c
>>>> +++ b/net/sched/sch_tbf.c
>>> [...]
>>>> @@ -339,30 +326,46 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt)
>>> [...]
>>>> + for (n = 0; n < 65536; n++)
>>>> + if (psched_l2t_ns(&q->rate, n) > q->buffer)
>>>> + break;
>>>> + max_size = min_t(u32, n, (256ULL << qopt->rate.cell_log) - 1);
>>> I'm a little uncertain about, if using the 65536 constant is okay, or
>>> considered "bad style".
>> I'll use a MACRO to instead of this constant in v4.
> I'm not saying you have to use a macro for this. I'm fine with using
> 65536 here, some other developers might have stronger opinions on this?
>
>
>>> I'm still a little confused/uncertain why we need the "qopt->rate.cell_log".
>>>
>> Because we need max_size be smaller than mtu(user input in bytes).
>> E.g. if user inputs like this "... burst 100KB rate 100mbit mtu 4095",
>> without this patch, max_size is 4095.
>> But with this patch, if don't use cell_log, max_size is 102400.
>> I think it's not correct, so I used cell_log here.
>
> Hmmmm... so you are using cell_log to approximate the MTU size, because
> struct tc_ratespec, does not contain the a mtu parameter. You do
> realize this is only an approximation. (This also smells like a bad
> transition away from the userspace rate tables)
Hmmm... not exactly, cell_log can be input by user,
if input like this "... burst 100KB/4 rate 100mbit mtu 4095",
with old calculation method, max_size would be 1023, but with
new calculation method, it's 65536. I use cell_log here make it
be same as before.
Maybe don't need be same, what's your opinion?
Thanks!
>
> What about the p->mtu parameter from struct tc_tbf_qopt. I know it is
> in a "time" format... but hey, you are already using this parameter
> requoting your patch:
>
>
> On Tue, 19 Nov 2013 15:25:38 +0800
> Yang Yingliang <yangyingliang@huawei.com> wrote:
>
>> diff --git a/net/sched/sch_tbf.c b/net/sched/sch_tbf.c
>> index 68f9859..c194129 100644
>> --- a/net/sched/sch_tbf.c
>> +++ b/net/sched/sch_tbf.c
> [... cut ...]
>> @@ -339,30 +326,46 @@ static int tbf_change(struct Qdisc *sch, struct nlattr *opt)
>> }
>> q->limit = qopt->limit;
>> q->mtu = PSCHED_TICKS2NS(qopt->mtu);
>> - q->max_size = max_size;
>> q->buffer = PSCHED_TICKS2NS(qopt->buffer);
>> q->tokens = q->buffer;
>> q->ptokens = q->mtu;
>>
>> if (tb[TCA_TBF_RATE64])
>> rate64 = nla_get_u64(tb[TCA_TBF_RATE64]);
>> - psched_ratecfg_precompute(&q->rate, &rtab->rate, rate64);
>> - if (ptab) {
>> + psched_ratecfg_precompute(&q->rate, &qopt->rate, rate64);
>> + if (!q->rate.rate_bytes_ps)
>> + goto unlock_done;
>> + for (n = 0; n < 65536; n++)
>> + if (psched_l2t_ns(&q->rate, n) > q->buffer)
>> + break;
>> + max_size = min_t(u32, n, (256ULL << qopt->rate.cell_log) - 1);
>> +
>> + if (qopt->peakrate.rate) {
>> + int size = 0;
>> if (tb[TCA_TBF_PRATE64])
>> prate64 = nla_get_u64(tb[TCA_TBF_PRATE64]);
>> - psched_ratecfg_precompute(&q->peak, &ptab->rate, prate64);
>> + psched_ratecfg_precompute(&q->peak, &qopt->peakrate, prate64);
>> + for (n = 0; n < 65536; n++)
>> + if (psched_l2t_ns(&q->peak, n) > q->mtu)
> You are using q->mtu here, BUT why are you only doing this, when there
> is a qopt->peakrate.rate, set??? (I might have missed something)
>
When qopt->peakrate.rate is not set, q->mtu is 0.
Regards,
Yang
>> + break;
>> + size = min_t(u32, n, (256ULL << qopt->rate.cell_log) - 1);
>> + max_size = min_t(u32, max_size, size);
>> q->peak_present = true;
>> } else {
>> q->peak_present = false;
>> }
>
^ 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