Re: [Qemu-trivial] [PATCH] cputlb: remove dead function tlb_update_dirty

Re: [Qemu-trivial] [PATCH] cputlb: remove dead function tlb_update_dirty

[Paolo Bonzini]

Il 03/09/2013 09:05, liguang ha scritto:
> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
> ---
>  cputlb.c |   15 ---------------
>  1 files changed, 0 insertions(+), 15 deletions(-)
> 
> diff --git a/cputlb.c b/cputlb.c
> index 977c0ca..08e50e0 100644
> --- a/cputlb.c
> +++ b/cputlb.c
> @@ -169,21 +169,6 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>      return ram_addr;
>  }
>  
> -static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
> -{
> -    ram_addr_t ram_addr;
> -    void *p;
> -
> -    if (tlb_is_dirty_ram(tlb_entry)) {
> -        p = (void *)(uintptr_t)((tlb_entry->addr_write & TARGET_PAGE_MASK)
> -            + tlb_entry->addend);
> -        ram_addr = qemu_ram_addr_from_host_nofail(p);
> -        if (!cpu_physical_memory_is_dirty(ram_addr)) {
> -            tlb_entry->addr_write |= TLB_NOTDIRTY;
> -        }
> -    }
> -}
> -
>  void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length)
>  {
>      CPUState *cpu;
> 

Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>

and CCing qemu-trivial.

Paolo

Re: [Qemu-trivial] [PATCH] cputlb: remove dead function tlb_update_dirty

[Andreas Färber]

Am 03.09.2013 09:22, schrieb Paolo Bonzini:
> Il 03/09/2013 09:05, liguang ha scritto:
>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>> ---
>>  cputlb.c |   15 ---------------
>>  1 files changed, 0 insertions(+), 15 deletions(-)
>>
>> diff --git a/cputlb.c b/cputlb.c
>> index 977c0ca..08e50e0 100644
>> --- a/cputlb.c
>> +++ b/cputlb.c
>> @@ -169,21 +169,6 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>>      return ram_addr;
>>  }
>>  
>> -static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
>> -{
>> -    ram_addr_t ram_addr;
>> -    void *p;
>> -
>> -    if (tlb_is_dirty_ram(tlb_entry)) {
>> -        p = (void *)(uintptr_t)((tlb_entry->addr_write & TARGET_PAGE_MASK)
>> -            + tlb_entry->addend);
>> -        ram_addr = qemu_ram_addr_from_host_nofail(p);
>> -        if (!cpu_physical_memory_is_dirty(ram_addr)) {
>> -            tlb_entry->addr_write |= TLB_NOTDIRTY;
>> -        }
>> -    }
>> -}
>> -
>>  void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length)
>>  {
>>      CPUState *cpu;
>>
> 
> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> and CCing qemu-trivial.

Negative, please keep qemu-trivial out of this. My qom-cpu pull was
already blocked by the s390 and ppc pulls, so let's not add yet another
potentially interfering one to the mix.

IF rth agrees as TCG maintainer that this is not needed in any of his
upcoming refactorings then I'll queue it on qom-cpu. My upcoming
qom-cpu-13 series touches upon pretty much every core CPU file
perceivable, including this cputlb.c.

I also don't understand why qemu-trivial is suddenly picking up Stefan's
arm translation patch, it used to be for unmaintained areas only. But
arm is not my problem.

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-trivial] [PATCH] cputlb: remove dead function tlb_update_dirty

[Paolo Bonzini]

Il 03/09/2013 10:35, Andreas Färber ha scritto:
> Am 03.09.2013 09:22, schrieb Paolo Bonzini:
>> Il 03/09/2013 09:05, liguang ha scritto:
>>> Signed-off-by: liguang <lig.fnst@cn.fujitsu.com>
>>> ---
>>>  cputlb.c |   15 ---------------
>>>  1 files changed, 0 insertions(+), 15 deletions(-)
>>>
>>> diff --git a/cputlb.c b/cputlb.c
>>> index 977c0ca..08e50e0 100644
>>> --- a/cputlb.c
>>> +++ b/cputlb.c
>>> @@ -169,21 +169,6 @@ static inline ram_addr_t qemu_ram_addr_from_host_nofail(void *ptr)
>>>      return ram_addr;
>>>  }
>>>  
>>> -static inline void tlb_update_dirty(CPUTLBEntry *tlb_entry)
>>> -{
>>> -    ram_addr_t ram_addr;
>>> -    void *p;
>>> -
>>> -    if (tlb_is_dirty_ram(tlb_entry)) {
>>> -        p = (void *)(uintptr_t)((tlb_entry->addr_write & TARGET_PAGE_MASK)
>>> -            + tlb_entry->addend);
>>> -        ram_addr = qemu_ram_addr_from_host_nofail(p);
>>> -        if (!cpu_physical_memory_is_dirty(ram_addr)) {
>>> -            tlb_entry->addr_write |= TLB_NOTDIRTY;
>>> -        }
>>> -    }
>>> -}
>>> -
>>>  void cpu_tlb_reset_dirty_all(ram_addr_t start1, ram_addr_t length)
>>>  {
>>>      CPUState *cpu;
>>>
>>
>> Reviewed-by: Paolo Bonzini <pbonzini@redhat.com>
>>
>> and CCing qemu-trivial.
> 
> Negative, please keep qemu-trivial out of this. My qom-cpu pull was
> already blocked by the s390 and ppc pulls, so let's not add yet another
> potentially interfering one to the mix.
> 
> IF rth agrees as TCG maintainer that this is not needed in any of his
> upcoming refactorings then I'll queue it on qom-cpu. My upcoming
> qom-cpu-13 series touches upon pretty much every core CPU file
> perceivable, including this cputlb.c.

Sure.

> I also don't understand why qemu-trivial is suddenly picking up Stefan's
> arm translation patch, it used to be for unmaintained areas only. But
> arm is not my problem.

That patch is also not "trivial", too.

Paolo

Re: [Qemu-trivial] [PATCH] cputlb: remove dead function tlb_update_dirty

[Peter Maydell]

On 3 September 2013 09:35, Andreas Färber <afaerber@suse.de> wrote:
> I also don't understand why qemu-trivial is suddenly picking up Stefan's
> arm translation patch, it used to be for unmaintained areas only. But
> arm is not my problem.

Yeah, I wasn't expecting that either. But I'd reviewed it and it
wasn't a big change that was likely to conflict with anything else
in my queue, so I didn't feel like making a fuss about it.

-- PMM

Re: [Qemu-trivial] [PATCH] cputlb: remove dead function tlb_update_dirty

[Michael Tokarev]

03.09.2013 12:35, Andreas Färber wrote:
> I also don't understand why qemu-trivial is suddenly picking up Stefan's
> arm translation patch, it used to be for unmaintained areas only. But
> arm is not my problem.

Which patch you're talking about?  Is it "target-arm: Report unimplemented
opcodes (LOG_UNIMP)" ?  If yes, that one appears to be trivial as it just
adds some logging before failing an instruction and should not conflict
with other work being done in this area.  Perhaps I was too aggressive
while picking up the backlog.  We should just draw the line *somewhere*, --
eg, it sure is possible to reject spelling fixes for maintained areas
from -trivial (like this arm tree), - will this be productive?

This change (cputlb: remove dead function) appears to be "trivial enough"
for me (after looking at the usage history of this function), and I'd
pick it up without this Andreas's request, too.

As for the "suddenly" - it's not really suddenly, it's because it
has been Cc'd to -trivial (by someone who submitted lots of good
trivial patches before) and actually looks trivial, too.  And also
because subsystem maintainer added his Reviewed-by, apparently (or
hopefully) after noticing it's submitted to -trivial.  I also Cc'd
both maintainers in my notice that it's been applied to -trivial.

