* Re: [RFC v1 0/3] Add VIRTIO_F_IOMMU_PLATFORM support
From: Michael S. Tsirkin @ 2017-07-21 23:48 UTC (permalink / raw)
To: Brijesh Singh
Cc: Tom Lendacky, David Airlie, Jordan Justen, dri-devel,
virtualization, Laszlo Ersek, edk2-devel
In-Reply-To: <25d78a89-59bd-157a-ccf5-70cd25e14a05@amd.com>
On Fri, Jul 21, 2017 at 06:17:15AM -0500, Brijesh Singh wrote:
>
> On 7/20/17 10:24 PM, Jason Wang wrote:
> >
> >
> > On 2017年07月20日 06:09, Brijesh Singh wrote:
> >> I have found that OVMF fails to detect the disk when iommu_platform
> >> is set from
> >> qemu cli. The failure occurs during the feature bit negotiation.
> >>
> >> Recently, EDKII introduced IOMMU protocol d1fddc4533bf. SEV patch
> >> series introduced
> >> a IoMmu protocol driver f9d129e68a45 to set a DMA access attribute
> >> and methods to
> >> allocate, free, map and unmap the DMA memory for the master bus devices
> >>
> >> In this patch series, I have tried to enable the IOMMU_PLATFORM
> >> feature for
> >> VirtioBlkDevice. I am sending this as RFC to seek feedback before I
> >> extend the support
> >> for other Virtio devices. The patch has been tested in SEV guest -
> >> mainly because
> >> IoMmuDxe driver installs the IOMMU protocol for SEV guest only. If
> >> needed, I can
> >> extend the IoMmuDxe driver to install IOMMU protocol for non SEV guests.
> >>
> >> qemu cli used for testing:
> >>
> >> # $QEMU \
> >> ...
> >> -drive file=${HDA_FILE},if=none,id=disk0,format=qcow2 \
> >> -device
> >> virtio-blk-pci,drive=disk0,disable-legacy=on,iommu_platform=true,disable-modern=off,scsi=off
> >> ...
> >>
> >> Cc: Jordan Justen <jordan.l.justen@intel.com>
> >> Cc: Laszlo Ersek <lersek@redhat.com>
> >> Cc: Jason Wang <jasowang@redhat.com>
> >> Cc: Michael S. Tsirkin <mst@redhat.com>
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Brijesh Singh <brijesh.singh@amd.com>
> >>
> >> Brijesh Singh (3):
> >> OvmfPkg/Include/Virtio10: Define VIRTIO_F_IOMMU_PLATFORM feature bit
> >> OvmfPkg/VirtioLib: Add IOMMU_PLATFORM support
> >> OvmfPkg/VirtioBlkDxe: Add VIRITO_F_IOMMU_PLATFORM support
> >
> > Hi, do we need change virtio-scsi driver as well?
> >
> I see that OVMF has the following virtio drivers, we need to update them
> all:
>
> VirtioBlkDxe
> VirtioGpuDxe
> VirtioNetDxe
> VirtioRngDxe
> VirtioScsiDxe
>
> I will wait for Laszlo and Jordan's initial feedback before changing
> other drivers.
I'm not sure about the GPU. Cc relevant maintainers -
can virtio GPU work from behind an IOMMU?
> > Thanks
> >
> >>
> >> OvmfPkg/Library/VirtioLib/VirtioLib.inf | 1 +
> >> OvmfPkg/VirtioBlkDxe/VirtioBlk.inf | 5 +
> >> OvmfPkg/VirtioGpuDxe/VirtioGpu.inf | 1 +
> >> OvmfPkg/VirtioNetDxe/VirtioNet.inf | 1 +
> >> OvmfPkg/VirtioRngDxe/VirtioRng.inf | 1 +
> >> OvmfPkg/VirtioScsiDxe/VirtioScsi.inf | 1 +
> >> OvmfPkg/Include/IndustryStandard/Virtio095.h | 1 +
> >> OvmfPkg/Include/IndustryStandard/Virtio10.h | 5 +
> >> OvmfPkg/Include/Library/VirtioLib.h | 20 ++++
> >> OvmfPkg/Library/VirtioLib/VirtioLib.c | 96 ++++++++++++++-
> >> OvmfPkg/VirtioBlkDxe/VirtioBlk.c | 125
> >> ++++++++++++++++++--
> >> 11 files changed, 244 insertions(+), 13 deletions(-)
> >>
> >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [virtio-dev] packed ring layout proposal v2
From: Michael S. Tsirkin @ 2017-07-20 13:06 UTC (permalink / raw)
To: Lior Narkis
Cc: virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <DB5PR05MB1766461893DB3FE20D338B96D3A60@DB5PR05MB1766.eurprd05.prod.outlook.com>
On Wed, Jul 19, 2017 at 07:41:55AM +0000, Lior Narkis wrote:
>
>
> > -----Original Message-----
> > From: Michael S. Tsirkin [mailto:mst@redhat.com]
> > Sent: Tuesday, July 18, 2017 7:23 PM
> > To: Lior Narkis <liorn@mellanox.com>
> > Cc: virtio-dev@lists.oasis-open.org; virtualization@lists.linux-foundation.org
> > Subject: Re: [virtio-dev] packed ring layout proposal v2
> >
> > On Sun, Jul 16, 2017 at 06:00:45AM +0000, Lior Narkis wrote:
> > > > -----Original Message-----
> > > > From: virtio-dev@lists.oasis-open.org [mailto:virtio-dev@lists.oasis-
> > open.org]
> > > > On Behalf Of Michael S. Tsirkin
> > > > Sent: Wednesday, February 08, 2017 5:20 AM
> > > > To: virtio-dev@lists.oasis-open.org
> > > > Cc: virtualization@lists.linux-foundation.org
> > > > Subject: [virtio-dev] packed ring layout proposal v2
> > > >
> > > > This is an update from v1 version.
> > > > Changes:
> > > > - update event suppression mechanism
> > > > - separate options for indirect and direct s/g
> > > > - lots of new features
> > > >
> > > > ---
> > > >
> > > > Performance analysis of this is in my kvm forum 2016 presentation.
> > > > The idea is to have a r/w descriptor in a ring structure,
> > > > replacing the used and available ring, index and descriptor
> > > > buffer.
> > > >
> > > > * Descriptor ring:
> > > >
> > > > Guest adds descriptors with unique index values and DESC_HW set in flags.
> > > > Host overwrites used descriptors with correct len, index, and DESC_HW
> > > > clear. Flags are always set/cleared last.
> > > >
> > > > #define DESC_HW 0x0080
> > > >
> > > > struct desc {
> > > > __le64 addr;
> > > > __le32 len;
> > > > __le16 index;
> > > > __le16 flags;
> > > > };
> > > >
> > > > When DESC_HW is set, descriptor belongs to device. When it is clear,
> > > > it belongs to the driver.
> > > >
> > > > We can use 1 bit to set direction
> > > > /* This marks a buffer as write-only (otherwise read-only). */
> > > > #define VRING_DESC_F_WRITE 2
> > > >
> > >
> > > A valid bit per descriptor does not let the device do an efficient prefetch.
> > > An alternative is to use a producer index(PI).
> > > Using the PI posted by the driver, and the Consumer Index(CI) maintained
> > by the device, the device knows how much work it has outstanding, so it can
> > do the prefetch accordingly.
> > > There are few options for the device to acquire the PI.
> > > Most efficient will be to write the PI in the doorbell together with the queue
> > number.
> >
> > Right. This was suggested in "Fwd: Virtio-1.1 Ring Layout".
> > Or just the PI if we don't need the queue number.
> >
> > > I would like to raise the need for a Completion Queue(CQ).
> > > Multiple Work Queues(hold the work descriptors, WQ in short) can be
> > connected to a single CQ.
> > > So when the device completes the work on the descriptor, it writes a
> > Completion Queue Entry (CQE) to the CQ.
> >
> > This seems similar to the design we currently have with a separate used
> > ring. It seems to underperform writing into the available ring
> > at least on a microbenchmark. The reason seems to be that
> > more cache lines need to get invalidated and re-fetched.
>
>
> Few points on that:
> Each PCIe write will cause invalidation to a cache line.
> Writing less than a cache line is inefficient, so it is better to put all metadata together and allocate a cache line for it.
> Putting the metadata in the data buffer means two writes of less than a cache line each, both will be accessed by the driver, so potential two misses.
>
I'm not sure how this is related to your suggestion. Sorry about being
dense. You suggested a separate used ring that can also be shared with
multiple available rings. Current design in effect makes used and
available rings overlap instead. One side effect is each entry is bigger
now (16 bytes as opposed to 8 bytes previously) so it should be easier
to move metadata from e.g. virtio net header to the descriptor entry.
> >
> > > CQEs are continuous in memory so prefetching by the driver is efficient,
> > although the device might complete work descriptors out of order.
> >
> > That's not different from proposal you are replying to - descriptors
> > can be used and written out in any order, they are contigious
> > so driver can prefetch.
>
> Point is that if descriptors 1, 2, 4 are being completed in that order, in the proposed layout the completion indication will be placed at 1, 2, 4 in the virtq desc buffer.
Note: I think you mean used, not completed. Let's use the standard virtio terminology.
The answer is - not necessarily. device can write them out at entries 1,2,3. The only
requirement is really that eventually all entries 1-4 are switches to
driver ownership.
v2 says:
Device can write descriptors out in order as they are used, overwriting
descriptors that are there.
I think it would be clearer if it said:
Device can write descriptors out in the order in which they are used, overwriting
descriptors that are there.
> With a CQ, completions on 1, 2, 4 will be placed at 1, 2, 3 CQ indexes.
> This is why it is better for prefetching.
Looks like a misunderstanding then.
> > I'd like to add that attempts to
> > add prefetch e.g. for the virtio used ring never showed any
> > conclusive gains - some workloads would get minor a speedup, others
> > lose a bit of performance. I do not think anyone looked
> > deeply into why that was the case. It would be easy for you to add this
> > code to current virtio drivers and/or devices and try it out.
>
> Noted.
> I will say though that mlx5 uses prefetch and gets good performance because of it...
It is pretty popular with drivers, worth revisiting if someone has the
time.
> >
> > > The interrupt handler is connected to the CQ, so an allocation of a single CQ
> > per core, with a single interrupt handler is possible although this core might be
> > using multiple WQs.
> >
> > Sending a single interrupt from multiple rings might save some
> > cycles. event index/interrupt disable are currently in
> > RAM so access is very cheap for the guest.
> > If you are going to share, just disable all interrupts
> > when you start processing.
> >
> > I was wondering how do people want to do this in hardware
> > in fact. Are you going to read event index after each descriptor?
>
> Not sure I got you here.
> Do you ask about how the device decides to write MSIX? And how interrupt moderation might work?
virtio has a flags/event index based interrupt suppression. It relies on
device reading flags/index after writing out a batch of descriptors.
Is this too costly and we should switch to driver writing the index,
or ok since it's only once per batch?
> >
> > It might make sense to move event index/flags into device memory. And
> > once you do this, writing these out might become slower, and then some
> > kind of sharing of the event index might help.
> >
> > No one suggested anything like this so far - maybe it's less of an issue
> > than I think, e.g. because of interrupt coalescing (which virtio also
> > does not have, but could be added if there is interest) or maybe people
> > just mostly care about polling performance.
> >
> > > One application for multiple WQs with a single CQ is Quality of Service(QoS).
> > > A user can open a WQ per QoS value(pcp value for example), and the device
> > will schedule the work accordingly.
> >
> > A ring per QOS level might make sense e.g. in a network device. I don't
> > see why it's helpful to write out completed entries into a separate
> > ring for that though.
>
> I would like to add that for rdma device there are many queues (QPs), understanding which QP completed work by traversing all QPs in not efficient.
>
> Another advantage of having a CQ connected to multiple WQs is that the interrupt moderation can be based on this single CQ,
> So the criteria if to write interrupt or not is based on all the aggregated work that was completed on that CQ.
>
> >
> > As we don't have QOS support in virtio net at all right now,
> > would that be best discussed once we have a working prototype
> > and everyone can see what the problem is?
>
> Understood.
> Although, I think the layout should not change frequently.
>
> >
> >
> > > > * Scatter/gather support
> > > >
> > > > We can use 1 bit to chain s/g entries in a request, same as virtio 1.0:
> > > >
> > > > /* This marks a buffer as continuing via the next field. */
> > > > #define VRING_DESC_F_NEXT 1
> > > >
> > > > Unlike virtio 1.0, all descriptors must have distinct ID values.
> > > >
> > > > Also unlike virtio 1.0, use of this flag will be an optional feature
> > > > (e.g. VIRTIO_F_DESC_NEXT) so both devices and drivers can opt out of it.
> > > >
> > > > * Indirect buffers
> > > >
> > > > Can be marked like in virtio 1.0:
> > > >
> > > > /* This means the buffer contains a table of buffer descriptors. */
> > > > #define VRING_DESC_F_INDIRECT 4
> > > >
> > > > Unlike virtio 1.0, this is a table, not a list:
> > > > struct indirect_descriptor_table {
> > > > /* The actual descriptors (16 bytes each) */
> > > > struct virtq_desc desc[len / 16];
> > > > };
> > > >
> > > > The first descriptor is located at start of the indirect descriptor
> > > > table, additional indirect descriptors come immediately afterwards.
> > > > DESC_F_WRITE is the only valid flag for descriptors in the indirect
> > > > table. Others should be set to 0 and are ignored. id is also set to 0
> > > > and should be ignored.
> > > >
> > > > virtio 1.0 seems to allow a s/g entry followed by
> > > > an indirect descriptor. This does not seem useful,
> > > > so we do not allow that anymore.
> > > >
> > > > This support would be an optional feature, same as in virtio 1.0
> > > >
> > > > * Batching descriptors:
> > > >
> > > > virtio 1.0 allows passing a batch of descriptors in both directions, by
> > > > incrementing the used/avail index by values > 1. We can support this by
> > > > chaining a list of descriptors through a bit the flags field.
> > > > To allow use together with s/g, a different bit will be used.
> > > >
> > > > #define VRING_DESC_F_BATCH_NEXT 0x0010
> > > >
> > > > Batching works for both driver and device descriptors.
> > > >
> > > >
> > > >
> > > > * Processing descriptors in and out of order
> > > >
> > > > Device processing all descriptors in order can simply flip
> > > > the DESC_HW bit as it is done with descriptors.
> > > >
> > > > Device can write descriptors out in order as they are used, overwriting
> > > > descriptors that are there.
> > > >
> > > > Device must not use a descriptor until DESC_HW is set.
> > > > It is only required to look at the first descriptor
> > > > submitted.
> > > >
> > > > Driver must not overwrite a descriptor until DESC_HW is clear.
> > > > It is only required to look at the first descriptor
> > > > submitted.
> > > >
> > > > * Device specific descriptor flags
> > > > We have a lot of unused space in the descriptor. This can be put to
> > > > good use by reserving some flag bits for device use.
> > > > For example, network device can set a bit to request
> > > > that header in the descriptor is suppressed
> > > > (in case it's all 0s anyway). This reduces cache utilization.
> > > >
> > > > Note: this feature can be supported in virtio 1.0 as well,
> > > > as we have unused bits in both descriptor and used ring there.
> > > >
> > > > * Descriptor length in device descriptors
> > > >
> > > > virtio 1.0 places strict requirements on descriptor length. For example
> > > > it must be 0 in used ring of TX VQ of a network device since nothing is
> > > > written. In practice guests do not seem to use this, so we can simplify
> > > > devices a bit by removing this requirement - if length is unused it
> > > > should be ignored by driver.
> > > >
> > > > Some devices use identically-sized buffers in all descriptors.
> > > > Ignoring length for driver descriptors there could be an option too.
> > > >
> > > > * Writing at an offset
> > > >
> > > > Some devices might want to write into some descriptors
> > > > at an offset, the offset would be in config space,
> > > > and a descriptor flag could indicate this:
> > > >
> > > > #define VRING_DESC_F_OFFSET 0x0020
> > > >
> > > > How exactly to use the offset would be device specific,
> > > > for example it can be used to align beginning of packet
> > > > in the 1st buffer for mergeable buffers to cache line boundary
> > > > while also aligning rest of buffers.
> > > >
> > > > * Non power-of-2 ring sizes
> > > >
> > > > As the ring simply wraps around, there's no reason to
> > > > require ring size to be power of two.
> > > > It can be made a separate feature though.
> > > >
> > > >
> > > > * Interrupt/event suppression
> > > >
> > > > virtio 1.0 has two mechanisms for suppression but only
> > > > one can be used at a time. we pack them together
> > > > in a structure - one for interrupts, one for notifications:
> > > >
> > > > struct event {
> > > > __le16 idx;
> > > > __le16 flags;
> > > > }
> > > >
> > > > Both fields would be optional, with a feature bit:
> > > > VIRTIO_F_EVENT_IDX
> > > > VIRTIO_F_EVENT_FLAGS
> > > >
> > > > * Flags can be used like in virtio 1.0, by storing a special
> > > > value there:
> > > >
> > > > #define VRING_F_EVENT_ENABLE 0x0
> > > >
> > > > #define VRING_F_EVENT_DISABLE 0x1
> > > >
> > > > * Event index would be in the range 0 to 2 * Queue Size
> > > > (to detect wrap arounds) and wrap to 0 after that.
> > > >
> > > > The assumption is that each side maintains an internal
> > > > descriptor counter 0 to 2 * Queue Size that wraps to 0.
> > > > In that case, interrupt triggers when counter reaches
> > > > the given value.
> > > >
> > > > * If both features are negotiated, a special flags value
> > > > can be used to switch to event idx:
> > > >
> > > > #define VRING_F_EVENT_IDX 0x2
> > > >
> > > >
> > > > * Prototype
> > > >
> > > > A partial prototype can be found under
> > > > tools/virtio/ringtest/ring.c
> > > >
> > > > Test run:
> > > > [mst@tuck ringtest]$ time ./ring
> > > > real 0m0.399s
> > > > user 0m0.791s
> > > > sys 0m0.000s
> > > > [mst@tuck ringtest]$ time ./virtio_ring_0_9
> > > > real 0m0.503s
> > > > user 0m0.999s
> > > > sys 0m0.000s
> > > >
> > > > It is planned to update it to this interface. Future changes and
> > > > enhancements can (and should) be tested against this prototype.
> > > >
> > > > * Feature sets
> > > > In particular are we going overboard with feature bits? It becomes hard
> > > > to support all combinations in drivers and devices. Maybe we should
> > > > document reasonable feature sets to be supported for each device.
> > > >
> > > > * Known issues/ideas
> > > >
> > > > This layout is optimized for host/guest communication,
> > > > in a sense even more aggressively than virtio 1.0.
> > > > It might be suboptimal for PCI hardware implementations.
> > > > However, one notes that current virtio pci drivers are
> > > > unlikely to work with PCI hardware implementations anyway
> > > > (e.g. due to use of SMP barriers for ordering).
> > > >
> > > > Suggestions for improving this are welcome but need to be tested to make
> > > > sure our main use case does not regress. Of course some improvements
> > > > might be made optional, but if we add too many of these driver becomes
> > > > unmanageable.
> > > >
> > > > ---
> > > >
> > > > Note: should this proposal be accepted and approved, one or more
> > > > claims disclosed to the TC admin and listed on the Virtio TC
> > > > IPR page
> > > >
> > https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > > > w.oasis-
> > > >
> > open.org%2Fcommittees%2Fvirtio%2Fipr.php&data=02%7C01%7Cliorn%40m
> > > >
> > ellanox.com%7Cf41239019c1441e73b0308d4c7b0a483%7Ca652971c7d2e4d9
> > > >
> > ba6a4d149256f461b%7C0%7C0%7C636353008872143792&sdata=L946V5o0P
> > > > k8th%2B2IkHgvALmhnIEWD9hcMZvMxLetavc%3D&reserved=0
> > > > might become Essential Claims.
> > > >
> > > > --
> > > > MST
> > > >
> > > > ---------------------------------------------------------------------
> > > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Wei Wang @ 2017-07-19 12:01 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange, virtio-dev, amit.shah, kvm, Michael S. Tsirkin,
linux-kernel, liliang.opensource, qemu-devel, virtualization,
linux-mm, yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm,
mgorman
In-Reply-To: <20170719081311.GC26779@dhcp22.suse.cz>
On 07/19/2017 04:13 PM, Michal Hocko wrote:
> On Tue 18-07-17 10:12:14, Wei Wang wrote:
> [...]
>> Probably I should have included the introduction of the usages in
>> the log. Hope it is not too later to explain here:
> Yes this should have been described in the cover.
OK, I will do it in the next version.
>
>> Live migration needs to transfer the VM's memory from the source
>> machine to the destination round by round. For the 1st round, all the VM's
>> memory is transferred. From the 2nd round, only the pieces of memory
>> that were written by the guest (after the 1st round) are transferred. One
>> method that is popularly used by the hypervisor to track which part of
>> memory is written is to write-protect all the guest memory.
>>
>> This patch enables the optimization of the 1st round memory transfer -
>> the hypervisor can skip the transfer of guest unused pages in the 1st round.
> All you should need is the check for the page reference count, no? I
> assume you do some sort of pfn walk and so you should be able to get an
> access to the struct page.
Not necessarily - the guest struct page is not seen by the hypervisor. The
hypervisor only gets those guest pfns which are hinted as unused. From the
hypervisor (host) point of view, a guest physical address corresponds to a
virtual address of a host process. So, once the hypervisor knows a guest
physical page is unsued, it knows that the corresponding virtual memory of
the process doesn't need to be transferred in the 1st round.
Best,
Wei
^ permalink raw reply
* [PATCH net-next V2 5/5] virtio-net: switch off offloads on demand if possible on XDP set
From: Jason Wang @ 2017-07-19 8:54 UTC (permalink / raw)
To: mst, virtualization, jasowang, netdev, linux-kernel
In-Reply-To: <1500454489-16805-1-git-send-email-jasowang@redhat.com>
Current XDP implementation wants guest offloads feature to be disabled
on device. This is inconvenient and means guest can't benefit from
offloads if XDP is not used. This patch tries to address this
limitation by disabling the offloads on demand through control guest
offloads. Guest offloads will be disabled and enabled on demand on XDP
set.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 70 ++++++++++++++++++++++++++++++++++++++++++++----
1 file changed, 65 insertions(+), 5 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index b3fc01d..5fbd15e 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -57,6 +57,11 @@ DECLARE_EWMA(pkt_len, 0, 64)
#define VIRTNET_DRIVER_VERSION "1.0.0"
+const unsigned long guest_offloads[] = { VIRTIO_NET_F_GUEST_TSO4,
+ VIRTIO_NET_F_GUEST_TSO6,
+ VIRTIO_NET_F_GUEST_ECN,
+ VIRTIO_NET_F_GUEST_UFO };
+
struct virtnet_stats {
struct u64_stats_sync tx_syncp;
struct u64_stats_sync rx_syncp;
@@ -164,10 +169,13 @@ struct virtnet_info {
u8 ctrl_promisc;
u8 ctrl_allmulti;
u16 ctrl_vid;
+ u64 ctrl_offloads;
/* Ethtool settings */
u8 duplex;
u32 speed;
+
+ unsigned long guest_offloads;
};
struct padded_vnet_hdr {
@@ -1896,6 +1904,47 @@ static int virtnet_restore_up(struct virtio_device *vdev)
return err;
}
+static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
+{
+ struct scatterlist sg;
+ vi->ctrl_offloads = cpu_to_virtio64(vi->vdev, offloads);
+
+ sg_init_one(&sg, &vi->ctrl_offloads, sizeof(vi->ctrl_offloads));
+
+ if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
+ VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) {
+ dev_warn(&vi->dev->dev, "Fail to set guest offload. \n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int virtnet_clear_guest_offloads(struct virtnet_info *vi)
+{
+ u64 offloads = 0;
+
+ if (!vi->guest_offloads)
+ return 0;
+
+ if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
+ offloads = 1ULL << VIRTIO_NET_F_GUEST_CSUM;
+
+ return virtnet_set_guest_offloads(vi, offloads);
+}
+
+static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
+{
+ u64 offloads = vi->guest_offloads;
+
+ if (!vi->guest_offloads)
+ return 0;
+ if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
+ offloads |= 1ULL << VIRTIO_NET_F_GUEST_CSUM;
+
+ return virtnet_set_guest_offloads(vi, offloads);
+}
+
static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
struct netlink_ext_ack *extack)
{
@@ -1905,10 +1954,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
u16 xdp_qp = 0, curr_qp;
int i, err;
- if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
- virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
- virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
- virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO)) {
+ if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)
+ && (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
+ virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
+ virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
+ virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO))) {
NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing LRO, disable LRO first");
return -EOPNOTSUPP;
}
@@ -1955,6 +2005,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
for (i = 0; i < vi->max_queue_pairs; i++) {
old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
+ if (i == 0) {
+ if (!old_prog)
+ virtnet_clear_guest_offloads(vi);
+ if (!prog)
+ virtnet_restore_guest_offloads(vi);
+ }
if (old_prog)
bpf_prog_put(old_prog);
virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
@@ -2588,6 +2644,10 @@ static int virtnet_probe(struct virtio_device *vdev)
netif_carrier_on(dev);
}
+ for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
+ if (virtio_has_feature(vi->vdev, guest_offloads[i]))
+ set_bit(guest_offloads[i], &vi->guest_offloads);
+
pr_debug("virtnet: registered device %s with %d RX and TX vq's\n",
dev->name, max_queue_pairs);
@@ -2684,7 +2744,7 @@ static struct virtio_device_id id_table[] = {
VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
VIRTIO_NET_F_CTRL_MAC_ADDR, \
- VIRTIO_NET_F_MTU
+ VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
static unsigned int features[] = {
VIRTNET_FEATURES,
--
2.7.4
^ permalink raw reply related
* [PATCH net-next V2 4/5] virtio-net: do not reset during XDP set
From: Jason Wang @ 2017-07-19 8:54 UTC (permalink / raw)
To: mst, virtualization, jasowang, netdev, linux-kernel
In-Reply-To: <1500454489-16805-1-git-send-email-jasowang@redhat.com>
We currently reset the device during XDP set, the main reason is
that we allocate more headroom with XDP (for header adjustment).
This works but causes network downtime for users.
Previous patches encoded the headroom in the buffer context,
this makes it possible to detect the case where a buffer
with headroom insufficient for XDP is added to the queue and
XDP is enabled afterwards.
Upon detection, we handle this case by copying the packet
(slow, but it's a temporary condition).
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 232 ++++++++++++++++++++++-------------------------
1 file changed, 106 insertions(+), 126 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 640f1de..b3fc01d 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -407,6 +407,69 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
return vi->xdp_queue_pairs ? VIRTIO_XDP_HEADROOM : 0;
}
+/* We copy the packet for XDP in the following cases:
+ *
+ * 1) Packet is scattered across multiple rx buffers.
+ * 2) Headroom space is insufficient.
+ *
+ * This is inefficient but it's a temporary condition that
+ * we hit right after XDP is enabled and until queue is refilled
+ * with large buffers with sufficient headroom - so it should affect
+ * at most queue size packets.
+ * Afterwards, the conditions to enable
+ * XDP should preclude the underlying device from sending packets
+ * across multiple buffers (num_buf > 1), and we make sure buffers
+ * have enough headroom.
+ */
+static struct page *xdp_linearize_page(struct receive_queue *rq,
+ u16 *num_buf,
+ struct page *p,
+ int offset,
+ int page_off,
+ unsigned int *len)
+{
+ struct page *page = alloc_page(GFP_ATOMIC);
+
+ if (!page)
+ return NULL;
+
+ memcpy(page_address(page) + page_off, page_address(p) + offset, *len);
+ page_off += *len;
+
+ while (--*num_buf) {
+ unsigned int buflen;
+ void *buf;
+ int off;
+
+ buf = virtqueue_get_buf(rq->vq, &buflen);
+ if (unlikely(!buf))
+ goto err_buf;
+
+ p = virt_to_head_page(buf);
+ off = buf - page_address(p);
+
+ /* guard against a misconfigured or uncooperative backend that
+ * is sending packet larger than the MTU.
+ */
+ if ((page_off + buflen) > PAGE_SIZE) {
+ put_page(p);
+ goto err_buf;
+ }
+
+ memcpy(page_address(page) + page_off,
+ page_address(p) + off, buflen);
+ page_off += buflen;
+ put_page(p);
+ }
+
+ /* Headroom does not contribute to packet length */
+ *len = page_off - VIRTIO_XDP_HEADROOM;
+ return page;
+err_buf:
+ __free_pages(page, 0);
+ return NULL;
+}
+
static struct sk_buff *receive_small(struct net_device *dev,
struct virtnet_info *vi,
struct receive_queue *rq,
@@ -415,12 +478,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
{
struct sk_buff *skb;
struct bpf_prog *xdp_prog;
- unsigned int xdp_headroom = virtnet_get_headroom(vi);
+ unsigned int xdp_headroom = (unsigned long)ctx;
unsigned int header_offset = VIRTNET_RX_PAD + xdp_headroom;
unsigned int headroom = vi->hdr_len + header_offset;
unsigned int buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+ struct page *page = virt_to_head_page(buf);
unsigned int delta = 0;
+ struct page *xdp_page;
len -= vi->hdr_len;
rcu_read_lock();
@@ -434,6 +499,27 @@ static struct sk_buff *receive_small(struct net_device *dev,
if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
goto err_xdp;
+ if (unlikely(xdp_headroom < virtnet_get_headroom(vi))) {
+ int offset = buf - page_address(page) + header_offset;
+ unsigned int tlen = len + vi->hdr_len;
+ u16 num_buf = 1;
+
+ xdp_headroom = virtnet_get_headroom(vi);
+ header_offset = VIRTNET_RX_PAD + xdp_headroom;
+ headroom = vi->hdr_len + header_offset;
+ buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
+ xdp_page = xdp_linearize_page(rq, &num_buf, page,
+ offset, header_offset,
+ &tlen);
+ if (!xdp_page)
+ goto err_xdp;
+
+ buf = page_address(xdp_page);
+ put_page(page);
+ page = xdp_page;
+ }
+
xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
xdp.data = xdp.data_hard_start + xdp_headroom;
xdp.data_end = xdp.data + len;
@@ -462,7 +548,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
skb = build_skb(buf, buflen);
if (!skb) {
- put_page(virt_to_head_page(buf));
+ put_page(page);
goto err;
}
skb_reserve(skb, headroom - delta);
@@ -478,7 +564,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
err_xdp:
rcu_read_unlock();
dev->stats.rx_dropped++;
- put_page(virt_to_head_page(buf));
+ put_page(page);
xdp_xmit:
return NULL;
}
@@ -503,66 +589,6 @@ static struct sk_buff *receive_big(struct net_device *dev,
return NULL;
}
-/* The conditions to enable XDP should preclude the underlying device from
- * sending packets across multiple buffers (num_buf > 1). However per spec
- * it does not appear to be illegal to do so but rather just against convention.
- * So in order to avoid making a system unresponsive the packets are pushed
- * into a page and the XDP program is run. This will be extremely slow and we
- * push a warning to the user to fix this as soon as possible. Fixing this may
- * require resolving the underlying hardware to determine why multiple buffers
- * are being received or simply loading the XDP program in the ingress stack
- * after the skb is built because there is no advantage to running it here
- * anymore.
- */
-static struct page *xdp_linearize_page(struct receive_queue *rq,
- u16 *num_buf,
- struct page *p,
- int offset,
- unsigned int *len)
-{
- struct page *page = alloc_page(GFP_ATOMIC);
- unsigned int page_off = VIRTIO_XDP_HEADROOM;
-
- if (!page)
- return NULL;
-
- memcpy(page_address(page) + page_off, page_address(p) + offset, *len);
- page_off += *len;
-
- while (--*num_buf) {
- unsigned int buflen;
- void *buf;
- int off;
-
- buf = virtqueue_get_buf(rq->vq, &buflen);
- if (unlikely(!buf))
- goto err_buf;
-
- p = virt_to_head_page(buf);
- off = buf - page_address(p);
-
- /* guard against a misconfigured or uncooperative backend that
- * is sending packet larger than the MTU.
- */
- if ((page_off + buflen) > PAGE_SIZE) {
- put_page(p);
- goto err_buf;
- }
-
- memcpy(page_address(page) + page_off,
- page_address(p) + off, buflen);
- page_off += buflen;
- put_page(p);
- }
-
- /* Headroom does not contribute to packet length */
- *len = page_off - VIRTIO_XDP_HEADROOM;
- return page;
-err_buf:
- __free_pages(page, 0);
- return NULL;
-}
-
static struct sk_buff *receive_mergeable(struct net_device *dev,
struct virtnet_info *vi,
struct receive_queue *rq,
@@ -577,6 +603,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
struct sk_buff *head_skb, *curr_skb;
struct bpf_prog *xdp_prog;
unsigned int truesize;
+ unsigned int headroom = mergeable_ctx_to_headroom(ctx);
head_skb = NULL;
@@ -589,10 +616,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
u32 act;
/* This happens when rx buffer size is underestimated */
- if (unlikely(num_buf > 1)) {
+ if (unlikely(num_buf > 1 ||
+ headroom < virtnet_get_headroom(vi))) {
/* linearize data for XDP */
xdp_page = xdp_linearize_page(rq, &num_buf,
- page, offset, &len);
+ page, offset,
+ VIRTIO_XDP_HEADROOM,
+ &len);
if (!xdp_page)
goto err_xdp;
offset = VIRTIO_XDP_HEADROOM;
@@ -835,7 +865,6 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
if (err < 0)
put_page(virt_to_head_page(buf));
-
return err;
}
@@ -1839,7 +1868,6 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
}
static int init_vqs(struct virtnet_info *vi);
-static void _remove_vq_common(struct virtnet_info *vi);
static int virtnet_restore_up(struct virtio_device *vdev)
{
@@ -1868,39 +1896,6 @@ static int virtnet_restore_up(struct virtio_device *vdev)
return err;
}
-static int virtnet_reset(struct virtnet_info *vi, int curr_qp, int xdp_qp)
-{
- struct virtio_device *dev = vi->vdev;
- int ret;
-
- virtio_config_disable(dev);
- dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
- virtnet_freeze_down(dev);
- _remove_vq_common(vi);
-
- virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
- virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
-
- ret = virtio_finalize_features(dev);
- if (ret)
- goto err;
-
- vi->xdp_queue_pairs = xdp_qp;
- ret = virtnet_restore_up(dev);
- if (ret)
- goto err;
- ret = _virtnet_set_queues(vi, curr_qp);
- if (ret)
- goto err;
-
- virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
- virtio_config_enable(dev);
- return 0;
-err:
- virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
- return ret;
-}
-
static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
struct netlink_ext_ack *extack)
{
@@ -1947,35 +1942,29 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
return PTR_ERR(prog);
}
- /* Changing the headroom in buffers is a disruptive operation because
- * existing buffers must be flushed and reallocated. This will happen
- * when a xdp program is initially added or xdp is disabled by removing
- * the xdp program resulting in number of XDP queues changing.
- */
- if (vi->xdp_queue_pairs != xdp_qp) {
- err = virtnet_reset(vi, curr_qp + xdp_qp, xdp_qp);
- if (err) {
- dev_warn(&dev->dev, "XDP reset failure.\n");
- goto virtio_reset_err;
- }
- }
+ /* Make sure NAPI is not using any XDP TX queues for RX. */
+ for (i = 0; i < vi->max_queue_pairs; i++)
+ napi_disable(&vi->rq[i].napi);
netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
+ err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
+ if (err)
+ goto err;
+ vi->xdp_queue_pairs = xdp_qp;
for (i = 0; i < vi->max_queue_pairs; i++) {
old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
if (old_prog)
bpf_prog_put(old_prog);
+ virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
}
return 0;
-virtio_reset_err:
- /* On reset error do our best to unwind XDP changes inflight and return
- * error up to user space for resolution. The underlying reset hung on
- * us so not much we can do here.
- */
+err:
+ for (i = 0; i < vi->max_queue_pairs; i++)
+ virtnet_napi_enable(vi->rq[i].vq, &vi->rq[i].napi);
if (prog)
bpf_prog_sub(prog, vi->max_queue_pairs - 1);
return err;
@@ -2619,15 +2608,6 @@ static int virtnet_probe(struct virtio_device *vdev)
return err;
}
-static void _remove_vq_common(struct virtnet_info *vi)
-{
- vi->vdev->config->reset(vi->vdev);
- free_unused_bufs(vi);
- _free_receive_bufs(vi);
- free_receive_page_frags(vi);
- virtnet_del_vqs(vi);
-}
-
static void remove_vq_common(struct virtnet_info *vi)
{
vi->vdev->config->reset(vi->vdev);
--
2.7.4
^ permalink raw reply related
* [PATCH net-next V2 3/5] virtio-net: switch to use new ctx API for small buffer
From: Jason Wang @ 2017-07-19 8:54 UTC (permalink / raw)
To: mst, virtualization, jasowang, netdev, linux-kernel
In-Reply-To: <1500454489-16805-1-git-send-email-jasowang@redhat.com>
Use ctx API to store headroom for small buffers.
Following patches will retrieve this info and use it for XDP.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 17 ++++++++++++-----
1 file changed, 12 insertions(+), 5 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 8fae9a8..640f1de 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -410,7 +410,8 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
static struct sk_buff *receive_small(struct net_device *dev,
struct virtnet_info *vi,
struct receive_queue *rq,
- void *buf, unsigned int len)
+ void *buf, void *ctx,
+ unsigned int len)
{
struct sk_buff *skb;
struct bpf_prog *xdp_prog;
@@ -773,7 +774,7 @@ static int receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
else if (vi->big_packets)
skb = receive_big(dev, vi, rq, buf, len);
else
- skb = receive_small(dev, vi, rq, buf, len);
+ skb = receive_small(dev, vi, rq, buf, ctx, len);
if (unlikely(!skb))
return 0;
@@ -806,12 +807,18 @@ static int receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
return 0;
}
+/* Unlike mergeable buffers, all buffers are allocated to the
+ * same size, except for the headroom. For this reason we do
+ * not need to use mergeable_len_to_ctx here - it is enough
+ * to store the headroom as the context ignoring the truesize.
+ */
static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
gfp_t gfp)
{
struct page_frag *alloc_frag = &rq->alloc_frag;
char *buf;
unsigned int xdp_headroom = virtnet_get_headroom(vi);
+ void *ctx = (void *)(unsigned long)xdp_headroom;
int len = vi->hdr_len + VIRTNET_RX_PAD + GOOD_PACKET_LEN + xdp_headroom;
int err;
@@ -825,7 +832,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
alloc_frag->offset += len;
sg_init_one(rq->sg, buf + VIRTNET_RX_PAD + xdp_headroom,
vi->hdr_len + GOOD_PACKET_LEN);
- err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, buf, gfp);
+ err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
if (err < 0)
put_page(virt_to_head_page(buf));
@@ -1034,7 +1041,7 @@ static int virtnet_receive(struct receive_queue *rq, int budget)
void *buf;
struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
- if (vi->mergeable_rx_bufs) {
+ if (!vi->big_packets || vi->mergeable_rx_bufs) {
void *ctx;
while (received < budget &&
@@ -2198,7 +2205,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
names = kmalloc(total_vqs * sizeof(*names), GFP_KERNEL);
if (!names)
goto err_names;
- if (vi->mergeable_rx_bufs) {
+ if (!vi->big_packets || vi->mergeable_rx_bufs) {
ctx = kzalloc(total_vqs * sizeof(*ctx), GFP_KERNEL);
if (!ctx)
goto err_ctx;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next V2 2/5] virtio-net: pack headroom into ctx for mergeable buffers
From: Jason Wang @ 2017-07-19 8:54 UTC (permalink / raw)
To: mst, virtualization, jasowang, netdev, linux-kernel
In-Reply-To: <1500454489-16805-1-git-send-email-jasowang@redhat.com>
Pack headroom into ctx - this way when we get a buffer we can figure out
the actual headroom that was allocated for the buffer. Will be helpful
to optimize switching between XDP and non-XDP modes which have different
headroom requirements.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++-----
1 file changed, 24 insertions(+), 5 deletions(-)
diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
index 1f8c15c..8fae9a8 100644
--- a/drivers/net/virtio_net.c
+++ b/drivers/net/virtio_net.c
@@ -270,6 +270,23 @@ static void skb_xmit_done(struct virtqueue *vq)
netif_wake_subqueue(vi->dev, vq2txq(vq));
}
+#define MRG_CTX_HEADER_SHIFT 22
+static void *mergeable_len_to_ctx(unsigned int truesize,
+ unsigned int headroom)
+{
+ return (void *)(unsigned long)((headroom << MRG_CTX_HEADER_SHIFT) | truesize);
+}
+
+static unsigned int mergeable_ctx_to_headroom(void *mrg_ctx)
+{
+ return (unsigned long)mrg_ctx >> MRG_CTX_HEADER_SHIFT;
+}
+
+static unsigned int mergeable_ctx_to_truesize(void *mrg_ctx)
+{
+ return (unsigned long)mrg_ctx & ((1 << MRG_CTX_HEADER_SHIFT) - 1);
+}
+
/* Called from bottom half context */
static struct sk_buff *page_to_skb(struct virtnet_info *vi,
struct receive_queue *rq,
@@ -639,13 +656,14 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
}
rcu_read_unlock();
- if (unlikely(len > (unsigned long)ctx)) {
+ truesize = mergeable_ctx_to_truesize(ctx);
+ if (unlikely(len > truesize)) {
pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
dev->name, len, (unsigned long)ctx);
dev->stats.rx_length_errors++;
goto err_skb;
}
- truesize = (unsigned long)ctx;
+
head_skb = page_to_skb(vi, rq, page, offset, len, truesize);
curr_skb = head_skb;
@@ -665,13 +683,14 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
}
page = virt_to_head_page(buf);
- if (unlikely(len > (unsigned long)ctx)) {
+
+ truesize = mergeable_ctx_to_truesize(ctx);
+ if (unlikely(len > truesize)) {
pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
dev->name, len, (unsigned long)ctx);
dev->stats.rx_length_errors++;
goto err_skb;
}
- truesize = (unsigned long)ctx;
num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
@@ -889,7 +908,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
buf += headroom; /* advance address leaving hole at front of pkt */
- ctx = (void *)(unsigned long)len;
+ ctx = mergeable_len_to_ctx(len, headroom);
get_page(alloc_frag->page);
alloc_frag->offset += len + headroom;
hole = alloc_frag->size - alloc_frag->offset;
--
2.7.4
^ permalink raw reply related
* [PATCH net-next V2 1/5] virtio_ring: allow to store zero as the ctx
From: Jason Wang @ 2017-07-19 8:54 UTC (permalink / raw)
To: mst, virtualization, jasowang, netdev, linux-kernel
In-Reply-To: <1500454489-16805-1-git-send-email-jasowang@redhat.com>
Allow zero to be store as a ctx, with this we could store e.g zero
value which could be meaningful for the case of storing headroom
through ctx.
Signed-off-by: Jason Wang <jasowang@redhat.com>
---
drivers/virtio/virtio_ring.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/virtio/virtio_ring.c b/drivers/virtio/virtio_ring.c
index 5e1b548..9aaa177 100644
--- a/drivers/virtio/virtio_ring.c
+++ b/drivers/virtio/virtio_ring.c
@@ -391,7 +391,7 @@ static inline int virtqueue_add(struct virtqueue *_vq,
vq->desc_state[head].data = data;
if (indirect)
vq->desc_state[head].indir_desc = desc;
- if (ctx)
+ else
vq->desc_state[head].indir_desc = ctx;
/* Put entry in available array (but don't update avail->idx until they
--
2.7.4
^ permalink raw reply related
* [PATCH net-next V2 0/5] Refine virtio-net XDP
From: Jason Wang @ 2017-07-19 8:54 UTC (permalink / raw)
To: mst, virtualization, jasowang, netdev, linux-kernel
Hi:
This series brings two optimizations for virtio-net XDP:
- avoid reset during XDP set
- turn off offloads on demand
Changes from V1:
- Various tweaks on commit logs and comments
- Use virtnet_napi_enable() when enabling NAPI on XDP set
- Copy the small buffer packet only if xdp_headroom is smaller than
required
Please review.
Thanks
Jason Wang (5):
virtio_ring: allow to store zero as the ctx
virtio-net: pack headroom into ctx for mergeable buffers
virtio-net: switch to use new ctx API for small buffer
virtio-net: do not reset during XDP set
virtio-net: switch off offloads on demand if possible on XDP set
drivers/net/virtio_net.c | 332 ++++++++++++++++++++++++++-----------------
drivers/virtio/virtio_ring.c | 2 +-
2 files changed, 200 insertions(+), 134 deletions(-)
--
2.7.4
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Michal Hocko @ 2017-07-19 8:13 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, amit.shah, kvm, Michael S. Tsirkin,
linux-kernel, liliang.opensource, qemu-devel, virtualization,
linux-mm, yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm,
mgorman
In-Reply-To: <596D6E7E.4070700@intel.com>
On Tue 18-07-17 10:12:14, Wei Wang wrote:
[...]
> Probably I should have included the introduction of the usages in
> the log. Hope it is not too later to explain here:
Yes this should have been described in the cover.
> Live migration needs to transfer the VM's memory from the source
> machine to the destination round by round. For the 1st round, all the VM's
> memory is transferred. From the 2nd round, only the pieces of memory
> that were written by the guest (after the 1st round) are transferred. One
> method that is popularly used by the hypervisor to track which part of
> memory is written is to write-protect all the guest memory.
>
> This patch enables the optimization of the 1st round memory transfer -
> the hypervisor can skip the transfer of guest unused pages in the 1st round.
All you should need is the check for the page reference count, no? I
assume you do some sort of pfn walk and so you should be able to get an
access to the struct page.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* RE: [virtio-dev] packed ring layout proposal v2
From: Lior Narkis @ 2017-07-19 7:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <20170718172312-mutt-send-email-mst@kernel.org>
> -----Original Message-----
> From: Michael S. Tsirkin [mailto:mst@redhat.com]
> Sent: Tuesday, July 18, 2017 7:23 PM
> To: Lior Narkis <liorn@mellanox.com>
> Cc: virtio-dev@lists.oasis-open.org; virtualization@lists.linux-foundation.org
> Subject: Re: [virtio-dev] packed ring layout proposal v2
>
> On Sun, Jul 16, 2017 at 06:00:45AM +0000, Lior Narkis wrote:
> > > -----Original Message-----
> > > From: virtio-dev@lists.oasis-open.org [mailto:virtio-dev@lists.oasis-
> open.org]
> > > On Behalf Of Michael S. Tsirkin
> > > Sent: Wednesday, February 08, 2017 5:20 AM
> > > To: virtio-dev@lists.oasis-open.org
> > > Cc: virtualization@lists.linux-foundation.org
> > > Subject: [virtio-dev] packed ring layout proposal v2
> > >
> > > This is an update from v1 version.
> > > Changes:
> > > - update event suppression mechanism
> > > - separate options for indirect and direct s/g
> > > - lots of new features
> > >
> > > ---
> > >
> > > Performance analysis of this is in my kvm forum 2016 presentation.
> > > The idea is to have a r/w descriptor in a ring structure,
> > > replacing the used and available ring, index and descriptor
> > > buffer.
> > >
> > > * Descriptor ring:
> > >
> > > Guest adds descriptors with unique index values and DESC_HW set in flags.
> > > Host overwrites used descriptors with correct len, index, and DESC_HW
> > > clear. Flags are always set/cleared last.
> > >
> > > #define DESC_HW 0x0080
> > >
> > > struct desc {
> > > __le64 addr;
> > > __le32 len;
> > > __le16 index;
> > > __le16 flags;
> > > };
> > >
> > > When DESC_HW is set, descriptor belongs to device. When it is clear,
> > > it belongs to the driver.
> > >
> > > We can use 1 bit to set direction
> > > /* This marks a buffer as write-only (otherwise read-only). */
> > > #define VRING_DESC_F_WRITE 2
> > >
> >
> > A valid bit per descriptor does not let the device do an efficient prefetch.
> > An alternative is to use a producer index(PI).
> > Using the PI posted by the driver, and the Consumer Index(CI) maintained
> by the device, the device knows how much work it has outstanding, so it can
> do the prefetch accordingly.
> > There are few options for the device to acquire the PI.
> > Most efficient will be to write the PI in the doorbell together with the queue
> number.
>
> Right. This was suggested in "Fwd: Virtio-1.1 Ring Layout".
> Or just the PI if we don't need the queue number.
>
> > I would like to raise the need for a Completion Queue(CQ).
> > Multiple Work Queues(hold the work descriptors, WQ in short) can be
> connected to a single CQ.
> > So when the device completes the work on the descriptor, it writes a
> Completion Queue Entry (CQE) to the CQ.
>
> This seems similar to the design we currently have with a separate used
> ring. It seems to underperform writing into the available ring
> at least on a microbenchmark. The reason seems to be that
> more cache lines need to get invalidated and re-fetched.
Few points on that:
Each PCIe write will cause invalidation to a cache line.
Writing less than a cache line is inefficient, so it is better to put all metadata together and allocate a cache line for it.
Putting the metadata in the data buffer means two writes of less than a cache line each, both will be accessed by the driver, so potential two misses.
>
> > CQEs are continuous in memory so prefetching by the driver is efficient,
> although the device might complete work descriptors out of order.
>
> That's not different from proposal you are replying to - descriptors
> can be used and written out in any order, they are contigious
> so driver can prefetch.
Point is that if descriptors 1, 2, 4 are being completed in that order, in the proposed layout the completion indication will be placed at 1, 2, 4 in the virtq desc buffer.
With a CQ, completions on 1, 2, 4 will be placed at 1, 2, 3 CQ indexes.
This is why it is better for prefetching.
> I'd like to add that attempts to
> add prefetch e.g. for the virtio used ring never showed any
> conclusive gains - some workloads would get minor a speedup, others
> lose a bit of performance. I do not think anyone looked
> deeply into why that was the case. It would be easy for you to add this
> code to current virtio drivers and/or devices and try it out.
Noted.
I will say though that mlx5 uses prefetch and gets good performance because of it...
>
> > The interrupt handler is connected to the CQ, so an allocation of a single CQ
> per core, with a single interrupt handler is possible although this core might be
> using multiple WQs.
>
> Sending a single interrupt from multiple rings might save some
> cycles. event index/interrupt disable are currently in
> RAM so access is very cheap for the guest.
> If you are going to share, just disable all interrupts
> when you start processing.
>
> I was wondering how do people want to do this in hardware
> in fact. Are you going to read event index after each descriptor?
Not sure I got you here.
Do you ask about how the device decides to write MSIX? And how interrupt moderation might work?
>
> It might make sense to move event index/flags into device memory. And
> once you do this, writing these out might become slower, and then some
> kind of sharing of the event index might help.
>
> No one suggested anything like this so far - maybe it's less of an issue
> than I think, e.g. because of interrupt coalescing (which virtio also
> does not have, but could be added if there is interest) or maybe people
> just mostly care about polling performance.
>
> > One application for multiple WQs with a single CQ is Quality of Service(QoS).
> > A user can open a WQ per QoS value(pcp value for example), and the device
> will schedule the work accordingly.
>
> A ring per QOS level might make sense e.g. in a network device. I don't
> see why it's helpful to write out completed entries into a separate
> ring for that though.
I would like to add that for rdma device there are many queues (QPs), understanding which QP completed work by traversing all QPs in not efficient.
Another advantage of having a CQ connected to multiple WQs is that the interrupt moderation can be based on this single CQ,
So the criteria if to write interrupt or not is based on all the aggregated work that was completed on that CQ.
>
> As we don't have QOS support in virtio net at all right now,
> would that be best discussed once we have a working prototype
> and everyone can see what the problem is?
Understood.
Although, I think the layout should not change frequently.
>
>
> > > * Scatter/gather support
> > >
> > > We can use 1 bit to chain s/g entries in a request, same as virtio 1.0:
> > >
> > > /* This marks a buffer as continuing via the next field. */
> > > #define VRING_DESC_F_NEXT 1
> > >
> > > Unlike virtio 1.0, all descriptors must have distinct ID values.
> > >
> > > Also unlike virtio 1.0, use of this flag will be an optional feature
> > > (e.g. VIRTIO_F_DESC_NEXT) so both devices and drivers can opt out of it.
> > >
> > > * Indirect buffers
> > >
> > > Can be marked like in virtio 1.0:
> > >
> > > /* This means the buffer contains a table of buffer descriptors. */
> > > #define VRING_DESC_F_INDIRECT 4
> > >
> > > Unlike virtio 1.0, this is a table, not a list:
> > > struct indirect_descriptor_table {
> > > /* The actual descriptors (16 bytes each) */
> > > struct virtq_desc desc[len / 16];
> > > };
> > >
> > > The first descriptor is located at start of the indirect descriptor
> > > table, additional indirect descriptors come immediately afterwards.
> > > DESC_F_WRITE is the only valid flag for descriptors in the indirect
> > > table. Others should be set to 0 and are ignored. id is also set to 0
> > > and should be ignored.
> > >
> > > virtio 1.0 seems to allow a s/g entry followed by
> > > an indirect descriptor. This does not seem useful,
> > > so we do not allow that anymore.
> > >
> > > This support would be an optional feature, same as in virtio 1.0
> > >
> > > * Batching descriptors:
> > >
> > > virtio 1.0 allows passing a batch of descriptors in both directions, by
> > > incrementing the used/avail index by values > 1. We can support this by
> > > chaining a list of descriptors through a bit the flags field.
> > > To allow use together with s/g, a different bit will be used.
> > >
> > > #define VRING_DESC_F_BATCH_NEXT 0x0010
> > >
> > > Batching works for both driver and device descriptors.
> > >
> > >
> > >
> > > * Processing descriptors in and out of order
> > >
> > > Device processing all descriptors in order can simply flip
> > > the DESC_HW bit as it is done with descriptors.
> > >
> > > Device can write descriptors out in order as they are used, overwriting
> > > descriptors that are there.
> > >
> > > Device must not use a descriptor until DESC_HW is set.
> > > It is only required to look at the first descriptor
> > > submitted.
> > >
> > > Driver must not overwrite a descriptor until DESC_HW is clear.
> > > It is only required to look at the first descriptor
> > > submitted.
> > >
> > > * Device specific descriptor flags
> > > We have a lot of unused space in the descriptor. This can be put to
> > > good use by reserving some flag bits for device use.
> > > For example, network device can set a bit to request
> > > that header in the descriptor is suppressed
> > > (in case it's all 0s anyway). This reduces cache utilization.
> > >
> > > Note: this feature can be supported in virtio 1.0 as well,
> > > as we have unused bits in both descriptor and used ring there.
> > >
> > > * Descriptor length in device descriptors
> > >
> > > virtio 1.0 places strict requirements on descriptor length. For example
> > > it must be 0 in used ring of TX VQ of a network device since nothing is
> > > written. In practice guests do not seem to use this, so we can simplify
> > > devices a bit by removing this requirement - if length is unused it
> > > should be ignored by driver.
> > >
> > > Some devices use identically-sized buffers in all descriptors.
> > > Ignoring length for driver descriptors there could be an option too.
> > >
> > > * Writing at an offset
> > >
> > > Some devices might want to write into some descriptors
> > > at an offset, the offset would be in config space,
> > > and a descriptor flag could indicate this:
> > >
> > > #define VRING_DESC_F_OFFSET 0x0020
> > >
> > > How exactly to use the offset would be device specific,
> > > for example it can be used to align beginning of packet
> > > in the 1st buffer for mergeable buffers to cache line boundary
> > > while also aligning rest of buffers.
> > >
> > > * Non power-of-2 ring sizes
> > >
> > > As the ring simply wraps around, there's no reason to
> > > require ring size to be power of two.
> > > It can be made a separate feature though.
> > >
> > >
> > > * Interrupt/event suppression
> > >
> > > virtio 1.0 has two mechanisms for suppression but only
> > > one can be used at a time. we pack them together
> > > in a structure - one for interrupts, one for notifications:
> > >
> > > struct event {
> > > __le16 idx;
> > > __le16 flags;
> > > }
> > >
> > > Both fields would be optional, with a feature bit:
> > > VIRTIO_F_EVENT_IDX
> > > VIRTIO_F_EVENT_FLAGS
> > >
> > > * Flags can be used like in virtio 1.0, by storing a special
> > > value there:
> > >
> > > #define VRING_F_EVENT_ENABLE 0x0
> > >
> > > #define VRING_F_EVENT_DISABLE 0x1
> > >
> > > * Event index would be in the range 0 to 2 * Queue Size
> > > (to detect wrap arounds) and wrap to 0 after that.
> > >
> > > The assumption is that each side maintains an internal
> > > descriptor counter 0 to 2 * Queue Size that wraps to 0.
> > > In that case, interrupt triggers when counter reaches
> > > the given value.
> > >
> > > * If both features are negotiated, a special flags value
> > > can be used to switch to event idx:
> > >
> > > #define VRING_F_EVENT_IDX 0x2
> > >
> > >
> > > * Prototype
> > >
> > > A partial prototype can be found under
> > > tools/virtio/ringtest/ring.c
> > >
> > > Test run:
> > > [mst@tuck ringtest]$ time ./ring
> > > real 0m0.399s
> > > user 0m0.791s
> > > sys 0m0.000s
> > > [mst@tuck ringtest]$ time ./virtio_ring_0_9
> > > real 0m0.503s
> > > user 0m0.999s
> > > sys 0m0.000s
> > >
> > > It is planned to update it to this interface. Future changes and
> > > enhancements can (and should) be tested against this prototype.
> > >
> > > * Feature sets
> > > In particular are we going overboard with feature bits? It becomes hard
> > > to support all combinations in drivers and devices. Maybe we should
> > > document reasonable feature sets to be supported for each device.
> > >
> > > * Known issues/ideas
> > >
> > > This layout is optimized for host/guest communication,
> > > in a sense even more aggressively than virtio 1.0.
> > > It might be suboptimal for PCI hardware implementations.
> > > However, one notes that current virtio pci drivers are
> > > unlikely to work with PCI hardware implementations anyway
> > > (e.g. due to use of SMP barriers for ordering).
> > >
> > > Suggestions for improving this are welcome but need to be tested to make
> > > sure our main use case does not regress. Of course some improvements
> > > might be made optional, but if we add too many of these driver becomes
> > > unmanageable.
> > >
> > > ---
> > >
> > > Note: should this proposal be accepted and approved, one or more
> > > claims disclosed to the TC admin and listed on the Virtio TC
> > > IPR page
> > >
> https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fww
> > > w.oasis-
> > >
> open.org%2Fcommittees%2Fvirtio%2Fipr.php&data=02%7C01%7Cliorn%40m
> > >
> ellanox.com%7Cf41239019c1441e73b0308d4c7b0a483%7Ca652971c7d2e4d9
> > >
> ba6a4d149256f461b%7C0%7C0%7C636353008872143792&sdata=L946V5o0P
> > > k8th%2B2IkHgvALmhnIEWD9hcMZvMxLetavc%3D&reserved=0
> > > might become Essential Claims.
> > >
> > > --
> > > MST
> > >
> > > ---------------------------------------------------------------------
> > > To unsubscribe, e-mail: virtio-dev-unsubscribe@lists.oasis-open.org
> > > For additional commands, e-mail: virtio-dev-help@lists.oasis-open.org
^ permalink raw reply
* Re: [PATCH 3/4] drm/qxl: Drop fbdev hwaccel flags
From: Daniel Vetter @ 2017-07-19 7:39 UTC (permalink / raw)
To: DRI Development
Cc: linux-fbdev, Daniel Vetter, Intel Graphics Development,
virtualization, Daniel Vetter, Dave Airlie
In-Reply-To: <20170706125735.28299-4-daniel.vetter@ffwll.ch>
On Thu, Jul 06, 2017 at 02:57:34PM +0200, Daniel Vetter wrote:
> It's not accelarated, just system memory. Note we don't even need to
> set the default flag since that's now always 0.
>
> Cc: Dave Airlie <airlied@redhat.com>
> Cc: Gerd Hoffmann <kraxel@redhat.com>
> Cc: virtualization@lists.linux-foundation.org
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
Merged with Dave's irc-ack + commit message amended that qxl once had
accel, but that was removed in 2015.
-Daniel
> ---
> drivers/gpu/drm/qxl/qxl_fb.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c
> index 573e7e9a5f98..69e7359b562a 100644
> --- a/drivers/gpu/drm/qxl/qxl_fb.c
> +++ b/drivers/gpu/drm/qxl/qxl_fb.c
> @@ -275,7 +275,7 @@ static int qxlfb_create(struct qxl_fbdev *qfbdev,
>
> drm_fb_helper_fill_fix(info, fb->pitches[0], fb->format->depth);
>
> - info->flags = FBINFO_DEFAULT | FBINFO_HWACCEL_COPYAREA | FBINFO_HWACCEL_FILLRECT;
> + info->flags = FBINFO_DEFAULT;
> info->fbops = &qxlfb_ops;
>
> /*
> --
> 2.13.2
>
--
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
^ permalink raw reply
* Re: [PATCH net-next 0/5] refine virtio-net XDP
From: Jason Wang @ 2017-07-19 2:40 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170718230748-mutt-send-email-mst@kernel.org>
On 2017年07月19日 04:13, Michael S. Tsirkin wrote:
> On Mon, Jul 17, 2017 at 08:43:56PM +0800, Jason Wang wrote:
>> Hi:
>>
>> This series brings two optimizations for virtio-net XDP:
>>
>> - avoid reset during XDP set
>> - turn off offloads on demand
> I'm glad to see this take shape - this can be
> extended to optimize virtnet_get_headroom so we don't
> waste room if adjust_head is enabled.
Right, we can do it on top.
> I see a couple of issues, responded to individual patches.
>
>
Thanks for the reviewing.
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next 5/5] virtio-net: switch off offloads on demand if possible on XDP set
From: Jason Wang @ 2017-07-19 2:39 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170718225011-mutt-send-email-mst@kernel.org>
On 2017年07月19日 04:07, Michael S. Tsirkin wrote:
> On Mon, Jul 17, 2017 at 08:44:01PM +0800, Jason Wang wrote:
>> Current XDP implementation want guest offloads feature to be disabled
> s/want/wants/
>
>> on qemu cli.
> on the device.
>
>> This is inconvenient and means guest can't benefit from
>> offloads if XDP is not used. This patch tries to address this
>> limitation by disable
> disabling
>
>> the offloads on demand through control guest
>> offloads. Guest offloads will be disabled and enabled on demand on XDP
>> set.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
> In fact, since we no longer reset when XDP is set,
> here device might have offloads enabled, buffers are
> used but not consumed, then XDP is set.
>
> This can result in
> - packet scattered across multiple buffers
> (handled correctly but need to update the comment)
Ok.
> - packet may have VIRTIO_NET_HDR_F_NEEDS_CSUM, in that case
> the spec says "The checksum on the packet is incomplete".
> (probably needs to be handled by calculating the checksum).
That's an option. Maybe it's tricky but I was thinking whether or not we
can just keep the CHECKSUM_PARTIAL here.
>
>
> Ideas for follow-up patches:
>
> - skip looking at packet data completely
> won't work if you play with checksums dynamically
> but can be done if disabled on device
> - allow ethtools to tweak offloads from userspace as well
Right.
Thanks
>
>> ---
>> drivers/net/virtio_net.c | 70 ++++++++++++++++++++++++++++++++++++++++++++----
>> 1 file changed, 65 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index e732bd6..d970c2d 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -57,6 +57,11 @@ DECLARE_EWMA(pkt_len, 0, 64)
>>
>> #define VIRTNET_DRIVER_VERSION "1.0.0"
>>
>> +const unsigned long guest_offloads[] = { VIRTIO_NET_F_GUEST_TSO4,
>> + VIRTIO_NET_F_GUEST_TSO6,
>> + VIRTIO_NET_F_GUEST_ECN,
>> + VIRTIO_NET_F_GUEST_UFO };
>> +
>> struct virtnet_stats {
>> struct u64_stats_sync tx_syncp;
>> struct u64_stats_sync rx_syncp;
>> @@ -164,10 +169,13 @@ struct virtnet_info {
>> u8 ctrl_promisc;
>> u8 ctrl_allmulti;
>> u16 ctrl_vid;
>> + u64 ctrl_offloads;
>>
>> /* Ethtool settings */
>> u8 duplex;
>> u32 speed;
>> +
>> + unsigned long guest_offloads;
>> };
>>
>> struct padded_vnet_hdr {
>> @@ -1889,6 +1897,47 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>> return err;
>> }
>>
>> +static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
>> +{
>> + struct scatterlist sg;
>> + vi->ctrl_offloads = cpu_to_virtio64(vi->vdev, offloads);
>> +
>> + sg_init_one(&sg, &vi->ctrl_offloads, sizeof(vi->ctrl_offloads));
>> +
>> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
>> + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) {
>> + dev_warn(&vi->dev->dev, "Fail to set guest offload. \n");
>> + return -EINVAL;
>> + }
>> +
>> + return 0;
>> +}
>> +
>> +static int virtnet_clear_guest_offloads(struct virtnet_info *vi)
>> +{
>> + u64 offloads = 0;
>> +
>> + if (!vi->guest_offloads)
>> + return 0;
>> +
>> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
>> + offloads = 1ULL << VIRTIO_NET_F_GUEST_CSUM;
>> +
>> + return virtnet_set_guest_offloads(vi, offloads);
>> +}
>> +
>> +static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
>> +{
>> + u64 offloads = vi->guest_offloads;
>> +
>> + if (!vi->guest_offloads)
>> + return 0;
>> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
>> + offloads |= 1ULL << VIRTIO_NET_F_GUEST_CSUM;
>> +
>> + return virtnet_set_guest_offloads(vi, offloads);
>> +}
>> +
>> static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> struct netlink_ext_ack *extack)
>> {
>> @@ -1898,10 +1947,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> u16 xdp_qp = 0, curr_qp;
>> int i, err;
>>
>> - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
>> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
>> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO)) {
>> + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)
>> + && (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
>> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO))) {
>> NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing LRO, disable LRO first");
>> return -EOPNOTSUPP;
>> }
>> @@ -1950,6 +2000,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> for (i = 0; i < vi->max_queue_pairs; i++) {
>> old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>> rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>> + if (i == 0) {
>> + if (!old_prog)
>> + virtnet_clear_guest_offloads(vi);
>> + if (!prog)
>> + virtnet_restore_guest_offloads(vi);
>> + }
>> if (old_prog)
>> bpf_prog_put(old_prog);
>> napi_enable(&vi->rq[i].napi);
>> @@ -2583,6 +2639,10 @@ static int virtnet_probe(struct virtio_device *vdev)
>> netif_carrier_on(dev);
>> }
>>
>> + for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
>> + if (virtio_has_feature(vi->vdev, guest_offloads[i]))
>> + set_bit(guest_offloads[i], &vi->guest_offloads);
>> +
>> pr_debug("virtnet: registered device %s with %d RX and TX vq's\n",
>> dev->name, max_queue_pairs);
>>
>> @@ -2679,7 +2739,7 @@ static struct virtio_device_id id_table[] = {
>> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
>> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
>> VIRTIO_NET_F_CTRL_MAC_ADDR, \
>> - VIRTIO_NET_F_MTU
>> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>>
>> static unsigned int features[] = {
>> VIRTNET_FEATURES,
>> --
>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next 4/5] virtio-net: do not reset during XDP set
From: Jason Wang @ 2017-07-19 2:35 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170718222042-mutt-send-email-mst@kernel.org>
On 2017年07月19日 03:49, Michael S. Tsirkin wrote:
> On Mon, Jul 17, 2017 at 08:44:00PM +0800, Jason Wang wrote:
>> We used to reset during XDP set, the main reason is we need allocate
>> extra headroom for header adjustment but there's no way to know the
>> headroom of exist receive buffer. This works buy maybe complex and may
>> cause the network down for a while which is bad for user
>> experience. So this patch tries to avoid this by:
>>
>> - packing headroom into receive buffer ctx
>> - check the headroom during XDP, and if it was not sufficient, copy
>> the packet into a location which has a large enough headroom
> The packing is actually done by previous patches. Here is a
> corrected version:
>
> We currently reset the device during XDP set, the main reason is
> that we allocate more headroom with XDP (for header adjustment).
>
> This works but causes network downtime for users.
>
> Previous patches encoded the headroom in the buffer context,
> this makes it possible to detect the case where a buffer
> with headroom insufficient for XDP is added to the queue and
> XDP is enabled afterwards.
>
> Upon detection, we handle this case by copying the packet
> (slow, but it's a temporary condition).
Ok.
>
>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> drivers/net/virtio_net.c | 230 ++++++++++++++++++++++-------------------------
>> 1 file changed, 105 insertions(+), 125 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index e31b5b2..e732bd6 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -407,6 +407,67 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
>> return vi->xdp_queue_pairs ? VIRTIO_XDP_HEADROOM : 0;
>> }
>>
>> +/* We copy and linearize packet in the following cases:
>> + *
>> + * 1) Packet across multiple buffers, this happens normally when rx
>> + * buffer size is underestimated. Rarely, since spec does not
>> + * forbid using more than one buffer even if a single buffer is
>> + * sufficient for the packet, we should also deal with this case.
> Latest SVN of the spec actually forbids this. See:
> net: clarify device rules for mergeable buffers
Good to know this.
>
>
>> + * 2) The header room is smaller than what XDP required. In this case
>> + * we should copy the packet and reserve enough headroom for this.
>> + * This would be slow but we at most we can copy times of queue
>> + * size, this is acceptable. What's more important, this help to
>> + * avoid resetting.
> Last part of the comment applies to both cases. So
>
> +/* We copy the packet for XDP in the following cases:
> + *
> + * 1) Packet is scattered across multiple rx buffers.
> + * 2) Headroom space is insufficient.
> + *
> + * This is inefficient but it's a temporary condition that
> + * we hit right after XDP is enabled and until queue is refilled
> + * with large buffers with sufficient headroom - so it should affect
> + * at most queue size packets.
>
> + * Afterwards, the conditions to enable
> + * XDP should preclude the underlying device from sending packets
> + * across multiple buffers (num_buf > 1), and we make sure buffers
> + * have enough headroom.
> + */
>
Ok.
>
>> + * 2) The header room is smaller than what XDP required. In this case
>> + * we should copy the packet and reserve enough headroom for this.
>> + * This would be slow but we at most we can copy times of queue
>> + * size, this is acceptable. What's more important, this help to
>> + * avoid resetting.
>
>
>> + */
>> +static struct page *xdp_linearize_page(struct receive_queue *rq,
>> + u16 *num_buf,
>> + struct page *p,
>> + int offset,
>> + int page_off,
>> + unsigned int *len)
>> +{
>> + struct page *page = alloc_page(GFP_ATOMIC);
>> +
>> + if (!page)
>> + return NULL;
>> +
>> + memcpy(page_address(page) + page_off, page_address(p) + offset, *len);
>> + page_off += *len;
>> +
>> + while (--*num_buf) {
>> + unsigned int buflen;
>> + void *buf;
>> + int off;
>> +
>> + buf = virtqueue_get_buf(rq->vq, &buflen);
>> + if (unlikely(!buf))
>> + goto err_buf;
>> +
>> + p = virt_to_head_page(buf);
>> + off = buf - page_address(p);
>> +
>> + /* guard against a misconfigured or uncooperative backend that
>> + * is sending packet larger than the MTU.
>> + */
>> + if ((page_off + buflen) > PAGE_SIZE) {
>> + put_page(p);
>> + goto err_buf;
>> + }
>> +
>> + memcpy(page_address(page) + page_off,
>> + page_address(p) + off, buflen);
>> + page_off += buflen;
>> + put_page(p);
>> + }
>> +
>> + /* Headroom does not contribute to packet length */
>> + *len = page_off - VIRTIO_XDP_HEADROOM;
>> + return page;
>> +err_buf:
>> + __free_pages(page, 0);
>> + return NULL;
>> +}
>> +
>> static struct sk_buff *receive_small(struct net_device *dev,
>> struct virtnet_info *vi,
>> struct receive_queue *rq,
>> @@ -415,12 +476,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> {
>> struct sk_buff *skb;
>> struct bpf_prog *xdp_prog;
>> - unsigned int xdp_headroom = virtnet_get_headroom(vi);
>> + unsigned int xdp_headroom = (unsigned long)ctx;
>> unsigned int header_offset = VIRTNET_RX_PAD + xdp_headroom;
>> unsigned int headroom = vi->hdr_len + header_offset;
>> unsigned int buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
>> SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>> + struct page *page = virt_to_head_page(buf);
>> unsigned int delta = 0;
>> + struct page *xdp_page;
>> len -= vi->hdr_len;
>>
>> rcu_read_lock();
>> @@ -434,6 +497,27 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
>> goto err_xdp;
>>
>> + if (unlikely(xdp_headroom != virtnet_get_headroom(vi))) {
> Should this be xdp_headroom < virtnet_get_headroom(vi)?
> Just in case we add more modes down the road.
Yes, this looks better.
>
>
>> + int offset = buf - page_address(page) + header_offset;
>> + unsigned int tlen = len + vi->hdr_len;
>> + u16 num_buf = 1;
>> +
>> + xdp_headroom = virtnet_get_headroom(vi);
>> + header_offset = VIRTNET_RX_PAD + xdp_headroom;
>> + headroom = vi->hdr_len + header_offset;
>> + buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
>> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
>> + xdp_page = xdp_linearize_page(rq, &num_buf, page,
>> + offset, header_offset,
>> + &tlen);
>> + if (!xdp_page)
>> + goto err_xdp;
>> +
>> + buf = page_address(xdp_page);
>> + put_page(page);
>> + page = xdp_page;
>> + }
>> +
>> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
>> xdp.data = xdp.data_hard_start + xdp_headroom;
>> xdp.data_end = xdp.data + len;
>> @@ -462,7 +546,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>>
>> skb = build_skb(buf, buflen);
>> if (!skb) {
>> - put_page(virt_to_head_page(buf));
>> + put_page(page);
>> goto err;
>> }
>> skb_reserve(skb, headroom - delta);
>> @@ -478,7 +562,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>> err_xdp:
>> rcu_read_unlock();
>> dev->stats.rx_dropped++;
>> - put_page(virt_to_head_page(buf));
>> + put_page(page);
>> xdp_xmit:
>> return NULL;
>> }
>> @@ -503,66 +587,6 @@ static struct sk_buff *receive_big(struct net_device *dev,
>> return NULL;
>> }
>>
>> -/* The conditions to enable XDP should preclude the underlying device from
>> - * sending packets across multiple buffers (num_buf > 1). However per spec
>> - * it does not appear to be illegal to do so but rather just against convention.
>> - * So in order to avoid making a system unresponsive the packets are pushed
>> - * into a page and the XDP program is run. This will be extremely slow and we
>> - * push a warning to the user to fix this as soon as possible. Fixing this may
>> - * require resolving the underlying hardware to determine why multiple buffers
>> - * are being received or simply loading the XDP program in the ingress stack
>> - * after the skb is built because there is no advantage to running it here
>> - * anymore.
>> - */
>> -static struct page *xdp_linearize_page(struct receive_queue *rq,
>> - u16 *num_buf,
>> - struct page *p,
>> - int offset,
>> - unsigned int *len)
>> -{
>> - struct page *page = alloc_page(GFP_ATOMIC);
>> - unsigned int page_off = VIRTIO_XDP_HEADROOM;
>> -
>> - if (!page)
>> - return NULL;
>> -
>> - memcpy(page_address(page) + page_off, page_address(p) + offset, *len);
>> - page_off += *len;
>> -
>> - while (--*num_buf) {
>> - unsigned int buflen;
>> - void *buf;
>> - int off;
>> -
>> - buf = virtqueue_get_buf(rq->vq, &buflen);
>> - if (unlikely(!buf))
>> - goto err_buf;
>> -
>> - p = virt_to_head_page(buf);
>> - off = buf - page_address(p);
>> -
>> - /* guard against a misconfigured or uncooperative backend that
>> - * is sending packet larger than the MTU.
>> - */
>> - if ((page_off + buflen) > PAGE_SIZE) {
>> - put_page(p);
>> - goto err_buf;
>> - }
>> -
>> - memcpy(page_address(page) + page_off,
>> - page_address(p) + off, buflen);
>> - page_off += buflen;
>> - put_page(p);
>> - }
>> -
>> - /* Headroom does not contribute to packet length */
>> - *len = page_off - VIRTIO_XDP_HEADROOM;
>> - return page;
>> -err_buf:
>> - __free_pages(page, 0);
>> - return NULL;
>> -}
>> -
>> static struct sk_buff *receive_mergeable(struct net_device *dev,
>> struct virtnet_info *vi,
>> struct receive_queue *rq,
>> @@ -577,6 +601,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> struct sk_buff *head_skb, *curr_skb;
>> struct bpf_prog *xdp_prog;
>> unsigned int truesize;
>> + unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>>
>> head_skb = NULL;
>>
>> @@ -589,10 +614,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> u32 act;
>>
>> /* This happens when rx buffer size is underestimated */
>> - if (unlikely(num_buf > 1)) {
>> + if (unlikely(num_buf > 1 ||
>> + headroom < virtnet_get_headroom(vi))) {
>> /* linearize data for XDP */
>> xdp_page = xdp_linearize_page(rq, &num_buf,
>> - page, offset, &len);
>> + page, offset,
>> + VIRTIO_XDP_HEADROOM,
>> + &len);
>> if (!xdp_page)
>> goto err_xdp;
>> offset = VIRTIO_XDP_HEADROOM;
>> @@ -830,7 +858,6 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
>> err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
>> if (err < 0)
>> put_page(virt_to_head_page(buf));
>> -
>> return err;
>> }
>>
>> @@ -1834,7 +1861,6 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
>> }
>>
>> static int init_vqs(struct virtnet_info *vi);
>> -static void _remove_vq_common(struct virtnet_info *vi);
>>
>> static int virtnet_restore_up(struct virtio_device *vdev)
>> {
>> @@ -1863,39 +1889,6 @@ static int virtnet_restore_up(struct virtio_device *vdev)
>> return err;
>> }
>>
>> -static int virtnet_reset(struct virtnet_info *vi, int curr_qp, int xdp_qp)
>> -{
>> - struct virtio_device *dev = vi->vdev;
>> - int ret;
>> -
>> - virtio_config_disable(dev);
>> - dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
>> - virtnet_freeze_down(dev);
>> - _remove_vq_common(vi);
>> -
>> - virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
>> - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
>> -
>> - ret = virtio_finalize_features(dev);
>> - if (ret)
>> - goto err;
>> -
>> - vi->xdp_queue_pairs = xdp_qp;
>> - ret = virtnet_restore_up(dev);
>> - if (ret)
>> - goto err;
>> - ret = _virtnet_set_queues(vi, curr_qp);
>> - if (ret)
>> - goto err;
>> -
>> - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
>> - virtio_config_enable(dev);
>> - return 0;
>> -err:
>> - virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
>> - return ret;
>> -}
>> -
>> static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> struct netlink_ext_ack *extack)
>> {
>> @@ -1942,35 +1935,31 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
>> return PTR_ERR(prog);
>> }
>>
>> - /* Changing the headroom in buffers is a disruptive operation because
>> - * existing buffers must be flushed and reallocated. This will happen
>> - * when a xdp program is initially added or xdp is disabled by removing
>> - * the xdp program resulting in number of XDP queues changing.
>> + /* synchronize with NAPI which may do XDP_TX based on queue
>> + * pair numbers.
> I think you mean
>
> /* Make sure NAPI is not using any XDP TX queues for RX. */
>
> is that it?
Yes.
>
>> - if (vi->xdp_queue_pairs != xdp_qp) {
>> - err = virtnet_reset(vi, curr_qp + xdp_qp, xdp_qp);
>> - if (err) {
>> - dev_warn(&dev->dev, "XDP reset failure.\n");
>> - goto virtio_reset_err;
>> - }
>> - }
>> + for (i = 0; i < vi->max_queue_pairs; i++)
>> + napi_disable(&vi->rq[i].napi);
>>
> This is pretty slow if queues are busy. Should we avoid this for queues
> which aren't effected?
The problem is we attach xdp prog to all RX queues.
>
>> netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
>> + err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
>> + if (err)
>> + goto err;
>> + vi->xdp_queue_pairs = xdp_qp;
>>
>> for (i = 0; i < vi->max_queue_pairs; i++) {
>> old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
>> rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
>> if (old_prog)
>> bpf_prog_put(old_prog);
>> + napi_enable(&vi->rq[i].napi);
> This seems racy. See comment around virtnet_napi_enable.
Right, will use virtnet_napi_enable() instead.
Thanks
>
>> }
>>
>> return 0;
>>
>> -virtio_reset_err:
>> - /* On reset error do our best to unwind XDP changes inflight and return
>> - * error up to user space for resolution. The underlying reset hung on
>> - * us so not much we can do here.
>> - */
>> +err:
>> + for (i = 0; i < vi->max_queue_pairs; i++)
>> + napi_enable(&vi->rq[i].napi);
>> if (prog)
>> bpf_prog_sub(prog, vi->max_queue_pairs - 1);
>> return err;
>> @@ -2614,15 +2603,6 @@ static int virtnet_probe(struct virtio_device *vdev)
>> return err;
>> }
>>
>> -static void _remove_vq_common(struct virtnet_info *vi)
>> -{
>> - vi->vdev->config->reset(vi->vdev);
>> - free_unused_bufs(vi);
>> - _free_receive_bufs(vi);
>> - free_receive_page_frags(vi);
>> - virtnet_del_vqs(vi);
>> -}
>> -
>> static void remove_vq_common(struct virtnet_info *vi)
>> {
>> vi->vdev->config->reset(vi->vdev);
>> --
>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next 3/5] virtio-net: switch to use new ctx API for small buffer
From: Jason Wang @ 2017-07-19 2:30 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170718220030-mutt-send-email-mst@kernel.org>
On 2017年07月19日 03:20, Michael S. Tsirkin wrote:
> what's needed is ability to store the headroom there.
>
> virtio-net: switch to use ctx API for small buffers
>
> Use ctx API to store headroom for small buffers.
> Following patches will retrieve this info and use it for XDP.
>
> On Mon, Jul 17, 2017 at 08:43:59PM +0800, Jason Wang wrote:
>> Switch to use ctx API for small buffer, this is need for avoiding
>> reset on XDP.
>>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> drivers/net/virtio_net.c | 12 +++++++-----
>> 1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 8fae9a8..e31b5b2 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -410,7 +410,8 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
>> static struct sk_buff *receive_small(struct net_device *dev,
>> struct virtnet_info *vi,
>> struct receive_queue *rq,
>> - void *buf, unsigned int len)
>> + void *buf, void *ctx,
>> + unsigned int len)
>> {
>> struct sk_buff *skb;
>> struct bpf_prog *xdp_prog;
>> @@ -773,7 +774,7 @@ static int receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
>> else if (vi->big_packets)
>> skb = receive_big(dev, vi, rq, buf, len);
>> else
>> - skb = receive_small(dev, vi, rq, buf, len);
>> + skb = receive_small(dev, vi, rq, buf, ctx, len);
>>
>> if (unlikely(!skb))
>> return 0;
>> @@ -812,6 +813,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
> Let's document that ctx API is used a bit differently here:
>
> /* Unlike mergeable buffers, all buffers are allocated to the same size,
> * except for the headroom. For this reason we do not need to use
> * mergeable_len_to_ctx here - it is enough to store the headroom as the
> * context ignoring the truesize.
> */
Ok.
Thanks
> as an alternative, reuse the same format as mergeable buffers.
>
>> struct page_frag *alloc_frag = &rq->alloc_frag;
>> char *buf;
>> unsigned int xdp_headroom = virtnet_get_headroom(vi);
>> + void *ctx = (void *)(unsigned long)xdp_headroom;
>> int len = vi->hdr_len + VIRTNET_RX_PAD + GOOD_PACKET_LEN + xdp_headroom;
>> int err;
>>
>> @@ -825,7 +827,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
>> alloc_frag->offset += len;
>> sg_init_one(rq->sg, buf + VIRTNET_RX_PAD + xdp_headroom,
>> vi->hdr_len + GOOD_PACKET_LEN);
>> - err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, buf, gfp);
>> + err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
>> if (err < 0)
>> put_page(virt_to_head_page(buf));
>>
>> @@ -1034,7 +1036,7 @@ static int virtnet_receive(struct receive_queue *rq, int budget)
>> void *buf;
>> struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
>>
>> - if (vi->mergeable_rx_bufs) {
>> + if (!vi->big_packets || vi->mergeable_rx_bufs) {
>> void *ctx;
>>
>> while (received < budget &&
>> @@ -2198,7 +2200,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
>> names = kmalloc(total_vqs * sizeof(*names), GFP_KERNEL);
>> if (!names)
>> goto err_names;
>> - if (vi->mergeable_rx_bufs) {
>> + if (!vi->big_packets || vi->mergeable_rx_bufs) {
>> ctx = kzalloc(total_vqs * sizeof(*ctx), GFP_KERNEL);
>> if (!ctx)
>> goto err_ctx;
>> --
>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next 2/5] virtio-net: pack headroom into ctx for mergeable buffer
From: Jason Wang @ 2017-07-19 2:29 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170718215432-mutt-send-email-mst@kernel.org>
On 2017年07月19日 02:59, Michael S. Tsirkin wrote:
> On Mon, Jul 17, 2017 at 08:43:58PM +0800, Jason Wang wrote:
>> Pack headroom into ctx, then during XDP set, we could know the size of
>> headroom and copy if needed. This is required for avoiding reset on
>> XDP.
> Not really when XDP is set - it's when buffers are used.
Of course :)
>
> virtio-net: pack headroom into ctx for mergeable buffers
>
> Pack headroom into ctx - this way when we get a buffer we can figure out
> the actual headroom that was allocated for the buffer. Will be helpful
> to optimize switching between XDP and non-XDP modes which have different
> headroom requirements.
Thanks, let me use this as the commit log.
>
>> Signed-off-by: Jason Wang <jasowang@redhat.com>
>> ---
>> drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++-----
>> 1 file changed, 24 insertions(+), 5 deletions(-)
>>
>> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
>> index 1f8c15c..8fae9a8 100644
>> --- a/drivers/net/virtio_net.c
>> +++ b/drivers/net/virtio_net.c
>> @@ -270,6 +270,23 @@ static void skb_xmit_done(struct virtqueue *vq)
>> netif_wake_subqueue(vi->dev, vq2txq(vq));
>> }
>>
>> +#define MRG_CTX_HEADER_SHIFT 22
>> +static void *mergeable_len_to_ctx(unsigned int truesize,
>> + unsigned int headroom)
>> +{
>> + return (void *)(unsigned long)((headroom << MRG_CTX_HEADER_SHIFT) | truesize);
>> +}
>> +
>> +static unsigned int mergeable_ctx_to_headroom(void *mrg_ctx)
>> +{
>> + return (unsigned long)mrg_ctx >> MRG_CTX_HEADER_SHIFT;
>> +}
>> +
>> +static unsigned int mergeable_ctx_to_truesize(void *mrg_ctx)
>> +{
>> + return (unsigned long)mrg_ctx & ((1 << MRG_CTX_HEADER_SHIFT) - 1);
>> +}
>> +
>> /* Called from bottom half context */
>> static struct sk_buff *page_to_skb(struct virtnet_info *vi,
>> struct receive_queue *rq,
>> @@ -639,13 +656,14 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> }
>> rcu_read_unlock();
>>
>> - if (unlikely(len > (unsigned long)ctx)) {
>> + truesize = mergeable_ctx_to_truesize(ctx);
>> + if (unlikely(len > truesize)) {
>> pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
>> dev->name, len, (unsigned long)ctx);
>> dev->stats.rx_length_errors++;
>> goto err_skb;
>> }
>> - truesize = (unsigned long)ctx;
>> +
>> head_skb = page_to_skb(vi, rq, page, offset, len, truesize);
>> curr_skb = head_skb;
>>
>> @@ -665,13 +683,14 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
>> }
>>
>> page = virt_to_head_page(buf);
>> - if (unlikely(len > (unsigned long)ctx)) {
>> +
>> + truesize = mergeable_ctx_to_truesize(ctx);
>> + if (unlikely(len > truesize)) {
>> pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
>> dev->name, len, (unsigned long)ctx);
>> dev->stats.rx_length_errors++;
>> goto err_skb;
>> }
>> - truesize = (unsigned long)ctx;
>>
>> num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
>> if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
>> @@ -889,7 +908,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
>>
>> buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
>> buf += headroom; /* advance address leaving hole at front of pkt */
>> - ctx = (void *)(unsigned long)len;
>> + ctx = mergeable_len_to_ctx(len, headroom);
>> get_page(alloc_frag->page);
>> alloc_frag->offset += len + headroom;
>> hole = alloc_frag->size - alloc_frag->offset;
>> --
>> 2.7.4
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net-next 0/5] refine virtio-net XDP
From: Michael S. Tsirkin @ 2017-07-18 20:13 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1500295441-19339-1-git-send-email-jasowang@redhat.com>
On Mon, Jul 17, 2017 at 08:43:56PM +0800, Jason Wang wrote:
> Hi:
>
> This series brings two optimizations for virtio-net XDP:
>
> - avoid reset during XDP set
> - turn off offloads on demand
I'm glad to see this take shape - this can be
extended to optimize virtnet_get_headroom so we don't
waste room if adjust_head is enabled.
I see a couple of issues, responded to individual patches.
> Please review.
>
> Thanks
>
> Jason Wang (5):
> virtio_ring: allow to store zero as the ctx
> virtio-net: pack headroom into ctx for mergeable buffer
> virtio-net: switch to use new ctx API for small buffer
> virtio-net: do not reset during XDP set
> virtio-net: switch off offloads on demand if possible on XDP set
>
> drivers/net/virtio_net.c | 325 +++++++++++++++++++++++++------------------
> drivers/virtio/virtio_ring.c | 2 +-
> 2 files changed, 194 insertions(+), 133 deletions(-)
>
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next 5/5] virtio-net: switch off offloads on demand if possible on XDP set
From: Michael S. Tsirkin @ 2017-07-18 20:07 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1500295441-19339-6-git-send-email-jasowang@redhat.com>
On Mon, Jul 17, 2017 at 08:44:01PM +0800, Jason Wang wrote:
> Current XDP implementation want guest offloads feature to be disabled
s/want/wants/
> on qemu cli.
on the device.
> This is inconvenient and means guest can't benefit from
> offloads if XDP is not used. This patch tries to address this
> limitation by disable
disabling
> the offloads on demand through control guest
> offloads. Guest offloads will be disabled and enabled on demand on XDP
> set.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
In fact, since we no longer reset when XDP is set,
here device might have offloads enabled, buffers are
used but not consumed, then XDP is set.
This can result in
- packet scattered across multiple buffers
(handled correctly but need to update the comment)
- packet may have VIRTIO_NET_HDR_F_NEEDS_CSUM, in that case
the spec says "The checksum on the packet is incomplete".
(probably needs to be handled by calculating the checksum).
Ideas for follow-up patches:
- skip looking at packet data completely
won't work if you play with checksums dynamically
but can be done if disabled on device
- allow ethtools to tweak offloads from userspace as well
> ---
> drivers/net/virtio_net.c | 70 ++++++++++++++++++++++++++++++++++++++++++++----
> 1 file changed, 65 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e732bd6..d970c2d 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -57,6 +57,11 @@ DECLARE_EWMA(pkt_len, 0, 64)
>
> #define VIRTNET_DRIVER_VERSION "1.0.0"
>
> +const unsigned long guest_offloads[] = { VIRTIO_NET_F_GUEST_TSO4,
> + VIRTIO_NET_F_GUEST_TSO6,
> + VIRTIO_NET_F_GUEST_ECN,
> + VIRTIO_NET_F_GUEST_UFO };
> +
> struct virtnet_stats {
> struct u64_stats_sync tx_syncp;
> struct u64_stats_sync rx_syncp;
> @@ -164,10 +169,13 @@ struct virtnet_info {
> u8 ctrl_promisc;
> u8 ctrl_allmulti;
> u16 ctrl_vid;
> + u64 ctrl_offloads;
>
> /* Ethtool settings */
> u8 duplex;
> u32 speed;
> +
> + unsigned long guest_offloads;
> };
>
> struct padded_vnet_hdr {
> @@ -1889,6 +1897,47 @@ static int virtnet_restore_up(struct virtio_device *vdev)
> return err;
> }
>
> +static int virtnet_set_guest_offloads(struct virtnet_info *vi, u64 offloads)
> +{
> + struct scatterlist sg;
> + vi->ctrl_offloads = cpu_to_virtio64(vi->vdev, offloads);
> +
> + sg_init_one(&sg, &vi->ctrl_offloads, sizeof(vi->ctrl_offloads));
> +
> + if (!virtnet_send_command(vi, VIRTIO_NET_CTRL_GUEST_OFFLOADS,
> + VIRTIO_NET_CTRL_GUEST_OFFLOADS_SET, &sg)) {
> + dev_warn(&vi->dev->dev, "Fail to set guest offload. \n");
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +static int virtnet_clear_guest_offloads(struct virtnet_info *vi)
> +{
> + u64 offloads = 0;
> +
> + if (!vi->guest_offloads)
> + return 0;
> +
> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
> + offloads = 1ULL << VIRTIO_NET_F_GUEST_CSUM;
> +
> + return virtnet_set_guest_offloads(vi, offloads);
> +}
> +
> +static int virtnet_restore_guest_offloads(struct virtnet_info *vi)
> +{
> + u64 offloads = vi->guest_offloads;
> +
> + if (!vi->guest_offloads)
> + return 0;
> + if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_CSUM))
> + offloads |= 1ULL << VIRTIO_NET_F_GUEST_CSUM;
> +
> + return virtnet_set_guest_offloads(vi, offloads);
> +}
> +
> static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> struct netlink_ext_ack *extack)
> {
> @@ -1898,10 +1947,11 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> u16 xdp_qp = 0, curr_qp;
> int i, err;
>
> - if (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
> - virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO)) {
> + if (!virtio_has_feature(vi->vdev, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS)
> + && (virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO4) ||
> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_TSO6) ||
> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_ECN) ||
> + virtio_has_feature(vi->vdev, VIRTIO_NET_F_GUEST_UFO))) {
> NL_SET_ERR_MSG_MOD(extack, "Can't set XDP while host is implementing LRO, disable LRO first");
> return -EOPNOTSUPP;
> }
> @@ -1950,6 +2000,12 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> for (i = 0; i < vi->max_queue_pairs; i++) {
> old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
> rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
> + if (i == 0) {
> + if (!old_prog)
> + virtnet_clear_guest_offloads(vi);
> + if (!prog)
> + virtnet_restore_guest_offloads(vi);
> + }
> if (old_prog)
> bpf_prog_put(old_prog);
> napi_enable(&vi->rq[i].napi);
> @@ -2583,6 +2639,10 @@ static int virtnet_probe(struct virtio_device *vdev)
> netif_carrier_on(dev);
> }
>
> + for (i = 0; i < ARRAY_SIZE(guest_offloads); i++)
> + if (virtio_has_feature(vi->vdev, guest_offloads[i]))
> + set_bit(guest_offloads[i], &vi->guest_offloads);
> +
> pr_debug("virtnet: registered device %s with %d RX and TX vq's\n",
> dev->name, max_queue_pairs);
>
> @@ -2679,7 +2739,7 @@ static struct virtio_device_id id_table[] = {
> VIRTIO_NET_F_CTRL_RX, VIRTIO_NET_F_CTRL_VLAN, \
> VIRTIO_NET_F_GUEST_ANNOUNCE, VIRTIO_NET_F_MQ, \
> VIRTIO_NET_F_CTRL_MAC_ADDR, \
> - VIRTIO_NET_F_MTU
> + VIRTIO_NET_F_MTU, VIRTIO_NET_F_CTRL_GUEST_OFFLOADS
>
> static unsigned int features[] = {
> VIRTNET_FEATURES,
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next 4/5] virtio-net: do not reset during XDP set
From: Michael S. Tsirkin @ 2017-07-18 19:49 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1500295441-19339-5-git-send-email-jasowang@redhat.com>
On Mon, Jul 17, 2017 at 08:44:00PM +0800, Jason Wang wrote:
> We used to reset during XDP set, the main reason is we need allocate
> extra headroom for header adjustment but there's no way to know the
> headroom of exist receive buffer. This works buy maybe complex and may
> cause the network down for a while which is bad for user
> experience. So this patch tries to avoid this by:
>
> - packing headroom into receive buffer ctx
> - check the headroom during XDP, and if it was not sufficient, copy
> the packet into a location which has a large enough headroom
The packing is actually done by previous patches. Here is a
corrected version:
We currently reset the device during XDP set, the main reason is
that we allocate more headroom with XDP (for header adjustment).
This works but causes network downtime for users.
Previous patches encoded the headroom in the buffer context,
this makes it possible to detect the case where a buffer
with headroom insufficient for XDP is added to the queue and
XDP is enabled afterwards.
Upon detection, we handle this case by copying the packet
(slow, but it's a temporary condition).
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/net/virtio_net.c | 230 ++++++++++++++++++++++-------------------------
> 1 file changed, 105 insertions(+), 125 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index e31b5b2..e732bd6 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -407,6 +407,67 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
> return vi->xdp_queue_pairs ? VIRTIO_XDP_HEADROOM : 0;
> }
>
> +/* We copy and linearize packet in the following cases:
> + *
> + * 1) Packet across multiple buffers, this happens normally when rx
> + * buffer size is underestimated. Rarely, since spec does not
> + * forbid using more than one buffer even if a single buffer is
> + * sufficient for the packet, we should also deal with this case.
Latest SVN of the spec actually forbids this. See:
net: clarify device rules for mergeable buffers
> + * 2) The header room is smaller than what XDP required. In this case
> + * we should copy the packet and reserve enough headroom for this.
> + * This would be slow but we at most we can copy times of queue
> + * size, this is acceptable. What's more important, this help to
> + * avoid resetting.
Last part of the comment applies to both cases. So
+/* We copy the packet for XDP in the following cases:
+ *
+ * 1) Packet is scattered across multiple rx buffers.
+ * 2) Headroom space is insufficient.
+ *
+ * This is inefficient but it's a temporary condition that
+ * we hit right after XDP is enabled and until queue is refilled
+ * with large buffers with sufficient headroom - so it should affect
+ * at most queue size packets.
+ * Afterwards, the conditions to enable
+ * XDP should preclude the underlying device from sending packets
+ * across multiple buffers (num_buf > 1), and we make sure buffers
+ * have enough headroom.
+ */
> + * 2) The header room is smaller than what XDP required. In this case
> + * we should copy the packet and reserve enough headroom for this.
> + * This would be slow but we at most we can copy times of queue
> + * size, this is acceptable. What's more important, this help to
> + * avoid resetting.
> + */
> +static struct page *xdp_linearize_page(struct receive_queue *rq,
> + u16 *num_buf,
> + struct page *p,
> + int offset,
> + int page_off,
> + unsigned int *len)
> +{
> + struct page *page = alloc_page(GFP_ATOMIC);
> +
> + if (!page)
> + return NULL;
> +
> + memcpy(page_address(page) + page_off, page_address(p) + offset, *len);
> + page_off += *len;
> +
> + while (--*num_buf) {
> + unsigned int buflen;
> + void *buf;
> + int off;
> +
> + buf = virtqueue_get_buf(rq->vq, &buflen);
> + if (unlikely(!buf))
> + goto err_buf;
> +
> + p = virt_to_head_page(buf);
> + off = buf - page_address(p);
> +
> + /* guard against a misconfigured or uncooperative backend that
> + * is sending packet larger than the MTU.
> + */
> + if ((page_off + buflen) > PAGE_SIZE) {
> + put_page(p);
> + goto err_buf;
> + }
> +
> + memcpy(page_address(page) + page_off,
> + page_address(p) + off, buflen);
> + page_off += buflen;
> + put_page(p);
> + }
> +
> + /* Headroom does not contribute to packet length */
> + *len = page_off - VIRTIO_XDP_HEADROOM;
> + return page;
> +err_buf:
> + __free_pages(page, 0);
> + return NULL;
> +}
> +
> static struct sk_buff *receive_small(struct net_device *dev,
> struct virtnet_info *vi,
> struct receive_queue *rq,
> @@ -415,12 +476,14 @@ static struct sk_buff *receive_small(struct net_device *dev,
> {
> struct sk_buff *skb;
> struct bpf_prog *xdp_prog;
> - unsigned int xdp_headroom = virtnet_get_headroom(vi);
> + unsigned int xdp_headroom = (unsigned long)ctx;
> unsigned int header_offset = VIRTNET_RX_PAD + xdp_headroom;
> unsigned int headroom = vi->hdr_len + header_offset;
> unsigned int buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
> SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> + struct page *page = virt_to_head_page(buf);
> unsigned int delta = 0;
> + struct page *xdp_page;
> len -= vi->hdr_len;
>
> rcu_read_lock();
> @@ -434,6 +497,27 @@ static struct sk_buff *receive_small(struct net_device *dev,
> if (unlikely(hdr->hdr.gso_type || hdr->hdr.flags))
> goto err_xdp;
>
> + if (unlikely(xdp_headroom != virtnet_get_headroom(vi))) {
Should this be xdp_headroom < virtnet_get_headroom(vi)?
Just in case we add more modes down the road.
> + int offset = buf - page_address(page) + header_offset;
> + unsigned int tlen = len + vi->hdr_len;
> + u16 num_buf = 1;
> +
> + xdp_headroom = virtnet_get_headroom(vi);
> + header_offset = VIRTNET_RX_PAD + xdp_headroom;
> + headroom = vi->hdr_len + header_offset;
> + buflen = SKB_DATA_ALIGN(GOOD_PACKET_LEN + headroom) +
> + SKB_DATA_ALIGN(sizeof(struct skb_shared_info));
> + xdp_page = xdp_linearize_page(rq, &num_buf, page,
> + offset, header_offset,
> + &tlen);
> + if (!xdp_page)
> + goto err_xdp;
> +
> + buf = page_address(xdp_page);
> + put_page(page);
> + page = xdp_page;
> + }
> +
> xdp.data_hard_start = buf + VIRTNET_RX_PAD + vi->hdr_len;
> xdp.data = xdp.data_hard_start + xdp_headroom;
> xdp.data_end = xdp.data + len;
> @@ -462,7 +546,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
>
> skb = build_skb(buf, buflen);
> if (!skb) {
> - put_page(virt_to_head_page(buf));
> + put_page(page);
> goto err;
> }
> skb_reserve(skb, headroom - delta);
> @@ -478,7 +562,7 @@ static struct sk_buff *receive_small(struct net_device *dev,
> err_xdp:
> rcu_read_unlock();
> dev->stats.rx_dropped++;
> - put_page(virt_to_head_page(buf));
> + put_page(page);
> xdp_xmit:
> return NULL;
> }
> @@ -503,66 +587,6 @@ static struct sk_buff *receive_big(struct net_device *dev,
> return NULL;
> }
>
> -/* The conditions to enable XDP should preclude the underlying device from
> - * sending packets across multiple buffers (num_buf > 1). However per spec
> - * it does not appear to be illegal to do so but rather just against convention.
> - * So in order to avoid making a system unresponsive the packets are pushed
> - * into a page and the XDP program is run. This will be extremely slow and we
> - * push a warning to the user to fix this as soon as possible. Fixing this may
> - * require resolving the underlying hardware to determine why multiple buffers
> - * are being received or simply loading the XDP program in the ingress stack
> - * after the skb is built because there is no advantage to running it here
> - * anymore.
> - */
> -static struct page *xdp_linearize_page(struct receive_queue *rq,
> - u16 *num_buf,
> - struct page *p,
> - int offset,
> - unsigned int *len)
> -{
> - struct page *page = alloc_page(GFP_ATOMIC);
> - unsigned int page_off = VIRTIO_XDP_HEADROOM;
> -
> - if (!page)
> - return NULL;
> -
> - memcpy(page_address(page) + page_off, page_address(p) + offset, *len);
> - page_off += *len;
> -
> - while (--*num_buf) {
> - unsigned int buflen;
> - void *buf;
> - int off;
> -
> - buf = virtqueue_get_buf(rq->vq, &buflen);
> - if (unlikely(!buf))
> - goto err_buf;
> -
> - p = virt_to_head_page(buf);
> - off = buf - page_address(p);
> -
> - /* guard against a misconfigured or uncooperative backend that
> - * is sending packet larger than the MTU.
> - */
> - if ((page_off + buflen) > PAGE_SIZE) {
> - put_page(p);
> - goto err_buf;
> - }
> -
> - memcpy(page_address(page) + page_off,
> - page_address(p) + off, buflen);
> - page_off += buflen;
> - put_page(p);
> - }
> -
> - /* Headroom does not contribute to packet length */
> - *len = page_off - VIRTIO_XDP_HEADROOM;
> - return page;
> -err_buf:
> - __free_pages(page, 0);
> - return NULL;
> -}
> -
> static struct sk_buff *receive_mergeable(struct net_device *dev,
> struct virtnet_info *vi,
> struct receive_queue *rq,
> @@ -577,6 +601,7 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> struct sk_buff *head_skb, *curr_skb;
> struct bpf_prog *xdp_prog;
> unsigned int truesize;
> + unsigned int headroom = mergeable_ctx_to_headroom(ctx);
>
> head_skb = NULL;
>
> @@ -589,10 +614,13 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> u32 act;
>
> /* This happens when rx buffer size is underestimated */
> - if (unlikely(num_buf > 1)) {
> + if (unlikely(num_buf > 1 ||
> + headroom < virtnet_get_headroom(vi))) {
> /* linearize data for XDP */
> xdp_page = xdp_linearize_page(rq, &num_buf,
> - page, offset, &len);
> + page, offset,
> + VIRTIO_XDP_HEADROOM,
> + &len);
> if (!xdp_page)
> goto err_xdp;
> offset = VIRTIO_XDP_HEADROOM;
> @@ -830,7 +858,6 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
> err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
> if (err < 0)
> put_page(virt_to_head_page(buf));
> -
> return err;
> }
>
> @@ -1834,7 +1861,6 @@ static void virtnet_freeze_down(struct virtio_device *vdev)
> }
>
> static int init_vqs(struct virtnet_info *vi);
> -static void _remove_vq_common(struct virtnet_info *vi);
>
> static int virtnet_restore_up(struct virtio_device *vdev)
> {
> @@ -1863,39 +1889,6 @@ static int virtnet_restore_up(struct virtio_device *vdev)
> return err;
> }
>
> -static int virtnet_reset(struct virtnet_info *vi, int curr_qp, int xdp_qp)
> -{
> - struct virtio_device *dev = vi->vdev;
> - int ret;
> -
> - virtio_config_disable(dev);
> - dev->failed = dev->config->get_status(dev) & VIRTIO_CONFIG_S_FAILED;
> - virtnet_freeze_down(dev);
> - _remove_vq_common(vi);
> -
> - virtio_add_status(dev, VIRTIO_CONFIG_S_ACKNOWLEDGE);
> - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER);
> -
> - ret = virtio_finalize_features(dev);
> - if (ret)
> - goto err;
> -
> - vi->xdp_queue_pairs = xdp_qp;
> - ret = virtnet_restore_up(dev);
> - if (ret)
> - goto err;
> - ret = _virtnet_set_queues(vi, curr_qp);
> - if (ret)
> - goto err;
> -
> - virtio_add_status(dev, VIRTIO_CONFIG_S_DRIVER_OK);
> - virtio_config_enable(dev);
> - return 0;
> -err:
> - virtio_add_status(dev, VIRTIO_CONFIG_S_FAILED);
> - return ret;
> -}
> -
> static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> struct netlink_ext_ack *extack)
> {
> @@ -1942,35 +1935,31 @@ static int virtnet_xdp_set(struct net_device *dev, struct bpf_prog *prog,
> return PTR_ERR(prog);
> }
>
> - /* Changing the headroom in buffers is a disruptive operation because
> - * existing buffers must be flushed and reallocated. This will happen
> - * when a xdp program is initially added or xdp is disabled by removing
> - * the xdp program resulting in number of XDP queues changing.
> + /* synchronize with NAPI which may do XDP_TX based on queue
> + * pair numbers.
I think you mean
/* Make sure NAPI is not using any XDP TX queues for RX. */
is that it?
> - if (vi->xdp_queue_pairs != xdp_qp) {
> - err = virtnet_reset(vi, curr_qp + xdp_qp, xdp_qp);
> - if (err) {
> - dev_warn(&dev->dev, "XDP reset failure.\n");
> - goto virtio_reset_err;
> - }
> - }
> + for (i = 0; i < vi->max_queue_pairs; i++)
> + napi_disable(&vi->rq[i].napi);
>
This is pretty slow if queues are busy. Should we avoid this for queues
which aren't effected?
> netif_set_real_num_rx_queues(dev, curr_qp + xdp_qp);
> + err = _virtnet_set_queues(vi, curr_qp + xdp_qp);
> + if (err)
> + goto err;
> + vi->xdp_queue_pairs = xdp_qp;
>
> for (i = 0; i < vi->max_queue_pairs; i++) {
> old_prog = rtnl_dereference(vi->rq[i].xdp_prog);
> rcu_assign_pointer(vi->rq[i].xdp_prog, prog);
> if (old_prog)
> bpf_prog_put(old_prog);
> + napi_enable(&vi->rq[i].napi);
This seems racy. See comment around virtnet_napi_enable.
> }
>
> return 0;
>
> -virtio_reset_err:
> - /* On reset error do our best to unwind XDP changes inflight and return
> - * error up to user space for resolution. The underlying reset hung on
> - * us so not much we can do here.
> - */
> +err:
> + for (i = 0; i < vi->max_queue_pairs; i++)
> + napi_enable(&vi->rq[i].napi);
> if (prog)
> bpf_prog_sub(prog, vi->max_queue_pairs - 1);
> return err;
> @@ -2614,15 +2603,6 @@ static int virtnet_probe(struct virtio_device *vdev)
> return err;
> }
>
> -static void _remove_vq_common(struct virtnet_info *vi)
> -{
> - vi->vdev->config->reset(vi->vdev);
> - free_unused_bufs(vi);
> - _free_receive_bufs(vi);
> - free_receive_page_frags(vi);
> - virtnet_del_vqs(vi);
> -}
> -
> static void remove_vq_common(struct virtnet_info *vi)
> {
> vi->vdev->config->reset(vi->vdev);
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next 3/5] virtio-net: switch to use new ctx API for small buffer
From: Michael S. Tsirkin @ 2017-07-18 19:20 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1500295441-19339-4-git-send-email-jasowang@redhat.com>
what's needed is ability to store the headroom there.
virtio-net: switch to use ctx API for small buffers
Use ctx API to store headroom for small buffers.
Following patches will retrieve this info and use it for XDP.
On Mon, Jul 17, 2017 at 08:43:59PM +0800, Jason Wang wrote:
> Switch to use ctx API for small buffer, this is need for avoiding
> reset on XDP.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/net/virtio_net.c | 12 +++++++-----
> 1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 8fae9a8..e31b5b2 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -410,7 +410,8 @@ static unsigned int virtnet_get_headroom(struct virtnet_info *vi)
> static struct sk_buff *receive_small(struct net_device *dev,
> struct virtnet_info *vi,
> struct receive_queue *rq,
> - void *buf, unsigned int len)
> + void *buf, void *ctx,
> + unsigned int len)
> {
> struct sk_buff *skb;
> struct bpf_prog *xdp_prog;
> @@ -773,7 +774,7 @@ static int receive_buf(struct virtnet_info *vi, struct receive_queue *rq,
> else if (vi->big_packets)
> skb = receive_big(dev, vi, rq, buf, len);
> else
> - skb = receive_small(dev, vi, rq, buf, len);
> + skb = receive_small(dev, vi, rq, buf, ctx, len);
>
> if (unlikely(!skb))
> return 0;
> @@ -812,6 +813,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
Let's document that ctx API is used a bit differently here:
/* Unlike mergeable buffers, all buffers are allocated to the same size,
* except for the headroom. For this reason we do not need to use
* mergeable_len_to_ctx here - it is enough to store the headroom as the
* context ignoring the truesize.
*/
as an alternative, reuse the same format as mergeable buffers.
> struct page_frag *alloc_frag = &rq->alloc_frag;
> char *buf;
> unsigned int xdp_headroom = virtnet_get_headroom(vi);
> + void *ctx = (void *)(unsigned long)xdp_headroom;
> int len = vi->hdr_len + VIRTNET_RX_PAD + GOOD_PACKET_LEN + xdp_headroom;
> int err;
>
> @@ -825,7 +827,7 @@ static int add_recvbuf_small(struct virtnet_info *vi, struct receive_queue *rq,
> alloc_frag->offset += len;
> sg_init_one(rq->sg, buf + VIRTNET_RX_PAD + xdp_headroom,
> vi->hdr_len + GOOD_PACKET_LEN);
> - err = virtqueue_add_inbuf(rq->vq, rq->sg, 1, buf, gfp);
> + err = virtqueue_add_inbuf_ctx(rq->vq, rq->sg, 1, buf, ctx, gfp);
> if (err < 0)
> put_page(virt_to_head_page(buf));
>
> @@ -1034,7 +1036,7 @@ static int virtnet_receive(struct receive_queue *rq, int budget)
> void *buf;
> struct virtnet_stats *stats = this_cpu_ptr(vi->stats);
>
> - if (vi->mergeable_rx_bufs) {
> + if (!vi->big_packets || vi->mergeable_rx_bufs) {
> void *ctx;
>
> while (received < budget &&
> @@ -2198,7 +2200,7 @@ static int virtnet_find_vqs(struct virtnet_info *vi)
> names = kmalloc(total_vqs * sizeof(*names), GFP_KERNEL);
> if (!names)
> goto err_names;
> - if (vi->mergeable_rx_bufs) {
> + if (!vi->big_packets || vi->mergeable_rx_bufs) {
> ctx = kzalloc(total_vqs * sizeof(*ctx), GFP_KERNEL);
> if (!ctx)
> goto err_ctx;
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next 2/5] virtio-net: pack headroom into ctx for mergeable buffer
From: Michael S. Tsirkin @ 2017-07-18 18:59 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <1500295441-19339-3-git-send-email-jasowang@redhat.com>
On Mon, Jul 17, 2017 at 08:43:58PM +0800, Jason Wang wrote:
> Pack headroom into ctx, then during XDP set, we could know the size of
> headroom and copy if needed. This is required for avoiding reset on
> XDP.
Not really when XDP is set - it's when buffers are used.
virtio-net: pack headroom into ctx for mergeable buffers
Pack headroom into ctx - this way when we get a buffer we can figure out
the actual headroom that was allocated for the buffer. Will be helpful
to optimize switching between XDP and non-XDP modes which have different
headroom requirements.
>
> Signed-off-by: Jason Wang <jasowang@redhat.com>
> ---
> drivers/net/virtio_net.c | 29 ++++++++++++++++++++++++-----
> 1 file changed, 24 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/net/virtio_net.c b/drivers/net/virtio_net.c
> index 1f8c15c..8fae9a8 100644
> --- a/drivers/net/virtio_net.c
> +++ b/drivers/net/virtio_net.c
> @@ -270,6 +270,23 @@ static void skb_xmit_done(struct virtqueue *vq)
> netif_wake_subqueue(vi->dev, vq2txq(vq));
> }
>
> +#define MRG_CTX_HEADER_SHIFT 22
> +static void *mergeable_len_to_ctx(unsigned int truesize,
> + unsigned int headroom)
> +{
> + return (void *)(unsigned long)((headroom << MRG_CTX_HEADER_SHIFT) | truesize);
> +}
> +
> +static unsigned int mergeable_ctx_to_headroom(void *mrg_ctx)
> +{
> + return (unsigned long)mrg_ctx >> MRG_CTX_HEADER_SHIFT;
> +}
> +
> +static unsigned int mergeable_ctx_to_truesize(void *mrg_ctx)
> +{
> + return (unsigned long)mrg_ctx & ((1 << MRG_CTX_HEADER_SHIFT) - 1);
> +}
> +
> /* Called from bottom half context */
> static struct sk_buff *page_to_skb(struct virtnet_info *vi,
> struct receive_queue *rq,
> @@ -639,13 +656,14 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> }
> rcu_read_unlock();
>
> - if (unlikely(len > (unsigned long)ctx)) {
> + truesize = mergeable_ctx_to_truesize(ctx);
> + if (unlikely(len > truesize)) {
> pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> dev->name, len, (unsigned long)ctx);
> dev->stats.rx_length_errors++;
> goto err_skb;
> }
> - truesize = (unsigned long)ctx;
> +
> head_skb = page_to_skb(vi, rq, page, offset, len, truesize);
> curr_skb = head_skb;
>
> @@ -665,13 +683,14 @@ static struct sk_buff *receive_mergeable(struct net_device *dev,
> }
>
> page = virt_to_head_page(buf);
> - if (unlikely(len > (unsigned long)ctx)) {
> +
> + truesize = mergeable_ctx_to_truesize(ctx);
> + if (unlikely(len > truesize)) {
> pr_debug("%s: rx error: len %u exceeds truesize %lu\n",
> dev->name, len, (unsigned long)ctx);
> dev->stats.rx_length_errors++;
> goto err_skb;
> }
> - truesize = (unsigned long)ctx;
>
> num_skb_frags = skb_shinfo(curr_skb)->nr_frags;
> if (unlikely(num_skb_frags == MAX_SKB_FRAGS)) {
> @@ -889,7 +908,7 @@ static int add_recvbuf_mergeable(struct virtnet_info *vi,
>
> buf = (char *)page_address(alloc_frag->page) + alloc_frag->offset;
> buf += headroom; /* advance address leaving hole at front of pkt */
> - ctx = (void *)(unsigned long)len;
> + ctx = mergeable_len_to_ctx(len, headroom);
> get_page(alloc_frag->page);
> alloc_frag->offset += len + headroom;
> hole = alloc_frag->size - alloc_frag->offset;
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH net-next 0/5] refine virtio-net XDP
From: Michael S. Tsirkin @ 2017-07-18 18:47 UTC (permalink / raw)
To: David Miller; +Cc: netdev, linux-kernel, virtualization
In-Reply-To: <20170718.112442.705495974245565173.davem@davemloft.net>
On Tue, Jul 18, 2017 at 11:24:42AM -0700, David Miller wrote:
> From: Jason Wang <jasowang@redhat.com>
> Date: Mon, 17 Jul 2017 20:43:56 +0800
>
> > This series brings two optimizations for virtio-net XDP:
> >
> > - avoid reset during XDP set
> > - turn off offloads on demand
> >
> > Please review.
>
> Michael, please review Jason's changes.
>
> Thanks.
Doing that, thanks for the reminder.
--
MST
^ permalink raw reply
* Re: [PATCH net-next 0/5] refine virtio-net XDP
From: David Miller @ 2017-07-18 18:24 UTC (permalink / raw)
To: jasowang; +Cc: netdev, virtualization, linux-kernel, mst
In-Reply-To: <1500295441-19339-1-git-send-email-jasowang@redhat.com>
From: Jason Wang <jasowang@redhat.com>
Date: Mon, 17 Jul 2017 20:43:56 +0800
> This series brings two optimizations for virtio-net XDP:
>
> - avoid reset during XDP set
> - turn off offloads on demand
>
> Please review.
Michael, please review Jason's changes.
Thanks.
^ permalink raw reply
* Re: [PATCH v1] virtio_blk: Use sysfs_match_string() helper
From: Michael S. Tsirkin @ 2017-07-18 16:25 UTC (permalink / raw)
To: Andy Shevchenko; +Cc: Jens Axboe, virtualization
In-Reply-To: <1500392812.29303.82.camel@linux.intel.com>
On Tue, Jul 18, 2017 at 06:46:52PM +0300, Andy Shevchenko wrote:
> On Mon, 2017-07-03 at 19:37 +0300, Michael S. Tsirkin wrote:
> > On Mon, Jul 03, 2017 at 03:05:30PM +0300, Andy Shevchenko wrote:
> > > On Fri, 2017-06-09 at 15:07 +0300, Andy Shevchenko wrote:
> > > > Use sysfs_match_string() helper instead of open coded variant.
> > >
> > > Did I miss maintainer?
> > >
> > You didn't, I'll merge this in the next PULL.
>
> I didn't see this in v4.13-rc1. Shall I resend?
Thanks for the patch and sorry about a delay.
It's because I didn't send a pull - this cycle is so quiet, that is the
single patch I have. I was hoping for the balloon free page hint patches
to get there but it does not look like that is happening soon. I'll
merge it you do not need to resend.
> --
> Andy Shevchenko <andriy.shevchenko@linux.intel.com>
> Intel Finland Oy
^ 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