* 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
* Re: [virtio-dev] packed ring layout proposal v2
From: Michael S. Tsirkin @ 2017-07-18 16:23 UTC (permalink / raw)
To: Lior Narkis
Cc: virtio-dev@lists.oasis-open.org,
virtualization@lists.linux-foundation.org
In-Reply-To: <DB5PR05MB176690DF180908ABCDDA0860D3A30@DB5PR05MB1766.eurprd05.prod.outlook.com>
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.
> 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. 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.
> 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?
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.
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?
> > * 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 v1] virtio_blk: Use sysfs_match_string() helper
From: Andy Shevchenko @ 2017-07-18 15:46 UTC (permalink / raw)
To: Michael S. Tsirkin; +Cc: Jens Axboe, virtualization
In-Reply-To: <20170703193730-mutt-send-email-mst@kernel.org>
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?
--
Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Intel Finland Oy
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Wei Wang @ 2017-07-18 2:12 UTC (permalink / raw)
To: Michal Hocko, Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170717152448.GN12888@dhcp22.suse.cz>
On 07/17/2017 11:24 PM, Michal Hocko wrote:
> On Fri 14-07-17 22:17:13, Michael S. Tsirkin wrote:
>> On Fri, Jul 14, 2017 at 02:30:23PM +0200, Michal Hocko wrote:
>>> On Wed 12-07-17 20:40:19, Wei Wang wrote:
>>>> This patch adds support for reporting blocks of pages on the free list
>>>> specified by the caller.
>>>>
>>>> As pages can leave the free list during this call or immediately
>>>> afterwards, they are not guaranteed to be free after the function
>>>> returns. The only guarantee this makes is that the page was on the free
>>>> list at some point in time after the function has been invoked.
>>>>
>>>> Therefore, it is not safe for caller to use any pages on the returned
>>>> block or to discard data that is put there after the function returns.
>>>> However, it is safe for caller to discard data that was in one of these
>>>> pages before the function was invoked.
>>> I do not understand what is the point of such a function and how it is
>>> used because the patch doesn't give us any user (I haven't checked other
>>> patches yet).
>>>
>>> But just from the semantic point of view this sounds like a horrible
>>> idea. The only way to get a free block of pages is to call the page
>>> allocator. I am tempted to give it Nack right on those grounds but I
>>> would like to hear more about what you actually want to achieve.
>> Basically it's a performance hint to the hypervisor.
>> For example, these pages would be good candidates to
>> move around as they are not mapped into any running
>> applications.
>>
>> As such, it's important not to slow down other parts of the system too
>> much - otherwise we are speeding up one part of the system while we slow
>> down other parts of it, which is why it's trying to drop the lock as
>> soon a possible.
Probably I should have included the introduction of the usages in
the log. Hope it is not too later to explain here:
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.
It is not concerned that the memory pages are used after they are given to
the hypervisor as a hint of the unused pages, because they will be tracked
by the hypervisor and transferred in the next round if they are used and
written.
> So why cannot you simply allocate those page and then do whatever you
> need. You can tell the page allocator to do only a lightweight
> allocation by the gfp_mask - e.g. GFP_NOWAIT or if you even do not want
> to risk kswapd intervening then 0 mask.
Here are the 2 reasons that we can't get the hint of unused pages by
allocating
them:
1) It's expected that live migration shouldn't affect the things running
inside
the VM - take away all the free pages from the guest would greatly slow
down the
activities inside guest (e.g. the network transmission may be stuck due
to the lack of
sk_buf).
2) The hint of free pages are used to optimize the 1st round memory
transfer, so the hint
is expect to be gotten by the hypervisor as quick as possible. Depending
on the memory
size of the guest, allocation of all the free memory would be too long
for the case.
Hope it clarifies the use case.
>> As long as hypervisor does not assume it can drop these pages, and as
>> long it's correct in most cases. we are OK even if the hint is slightly
>> wrong because hypervisor notifications are racing with allocations.
> But the page could have been reused anytime after the lock is dropped
> and you cannot check for that except for elevating the reference count.
As also introduced above, the hypervisor uses a dirty page logging mechanism
to track which memory page is written by the guest when live migration
begins.
Best,
Wei
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Michal Hocko @ 2017-07-17 15:24 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170714181523-mutt-send-email-mst@kernel.org>
On Fri 14-07-17 22:17:13, Michael S. Tsirkin wrote:
> On Fri, Jul 14, 2017 at 02:30:23PM +0200, Michal Hocko wrote:
> > On Wed 12-07-17 20:40:19, Wei Wang wrote:
> > > This patch adds support for reporting blocks of pages on the free list
> > > specified by the caller.
> > >
> > > As pages can leave the free list during this call or immediately
> > > afterwards, they are not guaranteed to be free after the function
> > > returns. The only guarantee this makes is that the page was on the free
> > > list at some point in time after the function has been invoked.
> > >
> > > Therefore, it is not safe for caller to use any pages on the returned
> > > block or to discard data that is put there after the function returns.
> > > However, it is safe for caller to discard data that was in one of these
> > > pages before the function was invoked.
> >
> > I do not understand what is the point of such a function and how it is
> > used because the patch doesn't give us any user (I haven't checked other
> > patches yet).
> >
> > But just from the semantic point of view this sounds like a horrible
> > idea. The only way to get a free block of pages is to call the page
> > allocator. I am tempted to give it Nack right on those grounds but I
> > would like to hear more about what you actually want to achieve.
>
> Basically it's a performance hint to the hypervisor.
> For example, these pages would be good candidates to
> move around as they are not mapped into any running
> applications.
>
> As such, it's important not to slow down other parts of the system too
> much - otherwise we are speeding up one part of the system while we slow
> down other parts of it, which is why it's trying to drop the lock as
> soon a possible.
So why cannot you simply allocate those page and then do whatever you
need. You can tell the page allocator to do only a lightweight
allocation by the gfp_mask - e.g. GFP_NOWAIT or if you even do not want
to risk kswapd intervening then 0 mask.
> As long as hypervisor does not assume it can drop these pages, and as
> long it's correct in most cases. we are OK even if the hint is slightly
> wrong because hypervisor notifications are racing with allocations.
But the page could have been reused anytime after the lock is dropped
and you cannot check for that except for elevating the reference count.
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* [PATCH net-next 5/5] virtio-net: switch off offloads on demand if possible on XDP set
From: Jason Wang @ 2017-07-17 12:44 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, netdev
In-Reply-To: <1500295441-19339-1-git-send-email-jasowang@redhat.com>
Current XDP implementation want guest offloads feature to be disabled
on qemu cli. 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 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 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 related
* [PATCH net-next 4/5] virtio-net: do not reset during XDP set
From: Jason Wang @ 2017-07-17 12:44 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, netdev
In-Reply-To: <1500295441-19339-1-git-send-email-jasowang@redhat.com>
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
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.
+ * 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))) {
+ 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.
*/
- 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);
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);
}
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 related
* [PATCH net-next 3/5] virtio-net: switch to use new ctx API for small buffer
From: Jason Wang @ 2017-07-17 12:43 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, netdev
In-Reply-To: <1500295441-19339-1-git-send-email-jasowang@redhat.com>
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,
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 related
* [PATCH net-next 2/5] virtio-net: pack headroom into ctx for mergeable buffer
From: Jason Wang @ 2017-07-17 12:43 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, netdev
In-Reply-To: <1500295441-19339-1-git-send-email-jasowang@redhat.com>
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.
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 1/5] virtio_ring: allow to store zero as the ctx
From: Jason Wang @ 2017-07-17 12:43 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, netdev
In-Reply-To: <1500295441-19339-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 0/5] refine virtio-net XDP
From: Jason Wang @ 2017-07-17 12:43 UTC (permalink / raw)
To: mst, jasowang, virtualization, linux-kernel, netdev
Hi:
This series brings two optimizations for virtio-net XDP:
- avoid reset during XDP set
- turn off offloads on demand
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: [virtio-dev] packed ring layout proposal v2
From: Lior Narkis @ 2017-07-16 6:00 UTC (permalink / raw)
To: Michael S. Tsirkin, virtio-dev@lists.oasis-open.org
Cc: virtualization@lists.linux-foundation.org
In-Reply-To: <20160915223915.qjlnlvf2w7u37bu3@redhat.com>
> -----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.
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.
CQEs are continuous in memory so prefetching by the driver is efficient, although the device might complete work descriptors out of order.
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.
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.
> * 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
* [RFC PATCH 12/33] Change __FIXADDR_TOP to leave room for the hypervisor.
From: Solen win2 @ 2017-07-15 15:59 UTC (permalink / raw)
To: virtualization
[-- Attachment #1.1: Type: text/plain, Size: 21 bytes --]
Support@btc.com
all
[-- Attachment #1.2: Type: text/html, Size: 160 bytes --]
[-- Attachment #2: Type: text/plain, Size: 183 bytes --]
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Michael S. Tsirkin @ 2017-07-14 19:17 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170714123023.GA2624@dhcp22.suse.cz>
On Fri, Jul 14, 2017 at 02:30:23PM +0200, Michal Hocko wrote:
> On Wed 12-07-17 20:40:19, Wei Wang wrote:
> > This patch adds support for reporting blocks of pages on the free list
> > specified by the caller.
> >
> > As pages can leave the free list during this call or immediately
> > afterwards, they are not guaranteed to be free after the function
> > returns. The only guarantee this makes is that the page was on the free
> > list at some point in time after the function has been invoked.
> >
> > Therefore, it is not safe for caller to use any pages on the returned
> > block or to discard data that is put there after the function returns.
> > However, it is safe for caller to discard data that was in one of these
> > pages before the function was invoked.
>
> I do not understand what is the point of such a function and how it is
> used because the patch doesn't give us any user (I haven't checked other
> patches yet).
>
> But just from the semantic point of view this sounds like a horrible
> idea. The only way to get a free block of pages is to call the page
> allocator. I am tempted to give it Nack right on those grounds but I
> would like to hear more about what you actually want to achieve.
Basically it's a performance hint to the hypervisor.
For example, these pages would be good candidates to
move around as they are not mapped into any running
applications.
As such, it's important not to slow down other parts of the system too
much - otherwise we are speeding up one part of the system while we slow
down other parts of it, which is why it's trying to drop the lock as
soon a possible.
As long as hypervisor does not assume it can drop these pages, and as
long it's correct in most cases. we are OK even if the hint is slightly
wrong because hypervisor notifications are racing with allocations.
There are patches to do more tricks - if hypervisor tracks all
memory writes we might actually use this hint to discard data -
but that is just implementation detail.
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > ---
> > include/linux/mm.h | 5 +++
> > mm/page_alloc.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 101 insertions(+)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 46b9ac5..76cb433 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1835,6 +1835,11 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
> > unsigned long zone_start_pfn, unsigned long *zholes_size);
> > extern void free_initmem(void);
> >
> > +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> > +extern int report_unused_page_block(struct zone *zone, unsigned int order,
> > + unsigned int migratetype,
> > + struct page **page);
> > +#endif
> > /*
> > * Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
> > * into the buddy system. The freed pages will be poisoned with pattern
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 64b7d82..8b3c9dd 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -4753,6 +4753,102 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
> > show_swap_cache_info();
> > }
> >
> > +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> > +
> > +/*
> > + * Heuristically get a page block in the system that is unused.
> > + * It is possible that pages from the page block are used immediately after
> > + * report_unused_page_block() returns. It is the caller's responsibility
> > + * to either detect or prevent the use of such pages.
> > + *
> > + * The free list to check: zone->free_area[order].free_list[migratetype].
> > + *
> > + * If the caller supplied page block (i.e. **page) is on the free list, offer
> > + * the next page block on the list to the caller. Otherwise, offer the first
> > + * page block on the list.
> > + *
> > + * Note: it is not safe for caller to use any pages on the returned
> > + * block or to discard data that is put there after the function returns.
> > + * However, it is safe for caller to discard data that was in one of these
> > + * pages before the function was invoked.
> > + *
> > + * Return 0 when a page block is found on the caller specified free list.
> > + */
> > +int report_unused_page_block(struct zone *zone, unsigned int order,
> > + unsigned int migratetype, struct page **page)
> > +{
> > + struct zone *this_zone;
> > + struct list_head *this_list;
> > + int ret = 0;
> > + unsigned long flags;
> > +
> > + /* Sanity check */
> > + if (zone == NULL || page == NULL || order >= MAX_ORDER ||
> > + migratetype >= MIGRATE_TYPES)
> > + return -EINVAL;
> > +
> > + /* Zone validity check */
> > + for_each_populated_zone(this_zone) {
> > + if (zone == this_zone)
> > + break;
> > + }
> > +
> > + /* Got a non-existent zone from the caller? */
> > + if (zone != this_zone)
> > + return -EINVAL;
>
> Huh, what do you check for here? Why don't you simply
> populated_zone(zone)?
>
> > +
> > + spin_lock_irqsave(&this_zone->lock, flags);
> > +
> > + this_list = &zone->free_area[order].free_list[migratetype];
> > + if (list_empty(this_list)) {
> > + *page = NULL;
> > + ret = 1;
> > + goto out;
> > + }
> > +
> > + /* The caller is asking for the first free page block on the list */
> > + if ((*page) == NULL) {
> > + *page = list_first_entry(this_list, struct page, lru);
> > + ret = 0;
> > + goto out;
> > + }
> > +
> > + /*
> > + * The page block passed from the caller is not on this free list
> > + * anymore (e.g. a 1MB free page block has been split). In this case,
> > + * offer the first page block on the free list that the caller is
> > + * asking for.
> > + */
> > + if (PageBuddy(*page) && order != page_order(*page)) {
> > + *page = list_first_entry(this_list, struct page, lru);
> > + ret = 0;
> > + goto out;
> > + }
> > +
> > + /*
> > + * The page block passed from the caller has been the last page block
> > + * on the list.
> > + */
> > + if ((*page)->lru.next == this_list) {
> > + *page = NULL;
> > + ret = 1;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Finally, fall into the regular case: the page block passed from the
> > + * caller is still on the free list. Offer the next one.
> > + */
> > + *page = list_next_entry((*page), lru);
> > + ret = 0;
> > +out:
> > + spin_unlock_irqrestore(&this_zone->lock, flags);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(report_unused_page_block);
> > +
> > +#endif
> > +
> > static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
> > {
> > zoneref->zone = zone;
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org. For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Michael S. Tsirkin @ 2017-07-14 15:46 UTC (permalink / raw)
To: Michal Hocko
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170714125429.GP2618@dhcp22.suse.cz>
On Fri, Jul 14, 2017 at 02:54:30PM +0200, Michal Hocko wrote:
> On Fri 14-07-17 14:30:23, Michal Hocko wrote:
> > On Wed 12-07-17 20:40:19, Wei Wang wrote:
> > > This patch adds support for reporting blocks of pages on the free list
> > > specified by the caller.
> > >
> > > As pages can leave the free list during this call or immediately
> > > afterwards, they are not guaranteed to be free after the function
> > > returns. The only guarantee this makes is that the page was on the free
> > > list at some point in time after the function has been invoked.
> > >
> > > Therefore, it is not safe for caller to use any pages on the returned
> > > block or to discard data that is put there after the function returns.
> > > However, it is safe for caller to discard data that was in one of these
> > > pages before the function was invoked.
> >
> > I do not understand what is the point of such a function and how it is
> > used because the patch doesn't give us any user (I haven't checked other
> > patches yet).
> >
> > But just from the semantic point of view this sounds like a horrible
> > idea. The only way to get a free block of pages is to call the page
> > allocator. I am tempted to give it Nack right on those grounds but I
> > would like to hear more about what you actually want to achieve.
>
> OK, so I gave it another thought and giving a page which is still on the
> free list to a random module is just a free ticket to a disaster.
> Nacked-by: Michal Hocko <mhocko@suse.com>
I agree it should be EXPORT_SYMBOL_GPL. Too much power
to give to non-GPL modules.
But pls take a look at the explanation I posted. Any kind of hypervisor
hinting will need to do this by definition - best we can do is keep a
lock while we do this.
> >
> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > > ---
> > > include/linux/mm.h | 5 +++
> > > mm/page_alloc.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > > 2 files changed, 101 insertions(+)
> > >
> > > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > > index 46b9ac5..76cb433 100644
> > > --- a/include/linux/mm.h
> > > +++ b/include/linux/mm.h
> > > @@ -1835,6 +1835,11 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
> > > unsigned long zone_start_pfn, unsigned long *zholes_size);
> > > extern void free_initmem(void);
> > >
> > > +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> > > +extern int report_unused_page_block(struct zone *zone, unsigned int order,
> > > + unsigned int migratetype,
> > > + struct page **page);
> > > +#endif
> > > /*
> > > * Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
> > > * into the buddy system. The freed pages will be poisoned with pattern
> > > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > > index 64b7d82..8b3c9dd 100644
> > > --- a/mm/page_alloc.c
> > > +++ b/mm/page_alloc.c
> > > @@ -4753,6 +4753,102 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
> > > show_swap_cache_info();
> > > }
> > >
> > > +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> > > +
> > > +/*
> > > + * Heuristically get a page block in the system that is unused.
> > > + * It is possible that pages from the page block are used immediately after
> > > + * report_unused_page_block() returns. It is the caller's responsibility
> > > + * to either detect or prevent the use of such pages.
> > > + *
> > > + * The free list to check: zone->free_area[order].free_list[migratetype].
> > > + *
> > > + * If the caller supplied page block (i.e. **page) is on the free list, offer
> > > + * the next page block on the list to the caller. Otherwise, offer the first
> > > + * page block on the list.
> > > + *
> > > + * Note: it is not safe for caller to use any pages on the returned
> > > + * block or to discard data that is put there after the function returns.
> > > + * However, it is safe for caller to discard data that was in one of these
> > > + * pages before the function was invoked.
> > > + *
> > > + * Return 0 when a page block is found on the caller specified free list.
> > > + */
> > > +int report_unused_page_block(struct zone *zone, unsigned int order,
> > > + unsigned int migratetype, struct page **page)
> > > +{
> > > + struct zone *this_zone;
> > > + struct list_head *this_list;
> > > + int ret = 0;
> > > + unsigned long flags;
> > > +
> > > + /* Sanity check */
> > > + if (zone == NULL || page == NULL || order >= MAX_ORDER ||
> > > + migratetype >= MIGRATE_TYPES)
> > > + return -EINVAL;
> > > +
> > > + /* Zone validity check */
> > > + for_each_populated_zone(this_zone) {
> > > + if (zone == this_zone)
> > > + break;
> > > + }
> > > +
> > > + /* Got a non-existent zone from the caller? */
> > > + if (zone != this_zone)
> > > + return -EINVAL;
> >
> > Huh, what do you check for here? Why don't you simply
> > populated_zone(zone)?
> >
> > > +
> > > + spin_lock_irqsave(&this_zone->lock, flags);
> > > +
> > > + this_list = &zone->free_area[order].free_list[migratetype];
> > > + if (list_empty(this_list)) {
> > > + *page = NULL;
> > > + ret = 1;
> > > + goto out;
> > > + }
> > > +
> > > + /* The caller is asking for the first free page block on the list */
> > > + if ((*page) == NULL) {
> > > + *page = list_first_entry(this_list, struct page, lru);
> > > + ret = 0;
> > > + goto out;
> > > + }
> > > +
> > > + /*
> > > + * The page block passed from the caller is not on this free list
> > > + * anymore (e.g. a 1MB free page block has been split). In this case,
> > > + * offer the first page block on the free list that the caller is
> > > + * asking for.
> > > + */
> > > + if (PageBuddy(*page) && order != page_order(*page)) {
> > > + *page = list_first_entry(this_list, struct page, lru);
> > > + ret = 0;
> > > + goto out;
> > > + }
> > > +
> > > + /*
> > > + * The page block passed from the caller has been the last page block
> > > + * on the list.
> > > + */
> > > + if ((*page)->lru.next == this_list) {
> > > + *page = NULL;
> > > + ret = 1;
> > > + goto out;
> > > + }
> > > +
> > > + /*
> > > + * Finally, fall into the regular case: the page block passed from the
> > > + * caller is still on the free list. Offer the next one.
> > > + */
> > > + *page = list_next_entry((*page), lru);
> > > + ret = 0;
> > > +out:
> > > + spin_unlock_irqrestore(&this_zone->lock, flags);
> > > + return ret;
> > > +}
> > > +EXPORT_SYMBOL(report_unused_page_block);
> > > +
> > > +#endif
> > > +
> > > static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
> > > {
> > > zoneref->zone = zone;
> > > --
> > > 2.7.4
> > >
> > > --
> > > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > > the body to majordomo@kvack.org. For more info on Linux MM,
> > > see: http://www.linux-mm.org/ .
> > > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
> >
> > --
> > Michal Hocko
> > SUSE Labs
>
> --
> Michal Hocko
> SUSE Labs
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Michal Hocko @ 2017-07-14 12:54 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170714123023.GA2624@dhcp22.suse.cz>
On Fri 14-07-17 14:30:23, Michal Hocko wrote:
> On Wed 12-07-17 20:40:19, Wei Wang wrote:
> > This patch adds support for reporting blocks of pages on the free list
> > specified by the caller.
> >
> > As pages can leave the free list during this call or immediately
> > afterwards, they are not guaranteed to be free after the function
> > returns. The only guarantee this makes is that the page was on the free
> > list at some point in time after the function has been invoked.
> >
> > Therefore, it is not safe for caller to use any pages on the returned
> > block or to discard data that is put there after the function returns.
> > However, it is safe for caller to discard data that was in one of these
> > pages before the function was invoked.
>
> I do not understand what is the point of such a function and how it is
> used because the patch doesn't give us any user (I haven't checked other
> patches yet).
>
> But just from the semantic point of view this sounds like a horrible
> idea. The only way to get a free block of pages is to call the page
> allocator. I am tempted to give it Nack right on those grounds but I
> would like to hear more about what you actually want to achieve.
OK, so I gave it another thought and giving a page which is still on the
free list to a random module is just a free ticket to a disaster.
Nacked-by: Michal Hocko <mhocko@suse.com>
>
> > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > ---
> > include/linux/mm.h | 5 +++
> > mm/page_alloc.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 2 files changed, 101 insertions(+)
> >
> > diff --git a/include/linux/mm.h b/include/linux/mm.h
> > index 46b9ac5..76cb433 100644
> > --- a/include/linux/mm.h
> > +++ b/include/linux/mm.h
> > @@ -1835,6 +1835,11 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
> > unsigned long zone_start_pfn, unsigned long *zholes_size);
> > extern void free_initmem(void);
> >
> > +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> > +extern int report_unused_page_block(struct zone *zone, unsigned int order,
> > + unsigned int migratetype,
> > + struct page **page);
> > +#endif
> > /*
> > * Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
> > * into the buddy system. The freed pages will be poisoned with pattern
> > diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> > index 64b7d82..8b3c9dd 100644
> > --- a/mm/page_alloc.c
> > +++ b/mm/page_alloc.c
> > @@ -4753,6 +4753,102 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
> > show_swap_cache_info();
> > }
> >
> > +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> > +
> > +/*
> > + * Heuristically get a page block in the system that is unused.
> > + * It is possible that pages from the page block are used immediately after
> > + * report_unused_page_block() returns. It is the caller's responsibility
> > + * to either detect or prevent the use of such pages.
> > + *
> > + * The free list to check: zone->free_area[order].free_list[migratetype].
> > + *
> > + * If the caller supplied page block (i.e. **page) is on the free list, offer
> > + * the next page block on the list to the caller. Otherwise, offer the first
> > + * page block on the list.
> > + *
> > + * Note: it is not safe for caller to use any pages on the returned
> > + * block or to discard data that is put there after the function returns.
> > + * However, it is safe for caller to discard data that was in one of these
> > + * pages before the function was invoked.
> > + *
> > + * Return 0 when a page block is found on the caller specified free list.
> > + */
> > +int report_unused_page_block(struct zone *zone, unsigned int order,
> > + unsigned int migratetype, struct page **page)
> > +{
> > + struct zone *this_zone;
> > + struct list_head *this_list;
> > + int ret = 0;
> > + unsigned long flags;
> > +
> > + /* Sanity check */
> > + if (zone == NULL || page == NULL || order >= MAX_ORDER ||
> > + migratetype >= MIGRATE_TYPES)
> > + return -EINVAL;
> > +
> > + /* Zone validity check */
> > + for_each_populated_zone(this_zone) {
> > + if (zone == this_zone)
> > + break;
> > + }
> > +
> > + /* Got a non-existent zone from the caller? */
> > + if (zone != this_zone)
> > + return -EINVAL;
>
> Huh, what do you check for here? Why don't you simply
> populated_zone(zone)?
>
> > +
> > + spin_lock_irqsave(&this_zone->lock, flags);
> > +
> > + this_list = &zone->free_area[order].free_list[migratetype];
> > + if (list_empty(this_list)) {
> > + *page = NULL;
> > + ret = 1;
> > + goto out;
> > + }
> > +
> > + /* The caller is asking for the first free page block on the list */
> > + if ((*page) == NULL) {
> > + *page = list_first_entry(this_list, struct page, lru);
> > + ret = 0;
> > + goto out;
> > + }
> > +
> > + /*
> > + * The page block passed from the caller is not on this free list
> > + * anymore (e.g. a 1MB free page block has been split). In this case,
> > + * offer the first page block on the free list that the caller is
> > + * asking for.
> > + */
> > + if (PageBuddy(*page) && order != page_order(*page)) {
> > + *page = list_first_entry(this_list, struct page, lru);
> > + ret = 0;
> > + goto out;
> > + }
> > +
> > + /*
> > + * The page block passed from the caller has been the last page block
> > + * on the list.
> > + */
> > + if ((*page)->lru.next == this_list) {
> > + *page = NULL;
> > + ret = 1;
> > + goto out;
> > + }
> > +
> > + /*
> > + * Finally, fall into the regular case: the page block passed from the
> > + * caller is still on the free list. Offer the next one.
> > + */
> > + *page = list_next_entry((*page), lru);
> > + ret = 0;
> > +out:
> > + spin_unlock_irqrestore(&this_zone->lock, flags);
> > + return ret;
> > +}
> > +EXPORT_SYMBOL(report_unused_page_block);
> > +
> > +#endif
> > +
> > static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
> > {
> > zoneref->zone = zone;
> > --
> > 2.7.4
> >
> > --
> > To unsubscribe, send a message with 'unsubscribe linux-mm' in
> > the body to majordomo@kvack.org. For more info on Linux MM,
> > see: http://www.linux-mm.org/ .
> > Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
>
> --
> Michal Hocko
> SUSE Labs
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v12 7/8] mm: export symbol of next_zone and first_online_pgdat
From: Michal Hocko @ 2017-07-14 12:31 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <1499863221-16206-8-git-send-email-wei.w.wang@intel.com>
On Wed 12-07-17 20:40:20, Wei Wang wrote:
> This patch enables for_each_zone()/for_each_populated_zone() to be
> invoked by a kernel module.
This needs much better justification with an example of who is going to
use these symbols and what for.
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> ---
> mm/mmzone.c | 2 ++
> 1 file changed, 2 insertions(+)
>
> diff --git a/mm/mmzone.c b/mm/mmzone.c
> index a51c0a6..08a2a3a 100644
> --- a/mm/mmzone.c
> +++ b/mm/mmzone.c
> @@ -13,6 +13,7 @@ struct pglist_data *first_online_pgdat(void)
> {
> return NODE_DATA(first_online_node);
> }
> +EXPORT_SYMBOL_GPL(first_online_pgdat);
>
> struct pglist_data *next_online_pgdat(struct pglist_data *pgdat)
> {
> @@ -41,6 +42,7 @@ struct zone *next_zone(struct zone *zone)
> }
> return zone;
> }
> +EXPORT_SYMBOL_GPL(next_zone);
>
> static inline int zref_in_nodemask(struct zoneref *zref, nodemask_t *nodes)
> {
> --
> 2.7.4
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v12 6/8] mm: support reporting free page blocks
From: Michal Hocko @ 2017-07-14 12:30 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, mst, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <1499863221-16206-7-git-send-email-wei.w.wang@intel.com>
On Wed 12-07-17 20:40:19, Wei Wang wrote:
> This patch adds support for reporting blocks of pages on the free list
> specified by the caller.
>
> As pages can leave the free list during this call or immediately
> afterwards, they are not guaranteed to be free after the function
> returns. The only guarantee this makes is that the page was on the free
> list at some point in time after the function has been invoked.
>
> Therefore, it is not safe for caller to use any pages on the returned
> block or to discard data that is put there after the function returns.
> However, it is safe for caller to discard data that was in one of these
> pages before the function was invoked.
I do not understand what is the point of such a function and how it is
used because the patch doesn't give us any user (I haven't checked other
patches yet).
But just from the semantic point of view this sounds like a horrible
idea. The only way to get a free block of pages is to call the page
allocator. I am tempted to give it Nack right on those grounds but I
would like to hear more about what you actually want to achieve.
> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> Signed-off-by: Liang Li <liang.z.li@intel.com>
> ---
> include/linux/mm.h | 5 +++
> mm/page_alloc.c | 96 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 101 insertions(+)
>
> diff --git a/include/linux/mm.h b/include/linux/mm.h
> index 46b9ac5..76cb433 100644
> --- a/include/linux/mm.h
> +++ b/include/linux/mm.h
> @@ -1835,6 +1835,11 @@ extern void free_area_init_node(int nid, unsigned long * zones_size,
> unsigned long zone_start_pfn, unsigned long *zholes_size);
> extern void free_initmem(void);
>
> +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> +extern int report_unused_page_block(struct zone *zone, unsigned int order,
> + unsigned int migratetype,
> + struct page **page);
> +#endif
> /*
> * Free reserved pages within range [PAGE_ALIGN(start), end & PAGE_MASK)
> * into the buddy system. The freed pages will be poisoned with pattern
> diff --git a/mm/page_alloc.c b/mm/page_alloc.c
> index 64b7d82..8b3c9dd 100644
> --- a/mm/page_alloc.c
> +++ b/mm/page_alloc.c
> @@ -4753,6 +4753,102 @@ void show_free_areas(unsigned int filter, nodemask_t *nodemask)
> show_swap_cache_info();
> }
>
> +#if IS_ENABLED(CONFIG_VIRTIO_BALLOON)
> +
> +/*
> + * Heuristically get a page block in the system that is unused.
> + * It is possible that pages from the page block are used immediately after
> + * report_unused_page_block() returns. It is the caller's responsibility
> + * to either detect or prevent the use of such pages.
> + *
> + * The free list to check: zone->free_area[order].free_list[migratetype].
> + *
> + * If the caller supplied page block (i.e. **page) is on the free list, offer
> + * the next page block on the list to the caller. Otherwise, offer the first
> + * page block on the list.
> + *
> + * Note: it is not safe for caller to use any pages on the returned
> + * block or to discard data that is put there after the function returns.
> + * However, it is safe for caller to discard data that was in one of these
> + * pages before the function was invoked.
> + *
> + * Return 0 when a page block is found on the caller specified free list.
> + */
> +int report_unused_page_block(struct zone *zone, unsigned int order,
> + unsigned int migratetype, struct page **page)
> +{
> + struct zone *this_zone;
> + struct list_head *this_list;
> + int ret = 0;
> + unsigned long flags;
> +
> + /* Sanity check */
> + if (zone == NULL || page == NULL || order >= MAX_ORDER ||
> + migratetype >= MIGRATE_TYPES)
> + return -EINVAL;
> +
> + /* Zone validity check */
> + for_each_populated_zone(this_zone) {
> + if (zone == this_zone)
> + break;
> + }
> +
> + /* Got a non-existent zone from the caller? */
> + if (zone != this_zone)
> + return -EINVAL;
Huh, what do you check for here? Why don't you simply
populated_zone(zone)?
> +
> + spin_lock_irqsave(&this_zone->lock, flags);
> +
> + this_list = &zone->free_area[order].free_list[migratetype];
> + if (list_empty(this_list)) {
> + *page = NULL;
> + ret = 1;
> + goto out;
> + }
> +
> + /* The caller is asking for the first free page block on the list */
> + if ((*page) == NULL) {
> + *page = list_first_entry(this_list, struct page, lru);
> + ret = 0;
> + goto out;
> + }
> +
> + /*
> + * The page block passed from the caller is not on this free list
> + * anymore (e.g. a 1MB free page block has been split). In this case,
> + * offer the first page block on the free list that the caller is
> + * asking for.
> + */
> + if (PageBuddy(*page) && order != page_order(*page)) {
> + *page = list_first_entry(this_list, struct page, lru);
> + ret = 0;
> + goto out;
> + }
> +
> + /*
> + * The page block passed from the caller has been the last page block
> + * on the list.
> + */
> + if ((*page)->lru.next == this_list) {
> + *page = NULL;
> + ret = 1;
> + goto out;
> + }
> +
> + /*
> + * Finally, fall into the regular case: the page block passed from the
> + * caller is still on the free list. Offer the next one.
> + */
> + *page = list_next_entry((*page), lru);
> + ret = 0;
> +out:
> + spin_unlock_irqrestore(&this_zone->lock, flags);
> + return ret;
> +}
> +EXPORT_SYMBOL(report_unused_page_block);
> +
> +#endif
> +
> static void zoneref_set_zone(struct zone *zone, struct zoneref *zoneref)
> {
> zoneref->zone = zone;
> --
> 2.7.4
>
> --
> To unsubscribe, send a message with 'unsubscribe linux-mm' in
> the body to majordomo@kvack.org. For more info on Linux MM,
> see: http://www.linux-mm.org/ .
> Don't email: <a href=mailto:"dont@kvack.org"> email@kvack.org </a>
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH v12 5/8] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Wei Wang @ 2017-07-14 7:12 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170713210819-mutt-send-email-mst@kernel.org>
On 07/14/2017 04:19 AM, Michael S. Tsirkin wrote:
> On Thu, Jul 13, 2017 at 03:42:35PM +0800, Wei Wang wrote:
>> On 07/12/2017 09:56 PM, Michael S. Tsirkin wrote:
>>> So the way I see it, there are several issues:
>>>
>>> - internal wait - forces multiple APIs like kick/kick_sync
>>> note how kick_sync can fail but your code never checks return code
>>> - need to re-write the last descriptor - might not work
>>> for alternative layouts which always expose descriptors
>>> immediately
>> Probably it wasn't clear. Please let me explain the two functions here:
>>
>> 1) virtqueue_add_chain_desc(vq, head_id, prev_id,..):
>> grabs a desc from the vq and inserts it to the chain tail (which is indexed
>> by
>> prev_id, probably better to call it tail_id). Then, the new added desc
>> becomes
>> the tail (i.e. the last desc). The _F_NEXT flag is cleared for each desc
>> when it's
>> added to the chain, and set when another desc comes to follow later.
> And this only works if there are multiple rings like
> avail + descriptor ring.
> It won't work e.g. with the proposed new layout where
> writing out a descriptor exposes it immediately.
I think it can support the 1.1 proposal, too. But before getting
into that, I think we first need to deep dive into the implementation
and usage of _first/next/last. The usage would need to lock the vq
from the first to the end (otherwise, the returned info about the number
of available desc in the vq, i.e. num_free, would be invalid):
lock(vq);
add_first();
add_next();
add_last();
unlock(vq);
However, I think the case isn't this simple, since we need to check more
things
after each add_xx() step. For example, if only one entry is available at
the time
we start to use the vq, that is, num_free is 0 after add_first(), we
wouldn't be
able to add_next and add_last. So, it would work like this:
start:
...get free page block..
lock(vq)
retry:
ret = add_first(..,&num_free,);
if(ret == -ENOSPC) {
goto retry;
} else if (!num_free) {
add_chain_head();
unlock(vq);
kick & wait;
goto start;
}
next_one:
...get free page block..
add_next(..,&num_free,);
if (!num_free) {
add_chain_head();
unlock(vq);
kick & wait;
goto start;
} if (num_free == 1) {
...get free page block..
add_last(..);
unlock(vq);
kick & wait;
goto start;
} else {
goto next_one;
}
The above seems unnecessary to me to have three different APIs.
That's the reason to combine them into one virtqueue_add_chain_desc().
-- or, do you have a different thought about using the three APIs?
Implementation Reference:
struct desc_iterator {
unsigned int head;
unsigned int tail;
};
add_first(*vq, *desc_iterator, *num_free, ..)
{
if (vq->vq.num_free < 1)
return -ENOSPC;
get_desc(&desc_id);
desc[desc_id].flag &= ~_F_NEXT;
desc_iterator->head = desc_id
desc_iterator->tail = desc_iterator->head;
*num_free = vq->vq.num_free;
}
add_next(vq, desc_iterator, *num_free,..)
{
get_desc(&desc_id);
desc[desc_id].flag &= ~_F_NEXT;
desc[desc_iterator.tail].next = desc_id;
desc[desc_iterator->tail].flag |= _F_NEXT;
desc_iterator->tail = desc_id;
*num_free = vq->vq.num_free;
}
add_last(vq, desc_iterator,..)
{
get_desc(&desc_id);
desc[desc_id].flag &= ~_F_NEXT;
desc[desc_iterator.tail].next = desc_id;
desc_iterator->tail = desc_id;
add_chain_head(); // put the desc_iterator.head to the ring
}
Best,
Wei
^ permalink raw reply
* Re: [PATCH v12 5/8] virtio-balloon: VIRTIO_BALLOON_F_SG
From: Michael S. Tsirkin @ 2017-07-13 20:19 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <5967246B.9030804@intel.com>
On Thu, Jul 13, 2017 at 03:42:35PM +0800, Wei Wang wrote:
> On 07/12/2017 09:56 PM, Michael S. Tsirkin wrote:
> >
> > So the way I see it, there are several issues:
> >
> > - internal wait - forces multiple APIs like kick/kick_sync
> > note how kick_sync can fail but your code never checks return code
> > - need to re-write the last descriptor - might not work
> > for alternative layouts which always expose descriptors
> > immediately
>
> Probably it wasn't clear. Please let me explain the two functions here:
>
> 1) virtqueue_add_chain_desc(vq, head_id, prev_id,..):
> grabs a desc from the vq and inserts it to the chain tail (which is indexed
> by
> prev_id, probably better to call it tail_id). Then, the new added desc
> becomes
> the tail (i.e. the last desc). The _F_NEXT flag is cleared for each desc
> when it's
> added to the chain, and set when another desc comes to follow later.
And this only works if there are multiple rings like
avail + descriptor ring.
It won't work e.g. with the proposed new layout where
writing out a descriptor exposes it immediately.
> 2) virtqueue_add_chain(vq, head_id,..): expose the chain to the other end.
>
> So, if people want to add a desc and immediately expose it to the other end,
> i.e. build a single desc chain, they can just add and expose:
>
> virtqueue_add_chain_desc(..);
> virtqueue_add_chain(..,head_id);
>
> Would you see any issues here?
The way the new APIs poll used ring internally.
>
> > - some kind of iterator type would be nicer instead of
> > maintaining head/prev explicitly
>
> Why would we need to iterate the chain?
In your patches prev/tail are iterators - they keep track of
where you are in the chain.
> I think it would be simpler to use
> a wrapper struct:
>
> struct virtqueue_desc_chain {
> unsigned int head; // head desc id of the chain
> unsigned int tail; // tail desc id of the chain
> }
>
> The new desc will be put to desc[tail].next, and we don't need to walk
> from the head desc[head].next when inserting a new desc to the chain, right?
>
>
> >
> > As for the use, it would be better to do
> >
> > if (!add_next(vq, ...)) {
> > add_last(vq, ...)
> > kick
> > wait
> > }
>
> "!add_next(vq, ...)" means that the vq is full?
No - it means there's only 1 entry left for the last descriptor.
> If so, what would add_last()
> do then?
>
> > Using VIRTQUEUE_DESC_ID_INIT seems to avoid a branch in the driver, but
> > in fact it merely puts the branch in the virtio code.
> >
>
> Actually it wasn't intended to improve performance. It is used to indicate
> the "init" state
> of the chain. So, when virtqueue_add_chain_desc(, head_id,..) finds head
> id=INIT, it will
> assign the grabbed desc id to &head_id. In some sense, it is equivalent to
> add_first().
>
> Do you have a different opinion here?
>
> Best,
> Wei
>
It is but let's make it explicit here - an API function is better
than a special value.
--
MST
^ permalink raw reply
* Re: [PATCH v12 8/8] virtio-balloon: VIRTIO_BALLOON_F_CMD_VQ
From: Michael S. Tsirkin @ 2017-07-13 17:59 UTC (permalink / raw)
To: Wei Wang
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <59673365.7080408@intel.com>
On Thu, Jul 13, 2017 at 04:46:29PM +0800, Wei Wang wrote:
> On 07/13/2017 08:22 AM, Michael S. Tsirkin wrote:
> > On Wed, Jul 12, 2017 at 08:40:21PM +0800, Wei Wang wrote:
> > > Add a new vq, cmdq, to handle requests between the device and driver.
> > >
> > > This patch implements two commands sent from the device and handled in
> > > the driver.
> > > 1) VIRTIO_BALLOON_CMDQ_REPORT_STATS: this command is used to report
> > > the guest memory statistics to the host. The stats_vq mechanism is not
> > > used when the cmdq mechanism is enabled.
> > > 2) VIRTIO_BALLOON_CMDQ_REPORT_UNUSED_PAGES: this command is used to
> > > report the guest unused pages to the host.
> > >
> > > Since now we have a vq to handle multiple commands, we need to keep only
> > > one vq operation at a time. Here, we change the existing START_USE()
> > > and END_USE() to lock on each vq operation.
> > >
> > > Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> > > Signed-off-by: Liang Li <liang.z.li@intel.com>
> > > ---
> > > drivers/virtio/virtio_balloon.c | 245 ++++++++++++++++++++++++++++++++++--
> > > drivers/virtio/virtio_ring.c | 25 +++-
> > > include/linux/virtio.h | 2 +
> > > include/uapi/linux/virtio_balloon.h | 10 ++
> > > 4 files changed, 265 insertions(+), 17 deletions(-)
> > >
> > > diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
> > > index aa4e7ec..ae91fbf 100644
> > > --- a/drivers/virtio/virtio_balloon.c
> > > +++ b/drivers/virtio/virtio_balloon.c
> > > @@ -54,11 +54,12 @@ static struct vfsmount *balloon_mnt;
> > > struct virtio_balloon {
> > > struct virtio_device *vdev;
> > > - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
> > > + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *cmd_vq;
> > > /* The balloon servicing is delegated to a freezable workqueue. */
> > > struct work_struct update_balloon_stats_work;
> > > struct work_struct update_balloon_size_work;
> > > + struct work_struct cmdq_handle_work;
> > > /* Prevent updating balloon when it is being canceled. */
> > > spinlock_t stop_update_lock;
> > > @@ -90,6 +91,12 @@ struct virtio_balloon {
> > > /* Memory statistics */
> > > struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
> > > + /* Cmdq msg buffer for memory statistics */
> > > + struct virtio_balloon_cmdq_hdr cmdq_stats_hdr;
> > > +
> > > + /* Cmdq msg buffer for reporting ununsed pages */
typo above btw
> > > + struct virtio_balloon_cmdq_hdr cmdq_unused_page_hdr;
> > > +
> > > /* To register callback in oom notifier call chain */
> > > struct notifier_block nb;
> > > };
> > > @@ -485,25 +492,214 @@ static void update_balloon_size_func(struct work_struct *work)
> > > queue_work(system_freezable_wq, work);
> > > }
> > > +static unsigned int cmdq_hdr_add(struct virtqueue *vq,
> > > + struct virtio_balloon_cmdq_hdr *hdr,
> > > + bool in)
> > > +{
> > > + unsigned int id = VIRTQUEUE_DESC_ID_INIT;
> > > + uint64_t hdr_pa = (uint64_t)virt_to_phys((void *)hdr);
> > > +
> > > + virtqueue_add_chain_desc(vq, hdr_pa, sizeof(*hdr), &id, &id, in);
> > > +
> > > + /* Deliver the hdr for the host to send commands. */
> > > + if (in) {
> > > + hdr->flags = 0;
> > > + virtqueue_add_chain(vq, id, 0, NULL, hdr, NULL);
> > > + virtqueue_kick(vq);
> > > + }
> > > +
> > > + return id;
> > > +}
> > > +
> > > +static void cmdq_add_chain_desc(struct virtio_balloon *vb,
> > > + struct virtio_balloon_cmdq_hdr *hdr,
> > > + uint64_t addr,
> > > + uint32_t len,
> > > + unsigned int *head_id,
> > > + unsigned int *prev_id)
> > > +{
> > > +retry:
> > > + if (*head_id == VIRTQUEUE_DESC_ID_INIT) {
> > > + *head_id = cmdq_hdr_add(vb->cmd_vq, hdr, 0);
> > > + *prev_id = *head_id;
> > > + }
> > > +
> > > + virtqueue_add_chain_desc(vb->cmd_vq, addr, len, head_id, prev_id, 0);
> > > + if (*head_id == *prev_id) {
> > That's an ugly way to detect ring full.
>
> It's actually not detecting ring full. I will call it tail_id, instead of
> prev_id.
> So, *head_id == *tail_id is the case that the first desc was just added by
> virtqueue_add_chain_desc().
>
> Best,
> Wei
Oh so it's adding header before each list. Ugh.
I don't think we should stay with this API. It's just too tricky to use.
If we have an API that fails when it can't add descriptors
(you can reserve space for the last descriptor)
the balloon knows whether it's the first descriptor in a chain
and can just use a boolean that tells it whether that is the case.
--
MST
^ permalink raw reply
* [PATCH v5 07/14] drm: cirrus: remove dead code and pointless local lut storage
From: Peter Rosin @ 2017-07-13 16:25 UTC (permalink / raw)
To: linux-kernel
Cc: Jani Nikula, Boris Brezillon, Lionel Landwerlin, David Airlie,
Daniel Vetter, dri-devel, virtualization, Sean Paul,
Daniel Vetter, Dave Airlie, Peter Rosin
In-Reply-To: <20170713162538.22788-1-peda@axentia.se>
The redundant fb helpers .load_lut, .gamma_set and .gamma_get are
no longer used. Remove the dead code and hook up the crtc .gamma_set
to use the crtc gamma_store directly instead of duplicating that
info locally.
Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>
Signed-off-by: Peter Rosin <peda@axentia.se>
---
drivers/gpu/drm/cirrus/cirrus_drv.h | 8 ----
drivers/gpu/drm/cirrus/cirrus_fbdev.c | 2 -
drivers/gpu/drm/cirrus/cirrus_mode.c | 71 ++++++++---------------------------
3 files changed, 16 insertions(+), 65 deletions(-)
diff --git a/drivers/gpu/drm/cirrus/cirrus_drv.h b/drivers/gpu/drm/cirrus/cirrus_drv.h
index 8690352d96f7..be2d7e488062 100644
--- a/drivers/gpu/drm/cirrus/cirrus_drv.h
+++ b/drivers/gpu/drm/cirrus/cirrus_drv.h
@@ -96,7 +96,6 @@
struct cirrus_crtc {
struct drm_crtc base;
- u8 lut_r[256], lut_g[256], lut_b[256];
int last_dpms;
bool enabled;
};
@@ -180,13 +179,6 @@ cirrus_bo(struct ttm_buffer_object *bo)
#define to_cirrus_obj(x) container_of(x, struct cirrus_gem_object, base)
#define DRM_FILE_PAGE_OFFSET (0x100000000ULL >> PAGE_SHIFT)
- /* cirrus_mode.c */
-void cirrus_crtc_fb_gamma_set(struct drm_crtc *crtc, u16 red, u16 green,
- u16 blue, int regno);
-void cirrus_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
- u16 *blue, int regno);
-
-
/* cirrus_main.c */
int cirrus_device_init(struct cirrus_device *cdev,
struct drm_device *ddev,
diff --git a/drivers/gpu/drm/cirrus/cirrus_fbdev.c b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
index 7fa58eeadc9d..1fedab03f659 100644
--- a/drivers/gpu/drm/cirrus/cirrus_fbdev.c
+++ b/drivers/gpu/drm/cirrus/cirrus_fbdev.c
@@ -265,8 +265,6 @@ static int cirrus_fbdev_destroy(struct drm_device *dev,
}
static const struct drm_fb_helper_funcs cirrus_fb_helper_funcs = {
- .gamma_set = cirrus_crtc_fb_gamma_set,
- .gamma_get = cirrus_crtc_fb_gamma_get,
.fb_probe = cirrusfb_create,
};
diff --git a/drivers/gpu/drm/cirrus/cirrus_mode.c b/drivers/gpu/drm/cirrus/cirrus_mode.c
index 53f6f0f84206..a4c4a465b385 100644
--- a/drivers/gpu/drm/cirrus/cirrus_mode.c
+++ b/drivers/gpu/drm/cirrus/cirrus_mode.c
@@ -31,25 +31,6 @@
* This file contains setup code for the CRTC.
*/
-static void cirrus_crtc_load_lut(struct drm_crtc *crtc)
-{
- struct cirrus_crtc *cirrus_crtc = to_cirrus_crtc(crtc);
- struct drm_device *dev = crtc->dev;
- struct cirrus_device *cdev = dev->dev_private;
- int i;
-
- if (!crtc->enabled)
- return;
-
- for (i = 0; i < CIRRUS_LUT_SIZE; i++) {
- /* VGA registers */
- WREG8(PALETTE_INDEX, i);
- WREG8(PALETTE_DATA, cirrus_crtc->lut_r[i]);
- WREG8(PALETTE_DATA, cirrus_crtc->lut_g[i]);
- WREG8(PALETTE_DATA, cirrus_crtc->lut_b[i]);
- }
-}
-
/*
* The DRM core requires DPMS functions, but they make little sense in our
* case and so are just stubs
@@ -330,15 +311,25 @@ static int cirrus_crtc_gamma_set(struct drm_crtc *crtc, u16 *red, u16 *green,
u16 *blue, uint32_t size,
struct drm_modeset_acquire_ctx *ctx)
{
- struct cirrus_crtc *cirrus_crtc = to_cirrus_crtc(crtc);
+ struct drm_device *dev = crtc->dev;
+ struct cirrus_device *cdev = dev->dev_private;
+ u16 *r, *g, *b;
int i;
- for (i = 0; i < size; i++) {
- cirrus_crtc->lut_r[i] = red[i];
- cirrus_crtc->lut_g[i] = green[i];
- cirrus_crtc->lut_b[i] = blue[i];
+ if (!crtc->enabled)
+ return 0;
+
+ r = crtc->gamma_store;
+ g = r + crtc->gamma_size;
+ b = g + crtc->gamma_size;
+
+ for (i = 0; i < CIRRUS_LUT_SIZE; i++) {
+ /* VGA registers */
+ WREG8(PALETTE_INDEX, i);
+ WREG8(PALETTE_DATA, *r++ >> 8);
+ WREG8(PALETTE_DATA, *g++ >> 8);
+ WREG8(PALETTE_DATA, *b++ >> 8);
}
- cirrus_crtc_load_lut(crtc);
return 0;
}
@@ -365,7 +356,6 @@ static const struct drm_crtc_helper_funcs cirrus_helper_funcs = {
.mode_set_base = cirrus_crtc_mode_set_base,
.prepare = cirrus_crtc_prepare,
.commit = cirrus_crtc_commit,
- .load_lut = cirrus_crtc_load_lut,
};
/* CRTC setup */
@@ -373,7 +363,6 @@ static void cirrus_crtc_init(struct drm_device *dev)
{
struct cirrus_device *cdev = dev->dev_private;
struct cirrus_crtc *cirrus_crtc;
- int i;
cirrus_crtc = kzalloc(sizeof(struct cirrus_crtc) +
(CIRRUSFB_CONN_LIMIT * sizeof(struct drm_connector *)),
@@ -387,37 +376,9 @@ static void cirrus_crtc_init(struct drm_device *dev)
drm_mode_crtc_set_gamma_size(&cirrus_crtc->base, CIRRUS_LUT_SIZE);
cdev->mode_info.crtc = cirrus_crtc;
- for (i = 0; i < CIRRUS_LUT_SIZE; i++) {
- cirrus_crtc->lut_r[i] = i;
- cirrus_crtc->lut_g[i] = i;
- cirrus_crtc->lut_b[i] = i;
- }
-
drm_crtc_helper_add(&cirrus_crtc->base, &cirrus_helper_funcs);
}
-/** Sets the color ramps on behalf of fbcon */
-void cirrus_crtc_fb_gamma_set(struct drm_crtc *crtc, u16 red, u16 green,
- u16 blue, int regno)
-{
- struct cirrus_crtc *cirrus_crtc = to_cirrus_crtc(crtc);
-
- cirrus_crtc->lut_r[regno] = red;
- cirrus_crtc->lut_g[regno] = green;
- cirrus_crtc->lut_b[regno] = blue;
-}
-
-/** Gets the color ramps on behalf of fbcon */
-void cirrus_crtc_fb_gamma_get(struct drm_crtc *crtc, u16 *red, u16 *green,
- u16 *blue, int regno)
-{
- struct cirrus_crtc *cirrus_crtc = to_cirrus_crtc(crtc);
-
- *red = cirrus_crtc->lut_r[regno];
- *green = cirrus_crtc->lut_g[regno];
- *blue = cirrus_crtc->lut_b[regno];
-}
-
static void cirrus_encoder_mode_set(struct drm_encoder *encoder,
struct drm_display_mode *mode,
struct drm_display_mode *adjusted_mode)
--
2.11.0
^ permalink raw reply related
* [PATCH v5 00/14] improve the fb_setcmap helper
From: Peter Rosin @ 2017-07-13 16:25 UTC (permalink / raw)
To: linux-kernel
Cc: David Airlie, nouveau, Lionel Landwerlin, virtualization, amd-gfx,
Benjamin Gaignard, Daniel Vetter, Boris Brezillon, Russell King,
Patrik Jakobsson, Ben Skeggs, Dave Airlie, intel-gfx, Jani Nikula,
Sean Paul, dri-devel, Vincent Abriou, Philippe Cornu, Peter Rosin,
Yannick Fertre, Alex Deucher, Christian König
Hi!
While trying to get CLUT support for the atmel_hlcdc driver, and
specifically for the emulated fbdev interface, I received some
push-back that my feeble in-driver attempts should be solved
by the core. This is my attempt to do it right.
I have obviously not tested all of this with more than a compile,
but patches 1 and 3 are enough to make the atmel-hlcdc driver
do what I need. The rest is just lots of removals and cleanup made
possible by the other improvements.
Please test, I would not be surprised if I have fouled up some
bit-manipulation somewhere, or if I have misunderstood something
about atomics...
Changes since v5:
- Rebased onto fresher drm-misc-next.
- Instead of just exporting drm_atomic_replace_propery_blob(), move
it to drm_propery.c, rename it to drm_property_replace_blob() and
change its semantics to return if the blob was replaced.
- Install the same gamma_lut blob for all crtc, regardless of any
variation in the gamma_lut state for the individual crtc prior to
the fbdev setcmap call.
- Add acks from Daniel for patches 4-14.
Changes since v3:
- Rebased onto drm-misc-next and dropped patches 1-3 from v3, since
they are already merged.
- Dropped the v3 patch 4/16 ("drm/color-mgmt: move atomic state/commit
out from .gamma_set") since the atomic setcmap no longer uses
the crtc .gamma_set callback.
- Added patch 1/14 which exports drm_atomic_replace_property_blob...
- ...and patch 2/14 which uses this new export to simplify
drm_atomic_helper_legacy_gamma_set.
- Big changes to patch 3/14 (was 5/16 in v3). It had various locking
issues and the atomic setcmap is rather different.
Changes since v2:
- Added patch 1/16 which factors out pseudo-palette handling.
- Removed the if (cmap->start + cmap->len < cmap->start)
sanity check on the assumption that the fbdev core handles it.
- Added patch 4/16 which factors out atomic state and commit
handling from drm_atomic_helper_legacy_gamma_set to
drm_mode_gamma_set_ioctl.
- Do one atomic commit for all affected crtc.
- Removed a now obsolete note in include/drm/drm_crtc.h (ammended
the last patch).
- Cc list is getting long, so I have redused the list for the
individual patches. If you would like to get the full series
(or nothing at all) for the next round (if that is needed) just
say so.
Changes since v1:
- Rebased to next-20170621
- Split 1/11 into a preparatory patch, a cleanup patch and then
the meat in 3/14.
- Handle pseudo-palette for FB_VISUAL_TRUECOLOR.
- Removed the empty .gamma_get/.gamma_set fb helpers from the
armada driver that I had somehow managed to ignore but which
0day found real quick.
- Be less judgemental on drivers only providing .gamma_get and
.gamma_set, but no .load_lut. That's actually a valid thing
to do if you only need pseudo-palette for FB_VISUAL_TRUECOLOR.
- Add a comment about colliding bitfields in the nouveau driver.
- Remove gamma_set/gamma_get declarations from the radeon driver
(the definitions were removed in v1).
Cheers,
peda
Peter Rosin (14):
drm: rename, adjust and export drm_atomic_replace_property_blob
drm/atomic-helper: update lut props directly in ..._legacy_gamma_set
drm/fb-helper: separate the fb_setcmap helper into atomic and legacy
paths
drm: amd: remove dead code and pointless local lut storage
drm: armada: remove dead empty functions
drm: ast: remove dead code and pointless local lut storage
drm: cirrus: remove dead code and pointless local lut storage
drm: gma500: remove dead code and pointless local lut storage
drm: i915: remove dead code and pointless local lut storage
drm: mgag200: remove dead code and pointless local lut storage
drm: nouveau: remove dead code and pointless local lut storage
drm: radeon: remove dead code and pointless local lut storage
drm: stm: remove dead code and pointless local lut storage
drm: remove unused and redundant callbacks
drivers/gpu/drm/amd/amdgpu/amdgpu_fb.c | 24 ---
drivers/gpu/drm/amd/amdgpu/amdgpu_mode.h | 1 -
drivers/gpu/drm/amd/amdgpu/dce_v10_0.c | 27 +---
drivers/gpu/drm/amd/amdgpu/dce_v11_0.c | 27 +---
drivers/gpu/drm/amd/amdgpu/dce_v6_0.c | 27 +---
drivers/gpu/drm/amd/amdgpu/dce_v8_0.c | 27 +---
drivers/gpu/drm/amd/amdgpu/dce_virtual.c | 23 ---
drivers/gpu/drm/armada/armada_crtc.c | 10 --
drivers/gpu/drm/armada/armada_crtc.h | 2 -
drivers/gpu/drm/armada/armada_fbdev.c | 2 -
drivers/gpu/drm/ast/ast_drv.h | 1 -
drivers/gpu/drm/ast/ast_fb.c | 20 ---
drivers/gpu/drm/ast/ast_mode.c | 26 +---
drivers/gpu/drm/cirrus/cirrus_drv.h | 8 -
drivers/gpu/drm/cirrus/cirrus_fbdev.c | 2 -
drivers/gpu/drm/cirrus/cirrus_mode.c | 71 ++-------
drivers/gpu/drm/drm_atomic.c | 30 +---
drivers/gpu/drm/drm_atomic_helper.c | 20 +--
drivers/gpu/drm/drm_fb_helper.c | 231 +++++++++++++++++++---------
drivers/gpu/drm/drm_property.c | 23 +++
drivers/gpu/drm/gma500/framebuffer.c | 22 ---
drivers/gpu/drm/gma500/gma_display.c | 32 ++--
drivers/gpu/drm/gma500/psb_intel_display.c | 7 +-
drivers/gpu/drm/gma500/psb_intel_drv.h | 1 -
drivers/gpu/drm/i915/intel_drv.h | 1 -
drivers/gpu/drm/i915/intel_fbdev.c | 31 ----
drivers/gpu/drm/mgag200/mgag200_drv.h | 5 -
drivers/gpu/drm/mgag200/mgag200_fb.c | 2 -
drivers/gpu/drm/mgag200/mgag200_mode.c | 62 ++------
drivers/gpu/drm/nouveau/dispnv04/crtc.c | 26 ++--
drivers/gpu/drm/nouveau/nouveau_crtc.h | 3 -
drivers/gpu/drm/nouveau/nouveau_fbcon.c | 22 ---
drivers/gpu/drm/nouveau/nv50_display.c | 40 ++---
drivers/gpu/drm/radeon/atombios_crtc.c | 1 -
drivers/gpu/drm/radeon/radeon_connectors.c | 7 +-
drivers/gpu/drm/radeon/radeon_display.c | 71 ++++-----
drivers/gpu/drm/radeon/radeon_fb.c | 2 -
drivers/gpu/drm/radeon/radeon_legacy_crtc.c | 1 -
drivers/gpu/drm/radeon/radeon_mode.h | 4 -
drivers/gpu/drm/stm/ltdc.c | 12 --
drivers/gpu/drm/stm/ltdc.h | 1 -
include/drm/drm_crtc.h | 8 -
include/drm/drm_fb_helper.h | 32 ----
include/drm/drm_modeset_helper_vtables.h | 16 --
include/drm/drm_property.h | 2 +
45 files changed, 323 insertions(+), 690 deletions(-)
--
2.11.0
^ permalink raw reply
* Re: [PATCH v12 8/8] virtio-balloon: VIRTIO_BALLOON_F_CMD_VQ
From: Wei Wang @ 2017-07-13 8:46 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170713032207-mutt-send-email-mst@kernel.org>
On 07/13/2017 08:22 AM, Michael S. Tsirkin wrote:
> On Wed, Jul 12, 2017 at 08:40:21PM +0800, Wei Wang wrote:
>> Add a new vq, cmdq, to handle requests between the device and driver.
>>
>> This patch implements two commands sent from the device and handled in
>> the driver.
>> 1) VIRTIO_BALLOON_CMDQ_REPORT_STATS: this command is used to report
>> the guest memory statistics to the host. The stats_vq mechanism is not
>> used when the cmdq mechanism is enabled.
>> 2) VIRTIO_BALLOON_CMDQ_REPORT_UNUSED_PAGES: this command is used to
>> report the guest unused pages to the host.
>>
>> Since now we have a vq to handle multiple commands, we need to keep only
>> one vq operation at a time. Here, we change the existing START_USE()
>> and END_USE() to lock on each vq operation.
>>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
>> Signed-off-by: Liang Li <liang.z.li@intel.com>
>> ---
>> drivers/virtio/virtio_balloon.c | 245 ++++++++++++++++++++++++++++++++++--
>> drivers/virtio/virtio_ring.c | 25 +++-
>> include/linux/virtio.h | 2 +
>> include/uapi/linux/virtio_balloon.h | 10 ++
>> 4 files changed, 265 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/virtio/virtio_balloon.c b/drivers/virtio/virtio_balloon.c
>> index aa4e7ec..ae91fbf 100644
>> --- a/drivers/virtio/virtio_balloon.c
>> +++ b/drivers/virtio/virtio_balloon.c
>> @@ -54,11 +54,12 @@ static struct vfsmount *balloon_mnt;
>>
>> struct virtio_balloon {
>> struct virtio_device *vdev;
>> - struct virtqueue *inflate_vq, *deflate_vq, *stats_vq;
>> + struct virtqueue *inflate_vq, *deflate_vq, *stats_vq, *cmd_vq;
>>
>> /* The balloon servicing is delegated to a freezable workqueue. */
>> struct work_struct update_balloon_stats_work;
>> struct work_struct update_balloon_size_work;
>> + struct work_struct cmdq_handle_work;
>>
>> /* Prevent updating balloon when it is being canceled. */
>> spinlock_t stop_update_lock;
>> @@ -90,6 +91,12 @@ struct virtio_balloon {
>> /* Memory statistics */
>> struct virtio_balloon_stat stats[VIRTIO_BALLOON_S_NR];
>>
>> + /* Cmdq msg buffer for memory statistics */
>> + struct virtio_balloon_cmdq_hdr cmdq_stats_hdr;
>> +
>> + /* Cmdq msg buffer for reporting ununsed pages */
>> + struct virtio_balloon_cmdq_hdr cmdq_unused_page_hdr;
>> +
>> /* To register callback in oom notifier call chain */
>> struct notifier_block nb;
>> };
>> @@ -485,25 +492,214 @@ static void update_balloon_size_func(struct work_struct *work)
>> queue_work(system_freezable_wq, work);
>> }
>>
>> +static unsigned int cmdq_hdr_add(struct virtqueue *vq,
>> + struct virtio_balloon_cmdq_hdr *hdr,
>> + bool in)
>> +{
>> + unsigned int id = VIRTQUEUE_DESC_ID_INIT;
>> + uint64_t hdr_pa = (uint64_t)virt_to_phys((void *)hdr);
>> +
>> + virtqueue_add_chain_desc(vq, hdr_pa, sizeof(*hdr), &id, &id, in);
>> +
>> + /* Deliver the hdr for the host to send commands. */
>> + if (in) {
>> + hdr->flags = 0;
>> + virtqueue_add_chain(vq, id, 0, NULL, hdr, NULL);
>> + virtqueue_kick(vq);
>> + }
>> +
>> + return id;
>> +}
>> +
>> +static void cmdq_add_chain_desc(struct virtio_balloon *vb,
>> + struct virtio_balloon_cmdq_hdr *hdr,
>> + uint64_t addr,
>> + uint32_t len,
>> + unsigned int *head_id,
>> + unsigned int *prev_id)
>> +{
>> +retry:
>> + if (*head_id == VIRTQUEUE_DESC_ID_INIT) {
>> + *head_id = cmdq_hdr_add(vb->cmd_vq, hdr, 0);
>> + *prev_id = *head_id;
>> + }
>> +
>> + virtqueue_add_chain_desc(vb->cmd_vq, addr, len, head_id, prev_id, 0);
>> + if (*head_id == *prev_id) {
> That's an ugly way to detect ring full.
It's actually not detecting ring full. I will call it tail_id, instead
of prev_id.
So, *head_id == *tail_id is the case that the first desc was just added by
virtqueue_add_chain_desc().
Best,
Wei
^ permalink raw reply
* Re: [virtio-dev] Re: [PATCH v12 7/8] mm: export symbol of next_zone and first_online_pgdat
From: Wei Wang @ 2017-07-13 8:41 UTC (permalink / raw)
To: Michael S. Tsirkin
Cc: aarcange, virtio-dev, kvm, qemu-devel, amit.shah,
liliang.opensource, linux-kernel, virtualization, linux-mm,
yang.zhang.wz, quan.xu, cornelia.huck, pbonzini, akpm, mgorman
In-Reply-To: <20170713031526-mutt-send-email-mst@kernel.org>
On 07/13/2017 08:16 AM, Michael S. Tsirkin wrote:
> On Wed, Jul 12, 2017 at 08:40:20PM +0800, Wei Wang wrote:
>> This patch enables for_each_zone()/for_each_populated_zone() to be
>> invoked by a kernel module.
> ... for use by virtio balloon.
With this patch, other kernel modules can also use the for_each_zone().
Would it be better to claim it broader?
>
>> Signed-off-by: Wei Wang <wei.w.wang@intel.com>
> balloon seems to only use
> + for_each_populated_zone(zone)
> + for_each_migratetype_order(order, type)
>
Yes. using for_each_populated_zone() requires the following export.
Best,
Wei
>> ---
>> mm/mmzone.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/mm/mmzone.c b/mm/mmzone.c
>> index a51c0a6..08a2a3a 100644
>> --- a/mm/mmzone.c
>> +++ b/mm/mmzone.c
>> @@ -13,6 +13,7 @@ struct pglist_data *first_online_pgdat(void)
>> {
>> return NODE_DATA(first_online_node);
>> }
>> +EXPORT_SYMBOL_GPL(first_online_pgdat);
>>
>> struct pglist_data *next_online_pgdat(struct pglist_data *pgdat)
>> {
>> @@ -41,6 +42,7 @@ struct zone *next_zone(struct zone *zone)
>> }
>> return zone;
>> }
>> +EXPORT_SYMBOL_GPL(next_zone);
>>
>> static inline int zref_in_nodemask(struct zoneref *zref, nodemask_t *nodes)
>> {
>> --
>> 2.7.4
> ---------------------------------------------------------------------
> 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
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