Speaking of linux headers sync, I did that once indeed, but don't think
it was a good idea.  It is "trivial" in a sense that it just makes
headers in qemu to be the same as in current kernel (this is easy to
verify), and the tree - at least in some configuration - compiles.
But indeed, the side effects might be quite a bit unexpected and
"non-trivial" - in other words, it is a "trivial change with
non-trivial possible consequences".

HTH.

/mjt

Re: [Qemu-trivial] [PATCH] cputlb: remove dead function tlb_update_dirty

[Andreas Färber]

Am 03.09.2013 13:17, schrieb Michael Tokarev:
> 03.09.2013 12:35, Andreas Färber wrote:
>> I also don't understand why qemu-trivial is suddenly picking up Stefan's
>> arm translation patch, it used to be for unmaintained areas only. But
>> arm is not my problem.
> 
> Which patch you're talking about?  Is it "target-arm: Report unimplemented
> opcodes (LOG_UNIMP)" ?

Yes.

>  If yes, that one appears to be trivial as it just
> adds some logging before failing an instruction and should not conflict
> with other work being done in this area.  Perhaps I was too aggressive
> while picking up the backlog.  We should just draw the line *somewhere*, --

Right, that line is what I'm reminding about here. I feel that lately an
increasing number of contributors and reviewers are deferring patches to
qemu-trivial that don't really belong there IMO. That Anthony doesn't
scale to cover Blue's maintainer work as well shouldn't lead to a surge
on qemu-trivial.

> eg, it sure is possible to reject spelling fixes for maintained areas
> from -trivial (like this arm tree), - will this be productive?

No, spelling fixes are not a concern to me as they are rather unlikely
to cause conflicts with patches being queued by submaintainers. :)

> This change (cputlb: remove dead function) appears to be "trivial enough"
> for me (after looking at the usage history of this function), and I'd
> pick it up without this Andreas's request, too.

Yes. This one here would've been okay usually, as there is no official
maintainer for cputlb.c and it's trivial in the sense that a git-grep
confirms it to be okay. I was just annoyed that I had to defer my pull
twice (sent it out now) because s390x added two CPU loops and then once
that was merged ppc added another loop, too. My upcoming 35+ patch
series qom-cpu-13 may hopefully explain the rest once you see it.

> As for the "suddenly" - it's not really suddenly, it's because it
> has been Cc'd to -trivial (by someone who submitted lots of good
> trivial patches before) and actually looks trivial, too.  And also
> because subsystem maintainer added his Reviewed-by, apparently (or
> hopefully) after noticing it's submitted to -trivial.  I also Cc'd
> both maintainers in my notice that it's been applied to -trivial.

"Suddenly" in the sense that the prupose of qemu-trivial used to be
handling patches that would otherwise fall through the cracks.

So by my understanding, e.g., "target-arm:" => !trivial, and I would've
expected there to be some on-list communication between PMM and you
before CC'ing someone on a "thanks, applied" after the fact.
By contrast, if there's a change to configure or "Fix spelling of" etc.
then you picking it up is highly appreciated. I just don't want
qemu-trivial becoming the least-resistance way of getting patches into
qemu.git that might otherwise get bounced/changed by submaintainers.

Also, I am seeing Paolo pull in huge memory changes but now pinging the
breakage fixes rather than assembling a pull to fix the fallout. ;)

Similarly target-i386 TCG is not suited for qemu-trivial IMO, instead
rth or someone who works on and/or reviews it (rth?) should volunteer as
proper maintainer. With the larger part of the community using KVM these
days, we simply can't have that be handled by the community at large any
more.

So yes, I know you were on vacation and you seem eager to take up work
again, that's great; I'm just cautioning that CC'ing everything on
qemu-trivial (not your fault, you're on the receiving end) can't be the
new solution, so feel encouraged to push back a little. :)

Cheers,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [Qemu-trivial] [PATCH] cputlb: remove dead function tlb_update_dirty

[Li Guang]

