Re: [PATCH RFC] s390x/vmem: get rid of memory segment list
On 29.06.20 13:55, Heiko Carstens wrote: > On Fri, Jun 26, 2020 at 08:46:21PM +0200, Gerald Schaefer wrote: >> Verified with DCSS overlapping boot and standby memory, works fine. >> As expected, the error message changes, but I don't think that is a >> problem, as long as you also remove the old -ENOSPC case / comment >> in arch/s390/mm/extmem.c. It is actually more correct now I guess, >> -ENOSPC doesn't look like the correct return value anyway. >> >> Thanks for cleaning up! Looks good to me, and removes > 100 LOC, >> unless Heiko remembers some other issues from ancient times. >> >> Reviewed-by: Gerald Schaefer >> Tested-by: Gerald Schaefer [DCSS] > > Looks good to me too. Gerald, thanks for looking and verifying this, > and David, thanks for providing the patch. > > Applied. > Thanks Gerald and Heiko! Should I send an addon patch to tweak the documentation or resend this patch? -- Thanks, David / dhildenb
Re: [PATCH RFC] s390x/vmem: get rid of memory segment list
On Mon, Jun 29, 2020 at 02:01:22PM +0200, David Hildenbrand wrote: > On 29.06.20 13:55, Heiko Carstens wrote: > > On Fri, Jun 26, 2020 at 08:46:21PM +0200, Gerald Schaefer wrote: > >> Verified with DCSS overlapping boot and standby memory, works fine. > >> As expected, the error message changes, but I don't think that is a > >> problem, as long as you also remove the old -ENOSPC case / comment > >> in arch/s390/mm/extmem.c. It is actually more correct now I guess, > >> -ENOSPC doesn't look like the correct return value anyway. > >> > >> Thanks for cleaning up! Looks good to me, and removes > 100 LOC, > >> unless Heiko remembers some other issues from ancient times. > >> > >> Reviewed-by: Gerald Schaefer > >> Tested-by: Gerald Schaefer [DCSS] > > > > Looks good to me too. Gerald, thanks for looking and verifying this, > > and David, thanks for providing the patch. > > > > Applied. > > > > Thanks Gerald and Heiko! Should I send an addon patch to tweak the > documentation or resend this patch? Please send an addon patch. I will merge it.
Re: [PATCH RFC] s390x/vmem: get rid of memory segment list
On Fri, Jun 26, 2020 at 08:46:21PM +0200, Gerald Schaefer wrote: > Verified with DCSS overlapping boot and standby memory, works fine. > As expected, the error message changes, but I don't think that is a > problem, as long as you also remove the old -ENOSPC case / comment > in arch/s390/mm/extmem.c. It is actually more correct now I guess, > -ENOSPC doesn't look like the correct return value anyway. > > Thanks for cleaning up! Looks good to me, and removes > 100 LOC, > unless Heiko remembers some other issues from ancient times. > > Reviewed-by: Gerald Schaefer > Tested-by: Gerald Schaefer [DCSS] Looks good to me too. Gerald, thanks for looking and verifying this, and David, thanks for providing the patch. Applied.
Re: [PATCH RFC] s390x/vmem: get rid of memory segment list
On Fri, 26 Jun 2020 19:22:53 +0200
Gerald Schaefer wrote:
> On Thu, 25 Jun 2020 17:00:29 +0200
> David Hildenbrand wrote:
>
> > I can't come up with a satisfying reason why we still need the memory
> > segment list. We used to represent in the list:
> > - boot memory
> > - standby memory added via add_memory()
> > - loaded dcss segments
> >
> > When loading/unloading dcss segments, we already track them in a
> > separate list and check for overlaps
> > (arch/s390/mm/extmem.c:segment_overlaps_others()) when loading segments.
> >
> > The overlap check was introduced for some segments in
> > commit b2300b9efe1b ("[S390] dcssblk: add >2G DCSSs support and stacked
> > contiguous DCSSs support.")
> > and was extended to cover all dcss segments in
> > commit ca57114609d1 ("s390/extmem: remove code for 31 bit addressing
> > mode").
> >
> > Although I doubt that overlaps with boot memory and standby memory
> > are relevant, let's reshuffle the checks in load_segment() to request
> > the resource first. This will bail out in case we have overlaps with
> > other resources (esp. boot memory and standby memory). The order
> > is now different compared to segment_unload() and segment_unload(), but
> > that should not matter.
>
> You are right that this is ancient, but I think "overlaps with boot
> memory and standby memory" were very relevant, that might actually
> have been the initial reason for this in ancient times (but I do not
> really remember).
>
> With DCSS you can define a fixed start address where the segment will
> be loaded into guest address space. The current code queries this address
> and directly gives it to vmem_add_mapping(), at least if there is no
> overlap with other DCSS segments. If there would be an overlap with
> boot memory, and not checked / rejected in vmem_add_mapping(), the
> following dcss_diag() would probably fail because AFAIR z/VM will
> not allow loading a DCSS with guest memory overlap. So far, so good,
> but the vmem_remove_mapping() on the exit path would then remove
> (part of) boot memory.
>
> That being said, I think your patch prevents this by moving
> request_resource() up, so we should not call vmem_add_mapping()
> for such overlaps. I still want to give it some test, but need
> to find / setup systems with that ancient technology first :-)
>
Verified with DCSS overlapping boot and standby memory, works fine.
As expected, the error message changes, but I don't think that is a
problem, as long as you also remove the old -ENOSPC case / comment
in arch/s390/mm/extmem.c. It is actually more correct now I guess,
-ENOSPC doesn't look like the correct return value anyway.
Thanks for cleaning up! Looks good to me, and removes > 100 LOC,
unless Heiko remembers some other issues from ancient times.
Reviewed-by: Gerald Schaefer
Tested-by: Gerald Schaefer [DCSS]
Re: [PATCH RFC] s390x/vmem: get rid of memory segment list
On Fri, 26 Jun 2020 19:22:53 +0200 Gerald Schaefer wrote: [...] > > That should be OK, as it is also the same message that we already > see for overlaps with other DCSSs. But you probably also should > remove that case from the segment_warning() function for > completeness. ... and also from the comment of segment_load()
Re: [PATCH RFC] s390x/vmem: get rid of memory segment list
On Thu, 25 Jun 2020 17:00:29 +0200
David Hildenbrand wrote:
> I can't come up with a satisfying reason why we still need the memory
> segment list. We used to represent in the list:
> - boot memory
> - standby memory added via add_memory()
> - loaded dcss segments
>
> When loading/unloading dcss segments, we already track them in a
> separate list and check for overlaps
> (arch/s390/mm/extmem.c:segment_overlaps_others()) when loading segments.
>
> The overlap check was introduced for some segments in
> commit b2300b9efe1b ("[S390] dcssblk: add >2G DCSSs support and stacked
> contiguous DCSSs support.")
> and was extended to cover all dcss segments in
> commit ca57114609d1 ("s390/extmem: remove code for 31 bit addressing
> mode").
>
> Although I doubt that overlaps with boot memory and standby memory
> are relevant, let's reshuffle the checks in load_segment() to request
> the resource first. This will bail out in case we have overlaps with
> other resources (esp. boot memory and standby memory). The order
> is now different compared to segment_unload() and segment_unload(), but
> that should not matter.
You are right that this is ancient, but I think "overlaps with boot
memory and standby memory" were very relevant, that might actually
have been the initial reason for this in ancient times (but I do not
really remember).
With DCSS you can define a fixed start address where the segment will
be loaded into guest address space. The current code queries this address
and directly gives it to vmem_add_mapping(), at least if there is no
overlap with other DCSS segments. If there would be an overlap with
boot memory, and not checked / rejected in vmem_add_mapping(), the
following dcss_diag() would probably fail because AFAIR z/VM will
not allow loading a DCSS with guest memory overlap. So far, so good,
but the vmem_remove_mapping() on the exit path would then remove
(part of) boot memory.
That being said, I think your patch prevents this by moving
request_resource() up, so we should not call vmem_add_mapping()
for such overlaps. I still want to give it some test, but need
to find / setup systems with that ancient technology first :-)
One change introduced by this patch is that we no longer
see -ENOSPC and the corresponding error message from extmem code:
"DCSS %s overlaps with used storage and cannot be loaded"
Instead, now we would see -EBUSY and this message:
"%s needs used memory resources and cannot be loaded or queried"
That should be OK, as it is also the same message that we already
see for overlaps with other DCSSs. But you probably also should
remove that case from the segment_warning() function for
completeness.
Regards,
Gerald
Re: [PATCH RFC] s390x/vmem: get rid of memory segment list
> Am 25.06.2020 um 21:38 schrieb Heiko Carstens : > > On Thu, Jun 25, 2020 at 05:00:29PM +0200, David Hildenbrand wrote: >> This smells like a leftover from ancient times, let's get rid of it. We >> can now convert vmem_remove_mapping() into a void function - everybody >> ignored the return value already. > > This buys us what? Except that we get rid of a bit of code? I‘m looking into virtio-mem support for s390x, including vmemmap/vmem optimizations. Virtio-mem adds/removes memory in memory block granularity, which results in one list entry for essentially each memory section. That seems to be easy to avoid. Thanks
Re: [PATCH RFC] s390x/vmem: get rid of memory segment list
On Thu, Jun 25, 2020 at 05:00:29PM +0200, David Hildenbrand wrote: > This smells like a leftover from ancient times, let's get rid of it. We > can now convert vmem_remove_mapping() into a void function - everybody > ignored the return value already. This buys us what? Except that we get rid of a bit of code?
[PATCH RFC] s390x/vmem: get rid of memory segment list
I can't come up with a satisfying reason why we still need the memory
segment list. We used to represent in the list:
- boot memory
- standby memory added via add_memory()
- loaded dcss segments
When loading/unloading dcss segments, we already track them in a
separate list and check for overlaps
(arch/s390/mm/extmem.c:segment_overlaps_others()) when loading segments.
The overlap check was introduced for some segments in
commit b2300b9efe1b ("[S390] dcssblk: add >2G DCSSs support and stacked
contiguous DCSSs support.")
and was extended to cover all dcss segments in
commit ca57114609d1 ("s390/extmem: remove code for 31 bit addressing
mode").
Although I doubt that overlaps with boot memory and standby memory
are relevant, let's reshuffle the checks in load_segment() to request
the resource first. This will bail out in case we have overlaps with
other resources (esp. boot memory and standby memory). The order
is now different compared to segment_unload() and segment_unload(), but
that should not matter.
This smells like a leftover from ancient times, let's get rid of it. We
can now convert vmem_remove_mapping() into a void function - everybody
ignored the return value already.
Cc: Heiko Carstens
Cc: Vasily Gorbik
Cc: Christian Borntraeger
Cc: Andrew Morton
Cc: Gerald Schaefer
Signed-off-by: David Hildenbrand
---
arch/s390/include/asm/pgtable.h | 2 +-
arch/s390/mm/extmem.c | 25 +++
arch/s390/mm/vmem.c | 115 ++--
3 files changed, 21 insertions(+), 121 deletions(-)
diff --git a/arch/s390/include/asm/pgtable.h b/arch/s390/include/asm/pgtable.h
index 19d603bd1f36e..7eb01a5459cdf 100644
--- a/arch/s390/include/asm/pgtable.h
+++ b/arch/s390/include/asm/pgtable.h
@@ -1669,7 +1669,7 @@ static inline swp_entry_t __swp_entry(unsigned long type,
unsigned long offset)
#define kern_addr_valid(addr) (1)
extern int vmem_add_mapping(unsigned long start, unsigned long size);
-extern int vmem_remove_mapping(unsigned long start, unsigned long size);
+extern void vmem_remove_mapping(unsigned long start, unsigned long size);
extern int s390_enable_sie(void);
extern int s390_enable_skey(void);
extern void s390_reset_cmma(struct mm_struct *mm);
diff --git a/arch/s390/mm/extmem.c b/arch/s390/mm/extmem.c
index 9e0aa7aa03ba4..105c09282f8c5 100644
--- a/arch/s390/mm/extmem.c
+++ b/arch/s390/mm/extmem.c
@@ -313,15 +313,10 @@ __segment_load (char *name, int do_nonshared, unsigned
long *addr, unsigned long
goto out_free;
}
- rc = vmem_add_mapping(seg->start_addr, seg->end - seg->start_addr + 1);
-
- if (rc)
- goto out_free;
-
seg->res = kzalloc(sizeof(struct resource), GFP_KERNEL);
if (seg->res == NULL) {
rc = -ENOMEM;
- goto out_shared;
+ goto out_free;
}
seg->res->flags = IORESOURCE_BUSY | IORESOURCE_MEM;
seg->res->start = seg->start_addr;
@@ -335,12 +330,17 @@ __segment_load (char *name, int do_nonshared, unsigned
long *addr, unsigned long
if (rc == SEG_TYPE_SC ||
((rc == SEG_TYPE_SR || rc == SEG_TYPE_ER) && !do_nonshared))
seg->res->flags |= IORESOURCE_READONLY;
+
+ /* Check for overlapping resources before adding the mapping. */
if (request_resource(_resource, seg->res)) {
rc = -EBUSY;
- kfree(seg->res);
- goto out_shared;
+ goto out_free_resource;
}
+ rc = vmem_add_mapping(seg->start_addr, seg->end - seg->start_addr + 1);
+ if (rc)
+ goto out_resource;
+
if (do_nonshared)
diag_cc = dcss_diag(_scode, seg->dcss_name,
_addr, _addr);
@@ -351,14 +351,14 @@ __segment_load (char *name, int do_nonshared, unsigned
long *addr, unsigned long
dcss_diag(_scode, seg->dcss_name,
, );
rc = diag_cc;
- goto out_resource;
+ goto out_mapping;
}
if (diag_cc > 1) {
pr_warn("Loading DCSS %s failed with rc=%ld\n", name, end_addr);
rc = dcss_diag_translate_rc(end_addr);
dcss_diag(_scode, seg->dcss_name,
, );
- goto out_resource;
+ goto out_mapping;
}
seg->start_addr = start_addr;
seg->end = end_addr;
@@ -377,11 +377,12 @@ __segment_load (char *name, int do_nonshared, unsigned
long *addr, unsigned long
(void*) seg->end, segtype_string[seg->vm_segtype]);
}
goto out;
+ out_mapping:
+ vmem_remove_mapping(seg->start_addr, seg->end - seg->start_addr + 1);
out_resource:
release_resource(seg->res);
+ out_free_resource:
kfree(seg->res);
- out_shared:
- vmem_remove_mapping(seg->start_addr, seg->end - seg->start_addr + 1);
out_free:
