| Message ID | 20191205223008.8623-6-andrew.cooper3@citrix.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | xen: Support continuations from tasklets | expand |
On 05.12.2019 23:30, Andrew Cooper wrote: > Some hypercalls tasklets want to create a continuation, rather than fail the > hypercall with a hard error. By the time the tasklet is executing, it is too > late to create the continuation, and even continue_hypercall_on_cpu() doesn't > have enough state to do it correctly. I think it would be quite nice if you made clear what piece of state it is actually missing. To be honest, I don't recall anymore. > All callers of continue_hypercall_on_cpu() have been updated to turn -ERESTART > into a continuation, where appropriate modifications can be made to register > and/or memory parameters. > > This changes the continue_hypercall_on_cpu() behaviour to unconditionally > create a hypercall continuation, in case the tasklet wants to use it, and then > to have arch_hypercall_tasklet_result() cancel the continuation when a result > is available. None of these hypercalls are fast paths. > > Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> > --- > CC: Jan Beulich <JBeulich@suse.com> > CC: Wei Liu <wl@xen.org> > CC: Roger Pau Monné <roger.pau@citrix.com> > CC: Stefano Stabellini <sstabellini@kernel.org> > CC: Julien Grall <julien@xen.org> > CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> > > There is one RFC point. The statement in the header file of "If this function > returns 0 then the function is guaranteed to run at some point in the future." > was never true. In the case of a CPU miss, the hypercall would be blindly > failed with -EINVAL. "Was never true" sounds like "completely broken". Afaict it was true in all cases except the purely hypothetical one of the tasklet ending up executing on the wrong CPU. > The current behaviour with this patch is to not cancel the continuation, which > I think is less bad, but still not great. Thoughts? Well, that's a guest live lock then aiui. Is there any way for the guest to make it out of there? If not, perhaps it'd be "better" to crash the guest? (I don't suppose there's anything we can do to still make the tasklet run on the intended CPU.) > --- a/xen/arch/arm/traps.c > +++ b/xen/arch/arm/traps.c > @@ -1489,6 +1489,7 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long res) > { > struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs; > > + regs->pc += 4; /* Skip over 'hvc #XEN_HYPERCALL_TAG' */ > HYPERCALL_RESULT_REG(regs) = res; > } > > --- a/xen/arch/x86/hypercall.c > +++ b/xen/arch/x86/hypercall.c > @@ -170,6 +170,13 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long res) > { > struct cpu_user_regs *regs = &v->arch.user_regs; > > + /* > + * PV hypercalls are all 2-byte instructions (INT $0x82, SYSCALL). HVM > + * hypercalls are all 3-byte instructions (VMCALL, VMMCALL). > + * > + * Move %rip forwards to complete the continuation. > + */ > + regs->rip += 2 + is_hvm_vcpu(v); > regs->rax = res; > } To leave the system in consistent state, perhaps better to call hypercall_cancel_continuation() along with the PC/RIP updating? > --- a/xen/include/xen/domain.h > +++ b/xen/include/xen/domain.h > @@ -96,9 +96,11 @@ void domctl_lock_release(void); > > /* > * Continue the current hypercall via func(data) on specified cpu. > - * If this function returns 0 then the function is guaranteed to run at some > - * point in the future. If this function returns an error code then the > - * function has not been and will not be executed. > + * > + * This function returns -ERESTART in the success case, and a higher level > + * caller is required to set up a hypercall continuation. func() will be run > + * at some point in the future. If this function returns any other error code > + * then func() has not, and will not be executed. > */ > int continue_hypercall_on_cpu( > unsigned int cpu, long (*func)(void *data), void *data); How is this comment any better wrt the "was never true" comment you've made above? The function still wouldn't be invoked in case of a CPU miss. Jan
On 09/12/2019 16:52, Jan Beulich wrote: > On 05.12.2019 23:30, Andrew Cooper wrote: >> Some hypercalls tasklets want to create a continuation, rather than fail the >> hypercall with a hard error. By the time the tasklet is executing, it is too >> late to create the continuation, and even continue_hypercall_on_cpu() doesn't >> have enough state to do it correctly. > I think it would be quite nice if you made clear what piece of state > it is actually missing. To be honest, I don't recall anymore. How to correctly mutate the registers and/or memory (which is specific to the hypercall subop in some cases). >> All callers of continue_hypercall_on_cpu() have been updated to turn -ERESTART >> into a continuation, where appropriate modifications can be made to register >> and/or memory parameters. >> >> This changes the continue_hypercall_on_cpu() behaviour to unconditionally >> create a hypercall continuation, in case the tasklet wants to use it, and then >> to have arch_hypercall_tasklet_result() cancel the continuation when a result >> is available. None of these hypercalls are fast paths. >> >> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> >> --- >> CC: Jan Beulich <JBeulich@suse.com> >> CC: Wei Liu <wl@xen.org> >> CC: Roger Pau Monné <roger.pau@citrix.com> >> CC: Stefano Stabellini <sstabellini@kernel.org> >> CC: Julien Grall <julien@xen.org> >> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> >> >> There is one RFC point. The statement in the header file of "If this function >> returns 0 then the function is guaranteed to run at some point in the future." >> was never true. In the case of a CPU miss, the hypercall would be blindly >> failed with -EINVAL. > "Was never true" sounds like "completely broken". Afaict it was true > in all cases except the purely hypothetical one of the tasklet ending > up executing on the wrong CPU. There is nothing hypothetical about it. It really will go wrong when a CPU gets offlined. > >> The current behaviour with this patch is to not cancel the continuation, which >> I think is less bad, but still not great. Thoughts? > Well, that's a guest live lock then aiui. It simply continues again. It will livelock only if the hypercall picks a bad cpu all the time. > Is there any way for the guest to make it out of there? If not, perhaps it'd be "better" to > crash the guest? (I don't suppose there's anything we can do to > still make the tasklet run on the intended CPU.) That is why I implemented it like this. If it is a stray interaction with a CPU offline, the next time around it will work fine. Anything else is rather more broken. > >> --- a/xen/arch/arm/traps.c >> +++ b/xen/arch/arm/traps.c >> @@ -1489,6 +1489,7 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long res) >> { >> struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs; >> >> + regs->pc += 4; /* Skip over 'hvc #XEN_HYPERCALL_TAG' */ >> HYPERCALL_RESULT_REG(regs) = res; >> } >> >> --- a/xen/arch/x86/hypercall.c >> +++ b/xen/arch/x86/hypercall.c >> @@ -170,6 +170,13 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long res) >> { >> struct cpu_user_regs *regs = &v->arch.user_regs; >> >> + /* >> + * PV hypercalls are all 2-byte instructions (INT $0x82, SYSCALL). HVM >> + * hypercalls are all 3-byte instructions (VMCALL, VMMCALL). >> + * >> + * Move %rip forwards to complete the continuation. >> + */ >> + regs->rip += 2 + is_hvm_vcpu(v); >> regs->rax = res; >> } > To leave the system in consistent state, perhaps better to call > hypercall_cancel_continuation() along with the PC/RIP updating? Probably, yes. > >> --- a/xen/include/xen/domain.h >> +++ b/xen/include/xen/domain.h >> @@ -96,9 +96,11 @@ void domctl_lock_release(void); >> >> /* >> * Continue the current hypercall via func(data) on specified cpu. >> - * If this function returns 0 then the function is guaranteed to run at some >> - * point in the future. If this function returns an error code then the >> - * function has not been and will not be executed. >> + * >> + * This function returns -ERESTART in the success case, and a higher level >> + * caller is required to set up a hypercall continuation. func() will be run >> + * at some point in the future. If this function returns any other error code >> + * then func() has not, and will not be executed. >> */ >> int continue_hypercall_on_cpu( >> unsigned int cpu, long (*func)(void *data), void *data); > How is this comment any better wrt the "was never true" comment > you've made above? The function still wouldn't be invoked in > case of a CPU miss. Depends now the miss came about. It certainly stands a far better chance now of actually executing. ~Andrew
On 09.12.2019 18:49, Andrew Cooper wrote: > On 09/12/2019 16:52, Jan Beulich wrote: >> On 05.12.2019 23:30, Andrew Cooper wrote: >>> Some hypercalls tasklets want to create a continuation, rather than fail the >>> hypercall with a hard error. By the time the tasklet is executing, it is too >>> late to create the continuation, and even continue_hypercall_on_cpu() doesn't >>> have enough state to do it correctly. >> I think it would be quite nice if you made clear what piece of state >> it is actually missing. To be honest, I don't recall anymore. > > How to correctly mutate the registers and/or memory (which is specific > to the hypercall subop in some cases). Well, in-memory arguments can be accessed as long as the mapping is the right one (which it typically wouldn't be inside a tasklet). Do existing continue_hypercall_on_cpu() users need this? Looking over patch 4 again, I didn't think so. (Which isn't to say that removing the latent issue is not a good thing.) In-register values can be changed as long as the respective exit path will suitably pick up the value, which I thought was always the case. Hence I'm afraid your single reply sentence didn't really clarify matters. I'm sorry if this is just because of me being dense. >>> There is one RFC point. The statement in the header file of "If this function >>> returns 0 then the function is guaranteed to run at some point in the future." >>> was never true. In the case of a CPU miss, the hypercall would be blindly >>> failed with -EINVAL. >> "Was never true" sounds like "completely broken". Afaict it was true >> in all cases except the purely hypothetical one of the tasklet ending >> up executing on the wrong CPU. > > There is nothing hypothetical about it. It really will go wrong when a > CPU gets offlined. Accepted, but it's still not like "completely broken". I would even suppose the case wasn't considered when CPU offlining support was introduced, not when continue_hypercall_on_cpu() came into existence (which presumably is when the comment was written). Anyway - yes, I agree this is a fair solution to the issue at hand. >>> The current behaviour with this patch is to not cancel the continuation, which >>> I think is less bad, but still not great. Thoughts? >> Well, that's a guest live lock then aiui. > > It simply continues again. It will livelock only if the hypercall picks > a bad cpu all the time. Oh, I see I was mislead by continue_hypercall_tasklet_handler() not updating info->cpu, not paying attention to it actually freeing info. Plus a crucial aspect looks to be that there are no "chained" uses of continue_hypercall_on_cpu() anymore (the microcode loading one being gone now) - afaict any such wouldn't guarantee forward progress with this new model (without recording somewhere which CPUs had been dealt with already). Jan
On 10/12/2019 08:55, Jan Beulich wrote: > On 09.12.2019 18:49, Andrew Cooper wrote: >> On 09/12/2019 16:52, Jan Beulich wrote: >>> On 05.12.2019 23:30, Andrew Cooper wrote: >>>> Some hypercalls tasklets want to create a continuation, rather than fail the >>>> hypercall with a hard error. By the time the tasklet is executing, it is too >>>> late to create the continuation, and even continue_hypercall_on_cpu() doesn't >>>> have enough state to do it correctly. >>> I think it would be quite nice if you made clear what piece of state >>> it is actually missing. To be honest, I don't recall anymore. >> How to correctly mutate the registers and/or memory (which is specific >> to the hypercall subop in some cases). > Well, in-memory arguments can be accessed as long as the mapping is > the right one (which it typically wouldn't be inside a tasklet). Do > existing continue_hypercall_on_cpu() users need this? Looking over > patch 4 again, I didn't think so. (Which isn't to say that removing > the latent issue is not a good thing.) > > In-register values can be changed as long as the respective exit > path will suitably pick up the value, which I thought was always > the case. > > Hence I'm afraid your single reply sentence didn't really clarify > matters. I'm sorry if this is just because of me being dense. How, physically, would you arrange for continue_hypercall_on_cpu() to make the requisite state adjustments? Yes - registers and memory can be accessed, but only the hypercall (sub?)op handler knows how to mutate them appropriately. You'd have to copy the mutation logic into continue_hypercall_on_cpu(), and pass in op/subops and a union of all pointers, *and* whatever intermediate state the subop handler needs. Or you can return -ERESTART and let the caller DTRT with the state it has in context, as it would in other cases requiring a continuation. > >>>> There is one RFC point. The statement in the header file of "If this function >>>> returns 0 then the function is guaranteed to run at some point in the future." >>>> was never true. In the case of a CPU miss, the hypercall would be blindly >>>> failed with -EINVAL. >>> "Was never true" sounds like "completely broken". Afaict it was true >>> in all cases except the purely hypothetical one of the tasklet ending >>> up executing on the wrong CPU. >> There is nothing hypothetical about it. It really will go wrong when a >> CPU gets offlined. > Accepted, but it's still not like "completely broken". I didn't mean it like that. I mean "it has never had the property it claimed", which is distinct from "the claim used to be true, but was then accidentally regressed". > I would even > suppose the case wasn't considered when CPU offlining support was > introduced, not when continue_hypercall_on_cpu() came into existence > (which presumably is when the comment was written). > > Anyway - yes, I agree this is a fair solution to the issue at hand. > >>>> The current behaviour with this patch is to not cancel the continuation, which >>>> I think is less bad, but still not great. Thoughts? >>> Well, that's a guest live lock then aiui. >> It simply continues again. It will livelock only if the hypercall picks >> a bad cpu all the time. > Oh, I see I was mislead by continue_hypercall_tasklet_handler() not > updating info->cpu, not paying attention to it actually freeing info. > Plus a crucial aspect looks to be that there are no "chained" uses of > continue_hypercall_on_cpu() anymore (the microcode loading one being > gone now) - afaict any such wouldn't guarantee forward progress with > this new model (without recording somewhere which CPUs had been dealt > with already). I'd forgotten that we had that, but I can't say I'm sad to see the back of it. I recall at the time saying that it wasn't a clever move. For now, I suggest that we ignore that case. If an when a real usecase appears, we can consider making adjustments. ~Andrew
On 10.12.2019 18:55, Andrew Cooper wrote: > On 10/12/2019 08:55, Jan Beulich wrote: >> On 09.12.2019 18:49, Andrew Cooper wrote: >>> On 09/12/2019 16:52, Jan Beulich wrote: >>>> On 05.12.2019 23:30, Andrew Cooper wrote: >>>>> Some hypercalls tasklets want to create a continuation, rather than fail the >>>>> hypercall with a hard error. By the time the tasklet is executing, it is too >>>>> late to create the continuation, and even continue_hypercall_on_cpu() doesn't >>>>> have enough state to do it correctly. >>>> I think it would be quite nice if you made clear what piece of state >>>> it is actually missing. To be honest, I don't recall anymore. >>> How to correctly mutate the registers and/or memory (which is specific >>> to the hypercall subop in some cases). >> Well, in-memory arguments can be accessed as long as the mapping is >> the right one (which it typically wouldn't be inside a tasklet). Do >> existing continue_hypercall_on_cpu() users need this? Looking over >> patch 4 again, I didn't think so. (Which isn't to say that removing >> the latent issue is not a good thing.) >> >> In-register values can be changed as long as the respective exit >> path will suitably pick up the value, which I thought was always >> the case. >> >> Hence I'm afraid your single reply sentence didn't really clarify >> matters. I'm sorry if this is just because of me being dense. > > How, physically, would you arrange for continue_hypercall_on_cpu() to > make the requisite state adjustments? You can't (at least not without having sufficient further context), I agree. Yet ... > Yes - registers and memory can be accessed, but only the hypercall > (sub?)op handler knows how to mutate them appropriately. > > You'd have to copy the mutation logic into continue_hypercall_on_cpu(), > and pass in op/subops and a union of all pointers, *and* whatever > intermediate state the subop handler needs. > > Or you can return -ERESTART and let the caller DTRT with the state it > has in context, as it would in other cases requiring a continuation. ... it continues to be unclear to me whether you're fixing an actual bug here, or just a latent one. The existing uses of continue_hypercall_on_cpu() don't look to require state updates beyond the hypercall return value (or else, aiui, they wouldn't have worked in the first place), and that one had a way to get modified. >>>>> The current behaviour with this patch is to not cancel the continuation, which >>>>> I think is less bad, but still not great. Thoughts? >>>> Well, that's a guest live lock then aiui. >>> It simply continues again. It will livelock only if the hypercall picks >>> a bad cpu all the time. >> Oh, I see I was mislead by continue_hypercall_tasklet_handler() not >> updating info->cpu, not paying attention to it actually freeing info. >> Plus a crucial aspect looks to be that there are no "chained" uses of >> continue_hypercall_on_cpu() anymore (the microcode loading one being >> gone now) - afaict any such wouldn't guarantee forward progress with >> this new model (without recording somewhere which CPUs had been dealt >> with already). > > I'd forgotten that we had that, but I can't say I'm sad to see the back > of it. I recall at the time saying that it wasn't a clever move. > > For now, I suggest that we ignore that case. If an when a real usecase > appears, we can consider making adjustments. Oh, of course - I didn't mean to even remotely suggest anything else. Jan
On 11/12/2019 07:41, Jan Beulich wrote: > On 10.12.2019 18:55, Andrew Cooper wrote: >> On 10/12/2019 08:55, Jan Beulich wrote: >>> On 09.12.2019 18:49, Andrew Cooper wrote: >>>> On 09/12/2019 16:52, Jan Beulich wrote: >>>>> On 05.12.2019 23:30, Andrew Cooper wrote: >>>>>> Some hypercalls tasklets want to create a continuation, rather than fail the >>>>>> hypercall with a hard error. By the time the tasklet is executing, it is too >>>>>> late to create the continuation, and even continue_hypercall_on_cpu() doesn't >>>>>> have enough state to do it correctly. >>>>> I think it would be quite nice if you made clear what piece of state >>>>> it is actually missing. To be honest, I don't recall anymore. >>>> How to correctly mutate the registers and/or memory (which is specific >>>> to the hypercall subop in some cases). >>> Well, in-memory arguments can be accessed as long as the mapping is >>> the right one (which it typically wouldn't be inside a tasklet). Do >>> existing continue_hypercall_on_cpu() users need this? Looking over >>> patch 4 again, I didn't think so. (Which isn't to say that removing >>> the latent issue is not a good thing.) >>> >>> In-register values can be changed as long as the respective exit >>> path will suitably pick up the value, which I thought was always >>> the case. >>> >>> Hence I'm afraid your single reply sentence didn't really clarify >>> matters. I'm sorry if this is just because of me being dense. >> How, physically, would you arrange for continue_hypercall_on_cpu() to >> make the requisite state adjustments? > You can't (at least not without having sufficient further context), > I agree. Yet ... > >> Yes - registers and memory can be accessed, but only the hypercall >> (sub?)op handler knows how to mutate them appropriately. >> >> You'd have to copy the mutation logic into continue_hypercall_on_cpu(), >> and pass in op/subops and a union of all pointers, *and* whatever >> intermediate state the subop handler needs. >> >> Or you can return -ERESTART and let the caller DTRT with the state it >> has in context, as it would in other cases requiring a continuation. > ... it continues to be unclear to me whether you're fixing an actual > bug here, or just a latent one. I'm not fixing any bug. I am making a fundamental change in behaviour, so tasklet context can use -ERESTART. Tasklet context doesn't even know what the primary hypercall index needs to be to correctly create a continuation. ~Andrew
diff --git a/xen/arch/arm/traps.c b/xen/arch/arm/traps.c index a20474f87c..5d35d2b7e9 100644 --- a/xen/arch/arm/traps.c +++ b/xen/arch/arm/traps.c @@ -1489,6 +1489,7 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long res) { struct cpu_user_regs *regs = &v->arch.cpu_info->guest_cpu_user_regs; + regs->pc += 4; /* Skip over 'hvc #XEN_HYPERCALL_TAG' */ HYPERCALL_RESULT_REG(regs) = res; } diff --git a/xen/arch/x86/hypercall.c b/xen/arch/x86/hypercall.c index 7f299d45c6..42d95f9b9a 100644 --- a/xen/arch/x86/hypercall.c +++ b/xen/arch/x86/hypercall.c @@ -170,6 +170,13 @@ void arch_hypercall_tasklet_result(struct vcpu *v, long res) { struct cpu_user_regs *regs = &v->arch.user_regs; + /* + * PV hypercalls are all 2-byte instructions (INT $0x82, SYSCALL). HVM + * hypercalls are all 3-byte instructions (VMCALL, VMMCALL). + * + * Move %rip forwards to complete the continuation. + */ + regs->rip += 2 + is_hvm_vcpu(v); regs->rax = res; } diff --git a/xen/common/domain.c b/xen/common/domain.c index ab7e4d09c0..eb69db3078 100644 --- a/xen/common/domain.c +++ b/xen/common/domain.c @@ -1665,7 +1665,7 @@ static void continue_hypercall_tasklet_handler(void *data) { struct migrate_info *info = data; struct vcpu *v = info->vcpu; - long res = -EINVAL; + long res = -ERESTART; /* Wait for vcpu to sleep so that we can access its register state. */ vcpu_sleep_sync(v); @@ -1675,7 +1675,8 @@ static void continue_hypercall_tasklet_handler(void *data) if ( likely(info->cpu == smp_processor_id()) ) res = info->func(info->data); - arch_hypercall_tasklet_result(v, res); + if ( res != -ERESTART ) + arch_hypercall_tasklet_result(v, res); this_cpu(continue_info) = NULL; @@ -1726,8 +1727,8 @@ int continue_hypercall_on_cpu( tasklet_schedule_on_cpu(&info->vcpu->continue_hypercall_tasklet, cpu); - /* Dummy return value will be overwritten by tasklet. */ - return 0; + /* Start a continuation. Value will be overwritten by the tasklet. */ + return -ERESTART; } /* diff --git a/xen/include/xen/domain.h b/xen/include/xen/domain.h index 1cb205d977..83c737bca4 100644 --- a/xen/include/xen/domain.h +++ b/xen/include/xen/domain.h @@ -96,9 +96,11 @@ void domctl_lock_release(void); /* * Continue the current hypercall via func(data) on specified cpu. - * If this function returns 0 then the function is guaranteed to run at some - * point in the future. If this function returns an error code then the - * function has not been and will not be executed. + * + * This function returns -ERESTART in the success case, and a higher level + * caller is required to set up a hypercall continuation. func() will be run + * at some point in the future. If this function returns any other error code + * then func() has not, and will not be executed. */ int continue_hypercall_on_cpu( unsigned int cpu, long (*func)(void *data), void *data); @@ -106,6 +108,9 @@ int continue_hypercall_on_cpu( /* * Companion to continue_hypercall_on_cpu(), to feed func()'s result back into * vcpu regsiter state. + * + * Must undo the effects of the hypercall continuation created by + * continue_hypercall_on_cpu()'s caller. */ void arch_hypercall_tasklet_result(struct vcpu *v, long res);
Some hypercalls tasklets want to create a continuation, rather than fail the hypercall with a hard error. By the time the tasklet is executing, it is too late to create the continuation, and even continue_hypercall_on_cpu() doesn't have enough state to do it correctly. All callers of continue_hypercall_on_cpu() have been updated to turn -ERESTART into a continuation, where appropriate modifications can be made to register and/or memory parameters. This changes the continue_hypercall_on_cpu() behaviour to unconditionally create a hypercall continuation, in case the tasklet wants to use it, and then to have arch_hypercall_tasklet_result() cancel the continuation when a result is available. None of these hypercalls are fast paths. Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com> --- CC: Jan Beulich <JBeulich@suse.com> CC: Wei Liu <wl@xen.org> CC: Roger Pau Monné <roger.pau@citrix.com> CC: Stefano Stabellini <sstabellini@kernel.org> CC: Julien Grall <julien@xen.org> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com> There is one RFC point. The statement in the header file of "If this function returns 0 then the function is guaranteed to run at some point in the future." was never true. In the case of a CPU miss, the hypercall would be blindly failed with -EINVAL. The current behaviour with this patch is to not cancel the continuation, which I think is less bad, but still not great. Thoughts? --- xen/arch/arm/traps.c | 1 + xen/arch/x86/hypercall.c | 7 +++++++ xen/common/domain.c | 9 +++++---- xen/include/xen/domain.h | 11 ++++++++--- 4 files changed, 21 insertions(+), 7 deletions(-)