* Re: [PATCH] perf/x86/intel: Fix unwind errors from PEBS entries (mk-II)
From: Peter Zijlstra @ 2018-07-23 14:14 UTC (permalink / raw)
To: Josh Poimboeuf; +Cc: Ingo Molnar, Prashant Bhole, linux-kernel
In-Reply-To: <20180723133006.7mkzrdeaa4jo56md@treble>
On Mon, Jul 23, 2018 at 08:30:06AM -0500, Josh Poimboeuf wrote:
> On Thu, Jul 19, 2018 at 11:19:54PM +0200, Peter Zijlstra wrote:
> > --- a/include/uapi/linux/perf_event.h
> > +++ b/include/uapi/linux/perf_event.h
> > @@ -143,6 +143,8 @@ enum perf_event_sample_format {
> > PERF_SAMPLE_PHYS_ADDR = 1U << 19,
> >
> > PERF_SAMPLE_MAX = 1U << 20, /* non-ABI */
> > +
> > + __PERF_SAMPLE_CALLCHAIN_EARLY = 1UL << 63,
>
> Given the kbuild test robot errors for building on 32-bit, looks like
> the 63 needs to be changed to 31 so it fits in an int?
I made it 1ULL << 63. The actual field (perf_event_attr::sample_type is
u64).
^ permalink raw reply
* [PATCH 2/2] blktests: add test for ANA state transition
From: Hannes Reinecke @ 2018-07-23 14:14 UTC (permalink / raw)
In-Reply-To: <BYAPR04MB450272E60922653AFF77D47B86500@BYAPR04MB4502.namprd04.prod.outlook.com>
On 07/21/2018 11:29 PM, Chaitanya Kulkarni wrote:
> From: linux-block-owner at vger.kernel.org <linux-block-owner at vger.kernel.org> on behalf of Hannes Reinecke <hare at suse.de>
> Sent: Tuesday, July 17, 2018 6:31 AM
> To: Omar Sandoval
> Cc: Christoph Hellwig; Sagi Grimberg; Keith Busch; linux-nvme at lists.infradead.org; linux-block at vger.kernel.org; Hannes Reinecke; Hannes Reinecke
> Subject: [PATCH 2/2] blktests: add test for ANA state transition
>
>
> Signed-off-by: Hannes Reinecke <hare at suse.com>
> ---
> ?tests/nvme/014???? | 158 +++++++++++++++++++++++++++++++++++++++++++++++++++++
> ?tests/nvme/014.out |? 17 ++++++
> ?2 files changed, 175 insertions(+)
> ?create mode 100755 tests/nvme/014
> ?create mode 100644 tests/nvme/014.out
>
> diff --git a/tests/nvme/014 b/tests/nvme/014
> new file mode 100755
> index 0000000..4b57229
> --- /dev/null
> +++ b/tests/nvme/014
> @@ -0,0 +1,158 @@
> +#!/bin/bash
> +#
> +# Regression test for ANA base support
> +#
> +# Copyright (C) 2018 Hannes Reinecke
> +#
> +# This program is free software: you can redistribute it and/or modify
> +# it under the terms of the GNU General Public License as published by
> +# the Free Software Foundation, either version 3 of the License, or
> +# (at your option) any later version.
> +#
> +# This program is distributed in the hope that it will be useful,
> +# but WITHOUT ANY WARRANTY; without even the implied warranty of
> +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.? See the
> +# GNU General Public License for more details.
> +#
> +# You should have received a copy of the GNU General Public License
> +# along with this program.? If not, see <http://www.gnu.org/licenses/>.
>
>
> Licenses - GNU Project - Free Software Foundation
> www.gnu.org
> Published software should be free software.To make it free software, you need to release it under
> a free software license. We normally use the GNU General Public License (GNU GPL), specifying version 3
> or any later version, but occasionally we use other free software licenses.
>
I don't mind, I just copied it over from testcase 10...
> +
> +. tests/nvme/rc
> +
> +DESCRIPTION="test ANA optimized/transitioning/inaccessible support"
> +QUICK=1
> +
> +switch_nvmet_anagroup() {
> +?????? local port1="$1"
> +?????? local port2="$2"
> +?????? local mode="$3"
> +
> +?????? echo "ANA state ${mode}"
> +
> +?????? if [ "${mode}" = "change" ] ; then
> +?????????????? _set_nvmet_anagroup_state "${port1}" "1" "change"
> +?????????????? _set_nvmet_anagroup_state "${port1}" "2" "change"
> +?????????????? _set_nvmet_anagroup_state "${port2}" "1" "change"
> +?????????????? _set_nvmet_anagroup_state "${port2}" "2" "change"
> +?????? elif [ "${mode}" = "failover" ] ; then
> +?????????????? _set_nvmet_anagroup_state "${port1}" "1" "inaccessible"
> +?????????????? _set_nvmet_anagroup_state "${port1}" "2" "optimized"
> +?????????????? _set_nvmet_anagroup_state "${port2}" "1" "optimized"
> +?????????????? _set_nvmet_anagroup_state "${port2}" "2" "inaccessible"
> +?????? else
> +?????????????? _set_nvmet_anagroup_state "${port1}" "1" "optimized"
> +?????????????? _set_nvmet_anagroup_state "${port1}" "2" "inaccessible"
> +?????????????? _set_nvmet_anagroup_state "${port2}" "1" "inaccessible"
> +?????????????? _set_nvmet_anagroup_state "${port2}" "2" "optimized"
> +?????? fi
> +}
> +
> +_display_ana_state() {
> +?????? local grpid state
> [CK] Newliine here ?
Okay
> +?????? for nvme in /sys/class/nvme/* ; do
> +?????????????? for c in ${nvme}/nvme* ; do
> +?????????????????????? if [ ! -d ${c} ] ; then
> +?????????????????????????????? echo "${nvme##*/}: ANA disabled"
> +?????????????????????????????? continue
> +?????????????????????? fi
> +?????????????????????? grpid="$(cat "${c}/ana_grpid")"
> +?????????????????????? state="$(cat "${c}/ana_state")"
> +?????????????????????? echo "${c##*/}: grpid ${grpid} state ${state}"
> +?????????????? done
> +?????? done
> +}
>
> I think we need to move above functions to the ${BLKTESTS_HOME}/tests/nvme/rc.
Okay, can do.
> +
> +_switch_ana_states() {
> +?????? local port1=$1
> +?????? local port2=$2
> +
> [CK] Please remove the extra line.
> +}
> [CK] I was not able to find a caller for above function, I'm I missing something ?
> +
Yeah, that's an older function which got left over during refactoring.
> +requires() {
> +?????? _have_program nvme && _have_module nvme-loop && _have_module loop && \
> +?????????????? _have_configfs && _have_fio
> [CK] Missing nvmet module from the above list.
> +}
>
> Can we split following test function into small routines, it will be easier to review and
> maintain?
>
> +
> +test() {
> +?????? echo "Running ${TEST_NAME}"
> +
> +?????? modprobe nvmet
> +?????? modprobe nvme-loop
> +
> +?????? local port1
> +?????? port1="$(_create_nvmet_port "loop")"
> [CK] Can we initialize variables at the time of declaration or after declaration of all the
> variables ?
Sure. But we should do it consistently; the older tests also do not
follow these rules...
> +?????? ag1="$(_create_nvmet_anagroup "${port1}")"
> [CK] Not sure if we need ag1 variable.
>
Yeah, it's more for symmetry than anything else.
> +
> +?????? local port2
> [CK] Can we plese declare all the variable at the top please see tests/nvme/006-013
> to maintain uniform style ?
Sure. But see above; not all tests follow the same scheme.
> +?????? port2="$(_create_nvmet_port "loop")"
> +?????? ag2="$(_create_nvmet_anagroup "${port2}")"
> +
> +?????? truncate -s 1G "$TMPDIR/img1"
> +
> +?????? local loop_dev1
> +?????? loop_dev1="$(losetup -f --show "$TMPDIR/img1")"
> +
> +?????? _create_nvmet_subsystem "blktests-subsystem-1" "${loop_dev1}" \
> +?????????????? "91fdba0d-f87b-4c25-b80f-db7be1418b9e" "1"
> +
> +?????? truncate -s 1G "$TMPDIR/img2"
> +
> [CK] Do we really need img2 ? is it possible to use the img1 as we are not doing data verification ?
Yes. There's an issue with the current implementation which fails to
signal a namespace state change for the _second_ namepsace.
(That's the one patch I've sent to the current ANA implementation from hch)
> +?????? local loop_dev2
> +?????? loop_dev2="$(losetup -f --show "$TMPDIR/img2")"
> +
> +?????? _create_nvmet_ns "blktests-subsystem-1" "2" "${loop_dev2}" \
> +?????????????? "9aed0138-bfd9-46f5-92ac-24c70377fd49" "2"
> +
> +?????? _add_nvmet_subsys_to_port "${port1}" "blktests-subsystem-1"
> +?????? _add_nvmet_subsys_to_port "${port2}" "blktests-subsystem-1"
> +
> +?????? switch_nvmet_anagroup "${port1}" "${port2}" failback
> +
> +?????? nvme connect -t loop -a "${port1}" -n blktests-subsystem-1
> +?????? nvme connect -t loop -a "${port2}" -n blktests-subsystem-1
> +
> +?????? _display_ana_state
> +
> +?????? _run_fio_rand_io --size=256m --filename="/dev/nvme0n1" &
> [CK] Hardcoded value for ns, on my machine I have PCIe ns with "/dev/nvme0n1",
> can we please make sure this is the right block device for the namespace that we just
> configured ?
Yeah, I know. Will see what I can do.
> +?????? trap "kill $!" EXIT
> +
> +?????? sleep 10
> +
> +?????? switch_nvmet_anagroup "${port1}" "${port2}" "change"
> +
> +?????? # Insert a delay to allow the AEN to be processed
> +?????? sleep 1
> +
> +?????? _display_ana_state
> +
> +?????? sleep 6
> +
> +?????? switch_nvmet_anagroup "${port1}" "${port2}" "failover"
> +
> +?????? # Insert a delay to allow the AEN to be processed
> +?????? sleep 1
> +
> +?????? _display_ana_state
> +
> +?????? wait
> +?????? trap - EXIT
> +
> +?????? nvme disconnect -n blktests-subsystem-1
> +
> +?????? _remove_nvmet_subsystem_from_port "${port1}" "blktests-subsystem-1"
> +?????? _remove_nvmet_subsystem_from_port "${port2}" "blktests-subsystem-1"
> +?????? _remove_nvmet_ns "blktests-subsystem-1" "2"
> +?????? _remove_nvmet_subsystem "blktests-subsystem-1"
> +?????? _remove_nvmet_anagroup "${port1}" "2"
> +?????? _remove_nvmet_port "${port1}"
> +?????? _remove_nvmet_anagroup "${port2}" "2"
> [CK] Plase make the groupid local varible instead of hard coding "2".
Hmm. Not easy. But I'll have a look.
Thanks for the review.
Cheers,
Hannes
^ permalink raw reply
* Re: [PATCH V2 0/6] mailbox: ti-msgmgr: Add support for AM654 Secure Proxy
From: Nishanth Menon @ 2018-07-23 14:14 UTC (permalink / raw)
To: Jassi Brar
Cc: Mark Rutland, Rob Herring, Suman Anna, Tero Kristo,
Linux Kernel Mailing List, Devicetree List,
, linux-arm-kernel@lists.infradead.org, linux-mediatek@lists.infradead.org, srv_heupstream
In-Reply-To: <CABb+yY1_SBTDPyUNGSr8=tpU4oObz060nJ3NrgR8JyQye23cfg@mail.gmail.com>
On 13:56-20180723, Jassi Brar wrote:
> On Mon, Jul 23, 2018 at 6:59 PM, Nishanth Menon <nm@ti.com> wrote:
[...]
> >> .../bindings/mailbox/ti,secure-proxy.txt | 50 +++
> >> drivers/mailbox/ti-msgmgr.c | 353 +++++++++++++++++----
> >> 2 files changed, 336 insertions(+), 67 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/mailbox/ti,secure-proxy.txt
> >
> >
> > Jassi,
> >
> > I dont see this in next-20180723. You'd asked me to hold off on pings
> > https://lkml.org/lkml/2017/12/16/353 -> so, held off as much as I could
> > before wondering on the status.
> >
> I didn't ask you to not ping me. I said I didn't miss your patchset. I
> still didn't.
>
> Especially for purely platform specific changes, that I don't have
> knowledge about, I leave them to cook in public as long as possible,
> before pulling them in around rc7.
Understood. Thanks.
> You submitted V2, 7days ago only and we are at rc6 today. So chill.
v2 is just wording of mailbox node example change recommended by Rob.
I was hoping to get any further changes prior to reposting a v2.. but
anyways.. Looks all good..
--
Regards,
Nishanth Menon
^ permalink raw reply
* [PATCH V2 0/6] mailbox: ti-msgmgr: Add support for AM654 Secure Proxy
From: Nishanth Menon @ 2018-07-23 14:14 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <CABb+yY1_SBTDPyUNGSr8=tpU4oObz060nJ3NrgR8JyQye23cfg@mail.gmail.com>
On 13:56-20180723, Jassi Brar wrote:
> On Mon, Jul 23, 2018 at 6:59 PM, Nishanth Menon <nm@ti.com> wrote:
[...]
> >> .../bindings/mailbox/ti,secure-proxy.txt | 50 +++
> >> drivers/mailbox/ti-msgmgr.c | 353 +++++++++++++++++----
> >> 2 files changed, 336 insertions(+), 67 deletions(-)
> >> create mode 100644 Documentation/devicetree/bindings/mailbox/ti,secure-proxy.txt
> >
> >
> > Jassi,
> >
> > I dont see this in next-20180723. You'd asked me to hold off on pings
> > https://lkml.org/lkml/2017/12/16/353 -> so, held off as much as I could
> > before wondering on the status.
> >
> I didn't ask you to not ping me. I said I didn't miss your patchset. I
> still didn't.
>
> Especially for purely platform specific changes, that I don't have
> knowledge about, I leave them to cook in public as long as possible,
> before pulling them in around rc7.
Understood. Thanks.
> You submitted V2, 7days ago only and we are at rc6 today. So chill.
v2 is just wording of mailbox node example change recommended by Rob.
I was hoping to get any further changes prior to reposting a v2.. but
anyways.. Looks all good..
--
Regards,
Nishanth Menon
^ permalink raw reply
* Re: [RFC PATCH] sched/deadline: sched_getattr() returns absolute dl-task information
From: Peter Zijlstra @ 2018-07-23 14:13 UTC (permalink / raw)
To: Patrick Bellasi
Cc: Alessio Balsini, linux-kernel, Joel Fernandes, Juri Lelli,
Tommaso Cucinotta, Luca Abeni, Claudio Scordino,
Daniel Bristot de Oliveira, Ingo Molnar
In-Reply-To: <20180723124946.GA2683@e110439-lin>
On Mon, Jul 23, 2018 at 01:49:46PM +0100, Patrick Bellasi wrote:
> On 23-Jul 11:49, Peter Zijlstra wrote:
>
> [...]
>
> > > -void __getparam_dl(struct task_struct *p, struct sched_attr *attr)
> > > +void __getparam_dl(struct task_struct *p, struct sched_attr *attr,
> > > + unsigned int flags)
> > > {
> > > struct sched_dl_entity *dl_se = &p->dl;
> > >
> > > attr->sched_priority = p->rt_priority;
> > > - attr->sched_runtime = dl_se->dl_runtime;
> > > - attr->sched_deadline = dl_se->dl_deadline;
> > > +
> > > + if (flags & SCHED_GETATTR_FLAGS_DL_ABSOLUTE) {
> > > + /*
> > > + * If the task is not running, its runtime is already
> > > + * properly accounted. Otherwise, update clocks and the
> > > + * statistics for the task.
> > > + */
> > > + if (task_running(task_rq(p), p)) {
> > > + struct rq_flags rf;
> > > + struct rq *rq;
> > > +
> > > + rq = task_rq_lock(p, &rf);
> > > + sched_clock_tick();
> > > + update_rq_clock(rq);
> > > + task_tick_dl(rq, p, 0);
> >
> > Do we really want task_tick_dl() here, or update_curr_dl()?
>
> I think this was to cover the case of a syscall being called while the
> task is running and we are midway between two ticks...
Sure, I know what it's there for, just saying that update_curr_dl()
would've updated the accounting as well. Calling tick stuff from !tick
context is a wee bit dodgy.
> > Also, who says the task still is dl ? :-)
>
> Good point, but what should be the rule in general for these cases?
>
> We already have:
>
> SYSCALL_DEFINE4(sched_getattr())
> ....
> if (task_has_dl_policy(p))
> __getparam_dl(p, &attr);
>
> which is also potentially racy, isn't it?
Yes, but only in so far as that the whole syscall is racy
per-definition. EVen if we'd lock the rq and get the absolute accurate
values, everything can change the moment we release the locks and return
to userspace again.
> Or just make the syscall return the most updated metrics for all the
> scheduling classes since we cannot grant the user anything about what
> the task will be once we return to userspace?
This.
> > > + task_rq_unlock(rq, p, &rf);
> > > + }
> > > +
> > > + /*
> > > + * If the task is throttled, this value could be negative,
> > > + * but sched_runtime is unsigned.
> > > + */
> > > + attr->sched_runtime = dl_se->runtime <= 0 ? 0 : dl_se->runtime;
> > > + attr->sched_deadline = dl_se->deadline;
> >
> > This is all very racy..
> >
> > Even if the task wasn't running when you did the task_running() test, it
> > could be running now. And if it was running, it might not be running
> > anymore by the time you've acquired the rq->lock.
>
> Which means we should use something like:
>
> if (flags & SCHED_GETATTR_FLAGS_DL_ABSOLUTE) {
> /* Lock the task and the RQ before any other check and upate */
> rq = task_rq_lock(p, &rf);
>
> /* Check the task is still DL ?*/
>
> /* Update task stats */
>
> task_rq_unlock(rq, p, &rf);
> }
>
> right?
Yeah, something along those lines.
> If that's better, then we should probably even better move the
> task_rq_lock at the beginning of SYSCALL_DEFINE4(sched_getattr()) ?
Hurm.. yes, we should probably have the has_dl_policy test under the
lock too. Which is really annoying, because this basically turns a
lockless syscall into locked one.
Another method would be to have __getparam_dl() 'fail' and retry if it
finds !has_dl_policy() once we have the lock. That would retain the
lockless nature for all current use-cases and only incur the locking
overhead for this new case.
^ permalink raw reply
* [virtio-dev] Re: [PATCH v36 2/5] virtio_balloon: replace oom notifier with shrinker
From: Michael S. Tsirkin @ 2018-07-23 14:13 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, linux-mm, mhocko,
akpm, torvalds, pbonzini, liliang.opensource, yang.zhang.wz,
quan.xu0, nilal, riel, peterx
In-Reply-To: <5B55AE56.5030404@intel.com>
On Mon, Jul 23, 2018 at 06:30:46PM +0800, Wei Wang wrote:
> On 07/22/2018 10:48 PM, Michael S. Tsirkin wrote:
> > On Fri, Jul 20, 2018 at 04:33:02PM +0800, Wei Wang wrote:
> > > +static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
> > > + struct shrink_control *sc)
> > > +{
> > > + unsigned long pages_to_free = balloon_pages_to_shrink,
> > > + pages_freed = 0;
> > > + struct virtio_balloon *vb = container_of(shrinker,
> > > + struct virtio_balloon, shrinker);
> > > +
> > > + /*
> > > + * One invocation of leak_balloon can deflate at most
> > > + * VIRTIO_BALLOON_ARRAY_PFNS_MAX balloon pages, so we call it
> > > + * multiple times to deflate pages till reaching
> > > + * balloon_pages_to_shrink pages.
> > > + */
> > > + while (vb->num_pages && pages_to_free) {
> > > + pages_to_free = balloon_pages_to_shrink - pages_freed;
> > > + pages_freed += leak_balloon(vb, pages_to_free);
> > > + }
> > > + update_balloon_size(vb);
> > Are you sure that this is never called if count returned 0?
>
> Yes. Please see do_shrink_slab, it just returns if count is 0.
>
> >
> > > +
> > > + return pages_freed / VIRTIO_BALLOON_PAGES_PER_PAGE;
> > > +}
> > > +
> > > +static unsigned long virtio_balloon_shrinker_count(struct shrinker *shrinker,
> > > + struct shrink_control *sc)
> > > +{
> > > + struct virtio_balloon *vb = container_of(shrinker,
> > > + struct virtio_balloon, shrinker);
> > > +
> > > + /*
> > > + * We continue to use VIRTIO_BALLOON_F_DEFLATE_ON_OOM to handle the
> > > + * case when shrinker needs to be invoked to relieve memory pressure.
> > > + */
> > > + if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > > + return 0;
> > So why not skip notifier registration when deflate on oom
> > is clear?
>
> Sounds good, thanks.
>
>
> > vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops;
> > #endif
> > + err = virtio_balloon_register_shrinker(vb);
> > + if (err)
> > + goto out_del_vqs;
> > So we can get scans before device is ready. Leak will fail
> > then. Why not register later after device is ready?
>
> Probably no.
>
> - it would be better not to set device ready when register_shrinker failed.
That's very rare so I won't be too worried.
> - When the device isn't ready, ballooning won't happen, that is,
> vb->num_pages will be 0, which results in shrinker_count=0 and shrinker_scan
> won't be called.
>
> So I think it would be better to have shrinker registered before
> device_ready.
>
> Best,
> Wei
---------------------------------------------------------------------
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 v36 2/5] virtio_balloon: replace oom notifier with shrinker
From: Michael S. Tsirkin @ 2018-07-23 14:13 UTC (permalink / raw)
To: Wei Wang
Cc: virtio-dev, linux-kernel, virtualization, kvm, linux-mm, mhocko,
akpm, torvalds, pbonzini, liliang.opensource, yang.zhang.wz,
quan.xu0, nilal, riel, peterx
In-Reply-To: <5B55AE56.5030404@intel.com>
On Mon, Jul 23, 2018 at 06:30:46PM +0800, Wei Wang wrote:
> On 07/22/2018 10:48 PM, Michael S. Tsirkin wrote:
> > On Fri, Jul 20, 2018 at 04:33:02PM +0800, Wei Wang wrote:
> > > +static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
> > > + struct shrink_control *sc)
> > > +{
> > > + unsigned long pages_to_free = balloon_pages_to_shrink,
> > > + pages_freed = 0;
> > > + struct virtio_balloon *vb = container_of(shrinker,
> > > + struct virtio_balloon, shrinker);
> > > +
> > > + /*
> > > + * One invocation of leak_balloon can deflate at most
> > > + * VIRTIO_BALLOON_ARRAY_PFNS_MAX balloon pages, so we call it
> > > + * multiple times to deflate pages till reaching
> > > + * balloon_pages_to_shrink pages.
> > > + */
> > > + while (vb->num_pages && pages_to_free) {
> > > + pages_to_free = balloon_pages_to_shrink - pages_freed;
> > > + pages_freed += leak_balloon(vb, pages_to_free);
> > > + }
> > > + update_balloon_size(vb);
> > Are you sure that this is never called if count returned 0?
>
> Yes. Please see do_shrink_slab, it just returns if count is 0.
>
> >
> > > +
> > > + return pages_freed / VIRTIO_BALLOON_PAGES_PER_PAGE;
> > > +}
> > > +
> > > +static unsigned long virtio_balloon_shrinker_count(struct shrinker *shrinker,
> > > + struct shrink_control *sc)
> > > +{
> > > + struct virtio_balloon *vb = container_of(shrinker,
> > > + struct virtio_balloon, shrinker);
> > > +
> > > + /*
> > > + * We continue to use VIRTIO_BALLOON_F_DEFLATE_ON_OOM to handle the
> > > + * case when shrinker needs to be invoked to relieve memory pressure.
> > > + */
> > > + if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > > + return 0;
> > So why not skip notifier registration when deflate on oom
> > is clear?
>
> Sounds good, thanks.
>
>
> > vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops;
> > #endif
> > + err = virtio_balloon_register_shrinker(vb);
> > + if (err)
> > + goto out_del_vqs;
> > So we can get scans before device is ready. Leak will fail
> > then. Why not register later after device is ready?
>
> Probably no.
>
> - it would be better not to set device ready when register_shrinker failed.
That's very rare so I won't be too worried.
> - When the device isn't ready, ballooning won't happen, that is,
> vb->num_pages will be 0, which results in shrinker_count=0 and shrinker_scan
> won't be called.
>
> So I think it would be better to have shrinker registered before
> device_ready.
>
> Best,
> Wei
^ permalink raw reply
* Re: [PATCH v36 2/5] virtio_balloon: replace oom notifier with shrinker
From: Michael S. Tsirkin @ 2018-07-23 14:13 UTC (permalink / raw)
To: Wei Wang
Cc: yang.zhang.wz, virtio-dev, riel, quan.xu0, kvm, nilal,
liliang.opensource, linux-kernel, mhocko, linux-mm, pbonzini,
akpm, virtualization, torvalds
In-Reply-To: <5B55AE56.5030404@intel.com>
On Mon, Jul 23, 2018 at 06:30:46PM +0800, Wei Wang wrote:
> On 07/22/2018 10:48 PM, Michael S. Tsirkin wrote:
> > On Fri, Jul 20, 2018 at 04:33:02PM +0800, Wei Wang wrote:
> > > +static unsigned long virtio_balloon_shrinker_scan(struct shrinker *shrinker,
> > > + struct shrink_control *sc)
> > > +{
> > > + unsigned long pages_to_free = balloon_pages_to_shrink,
> > > + pages_freed = 0;
> > > + struct virtio_balloon *vb = container_of(shrinker,
> > > + struct virtio_balloon, shrinker);
> > > +
> > > + /*
> > > + * One invocation of leak_balloon can deflate at most
> > > + * VIRTIO_BALLOON_ARRAY_PFNS_MAX balloon pages, so we call it
> > > + * multiple times to deflate pages till reaching
> > > + * balloon_pages_to_shrink pages.
> > > + */
> > > + while (vb->num_pages && pages_to_free) {
> > > + pages_to_free = balloon_pages_to_shrink - pages_freed;
> > > + pages_freed += leak_balloon(vb, pages_to_free);
> > > + }
> > > + update_balloon_size(vb);
> > Are you sure that this is never called if count returned 0?
>
> Yes. Please see do_shrink_slab, it just returns if count is 0.
>
> >
> > > +
> > > + return pages_freed / VIRTIO_BALLOON_PAGES_PER_PAGE;
> > > +}
> > > +
> > > +static unsigned long virtio_balloon_shrinker_count(struct shrinker *shrinker,
> > > + struct shrink_control *sc)
> > > +{
> > > + struct virtio_balloon *vb = container_of(shrinker,
> > > + struct virtio_balloon, shrinker);
> > > +
> > > + /*
> > > + * We continue to use VIRTIO_BALLOON_F_DEFLATE_ON_OOM to handle the
> > > + * case when shrinker needs to be invoked to relieve memory pressure.
> > > + */
> > > + if (!virtio_has_feature(vb->vdev, VIRTIO_BALLOON_F_DEFLATE_ON_OOM))
> > > + return 0;
> > So why not skip notifier registration when deflate on oom
> > is clear?
>
> Sounds good, thanks.
>
>
> > vb->vb_dev_info.inode->i_mapping->a_ops = &balloon_aops;
> > #endif
> > + err = virtio_balloon_register_shrinker(vb);
> > + if (err)
> > + goto out_del_vqs;
> > So we can get scans before device is ready. Leak will fail
> > then. Why not register later after device is ready?
>
> Probably no.
>
> - it would be better not to set device ready when register_shrinker failed.
That's very rare so I won't be too worried.
> - When the device isn't ready, ballooning won't happen, that is,
> vb->num_pages will be 0, which results in shrinker_count=0 and shrinker_scan
> won't be called.
>
> So I think it would be better to have shrinker registered before
> device_ready.
>
> Best,
> Wei
^ permalink raw reply
* [PATCH] block: zram: Replace GFP_ATOMIC with GFP_KERNEL
From: Jia-Ju Bai @ 2018-07-23 14:13 UTC (permalink / raw)
To: minchan, ngupta, sergey.senozhatsky.work, axboe
Cc: linux-kernel, linux-block, Jia-Ju Bai
read_from_bdev_async() and write_to_bdev() are never called in atomic
context. They call bio_alloc() with GFP_ATOMIC, which is not necessary.
GFP_ATOMIC can be replaced with GFP_KERNEL.
This is found by a static analysis tool named DCNS written by myself.
Signed-off-by: Jia-Ju Bai <baijiaju1990@gmail.com>
---
drivers/block/zram/zram_drv.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/block/zram/zram_drv.c b/drivers/block/zram/zram_drv.c
index 0f3fadd71230..b958ed0b8c35 100644
--- a/drivers/block/zram/zram_drv.c
+++ b/drivers/block/zram/zram_drv.c
@@ -450,7 +450,7 @@ static int read_from_bdev_async(struct zram *zram, struct bio_vec *bvec,
{
struct bio *bio;
- bio = bio_alloc(GFP_ATOMIC, 1);
+ bio = bio_alloc(GFP_KERNEL, 1);
if (!bio)
return -ENOMEM;
@@ -538,7 +538,7 @@ static int write_to_bdev(struct zram *zram, struct bio_vec *bvec,
struct bio *bio;
unsigned long entry;
- bio = bio_alloc(GFP_ATOMIC, 1);
+ bio = bio_alloc(GFP_KERNEL, 1);
if (!bio)
return -ENOMEM;
--
2.17.0
^ permalink raw reply related
* Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
From: Kevin Hilman @ 2018-07-23 14:12 UTC (permalink / raw)
To: Yixun Lan
Cc: Rob Herring, Jerome Brunet, Neil Armstrong, Carlo Caione,
Michael Turquette, Stephen Boyd, Miquèl Raynal,
Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai,
Jian Hu, linux-clk, linux-amlogic,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org, devicetree
In-Reply-To: <8bf565d3-dbfd-e06d-7076-ba78d7a2e766@amlogic.com>
Yixun Lan <yixun.lan@amlogic.com> writes:
[...]
>>
>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>
>> IMO, this should be done as part of merging this series. Otherwise, we
>> have duplicated code for the same thing.
>
> IMO, I'd leave this out of this series, since this patch series is quite
> complete as itself. Although, the downside is code duplication.
>
> Still, I need to hear Jerome, or Kevin's option, to see if or how we
> should proceed the eMMC's clock conversion.
>
> I could think of three option myself
> 1) don't do the conversion, downside is code duplication, upside is NO
> DT change, no compatibility issue
> 2) add a syscon node into eMMC DT node, then only convert clock part
> into this mmc-clkc model, while still leave other eMMC register access
> as the usual iomap way (still no race condition)
> 3) convert all eMMC register access by using regmap interface.
>
> both 2) and 3) need to update the DT.
>
> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
> the worst way due to dramatically change (I think this was already
> rejected in the previous discussion)
Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
step-by-step approach is fine (and preferred) by me.
Phase 1:
- add new mmc-clk provider
- add NAND driver using new mmc-clk provider
- boards using NAND should ensure emmc_c is disabled in DT
This allows us to not touch the MMC driver or existing upstream
bindings. Yes, this means there is duplicate code in the MMC driver and
the new mmc-clk provider, but that can be removed in the next phase.
Phase 2:
- convert MMC driver to use new mmc-clk provider
- update MMC users in DT and bindings
Kevin
^ permalink raw reply
* [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
From: Kevin Hilman @ 2018-07-23 14:12 UTC (permalink / raw)
To: linux-arm-kernel
In-Reply-To: <8bf565d3-dbfd-e06d-7076-ba78d7a2e766@amlogic.com>
Yixun Lan <yixun.lan@amlogic.com> writes:
[...]
>>
>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>
>> IMO, this should be done as part of merging this series. Otherwise, we
>> have duplicated code for the same thing.
>
> IMO, I'd leave this out of this series, since this patch series is quite
> complete as itself. Although, the downside is code duplication.
>
> Still, I need to hear Jerome, or Kevin's option, to see if or how we
> should proceed the eMMC's clock conversion.
>
> I could think of three option myself
> 1) don't do the conversion, downside is code duplication, upside is NO
> DT change, no compatibility issue
> 2) add a syscon node into eMMC DT node, then only convert clock part
> into this mmc-clkc model, while still leave other eMMC register access
> as the usual iomap way (still no race condition)
> 3) convert all eMMC register access by using regmap interface.
>
> both 2) and 3) need to update the DT.
>
> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
> the worst way due to dramatically change (I think this was already
> rejected in the previous discussion)
Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
step-by-step approach is fine (and preferred) by me.
Phase 1:
- add new mmc-clk provider
- add NAND driver using new mmc-clk provider
- boards using NAND should ensure emmc_c is disabled in DT
This allows us to not touch the MMC driver or existing upstream
bindings. Yes, this means there is duplicate code in the MMC driver and
the new mmc-clk provider, but that can be removed in the next phase.
Phase 2:
- convert MMC driver to use new mmc-clk provider
- update MMC users in DT and bindings
Kevin
^ permalink raw reply
* Re: [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
From: Kevin Hilman @ 2018-07-23 14:12 UTC (permalink / raw)
To: Yixun Lan
Cc: Rob Herring, Jerome Brunet, Neil Armstrong, Carlo Caione,
Michael Turquette, Stephen Boyd, Miquèl Raynal,
Boris Brezillon, Martin Blumenstingl, Liang Yang, Qiufang Dai,
Jian Hu, linux-clk, linux-amlogic,
moderated list:ARM/FREESCALE IMX / MXC ARM ARCHITECTURE,
linux-kernel@vger.kernel.org, devicetree
In-Reply-To: <8bf565d3-dbfd-e06d-7076-ba78d7a2e766@amlogic.com>
Yixun Lan <yixun.lan@amlogic.com> writes:
[...]
>>
>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>
>> IMO, this should be done as part of merging this series. Otherwise, we
>> have duplicated code for the same thing.
>
> IMO, I'd leave this out of this series, since this patch series is quite
> complete as itself. Although, the downside is code duplication.
>
> Still, I need to hear Jerome, or Kevin's option, to see if or how we
> should proceed the eMMC's clock conversion.
>
> I could think of three option myself
> 1) don't do the conversion, downside is code duplication, upside is NO
> DT change, no compatibility issue
> 2) add a syscon node into eMMC DT node, then only convert clock part
> into this mmc-clkc model, while still leave other eMMC register access
> as the usual iomap way (still no race condition)
> 3) convert all eMMC register access by using regmap interface.
>
> both 2) and 3) need to update the DT.
>
> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
> the worst way due to dramatically change (I think this was already
> rejected in the previous discussion)
Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
step-by-step approach is fine (and preferred) by me.
Phase 1:
- add new mmc-clk provider
- add NAND driver using new mmc-clk provider
- boards using NAND should ensure emmc_c is disabled in DT
This allows us to not touch the MMC driver or existing upstream
bindings. Yes, this means there is duplicate code in the MMC driver and
the new mmc-clk provider, but that can be removed in the next phase.
Phase 2:
- convert MMC driver to use new mmc-clk provider
- update MMC users in DT and bindings
Kevin
^ permalink raw reply
* [PATCH v2 1/3] clk: meson: add DT documentation for emmc clock controller
From: Kevin Hilman @ 2018-07-23 14:12 UTC (permalink / raw)
To: linus-amlogic
In-Reply-To: <8bf565d3-dbfd-e06d-7076-ba78d7a2e766@amlogic.com>
Yixun Lan <yixun.lan@amlogic.com> writes:
[...]
>>
>>> Second, we might like to convert eMMC driver to also use mmc-clkc model.
>>
>> IMO, this should be done as part of merging this series. Otherwise, we
>> have duplicated code for the same thing.
>
> IMO, I'd leave this out of this series, since this patch series is quite
> complete as itself. Although, the downside is code duplication.
>
> Still, I need to hear Jerome, or Kevin's option, to see if or how we
> should proceed the eMMC's clock conversion.
>
> I could think of three option myself
> 1) don't do the conversion, downside is code duplication, upside is NO
> DT change, no compatibility issue
> 2) add a syscon node into eMMC DT node, then only convert clock part
> into this mmc-clkc model, while still leave other eMMC register access
> as the usual iomap way (still no race condition)
> 3) convert all eMMC register access by using regmap interface.
>
> both 2) and 3) need to update the DT.
>
> and probably 2) is a compromise way, and 1) is also OK, 3) is probably
> the worst way due to dramatically change (I think this was already
> rejected in the previous discussion)
Because the devices (NAND and eMMC_C) are mutually exclusive, taking the
step-by-step approach is fine (and preferred) by me.
Phase 1:
- add new mmc-clk provider
- add NAND driver using new mmc-clk provider
- boards using NAND should ensure emmc_c is disabled in DT
This allows us to not touch the MMC driver or existing upstream
bindings. Yes, this means there is duplicate code in the MMC driver and
the new mmc-clk provider, but that can be removed in the next phase.
Phase 2:
- convert MMC driver to use new mmc-clk provider
- update MMC users in DT and bindings
Kevin
^ permalink raw reply
* Re: cgroup-aware OOM killer, how to move forward
From: Michal Hocko @ 2018-07-23 14:12 UTC (permalink / raw)
To: David Rientjes; +Cc: Tejun Heo, Roman Gushchin, linux-mm, akpm, hannes, gthelen
In-Reply-To: <alpine.DEB.2.21.1807201321040.231119@chino.kir.corp.google.com>
On Fri 20-07-18 13:28:56, David Rientjes wrote:
> On Fri, 20 Jul 2018, Tejun Heo wrote:
>
> > > process chosen for oom kill. I know that you care about the latter. My
> > > *only* suggestion was for the tunable to take a string instead of a
> > > boolean so it is extensible for future use. This seems like something so
> > > trivial.
> >
> > So, I'd much prefer it as boolean. It's a fundamentally binary
> > property, either handle the cgroup as a unit when chosen as oom victim
> > or not, nothing more.
>
> With the single hierarchy mandate of cgroup v2, the need arises to
> separate processes from a single job into subcontainers for use with
> controllers other than mem cgroup. In that case, we have no functionality
> to oom kill all processes in the subtree.
>
> A boolean can kill all processes attached to the victim's mem cgroup, but
> cannot kill all processes in a subtree if the limit of a common ancestor
> is reached. The common ancestor is needed to enforce a single memory
> limit but allow for processes to be constrained separately with other
> controllers.
I think you misunderstood the proposed semantic. oom.group is a property
of any (including inter-node) memcg. Once set all the processes in its
domain are killed in one go because they are considered indivisible
workload. Note how this doesn't tell anything about _how_ we select
a victim. That is not important and an in fact an implementation
detail. All we care about is that a selected victim is a part of an
indivisible workload and we have to tear down all of it. Future
extensions can talk more about how we select the victim but the
fundamental property of a group to be indivisible workload or a group of
semi raleted processes is a 0/1 IMHO.
Now there still are questions to iron out for that model. E.g. should
we allow to make a subtree of oom.group == 1 to be group == 0? In other
words something would be indivisible workload for one OOM context while
it is not for more restrictive OOM scope. If yes, then what is the
usecase?
--
Michal Hocko
SUSE Labs
^ permalink raw reply
* Re: [PATCH] x86/hvm: Disallow unknown MSR_EFER bits
From: Roger Pau Monné @ 2018-07-23 14:11 UTC (permalink / raw)
To: Andrew Cooper; +Cc: Sergey Dyasli, Wei Liu, Jan Beulich, Xen-devel
In-Reply-To: <1532353790-28875-1-git-send-email-andrew.cooper3@citrix.com>
On Mon, Jul 23, 2018 at 02:49:50PM +0100, Andrew Cooper wrote:
> It turns out that nothing ever prevented HVM guests from trying to set unknown
> EFER bits. Generally, this results in a vmentry failure.
>
> For Intel hardware, all implemented bits are covered by the checks.
>
> For AMD hardware, the only EFER bit which isn't covered by the checks is TCE
> (which AFAICT is specific to AMD Fam15/16 hardware). We never advertise TCE
> in CPUID, but it isn't a security problem to have TCE unexpected enabled in
> guest context.
>
> Disallow the setting of bits outside of the EFER_KNOWN_MASK, which prevents
> any vmentry failures for guests, yielding #GP instead.
>
> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
Reviewed-by: Roger Pau Monné <roger.pau@citrix.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* Re: [RFC] How to deal allarch packages in dependency chain for multilib rootfs?
From: Kang Kai @ 2018-07-23 14:09 UTC (permalink / raw)
To: Mark Hatle, openembedded-core
In-Reply-To: <24847408-a32e-2109-8b5d-c718e900cb9f@windriver.com>
On 2018年07月18日 04:21, Mark Hatle wrote:
> On 7/2/18 3:50 AM, Kang Kai wrote:
>> On 2018年06月28日 22:26, Mark Hatle wrote:
>>> On 6/28/18 3:48 AM, Kang Kai wrote:
>>>> Hi all,
>>>>
>>>> When build 32 bits rootfs with 64 bits bsp, if an allarch/noarch package is
>>>> installed to lib32 rootfs, it causes
>>>> unexpected 64 bits packages which is required by the allarch package installed
>>>> to lib32 rootfs.
>>>>
>>>> Take ca-certificates as example. ca-certificates rdepends on openssl, so if
>>>> ca-certificates is installed to image,
>>>> 64 bits package openssl will be installed too no matter what the rootfs is. But
>>>> only 32 bits openssl package should
>>>> be installed to 32 bits rootfs.
>> Hi Mark,
>>
>> Thanks for your detailed reply.
>>
>>> With -RPM-, and using names that do not include 'lib32' or other multilib
>>> prefixes. RPM doesn't care 'who' provides openssl, as long as someone provides
>>> openssl. There is a series of weights added to the available providers of
>>> openssl. The package with the highest weight (or maybe lowest) is what ends up
>>> being installed.
>>>
>>> In the case where there are two multilibs providing openssl, the system will
>>> usually do one of two things:
>>>
>>> 1) Look to see if an existing openssl is set to be installed (just use that version)
>>>
>>> 2) Look to see if the default architecture has the package and use that,
>>> otherwise fall back to less priority architectures. (Again, this is stock RPM
>>> behavior.)
>>>
>>> OE has modified the behaviors slightly, so I can't be 100% confident as to how
>>> it works in OE in all cases.
>> Do you mean make lib32-openssl also provides openssl? It should work for
>> do rootfs with repo priorities. But my concern is that
>> install package -foo- which requires openssl during runtime by command
>> rpm. If lib32-openssl is installed and it also provides 'openssl',
>> then package -foo- could be installed without real 64bits openssl. It
>> may cause malfunction.
> RPM has ELF dependencies and named dependencies. The named dependencies (at
> least in the past) had been translated to the generic name, as well as the
> multilib name. This allowed someone to say 'give me openssl', and it didn't
> matter which.
>
> For another package that required libssl (64-bit) it would have had a dependency
> on libssl(64) added to it, automatically, by the packaging system when building
> RPMs. These dependencies are only added when using RPMs, and there is
> (intentionally) no manual way to specify them.
>
> Where this falls down is perl where we might have a 32-bit and/or 64-bit perl..
> and we wanted one or the other for a specific module. Pam historically had a
> similar problem as the PAM subsystem was 64-bit -- but for some reason we'd end
> up installing only the 32-bit components.
>
> (Mind you as far as I know neither of these cases are part of the allarch issues
> you are concerned with.)
>
>>>> There are 2 ways to fixed the issue.
>>>>
>>>> 1 expand allarch/noarch packages with multilib. So if add ca-certificates to
>>>> image, lib32-ca-certificates will be
>>>> installed to 32bits rootfs. And then also the dependency lib32-openssl is
>>>> installed. That is what we expected.
>>> This really isn't the right way to do it, because then you end up with two
>>> packages with identical content -- and you then need to ensure that the internal
>>> dependencies all 'match'.
>>>
>>> So you've added another layer where you need to ensure that the multilib
>>> rdepends are 'specified' and managed properly. [Yes this can be handled by OE
>>> automaticallly], but it doubles the size of the package respository (for no-arch
>>> packages), as well as any field upgrades BOTH have to be upgraded together or
>>> you will get an error.
>> Got it.
>>
>>>> 2 expand DEPENDS/RDEPENDS of allarch/noarch packages with a prefix 'noarch-'
>>>> when multilib is enabled. So then
>>>> ca-certificates requires 'noarch-openssl'. And make both lib32-openssl and
>>>> openssl provides 'noarch-openssl'.
>>> -today- it should be depending on 'openssl', and all variants of openssl SHOULD
>>> be calling themselves/providing 'openssl'. (The other components of the
>>> dependency system will ensure the right 32-bit/64-bit version is selected if
>>> needed.)
>>>
>>> And unless the image creator is explicit in their check they may end up with
>>> BOTH the 32-bit and 64-bit versions...
>>>
>>>> When do_rootfs, there is only one rpm repo 'oe-rootfs-repo' now. We will
>>>> create repos with different priorities
>>> And this is creating problems. This is why the priority system (order for
>>> dependencies) was put into place originally. So it would start at the highest
>>> priority components and work it's way until it found the right set of matches.
>>>
>>> With DNF and RPM(4), things have changed.. I'm not sure how the priorities are
>>> defined in this system -- but really each arch type should be it's own
>>> repository in DNF.. and the priorities for each should be ordered to match the
>>> user's preferred order for package installation. (The default behavior is fine
>>> for most users, but a way to say -- no really I want 32-bit installed first.. is
>>> needed for the multilib image cases..)
>> Thanks. I'll figure out the priority in DNF and RPM(4).
>>
>>> The reality though for most multilib users is they have one of two use-cases:
>>>
>>> 1) 64-bit kernel, 32-bit userspace (and possibly a 'few' 64-bit binaries)
>>>
>>> 2) 64-bit kernel, 64-bit userspace... They want a small number of libraries from
>>> the multilib installed into their main system
>>>
>>> It's the second case where we often get into problems, because the system is
>>> really intended for that -- where you would say I expect BOTH openssl 32-bit and
>>> 64-bit installed, but many of our users are trying to do I want only the openssl
>>> installed for the executables I need. (reasonable request, just not exactly
>>> what RPM was intended to be doing.)
>>>
>>>> according to different archs/subdirectories. For 32 bits image, make 32 bits
>>>> rpm repo has higher priority, so lib32-openssl
>>>> will be installed to 32 bits rootfs rather than 64bits.
>>> Yes, this covers the use case #1 better then anything else we have at this point.
>>>
>>>> I know these 2 ways are not perfect, but only possible ways I have in mind to
>>>> solve the problem.
>>>>
>>>> Any comment or suggestion is greatly appreciated. Thanks a lot.
>>> So I can sum this up to:
>>>
>>> Use case 1.. kernel and primary userspace library type don't match is
>>> problematic, but not used by a lot of people (AFAIK).
>> I am sorry that it is the Use case 1 (64-bit kernel, 32-bit userspace)
>> that we want to solve. Because both lib32 (imported by other lib32
>> packages) and
>> 64bits (imported by noarch pacakge) packages such as perl may be
>> installed to image at same time , it causes some runtime failures such
>> as some perl
>> modules don't work.
Hi Mark,
Thanks a lot for your reply.
> There should -never- be a situation with a 32-bit and 64-bit (or any multilib)
> 'allarch' package changes. If it does, it's simply NOT allarch. Allarch means
> there are no architecture dependencies in that package. If there are -any-, the
> thing should be properly declared to a specific arch, even if it only contains
> configuration files.
For allarch packages such as ca-certificates, I believe that it only
needs an 'openssl' and no matter whether lib64-openssl or lib32-openssl
exists.
So in my WIP patch, I make allarch package only depends on a virtual
'noarch-foo' and both lib64-foo and lib32-foo provide foo.
And during do rootfs, according to image type(whether core-image-sato or
lib32-core-image-sato) to install proper provider.
>
> Maybe the right answer is to add a QA step that verifies all 'allarch' versions
> are the same.. (I'm not exactly sure how you would do this, try to build all
> 'multilib' variants of any specific allarch and verify they didn't change perhaps?)
>
>> I once thought to solve the dependency issue in package libsolv. The
>> original thought is to pass a environment variable to identify whether
>> build a lib32 image.
>> When resolve the dependencies of noarch package, expand the dependencies
>> according the environment variable. But it seems libsolv treats "all
>> strings and
>> relations as unique 32-bit numbers". So it seems it doesn't work this way.
> Ya, I don't think fixing it in the solver is the right answer. Whatever the
> solution, it has to be generic to work for all of the packaging formats.
>
> The basic rules, as I understand them, are:
>
> 1) allarch/noarch -- must be -identical- across all architectures [with the same
> distro configuration]
> 2) recipes/packages that require allarch must not expect any architecture
> dependent behavior, including path names. (lib vs lib64, etc)
Yes. Currently the problem is to resolve dependency of allarch packages.
And will not affect packages which depends on allarch packages.
Regards,
Kai
>
> 3) regular arch packages should do dependencies based on package names (and
> other declared virtual dependencies)
>
> 4) rpm [automatic] internal dependencies should resolve executable dynamic
> linking issues.
>
> Known caveats are PAM, Perl and maybe Python modules that have no ELF component,
> but may have be loaded at runtime and need to match another executable
> environment. How to resolve this, we need to be explicit in the recipes.. i.e.
> a 32-bit pam module must require 32-bit pam and NOT satisfy a 64-bit
> applications PAM needs.
>
> (Not sure the above really helps, but it may clarify things a bit.)
>
> --Mark
>
>> Thanks a lot.
>>
>> --Kai
>>
>>> Use case 2.. the embedded nature of trying to develop 'small' [in terms of
>>> number of packages installed] filesystems is causing some odd behavior, but a
>>> workaround of specifying the -exact- packages you want when this oddity arrives
>>> is available.
>>>
>>> --Mark
>>>
>>>> --
>>>> Regards,
>>>> Neil | Kai Kang
>>>>
>
--
Regards,
Neil | Kai Kang
^ permalink raw reply
* [PATCH 5/5] f2fs: add proc entry to show victim_secmap bitmap
From: Yunlong Song @ 2018-07-23 14:10 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
In-Reply-To: <1532355022-163029-1-git-send-email-yunlong.song@huawei.com>
This patch adds a new proc entry to show victim_secmap information in
more detail, which is very helpful to know the get_victim candidate
status clearly, and helpful to debug problems (e.g., some sections can
not gc all of its blocks, since some blocks belong to atomic file,
leaving victim_secmap with section bit setting, in extrem case, this
will lead all bytes of victim_secmap setting with 0xff).
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/sysfs.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index bca1236..f22782a 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -615,6 +615,28 @@ static int __maybe_unused iostat_info_seq_show(struct seq_file *seq,
return 0;
}
+static int __maybe_unused victim_bits_seq_show(struct seq_file *seq,
+ void *offset)
+{
+ struct super_block *sb = seq->private;
+ struct f2fs_sb_info *sbi = F2FS_SB(sb);
+ struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
+ int i;
+
+ seq_puts(seq, "format: victim_secmap bitmaps\n");
+
+ for (i = 0; i < MAIN_SECS(sbi); i++) {
+ if ((i % 10) == 0)
+ seq_printf(seq, "%-10d", i);
+ seq_printf(seq, "%d", test_bit(i, dirty_i->victim_secmap) ? 1 : 0);
+ if ((i % 10) == 9 || i == (MAIN_SECS(sbi) - 1))
+ seq_putc(seq, '\n');
+ else
+ seq_putc(seq, ' ');
+ }
+ return 0;
+}
+
int __init f2fs_init_sysfs(void)
{
int ret;
@@ -664,6 +686,8 @@ int f2fs_register_sysfs(struct f2fs_sb_info *sbi)
segment_bits_seq_show, sb);
proc_create_single_data("iostat_info", S_IRUGO, sbi->s_proc,
iostat_info_seq_show, sb);
+ proc_create_single_data("victim_bits", S_IRUGO, sbi->s_proc,
+ victim_bits_seq_show, sb);
}
return 0;
}
@@ -674,6 +698,7 @@ void f2fs_unregister_sysfs(struct f2fs_sb_info *sbi)
remove_proc_entry("iostat_info", sbi->s_proc);
remove_proc_entry("segment_info", sbi->s_proc);
remove_proc_entry("segment_bits", sbi->s_proc);
+ remove_proc_entry("victim_bits", sbi->s_proc);
remove_proc_entry(sbi->sb->s_id, f2fs_proc_root);
}
kobject_del(&sbi->s_kobj);
--
1.8.5.2
^ permalink raw reply related
* [PATCH 5/5] f2fs: add proc entry to show victim_secmap bitmap
From: Yunlong Song @ 2018-07-23 14:10 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: linux-kernel, linux-f2fs-devel, miaoxie
In-Reply-To: <1532355022-163029-1-git-send-email-yunlong.song@huawei.com>
This patch adds a new proc entry to show victim_secmap information in
more detail, which is very helpful to know the get_victim candidate
status clearly, and helpful to debug problems (e.g., some sections can
not gc all of its blocks, since some blocks belong to atomic file,
leaving victim_secmap with section bit setting, in extrem case, this
will lead all bytes of victim_secmap setting with 0xff).
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/sysfs.c | 25 +++++++++++++++++++++++++
1 file changed, 25 insertions(+)
diff --git a/fs/f2fs/sysfs.c b/fs/f2fs/sysfs.c
index bca1236..f22782a 100644
--- a/fs/f2fs/sysfs.c
+++ b/fs/f2fs/sysfs.c
@@ -615,6 +615,28 @@ static int __maybe_unused iostat_info_seq_show(struct seq_file *seq,
return 0;
}
+static int __maybe_unused victim_bits_seq_show(struct seq_file *seq,
+ void *offset)
+{
+ struct super_block *sb = seq->private;
+ struct f2fs_sb_info *sbi = F2FS_SB(sb);
+ struct dirty_seglist_info *dirty_i = DIRTY_I(sbi);
+ int i;
+
+ seq_puts(seq, "format: victim_secmap bitmaps\n");
+
+ for (i = 0; i < MAIN_SECS(sbi); i++) {
+ if ((i % 10) == 0)
+ seq_printf(seq, "%-10d", i);
+ seq_printf(seq, "%d", test_bit(i, dirty_i->victim_secmap) ? 1 : 0);
+ if ((i % 10) == 9 || i == (MAIN_SECS(sbi) - 1))
+ seq_putc(seq, '\n');
+ else
+ seq_putc(seq, ' ');
+ }
+ return 0;
+}
+
int __init f2fs_init_sysfs(void)
{
int ret;
@@ -664,6 +686,8 @@ int f2fs_register_sysfs(struct f2fs_sb_info *sbi)
segment_bits_seq_show, sb);
proc_create_single_data("iostat_info", S_IRUGO, sbi->s_proc,
iostat_info_seq_show, sb);
+ proc_create_single_data("victim_bits", S_IRUGO, sbi->s_proc,
+ victim_bits_seq_show, sb);
}
return 0;
}
@@ -674,6 +698,7 @@ void f2fs_unregister_sysfs(struct f2fs_sb_info *sbi)
remove_proc_entry("iostat_info", sbi->s_proc);
remove_proc_entry("segment_info", sbi->s_proc);
remove_proc_entry("segment_bits", sbi->s_proc);
+ remove_proc_entry("victim_bits", sbi->s_proc);
remove_proc_entry(sbi->sb->s_id, f2fs_proc_root);
}
kobject_del(&sbi->s_kobj);
--
1.8.5.2
------------------------------------------------------------------------------
Check out the vibrant tech community on one of the world's most
engaging tech sites, Slashdot.org! http://sdm.link/slashdot
^ permalink raw reply related
* [PATCH 4/5] f2fs: let BG_GC check every dirty segments and gc over a threshold
From: Yunlong Song @ 2018-07-23 14:10 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
In-Reply-To: <1532355022-163029-1-git-send-email-yunlong.song@huawei.com>
BG_GC is triggered in idle time, so it is better check every dirty
segment and finds the best victim to gc. Otherwise, BG_GC will be
limited to only 8G areas, and probably select a victim which has nearly
full of valid blocks, resulting a big WAI. Besides, we also add a
bggc_threshold (which is the old "fggc_threshold", so revert commit
"299254") to stop BG_GC when there is no good choice. This is especially
good for large section case to reduce WAI.
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/f2fs.h | 2 ++
fs/f2fs/gc.c | 23 ++++++++++++++++++++---
fs/f2fs/segment.h | 9 +++++++++
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index f8a7b42..24a9d7f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1220,6 +1220,8 @@ struct f2fs_sb_info {
unsigned int cur_fg_victim_sec; /* current FG_GC victim section num */
unsigned int cur_bg_victim_sec; /* current BG_GC victim section num */
unsigned int gc_mode; /* current GC state */
+ /* threshold for selecting bg victims */
+ u64 bggc_threshold;
/* for skip statistic */
unsigned long long skipped_atomic_files[2]; /* FG_GC and BG_GC */
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 0e7a265..21e8d59 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -189,9 +189,8 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type,
p->ofs_unit = sbi->segs_per_sec;
}
- /* we need to check every dirty segments in the FG_GC case */
- if (gc_type != FG_GC &&
- (sbi->gc_mode != GC_URGENT) &&
+ /* we need to check every dirty segments in the GC case */
+ if (p->alloc_mode == SSR &&
p->max_search > sbi->max_victim_search)
p->max_search = sbi->max_victim_search;
@@ -230,6 +229,10 @@ static unsigned int check_bg_victims(struct f2fs_sb_info *sbi)
for_each_set_bit(secno, dirty_i->victim_secmap, MAIN_SECS(sbi)) {
if (sec_usage_check(sbi, secno))
continue;
+
+ if (no_bggc_candidate(sbi, secno))
+ continue;
+
clear_bit(secno, dirty_i->victim_secmap);
return GET_SEG_FROM_SEC(sbi, secno);
}
@@ -368,6 +371,10 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
if (sec_usage_check(sbi, secno))
goto next;
+ if (gc_type == BG_GC && p.alloc_mode == LFS &&
+ no_bggc_candidate(sbi, secno))
+ goto next;
+
cost = get_gc_cost(sbi, segno, &p);
if (p.min_cost > cost) {
@@ -1140,8 +1147,18 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
void f2fs_build_gc_manager(struct f2fs_sb_info *sbi)
{
+ u64 main_count, resv_count, ovp_count;
+
DIRTY_I(sbi)->v_ops = &default_v_ops;
+ /* threshold of # of valid blocks in a section for victims of BG_GC */
+ main_count = SM_I(sbi)->main_segments << sbi->log_blocks_per_seg;
+ resv_count = SM_I(sbi)->reserved_segments << sbi->log_blocks_per_seg;
+ ovp_count = SM_I(sbi)->ovp_segments << sbi->log_blocks_per_seg;
+
+ sbi->bggc_threshold = div64_u64((main_count - ovp_count) *
+ BLKS_PER_SEC(sbi), (main_count - resv_count));
+
sbi->gc_pin_file_threshold = DEF_GC_FAILED_PINNED_FILES;
/* give warm/cold data area from slower device */
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index b21bb96..932e59b 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -785,6 +785,15 @@ static inline block_t sum_blk_addr(struct f2fs_sb_info *sbi, int base, int type)
- (base + 1) + type;
}
+static inline bool no_bggc_candidate(struct f2fs_sb_info *sbi,
+ unsigned int secno)
+{
+ if (get_valid_blocks(sbi, GET_SEG_FROM_SEC(sbi, secno), true) >
+ sbi->bggc_threshold)
+ return true;
+ return false;
+}
+
static inline bool sec_usage_check(struct f2fs_sb_info *sbi, unsigned int secno)
{
if (IS_CURSEC(sbi, secno) || (sbi->cur_fg_victim_sec == secno) ||
--
1.8.5.2
^ permalink raw reply related
* [PATCH 3/5] f2fs: clear_bit the SSR selected section in the victim_secmap
From: Yunlong Song @ 2018-07-23 14:10 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
In-Reply-To: <1532355022-163029-1-git-send-email-yunlong.song@huawei.com>
SSR uses get_victim to select ssr segment to allocate data blocks, which
makes the previous result of victim_secmap inaccurately, so we would
better clear the bit of the section in the victim_secmap.
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/gc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 705d419..0e7a265 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -394,7 +394,8 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
set_bit(secno, dirty_i->victim_secmap);
sbi->cur_bg_victim_sec = secno;
}
- }
+ } else
+ clear_bit(secno, dirty_i->victim_secmap);
*result = (p.min_segno / p.ofs_unit) * p.ofs_unit;
trace_f2fs_get_victim(sbi->sb, type, gc_type, &p,
--
1.8.5.2
^ permalink raw reply related
* [PATCH 2/5] f2fs: add cur_victim_sec for BG_GC to avoid skipping BG_GC victim
From: Yunlong Song @ 2018-07-23 14:10 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
In-Reply-To: <1532355022-163029-1-git-send-email-yunlong.song@huawei.com>
If f2fs aborts BG_GC, then the section bit of victim_secmap will be set,
which will cause the section skipped in the future get_victim of BG_GC.
In a worst case that each section in the victim_secmap is set and there
are enough free sections (so FG_GC can not be triggered), then BG_GC
will skip all the sections and cannot find any victims, causing BG_GC
failed each time. Besides, SSR also uses BG_GC to get ssr segment, if
many sections in the victim_secmap are set, then SSR cannot get a proper
ssr segment to allocate blocks, which makes SSR inefficiently. To fix
this problem, we can add cur_victim_sec for BG_GC similar like that in
FG_GC to avoid selecting the same section repeatedly.
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/f2fs.h | 3 ++-
fs/f2fs/gc.c | 15 +++++++++------
fs/f2fs/segment.h | 3 ++-
fs/f2fs/super.c | 3 ++-
include/trace/events/f2fs.h | 18 ++++++++++++------
5 files changed, 27 insertions(+), 15 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index 57a8851..f8a7b42 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1217,7 +1217,8 @@ struct f2fs_sb_info {
/* for cleaning operations */
struct mutex gc_mutex; /* mutex for GC */
struct f2fs_gc_kthread *gc_thread; /* GC thread */
- unsigned int cur_victim_sec; /* current victim section num */
+ unsigned int cur_fg_victim_sec; /* current FG_GC victim section num */
+ unsigned int cur_bg_victim_sec; /* current BG_GC victim section num */
unsigned int gc_mode; /* current GC state */
/* for skip statistic */
unsigned long long skipped_atomic_files[2]; /* FG_GC and BG_GC */
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 2ba470d..705d419 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -367,8 +367,6 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
if (sec_usage_check(sbi, secno))
goto next;
- if (gc_type == BG_GC && test_bit(secno, dirty_i->victim_secmap))
- goto next;
cost = get_gc_cost(sbi, segno, &p);
@@ -391,14 +389,17 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
if (p.alloc_mode == LFS) {
secno = GET_SEC_FROM_SEG(sbi, p.min_segno);
if (gc_type == FG_GC)
- sbi->cur_victim_sec = secno;
- else
+ sbi->cur_fg_victim_sec = secno;
+ else {
set_bit(secno, dirty_i->victim_secmap);
+ sbi->cur_bg_victim_sec = secno;
+ }
}
*result = (p.min_segno / p.ofs_unit) * p.ofs_unit;
trace_f2fs_get_victim(sbi->sb, type, gc_type, &p,
- sbi->cur_victim_sec,
+ sbi->cur_fg_victim_sec,
+ sbi->cur_bg_victim_sec,
prefree_segments(sbi), free_segments(sbi));
}
out:
@@ -1098,7 +1099,9 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
}
if (gc_type == FG_GC)
- sbi->cur_victim_sec = NULL_SEGNO;
+ sbi->cur_fg_victim_sec = NULL_SEGNO;
+ else
+ sbi->cur_bg_victim_sec = NULL_SEGNO;
if (!sync) {
if (has_not_enough_free_secs(sbi, sec_freed, 0)) {
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index 5049551..b21bb96 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -787,7 +787,8 @@ static inline block_t sum_blk_addr(struct f2fs_sb_info *sbi, int base, int type)
static inline bool sec_usage_check(struct f2fs_sb_info *sbi, unsigned int secno)
{
- if (IS_CURSEC(sbi, secno) || (sbi->cur_victim_sec == secno))
+ if (IS_CURSEC(sbi, secno) || (sbi->cur_fg_victim_sec == secno) ||
+ (sbi->cur_bg_victim_sec == secno))
return true;
return false;
}
diff --git a/fs/f2fs/super.c b/fs/f2fs/super.c
index 7187885..ef69ebf 100644
--- a/fs/f2fs/super.c
+++ b/fs/f2fs/super.c
@@ -2386,7 +2386,8 @@ static void init_sb_info(struct f2fs_sb_info *sbi)
sbi->root_ino_num = le32_to_cpu(raw_super->root_ino);
sbi->node_ino_num = le32_to_cpu(raw_super->node_ino);
sbi->meta_ino_num = le32_to_cpu(raw_super->meta_ino);
- sbi->cur_victim_sec = NULL_SECNO;
+ sbi->cur_fg_victim_sec = NULL_SECNO;
+ sbi->cur_bg_victim_sec = NULL_SECNO;
sbi->max_victim_search = DEF_MAX_VICTIM_SEARCH;
sbi->dir_level = DEF_DIR_LEVEL;
diff --git a/include/trace/events/f2fs.h b/include/trace/events/f2fs.h
index 7956989..0f01f82 100644
--- a/include/trace/events/f2fs.h
+++ b/include/trace/events/f2fs.h
@@ -693,10 +693,12 @@
TRACE_EVENT(f2fs_get_victim,
TP_PROTO(struct super_block *sb, int type, int gc_type,
- struct victim_sel_policy *p, unsigned int pre_victim,
+ struct victim_sel_policy *p, unsigned int pre_fg_victim,
+ unsigned int pre_bg_victim,
unsigned int prefree, unsigned int free),
- TP_ARGS(sb, type, gc_type, p, pre_victim, prefree, free),
+ TP_ARGS(sb, type, gc_type, p, pre_fg_victim, pre_bg_victim,
+ prefree, free),
TP_STRUCT__entry(
__field(dev_t, dev)
@@ -707,7 +709,8 @@
__field(unsigned int, victim)
__field(unsigned int, cost)
__field(unsigned int, ofs_unit)
- __field(unsigned int, pre_victim)
+ __field(unsigned int, pre_fg_victim)
+ __field(unsigned int, pre_bg_victim)
__field(unsigned int, prefree)
__field(unsigned int, free)
),
@@ -721,14 +724,16 @@
__entry->victim = p->min_segno;
__entry->cost = p->min_cost;
__entry->ofs_unit = p->ofs_unit;
- __entry->pre_victim = pre_victim;
+ __entry->pre_fg_victim = pre_fg_victim;
+ __entry->pre_bg_victim = pre_bg_victim;
__entry->prefree = prefree;
__entry->free = free;
),
TP_printk("dev = (%d,%d), type = %s, policy = (%s, %s, %s), "
"victim = %u, cost = %u, ofs_unit = %u, "
- "pre_victim_secno = %d, prefree = %u, free = %u",
+ "pre_fg_victim_secno = %d, pre_bg_victim_secno = %d, "
+ "prefree = %u, free = %u",
show_dev(__entry->dev),
show_data_type(__entry->type),
show_gc_type(__entry->gc_type),
@@ -737,7 +742,8 @@
__entry->victim,
__entry->cost,
__entry->ofs_unit,
- (int)__entry->pre_victim,
+ (int)__entry->pre_fg_victim,
+ (int)__entry->pre_bg_victim,
__entry->prefree,
__entry->free)
);
--
1.8.5.2
^ permalink raw reply related
* [PATCH 1/5] f2fs: clear victim_secmap when section has full valid blocks
From: Yunlong Song @ 2018-07-23 14:10 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
In-Reply-To: <1532355022-163029-1-git-send-email-yunlong.song@huawei.com>
Without this patch, f2fs only clears victim_secmap when it finds out
that the section has no valid blocks at all, but forgets to clear the
victim_secmap when the whole section has full valid blocks.
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/segment.c | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/segment.c b/fs/f2fs/segment.c
index cfff7cf..255bff5 100644
--- a/fs/f2fs/segment.c
+++ b/fs/f2fs/segment.c
@@ -776,7 +776,9 @@ static void __remove_dirty_segment(struct f2fs_sb_info *sbi, unsigned int segno,
if (test_and_clear_bit(segno, dirty_i->dirty_segmap[t]))
dirty_i->nr_dirty[t]--;
- if (get_valid_blocks(sbi, segno, true) == 0)
+ if (get_valid_blocks(sbi, segno, true) == 0 ||
+ get_valid_blocks(sbi, segno, true) ==
+ (sbi->segs_per_sec << sbi->log_blocks_per_seg))
clear_bit(GET_SEC_FROM_SEG(sbi, segno),
dirty_i->victim_secmap);
}
--
1.8.5.2
^ permalink raw reply related
* [PATCH 0/5] f2fs: fix and improve for victim_secmap
From: Yunlong Song @ 2018-07-23 14:10 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
There are some fixes and improvements for using with victim_secmap.
Yunlong Song (5):
f2fs: clear victim_secmap when section has full valid blocks
f2fs: add cur_victim_sec for BG_GC to avoid skipping BG_GC victim
f2fs: clear_bit the SSR selected section in the victim_secmap
f2fs: let BG_GC check every dirty segments and gc over a threshold
f2fs: add proc entry to show victim_secmap bitmap
fs/f2fs/f2fs.h | 5 ++++-
fs/f2fs/gc.c | 39 ++++++++++++++++++++++++++++++---------
fs/f2fs/segment.c | 4 +++-
fs/f2fs/segment.h | 12 +++++++++++-
fs/f2fs/super.c | 3 ++-
fs/f2fs/sysfs.c | 25 +++++++++++++++++++++++++
include/trace/events/f2fs.h | 18 ++++++++++++------
7 files changed, 87 insertions(+), 19 deletions(-)
--
1.8.5.2
^ permalink raw reply
* [PATCH 4/5] f2fs: let BG_GC check every dirty segments and gc over a threshold
From: Yunlong Song @ 2018-07-23 14:10 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
In-Reply-To: <1532355022-163029-1-git-send-email-yunlong.song@huawei.com>
BG_GC is triggered in idle time, so it is better check every dirty
segment and finds the best victim to gc. Otherwise, BG_GC will be
limited to only 8G areas, and probably select a victim which has nearly
full of valid blocks, resulting a big WAI. Besides, we also add a
bggc_threshold (which is the old "fggc_threshold", so revert commit
"299254") to stop BG_GC when there is no good choice. This is especially
good for large section case to reduce WAI.
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/f2fs.h | 2 ++
fs/f2fs/gc.c | 23 ++++++++++++++++++++---
fs/f2fs/segment.h | 9 +++++++++
3 files changed, 31 insertions(+), 3 deletions(-)
diff --git a/fs/f2fs/f2fs.h b/fs/f2fs/f2fs.h
index f8a7b42..24a9d7f 100644
--- a/fs/f2fs/f2fs.h
+++ b/fs/f2fs/f2fs.h
@@ -1220,6 +1220,8 @@ struct f2fs_sb_info {
unsigned int cur_fg_victim_sec; /* current FG_GC victim section num */
unsigned int cur_bg_victim_sec; /* current BG_GC victim section num */
unsigned int gc_mode; /* current GC state */
+ /* threshold for selecting bg victims */
+ u64 bggc_threshold;
/* for skip statistic */
unsigned long long skipped_atomic_files[2]; /* FG_GC and BG_GC */
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 0e7a265..21e8d59 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -189,9 +189,8 @@ static void select_policy(struct f2fs_sb_info *sbi, int gc_type,
p->ofs_unit = sbi->segs_per_sec;
}
- /* we need to check every dirty segments in the FG_GC case */
- if (gc_type != FG_GC &&
- (sbi->gc_mode != GC_URGENT) &&
+ /* we need to check every dirty segments in the GC case */
+ if (p->alloc_mode == SSR &&
p->max_search > sbi->max_victim_search)
p->max_search = sbi->max_victim_search;
@@ -230,6 +229,10 @@ static unsigned int check_bg_victims(struct f2fs_sb_info *sbi)
for_each_set_bit(secno, dirty_i->victim_secmap, MAIN_SECS(sbi)) {
if (sec_usage_check(sbi, secno))
continue;
+
+ if (no_bggc_candidate(sbi, secno))
+ continue;
+
clear_bit(secno, dirty_i->victim_secmap);
return GET_SEG_FROM_SEC(sbi, secno);
}
@@ -368,6 +371,10 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
if (sec_usage_check(sbi, secno))
goto next;
+ if (gc_type == BG_GC && p.alloc_mode == LFS &&
+ no_bggc_candidate(sbi, secno))
+ goto next;
+
cost = get_gc_cost(sbi, segno, &p);
if (p.min_cost > cost) {
@@ -1140,8 +1147,18 @@ int f2fs_gc(struct f2fs_sb_info *sbi, bool sync,
void f2fs_build_gc_manager(struct f2fs_sb_info *sbi)
{
+ u64 main_count, resv_count, ovp_count;
+
DIRTY_I(sbi)->v_ops = &default_v_ops;
+ /* threshold of # of valid blocks in a section for victims of BG_GC */
+ main_count = SM_I(sbi)->main_segments << sbi->log_blocks_per_seg;
+ resv_count = SM_I(sbi)->reserved_segments << sbi->log_blocks_per_seg;
+ ovp_count = SM_I(sbi)->ovp_segments << sbi->log_blocks_per_seg;
+
+ sbi->bggc_threshold = div64_u64((main_count - ovp_count) *
+ BLKS_PER_SEC(sbi), (main_count - resv_count));
+
sbi->gc_pin_file_threshold = DEF_GC_FAILED_PINNED_FILES;
/* give warm/cold data area from slower device */
diff --git a/fs/f2fs/segment.h b/fs/f2fs/segment.h
index b21bb96..932e59b 100644
--- a/fs/f2fs/segment.h
+++ b/fs/f2fs/segment.h
@@ -785,6 +785,15 @@ static inline block_t sum_blk_addr(struct f2fs_sb_info *sbi, int base, int type)
- (base + 1) + type;
}
+static inline bool no_bggc_candidate(struct f2fs_sb_info *sbi,
+ unsigned int secno)
+{
+ if (get_valid_blocks(sbi, GET_SEG_FROM_SEC(sbi, secno), true) >
+ sbi->bggc_threshold)
+ return true;
+ return false;
+}
+
static inline bool sec_usage_check(struct f2fs_sb_info *sbi, unsigned int secno)
{
if (IS_CURSEC(sbi, secno) || (sbi->cur_fg_victim_sec == secno) ||
--
1.8.5.2
^ permalink raw reply related
* [PATCH 3/5] f2fs: clear_bit the SSR selected section in the victim_secmap
From: Yunlong Song @ 2018-07-23 14:10 UTC (permalink / raw)
To: jaegeuk, chao, yuchao0, yunlong.song, yunlong.song
Cc: miaoxie, bintian.wang, shengyong1, heyunlei, linux-f2fs-devel,
linux-kernel
In-Reply-To: <1532355022-163029-1-git-send-email-yunlong.song@huawei.com>
SSR uses get_victim to select ssr segment to allocate data blocks, which
makes the previous result of victim_secmap inaccurately, so we would
better clear the bit of the section in the victim_secmap.
Signed-off-by: Yunlong Song <yunlong.song@huawei.com>
---
fs/f2fs/gc.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/fs/f2fs/gc.c b/fs/f2fs/gc.c
index 705d419..0e7a265 100644
--- a/fs/f2fs/gc.c
+++ b/fs/f2fs/gc.c
@@ -394,7 +394,8 @@ static int get_victim_by_default(struct f2fs_sb_info *sbi,
set_bit(secno, dirty_i->victim_secmap);
sbi->cur_bg_victim_sec = secno;
}
- }
+ } else
+ clear_bit(secno, dirty_i->victim_secmap);
*result = (p.min_segno / p.ofs_unit) * p.ofs_unit;
trace_f2fs_get_victim(sbi->sb, type, gc_type, &p,
--
1.8.5.2
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.