On Wed 23-03-22 20:54:10, Charan Teja Kalla wrote:
> From: Charan Teja Reddy <quic_charante@xxxxxxxxxxx>
>
> The commit 5bd009c7c9a9 ("mm: madvise: return correct bytes advised with
> process_madvise") fixes the issue to return number of bytes that are
> successfully advised before hitting error with iovec elements
> processing. But, when the user passed unmapped ranges in iovec, the
> syscall ignores these holes and continues processing and returns ENOMEM
> in the end, which is same as madvise semantic. This is a problem for
> vector processing where user may want to know how many bytes were
> exactly processed in a iovec element to make better decissions in the
> user space. As in ENOMEM case, we processed all bytes in a iovec element
> but still returned error which will confuse the user whether it is
> failed or succeeded to advise.
Do you have any specific example where the initial semantic is really
problematic or is this mostly a theoretical problem you have found when
reading the code?
> As an example, consider below ranges were passed by the user in struct
> iovec: iovec1(ranges: vma1), iovec2(ranges: vma2 -- vma3 -- hole) and
> iovec3(ranges: vma4). In the current implementation, it fully advise
> iovec1 and iovec2 but just returns number of processed bytes as iovec1
> range. Then user may repeat the processing of iovec2, which is already
> processed, which then returns with ENOMEM. Then user may want to skip
> iovec2 and starts processing from iovec3. Here because of wrong return
> processed bytes, iovec2 is processed twice.
I think you should be much more specific why this is actually a problem.
This would surely be less optimal but is this a correctness issue?
[...]
> + vma = find_vma_prev(mm, start, &prev);
> + if (vma && start > vma->vm_start)
> + prev = vma;
> +
> + blk_start_plug(&plug);
> + for (;;) {
> + /*
> + * It it hits a unmapped address range in the [start, end),
> + * stop processing and return ENOMEM.
> + */
> + if (!vma || start < vma->vm_start) {
> + error = -ENOMEM;
> + goto out;
> + }
> +
> + tmp = vma->vm_end;
> + if (end < tmp)
> + tmp = end;
> +
> + error = madvise_vma_behavior(vma, &prev, start, tmp, behavior);
> + if (error)
> + goto out;
> + tmp_bytes_advised += tmp - start;
> + start = tmp;
> + if (prev && start < prev->vm_end)
> + start = prev->vm_end;
> + if (start >= end)
> + goto out;
> + if (prev)
> + vma = prev->vm_next;
> + else
> + vma = find_vma(mm, start);
> + }
> +out:
> + /*
> + * partial_bytes_advised may contain non-zero bytes indicating
> + * the number of bytes advised before failure. Holds zero incase
> + * of success.
> + */
> + *partial_bytes_advised = error ? tmp_bytes_advised : 0;
Although this looks like a fix I am not sure it is future proof.
madvise_vma_behavior doesn't report which part of the range has been
really processed. I do not think that currently supported madvise modes
for process_madvise support an early break out with return to the
userspace (madvise_cold_or_pageout_pte_range bails on fatal signals for
example) but this can change in the future and then you are back to
"imprecise" return value problem. Yes, this is a theoretical problem
but so it sounds the problem you are trying to fix IMHO. I think it
would be better to live with imprecise return values reporting rather
than aiming for perfection which would be fragile and add a future
maintenance burden.
On the other hand if there are _real_ workloads which suffer from the
existing semantic then sure the above seems to be an appropriate fix
AFAICS.
--
Michal Hocko
SUSE Labs
