On 30.01.2018 19:52, Claudio Imbrenda wrote:
> This is a fix for several issues that were found in the original code
> for storage attributes migration.
>
> Now no bitmap is allocated to keep track of dirty storage attributes;
> the extra bits of the per-memslot bitmap that are always present anyway
> are now used for this purpose.
>
> The code has also been refactored a little to improve readability.
>
> Fixes: 190df4a212a ("KVM: s390: CMMA tracking, ESSA emulation, migration mode")
> Fixes: 4036e3874a1 ("KVM: s390: ioctls to get and set guest storage attributes")
> Signed-off-by: Claudio Imbrenda <imbrenda@xxxxxxxxxxxxxxxxxx>
> ---
So, this mostly makes sense to me, but is rather hard to read as a patch.
Acked-by: Janosch Frank <frankja@xxxxxxxxxxxxxxxxxx>
[...]
> +
> +static unsigned long kvm_s390_next_dirty_cmma(struct kvm *kvm,
> + unsigned long cur)
s/cur/cur_gfn/ ?
> +{
> + struct kvm_memslots *slots = kvm_memslots(kvm);
You might be able to pass *slots instead of *kvm.
> + struct kvm_memory_slot *ms;
> + unsigned long ofs;
> + int slotidx;
> +
> + slotidx = gfn_to_memslot_approx(kvm, cur);
> + ms = slots->memslots + slotidx;
> + ofs = cur - ms->base_gfn;
These 6 lines should be merged, if they don't get too long.
> +
> + if (ms->base_gfn + ms->npages <= cur) {
> + slotidx--;
> + /* If we are above the highest slot, wrap around */
> + if (slotidx < 0)
> + slotidx = slots->used_slots - 1;
> +
> + ms = slots->memslots + slotidx;
> + ofs = 0;
> + }
> + ofs = find_next_bit(kvm_shadow_dirty_bitmap(ms), ms->npages, ofs);
> + while ((slotidx > 0) && (ofs >= ms->npages)) {
> + slotidx--;
> + ms = slots->memslots + slotidx;
> + ofs = find_next_bit(kvm_shadow_dirty_bitmap(ms), ms->npages, 0);
> + }
> + return ms->base_gfn + ofs;
> +}
> +
> +static int kvm_s390_get_cmma(struct kvm *kvm, struct kvm_s390_cmma_log *args,
> + u8 *res, unsigned long bufsize)
So I guess this could possibly be a void function, but you want to
preserve the symmetry in kvm_s390_get_cmma_bits. :)
> +{
> + unsigned long mem_end, cur_gfn, next_gfn, hva, pgstev;
> + struct kvm_memslots *slots = kvm_memslots(kvm);
> + struct kvm_memory_slot *ms;
> +
> + cur_gfn = kvm_s390_next_dirty_cmma(kvm, args->start_gfn);
> + ms = gfn_to_memslot(kvm, cur_gfn);
> + args->count = 0;
> + args->start_gfn = cur_gfn;
> + if (!ms)
> + return 0;
> + next_gfn = kvm_s390_next_dirty_cmma(kvm, cur_gfn + 1);
> + mem_end = slots->memslots[0].base_gfn + slots->memslots[0].npages;
> +
> + while (args->count < bufsize) {
> + hva = gfn_to_hva(kvm, cur_gfn);
> + if (kvm_is_error_hva(hva))
> + break;
return
> + /* decrement only if we actually flipped the bit to 0 */
> + if (test_and_clear_bit(cur_gfn - ms->base_gfn, kvm_shadow_dirty_bitmap(ms)))
> + atomic64_dec(&kvm->arch.cmma_dirty_pages);
> + if (get_pgste(kvm->mm, hva, &pgstev) < 0)
> + pgstev = 0;
> + /* save the value */
> + res[args->count++] = (pgstev >> 24) & 0x43;
> + /*
> + * if the next bit is too far away, stop.
> + * if we reached the previous "next", find the next one
> + */
You could split that up onto the respective lines.
> + if (next_gfn > cur_gfn + KVM_S390_MAX_BIT_DISTANCE)
> + break;
return
> + if (cur_gfn == next_gfn)
> + next_gfn = kvm_s390_next_dirty_cmma(kvm, cur_gfn + 1);
> + /* reached the end of memory or of the buffer, stop */
> + if ((next_gfn >= mem_end) ||
> + (next_gfn - args->start_gfn >= bufsize))
> + break;
> + cur_gfn++;
/* Reached the end of the current memslot, let's take the next one. */
> + if (cur_gfn - ms->base_gfn >= ms->npages) {
> + ms = gfn_to_memslot(kvm, cur_gfn);
> + if (!ms)
> + break;
return
> + }
> + }
> + return 0;
> +}
> +
> /*
> * This function searches for the next page with dirty CMMA attributes, and
> * saves the attributes in the buffer up to either the end of the buffer or
> @@ -1547,97 +1624,54 @@ static int gfn_to_memslot_approx(struct kvm *kvm, gfn_t gfn)
> static int kvm_s390_get_cmma_bits(struct kvm *kvm,
> struct kvm_s390_cmma_log *args)
> {
> - struct kvm_s390_migration_state *s = kvm->arch.migration_state;
> - unsigned long bufsize, hva, pgstev, i, next, cur;
> - int srcu_idx, peek, r = 0, rr;
> - u8 *res;
> -
> - cur = args->start_gfn;
> - i = next = pgstev = 0;
> + unsigned long bufsize;
> + int srcu_idx, peek, ret;
> + u8 *values;
>
> if (unlikely(!kvm->arch.use_cmma))
> return -ENXIO;
> /* Invalid/unsupported flags were specified */
> - if (args->flags & ~KVM_S390_CMMA_PEEK)
> + if (unlikely(args->flags & ~KVM_S390_CMMA_PEEK))
Somebody still has to explain to me when (un)likely is useful and
expected to be used. This feels like being sprinkled for good measure in
a non performance-critical path, i.e. strange.
Attachment:
signature.asc
Description: OpenPGP digital signature