在 2013-09-03二的 18:54 +0200，Andreas Färber写道：
> Am 03.09.2013 13:17, schrieb Michael Tokarev:
> > 03.09.2013 12:35, Andreas Färber wrote:
> >> I also don't understand why qemu-trivial is suddenly picking up Stefan's
> >> arm translation patch, it used to be for unmaintained areas only. But
> >> arm is not my problem.
> > 
> > Which patch you're talking about?  Is it "target-arm: Report unimplemented
> > opcodes (LOG_UNIMP)" ?
> 
> Yes.
> 
> >  If yes, that one appears to be trivial as it just
> > adds some logging before failing an instruction and should not conflict
> > with other work being done in this area.  Perhaps I was too aggressive
> > while picking up the backlog.  We should just draw the line *somewhere*, --
> 
> Right, that line is what I'm reminding about here. I feel that lately an
> increasing number of contributors and reviewers are deferring patches to
> qemu-trivial that don't really belong there IMO. That Anthony doesn't
> scale to cover Blue's maintainer work as well shouldn't lead to a surge
> on qemu-trivial.
> 
> > eg, it sure is possible to reject spelling fixes for maintained areas
> > from -trivial (like this arm tree), - will this be productive?
> 
> No, spelling fixes are not a concern to me as they are rather unlikely
> to cause conflicts with patches being queued by submaintainers. :)
> 
> > This change (cputlb: remove dead function) appears to be "trivial enough"
> > for me (after looking at the usage history of this function), and I'd
> > pick it up without this Andreas's request, too.
> 
> Yes. This one here would've been okay usually, as there is no official
> maintainer for cputlb.c and it's trivial in the sense that a git-grep
> confirms it to be okay. I was just annoyed that I had to defer my pull
> twice (sent it out now) because s390x added two CPU loops and then once
> that was merged ppc added another loop, too. My upcoming 35+ patch
> series qom-cpu-13 may hopefully explain the rest once you see it.
> 
> > As for the "suddenly" - it's not really suddenly, it's because it
> > has been Cc'd to -trivial (by someone who submitted lots of good
> > trivial patches before) and actually looks trivial, too.  And also
> > because subsystem maintainer added his Reviewed-by, apparently (or
> > hopefully) after noticing it's submitted to -trivial.  I also Cc'd
> > both maintainers in my notice that it's been applied to -trivial.
> 
> "Suddenly" in the sense that the prupose of qemu-trivial used to be
> handling patches that would otherwise fall through the cracks.
> 
> So by my understanding, e.g., "target-arm:" => !trivial, and I would've
> expected there to be some on-list communication between PMM and you
> before CC'ing someone on a "thanks, applied" after the fact.
> By contrast, if there's a change to configure or "Fix spelling of" etc.
> then you picking it up is highly appreciated. I just don't want
> qemu-trivial becoming the least-resistance way of getting patches into
> qemu.git that might otherwise get bounced/changed by submaintainers.
> 
> Also, I am seeing Paolo pull in huge memory changes but now pinging the
> breakage fixes rather than assembling a pull to fix the fallout. ;)
> 
> Similarly target-i386 TCG is not suited for qemu-trivial IMO, instead
> rth or someone who works on and/or reviews it (rth?) should volunteer as
> proper maintainer. 

 I'd like to maintain cputlb.c, can I?

> With the larger part of the community using KVM these
> days, we simply can't have that be handled by the community at large any
> more.
> 
> So yes, I know you were on vacation and you seem eager to take up work
> again, that's great; I'm just cautioning that CC'ing everything on
> qemu-trivial (not your fault, you're on the receiving end) can't be the
> new solution, so feel encouraged to push back a little. :)
> 
> Cheers,
> Andreas
> 

-- 
Thanks!

Li Guang

Re: [Qemu-trivial] [PATCH] cputlb: remove dead function tlb_update_dirty

[Paolo Bonzini]

Il 03/09/2013 18:54, Andreas Färber ha scritto:
> Also, I am seeing Paolo pull in huge memory changes but now pinging the
> breakage fixes rather than assembling a pull to fix the fallout. ;)

Well, I didn't put myself as memory maintainer for a reason. :)  But it
looks like a pull request is the best thing to do, so I'll find some
time this week to make one.

Paolo