| Message ID | 20090902074905.GB25711@chrom.inf.tu-dresden.de |
|---|---|
| State | Superseded |
| Headers | show |
Its wednesday again, time to resend a patch to the list. Until today the following happened in this endless story: 1. I wrote an RTC driver according to the manual which works on real hardware but not on Qemu. 2. I traced the bug back to the first public version of Bochs. Its seems to be also present in all derived version: Qemu, KVM, Xen, VBox... 3. I send a patch to the Qemu list at least 4 times. 4. I never got a single response to my patch. Thus I have to assume that device models in Qemu are unmaintained: further resending does not make any sense in this case. I still hope somebody will step up and take a stake in this story. Otherwise Qemu will be more and more unusable. For me and probably others. Greetings, Bernhard Kauer On Wed, Sep 02, 2009 at 09:49:05AM +0200, Bernhard Kauer wrote: > The RTC emulation does not set the IRQ flags independent of the IRQ enable bits. > > The original MC146818A datasheet from 1984 notes: > "flag bits in Register C [...] are set independent of the > state of the corresponding enable bits in Register B" > Similar sections can be found in newer documentation e.g. in rtc82885. > > Qemu and Bochs set the IRQ flags only if they are enabled, > which breaks drivers polling on them. > > The following patch corrects this for the update-ended-flag in Qemu only. > It does not fix the handling of the other flags. > > > Signed-off-by: Bernhard Kauer <kauer@tudos.org> > > > diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c > index 2022548..2b040a7 100644 > --- a/hw/mc146818rtc.c > +++ b/hw/mc146818rtc.c > @@ -421,9 +421,10 @@ static void rtc_update_second2(void *opaque) > } > > /* update ended interrupt */ > + s->cmos_data[RTC_REG_C] |= REG_C_UF; > if (s->cmos_data[RTC_REG_B] & REG_B_UIE) { > - s->cmos_data[RTC_REG_C] |= 0x90; > - rtc_irq_raise(s->irq); > + s->cmos_data[RTC_REG_C] |= REG_C_IRQF; > + rtc_irq_raise(s->irq); > } > > /* clear update in progress bit */ > > >
On (Wed) Sep 09 2009 [14:18:17], Bernhard Kauer wrote: > Its wednesday again, time to resend a patch to the list. > Until today the following happened in this endless story: > > 1. I wrote an RTC driver according to the manual which works on real hardware but not on Qemu. > 2. I traced the bug back to the first public version of Bochs. Its seems to be also > present in all derived version: Qemu, KVM, Xen, VBox... > 3. I send a patch to the Qemu list at least 4 times. > 4. I never got a single response to my patch. It's included in Anthony's staging tree: http://repo.or.cz/w/qemu/aliguori-queue.git which means it can be flushed to the main git tree in the near future. The current process of picking up patches is not verbose at all; developers either have to wait till the patch magically appears in the git tree or keep resending it. Amit
Hi Bernhard, Am 09.09.2009 14:18, schrieb Bernhard Kauer: > Its wednesday again, time to resend a patch to the list. > Until today the following happened in this endless story: > > 1. I wrote an RTC driver according to the manual which works on real hardware but not on Qemu. > 2. I traced the bug back to the first public version of Bochs. Its seems to be also > present in all derived version: Qemu, KVM, Xen, VBox... > 3. I send a patch to the Qemu list at least 4 times. > 4. I never got a single response to my patch. > > Thus I have to assume that device models in Qemu are unmaintained: further resending > does not make any sense in this case. > > I still hope somebody will step up and take a stake in this story. Otherwise Qemu > will be more and more unusable. For me and probably others. The patch seems to be in Anthony's queue at least (see http://repo.or.cz/w/qemu/aliguori-queue.git), so it might be committed the next time he pushes. Kevin
On Wed, Sep 09, 2009 at 05:53:49PM +0530, Amit Shah wrote: > On (Wed) Sep 09 2009 [14:18:17], Bernhard Kauer wrote: > > Its wednesday again, time to resend a patch to the list. > > Until today the following happened in this endless story: > > > > 1. I wrote an RTC driver according to the manual which works on real hardware but not on Qemu. > > 2. I traced the bug back to the first public version of Bochs. Its seems to be also > > present in all derived version: Qemu, KVM, Xen, VBox... > > 3. I send a patch to the Qemu list at least 4 times. > > 4. I never got a single response to my patch. > > It's included in Anthony's staging tree: > > http://repo.or.cz/w/qemu/aliguori-queue.git > > which means it can be flushed to the main git tree in the near future. Ah i missed this. > The current process of picking up patches is not verbose at all; > developers either have to wait till the patch magically appears in the > git tree or keep resending it. Yes, it would be nice to get a mail when somebody picked up the patch. Thanks, Bernhard
Bernhard Kauer wrote: > Its wednesday again, time to resend a patch to the list. > Until today the following happened in this endless story: > Really, the whining just makes me want to drop your patch.. The first patch you sent didn't have [PATCH] in the subject. There are hundreds of mails that are sent to the list every day. If you don't mark a patch in a way that allows automatic filtering, it will get dropped. When you resent, you did include [PATCH]. That patch made it into my staging tree. See http://repo.or.cz/w/qemu/aliguori-queue.git. We had a holiday this past weekend so I'm a little slow in flushing that queue to master. Regards, Anthony Liguori
On Wed, Sep 09, 2009 at 08:00:28AM -0500, Anthony Liguori wrote: > Bernhard Kauer wrote: > >Its wednesday again, time to resend a patch to the list. > >Until today the following happened in this endless story: > > Really, the whining just makes me want to drop your patch.. > > The first patch you sent didn't have [PATCH] in the subject. There > are hundreds of mails that are sent to the list every day. If you > don't mark a patch in a way that allows automatic filtering, it will > get dropped. > > When you resent, you did include [PATCH]. That patch made it into > my staging tree. See http://repo.or.cz/w/qemu/aliguori-queue.git. > We had a holiday this past weekend so I'm a little slow in flushing > that queue to master. > It would be really nice to get mail that your patch was applied to staging. Casual contributor don't know that there is staging tree that should be checked or separate mailing list that he should subscribe too. -- Gleb.
On Wed, Sep 09, 2009 at 08:00:28AM -0500, Anthony Liguori wrote: > The first patch you sent didn't have [PATCH] in the subject. There > are hundreds of mails that are sent to the list every day. If you > don't mark a patch in a way that allows automatic filtering, it will > get dropped. That was at a time I thought: it cann't be true, that nobody noticed the bug that many years. Time proved otherwise... > When you resent, you did include [PATCH]. That patch made it into > my staging tree. See http://repo.or.cz/w/qemu/aliguori-queue.git. > We had a holiday this past weekend so I'm a little slow in flushing > that queue to master. Your speed is not the problem. Getting a mail when you picked up the patch would have avoided my frustration. A mail to the mailinglist with all patches that made it in your tree could be also an option. Greetings, Bernhard Kauer
Gleb Natapov wrote: > On Wed, Sep 09, 2009 at 08:00:28AM -0500, Anthony Liguori wrote: > >> Bernhard Kauer wrote: >> >>> Its wednesday again, time to resend a patch to the list. >>> Until today the following happened in this endless story: >>> >> Really, the whining just makes me want to drop your patch.. >> >> The first patch you sent didn't have [PATCH] in the subject. There >> are hundreds of mails that are sent to the list every day. If you >> don't mark a patch in a way that allows automatic filtering, it will >> get dropped. >> >> When you resent, you did include [PATCH]. That patch made it into >> my staging tree. See http://repo.or.cz/w/qemu/aliguori-queue.git. >> We had a holiday this past weekend so I'm a little slow in flushing >> that queue to master. >> >> > It would be really nice to get mail that your patch was applied to staging. > Casual contributor don't know that there is staging tree that should be > checked or separate mailing list that he should subscribe too. > I would like to use patchworks for this but I haven't figured out a sane way to integrate it into my workflow yet. Regards, Anthony Liguori > -- > Gleb. >
On 09/09/2009 04:26 PM, Gleb Natapov wrote: > On Wed, Sep 09, 2009 at 08:00:28AM -0500, Anthony Liguori wrote: > >> Bernhard Kauer wrote: >> >>> Its wednesday again, time to resend a patch to the list. >>> Until today the following happened in this endless story: >>> >> Really, the whining just makes me want to drop your patch.. >> >> The first patch you sent didn't have [PATCH] in the subject. There >> are hundreds of mails that are sent to the list every day. If you >> don't mark a patch in a way that allows automatic filtering, it will >> get dropped. >> >> When you resent, you did include [PATCH]. That patch made it into >> my staging tree. See http://repo.or.cz/w/qemu/aliguori-queue.git. >> We had a holiday this past weekend so I'm a little slow in flushing >> that queue to master. >> >> > It would be really nice to get mail that your patch was applied to staging. > Casual contributor don't know that there is staging tree that should be > checked or separate mailing list that he should subscribe too. > Also, if the queue can be a branch of the main repository, a 'git fetch' will get both the master branch and the queue.
On (Wed) Sep 09 2009 [08:00:28], Anthony Liguori wrote: > Bernhard Kauer wrote: >> Its wednesday again, time to resend a patch to the list. >> Until today the following happened in this endless story: >> > > Really, the whining just makes me want to drop your patch.. Let's be courteous and not drive away contributors. There's no relation between accepting patches and some nudging on the contributor's part especially when there's no feedback on patches; positive or negative. If there'd be a daemon sending a mail saying the patch is in some staging queue it'll reduce everyone's effort. Such extra mails definitely aren't a problem. If it's later reverted because of any kind of failure, again a polite mail wouldn't hurt. Amit
On Thu, Sep 10, 2009 at 12:33:36PM +0530, Amit Shah wrote: > On (Wed) Sep 09 2009 [08:00:28], Anthony Liguori wrote: > > Bernhard Kauer wrote: > >> Its wednesday again, time to resend a patch to the list. > >> Until today the following happened in this endless story: > >> > > > > Really, the whining just makes me want to drop your patch.. > > Let's be courteous and not drive away contributors. > > There's no relation between accepting patches and some nudging on the > contributor's part especially when there's no feedback on patches; > positive or negative. > > If there'd be a daemon sending a mail saying the patch is in some > staging queue it'll reduce everyone's effort. Such extra mails > definitely aren't a problem. If it's later reverted because of any kind > of failure, again a polite mail wouldn't hurt. May I repeat my previous question: Is there some CODING file or something that has some hints? If not, would a patch to add such a file be welcome? I have the impression that some rules for the Linux kernel development apply, but I wouldn't know out of my head where to find the guide for that. (Things that should be in such a file, at least: - Patches should be sent to this list - Patches should have [PATCH] in subject - Patches should be sent as a new thread even when they are in response to some other mail (not sure about that? I didn't really understand what that one response I got meant to say, I am just guessing. I also think it makes quite a mess for people just reading the list and not trying to apply the patches). - Patches are preferred inline, even better created by git format-patch (not sure about that either) - URL of staging tree - Possibly coding style rules once decided (no "pointless" casts of void *)? )
On (Thu) Sep 10 2009 [09:56:44], Reimar Döffinger wrote: > May I repeat my previous question: Is there some CODING file or > something that has some hints? There is: CODIING_STYLE. But that file talks about how to write qemu code. > If not, would a patch to add such a file be welcome? Sure. > I have the impression that some rules for the Linux kernel development > apply, but I wouldn't know out of my head where to find the guide for > that. > (Things that should be in such a file, at least: > - Patches should be sent to this list Yes > - Patches should have [PATCH] in subject Yes > - Patches should be sent as a new thread even when they are in response > to some other mail (not sure about that? I didn't really understand > what that one response I got meant to say, I am just guessing. I also > think it makes quite a mess for people just reading the list and not > trying to apply the patches). Right; that's what seems to be requested. > - Patches are preferred inline, even better created by git format-patch > (not sure about that either) This is right too > - URL of staging tree Might help; but various developers might have their own staging trees (as it now is). If things change in the future, this will have to be modified. > - Possibly coding style rules once decided (no "pointless" casts of void > *)? That should be covered in CODING_STYLE. The list you're talking about makes sense for something like SUBMITTING_PATCHES or CONTRIBUTING. Amit
On Thu, 10 Sep 2009 15:38:04 +0530 Amit Shah <amit.shah@redhat.com> wrote: > > - URL of staging tree > > Might help; but various developers might have their own staging trees > (as it now is). If things change in the future, this will have to be > modified. I think he's talking about Anthony's staging, which is the most important one for those submitting patches. I might be wrong about this but, the Linux kernel way of having people maintaining subsystems didn't work out here yet. This discussion makes me think that the subject should be 'QEMU development process'.
On (Thu) Sep 10 2009 [08:47:13], Luiz Capitulino wrote: > On Thu, 10 Sep 2009 15:38:04 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > > > - URL of staging tree > > > > Might help; but various developers might have their own staging trees > > (as it now is). If things change in the future, this will have to be > > modified. > > I think he's talking about Anthony's staging, which is the most > important one for those submitting patches. Right; and what I mean is like Anthony has his own queue, others might have theirs too. If something along the lines of what Avi suggested (having branches in the qemu repo for staging) gets implemented, this will have to be redone. > I might be wrong about this but, the Linux kernel way of having > people maintaining subsystems didn't work out here yet. It can easily be done. The process has to scale considering the number of submissions we're seeing. > This discussion makes me think that the subject should be > 'QEMU development process'. Amit
On Thu, 10 Sep 2009 17:31:21 +0530 Amit Shah <amit.shah@redhat.com> wrote: > On (Thu) Sep 10 2009 [08:47:13], Luiz Capitulino wrote: > > On Thu, 10 Sep 2009 15:38:04 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > > > > - URL of staging tree > > > > > > Might help; but various developers might have their own staging trees > > > (as it now is). If things change in the future, this will have to be > > > modified. > > > > I think he's talking about Anthony's staging, which is the most > > important one for those submitting patches. > > Right; and what I mean is like Anthony has his own queue, others might > have theirs too. If something along the lines of what Avi suggested > (having branches in the qemu repo for staging) gets implemented, this > will have to be redone. Yes, the point is: if we have a central tree where patches are merged before going to master, the address of that tree should be listed somewhere. > > I might be wrong about this but, the Linux kernel way of having > > people maintaining subsystems didn't work out here yet. > > It can easily be done. The process has to scale considering the number > of submissions we're seeing. I agree, with more maintainers QEMU would evolve a lot faster and I think this is becoming an issue. On the other hand I don't think we will have them by distributing responsibilities, those interested should setup a tree and start collecting patches.
On Thu, Sep 10, 2009 at 09:29:38AM -0300, Luiz Capitulino wrote: > On Thu, 10 Sep 2009 17:31:21 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > It can easily be done. The process has to scale considering the number > > of submissions we're seeing. > > I agree, with more maintainers QEMU would evolve a lot faster and I think > this is becoming an issue. > > On the other hand I don't think we will have them by distributing > responsibilities, those interested should setup a tree and > start collecting patches. You're saying that just like that, but if you just set up your own tree, at least if you are not a git guru you end up with the same mess as I have now: The official qemu git tree, Stefan Weil's git tree and the kvm tree, all have slightly different eepro100 code and for each one I have to do merges since I have no idea which path has a chance to get patches accepted (since after all I got no response anywhere). For me that is simply too much effort (I am after all a FFmpeg/MPlayer developer in the first place) and I'll probably just drop all the non-trivial changes I made because they have a too high merging effort. Greetings, Reimar Döffinger
On Thu, 10 Sep 2009 14:51:09 +0200 Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > On Thu, Sep 10, 2009 at 09:29:38AM -0300, Luiz Capitulino wrote: > > On Thu, 10 Sep 2009 17:31:21 +0530 > > Amit Shah <amit.shah@redhat.com> wrote: > > > It can easily be done. The process has to scale considering the number > > > of submissions we're seeing. > > > > I agree, with more maintainers QEMU would evolve a lot faster and I think > > this is becoming an issue. > > > > On the other hand I don't think we will have them by distributing > > responsibilities, those interested should setup a tree and > > start collecting patches. > > You're saying that just like that, but if you just set up your own tree, > at least if you are not a git guru you end up with the same mess as I > have now: > The official qemu git tree, Stefan Weil's git tree and the kvm tree, all > have slightly different eepro100 code and for each one I have to do > merges since I have no idea which path has a chance to get patches > accepted (since after all I got no response anywhere). This is a sightly different problem that is related to the fact that qemu is forked. > For me that is simply too much effort (I am after all a FFmpeg/MPlayer developer > in the first place) and I'll probably just drop all the non-trivial > changes I made because they have a too high merging effort. Not sure if this is what you meant but, having different trees for different subsystems is one of the best ways to get large scale development and that's how git was designed to work.
Amit Shah wrote: > On (Wed) Sep 09 2009 [08:00:28], Anthony Liguori wrote: > >> Bernhard Kauer wrote: >> >>> Its wednesday again, time to resend a patch to the list. >>> Until today the following happened in this endless story: >>> >>> >> Really, the whining just makes me want to drop your patch.. >> > > Let's be courteous and not drive away contributors. > > There's no relation between accepting patches and some nudging on the > contributor's part especially when there's no feedback on patches; > positive or negative. > > If there'd be a daemon sending a mail saying the patch is in some > staging queue it'll reduce everyone's effort. Such extra mails > definitely aren't a problem. If it's later reverted because of any kind > of failure, again a polite mail wouldn't hurt. > The problem is patch volume. We often see hundreds of patches a day. If typing a mail for each patch takes 2 minutes, that's potentially hours spent just on sending these mails. What I really need is some way to automatically generate these notifications. It's pretty easy to send a mail when a patch enters the queue but it's more difficult to send a mail when a patch is removed from the queue via a rebase. Often times, I remove patches from the queue simply because I'm not the right path for the patches to be committed from (like linux-user). Usually, if I remove a patch from the queue because something is wrong with it, I send out an email explaining what is wrong. Regards, Anthony Liguori > Amit >
Luiz Capitulino wrote: > On Thu, 10 Sep 2009 15:38:04 +0530 > Amit Shah <amit.shah@redhat.com> wrote: > > >>> - URL of staging tree >>> >> Might help; but various developers might have their own staging trees >> (as it now is). If things change in the future, this will have to be >> modified. >> > > I think he's talking about Anthony's staging, which is the most > important one for those submitting patches. > > I might be wrong about this but, the Linux kernel way of having > people maintaining subsystems didn't work out here yet. > I believe this is the long term solution but I do think our previous attempt wasn't successful. As we do a better job isolating the subsystems and people get more familiar with the internal bits of QEMU, I believe we'll naturally migrate to this. Regards, Anthony Liguori
On (Thu) Sep 10 2009 [08:56:34], Anthony Liguori wrote: >> >> If there'd be a daemon sending a mail saying the patch is in some >> staging queue it'll reduce everyone's effort. Such extra mails >> definitely aren't a problem. If it's later reverted because of any kind >> of failure, again a polite mail wouldn't hurt. >> > > The problem is patch volume. We often see hundreds of patches a day. > If typing a mail for each patch takes 2 minutes, that's potentially > hours spent just on sending these mails. How about a bot that goes through your mailbox (or your git queue) and sends the email? One reply-to-all email for each unique hash you have against qemu-master? > What I really need is some way to automatically generate these > notifications. It's pretty easy to send a mail when a patch enters the > queue but it's more difficult to send a mail when a patch is removed > from the queue via a rebase. Often times, I remove patches from the > queue simply because I'm not the right path for the patches to be > committed from (like linux-user). ... and store the commit ids and subject in some file. When you rebase and that particular commit-id + subject combination is not present, an auto-email can be triggered > Usually, if I remove a patch from the queue because something is wrong > with it, I send out an email explaining what is wrong. ... but this is simpler and more efficient also because you can give the reason along with the email rather than people mailing you later asking why the patch was dropped. Amit
Amit Shah wrote: > On (Thu) Sep 10 2009 [08:56:34], Anthony Liguori wrote: > >>> If there'd be a daemon sending a mail saying the patch is in some >>> staging queue it'll reduce everyone's effort. Such extra mails >>> definitely aren't a problem. If it's later reverted because of any kind >>> of failure, again a polite mail wouldn't hurt. >>> >>> >> The problem is patch volume. We often see hundreds of patches a day. >> If typing a mail for each patch takes 2 minutes, that's potentially >> hours spent just on sending these mails. >> > > How about a bot that goes through your mailbox (or your git queue) and > sends the email? One reply-to-all email for each unique hash you have > against qemu-master? > Yeah, it's easy to do that for initial commit. Git lacks the hooks to do it for when you drop something via git rebase though. A few weeks ago, a bunch of commits slipped through with Message-Id tags. That was my attempt to add some smarts to git to allow for this but it didn't work out the way I wanted it to. > ... and store the commit ids and subject in some file. When you rebase > and that particular commit-id + subject combination is not present, an > auto-email can be triggered > Yeah, I tried that without a lot of success. Basically, my work flow is the following: 1) Mail client filters things that look like patches into a folder 2) I do a once through to remove obvious dups or non-patches 3) I have some scripts that pull patches into an mbox format. They are smart about identifying threads and trying to find a sane sort order. 4) I have more scripts that let me split the mbox into smaller mboxes. This is because git am is not very forgiving and having to do a git am --abort can lead to a lot of wasted effort if the mbox has 100+ patches 5) I git am the smaller mboxes into staging 6) I push to staging 7) I do a build check, and fix errors, and push to staging 8) I start the testing cycle, removing patches based on testing results and pushing to staging 9) I manually review each patch that's survived testing 10) I do final build/test, then push to master >> Usually, if I remove a patch from the queue because something is wrong >> with it, I send out an email explaining what is wrong. >> > > ... but this is simpler and more efficient also because you can give the > reason along with the email rather than people mailing you later asking > why the patch was dropped. > And commit mails are sent whenever something goes to master. So someone should get a mail when a patch goes to master or when I reject it. To me, the area to optimize is reducing the time things are spent in staging. I don't think people really need visibility into staging. Regards, Anthony Liguori
On 09/10/2009 04:56 PM, Anthony Liguori wrote: > > The problem is patch volume. We often see hundreds of patches a day. > If typing a mail for each patch takes 2 minutes, that's potentially > hours spent just on sending these mails. > You exaggerate. The average rate is 13 patches per calendar day. The bulk of the patches are in patchsets which can be acked as a set, not once per patch. > What I really need is some way to automatically generate these > notifications. It's pretty easy to send a mail when a patch enters > the queue but it's more difficult to send a mail when a patch is > removed from the queue via a rebase. Often times, I remove patches > from the queue simply because I'm not the right path for the patches > to be committed from (like linux-user). I think more per-patch attention is needed, not less, for example see this commit: commit e09a5267adf0af25b55d2abaf06e288b2d9537ea Author: Dustin Kirkland <kirkland@canonical.com> Date: Thu Sep 3 12:31:33 2009 -0500 qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back to non-accelerated mode qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back to non-accelerated mode We're seeing segfaults on systems without access to /dev/kvm. It looks like the global kvm_allowed is being set just a little too late in vl.c. This patch moves the kvm initialization a bit higher in the vl.c main, just after options processing, and solves the segfaults. We're carrying this patch in Ubuntu 9.10 Alpha. Please apply upstream, or advise if and why this might not be the optimal solution. Signed-off-by: Dustin Kirkland <kirkland@canonical.com> Move the kvm_init() call a bit higher to fix a segfault when /dev/kvm is not available. The kvm_allowed global needs to be set correctly a little earlier. Signed-off-by: Dustin Kirkland <kirkland@canonical.com> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> There are many examples like this in the tree which is a pity. Others include parts of an email conversation. I'd like history to look better than this.
On (Thu) Sep 10 2009 [09:12:21], Anthony Liguori wrote: > > Yeah, I tried that without a lot of success. Basically, my work flow is > the following: > > 1) Mail client filters things that look like patches into a folder > 2) I do a once through to remove obvious dups or non-patches > 3) I have some scripts that pull patches into an mbox format. They are > smart about identifying threads and trying to find a sane sort order. > 4) I have more scripts that let me split the mbox into smaller mboxes. > This is because git am is not very forgiving and having to do a git am > --abort can lead to a lot of wasted effort if the mbox has 100+ patches > 5) I git am the smaller mboxes into staging > 6) I push to staging > 7) I do a build check, and fix errors, and push to staging > 8) I start the testing cycle, removing patches based on testing results > and pushing to staging > 9) I manually review each patch that's survived testing > 10) I do final build/test, then push to master Obviously there's a lot of manual intervention and can get you overloaded. But if some developer knows that his patch is in some queue or getting some consideration, he'll rest easier than to check if his patch has made it either to master or to some staging queue somewhere. If it can't be automated, it sadly has to drop back to manual acking. > And commit mails are sent whenever something goes to master. So someone > should get a mail when a patch goes to master or when I reject it. To > me, the area to optimize is reducing the time things are spent in > staging. I don't think people really need visibility into staging. Absolutely. I think the trend has been anywhere from 2 to 4 weeks for patches landing on the list to the master tree. Also, this results in delays for important patches to get to the master tree because they're delayed behind a huge pile of other patches. Avi has mentioned this before but I don't know if there was any plan to address it. Amit
Avi Kivity wrote: > On 09/10/2009 04:56 PM, Anthony Liguori wrote: >> >> The problem is patch volume. We often see hundreds of patches a >> day. If typing a mail for each patch takes 2 minutes, that's >> potentially hours spent just on sending these mails. >> > > You exaggerate. The average rate is 13 patches per calendar day. On list or committed? There are a lot more on list than 13.. > commit e09a5267adf0af25b55d2abaf06e288b2d9537ea > Author: Dustin Kirkland <kirkland@canonical.com> > Date: Thu Sep 3 12:31:33 2009 -0500 > > qemu-kvm: fix segfault when running kvm without /dev/kvm, falling > back to non-accelerated mode > > qemu-kvm: fix segfault when running kvm without /dev/kvm, falling > back > to non-accelerated mode > > We're seeing segfaults on systems without access to /dev/kvm. It > looks like the global kvm_allowed is being set just a little too late > in vl.c. This patch moves the kvm initialization a bit higher in the > vl.c main, just after options processing, and solves the segfaults. > We're carrying this patch in Ubuntu 9.10 Alpha. Please apply > upstream, or advise if and why this might not be the optimal > solution. > > Signed-off-by: Dustin Kirkland <kirkland@canonical.com> > > Move the kvm_init() call a bit higher to fix a segfault when > /dev/kvm is not available. The kvm_allowed global needs > to be set correctly a little earlier. > > Signed-off-by: Dustin Kirkland <kirkland@canonical.com> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > There are many examples like this in the tree which is a pity. Others > include parts of an email conversation. I'd like history to look > better than this. This is goofy and is caused by improper patch submission. But when people quote email threads in a commit message, I don't remove them. It don't see it as a problem. Regards, Anthony Liguori
Avi Kivity wrote: > On 09/10/2009 04:56 PM, Anthony Liguori wrote: >> >> The problem is patch volume. We often see hundreds of patches a >> day. If typing a mail for each patch takes 2 minutes, that's >> potentially hours spent just on sending these mails. >> > > You exaggerate. The average rate is 13 patches per calendar day. FYI, in the past 14 hours, there were 92 patches posted to the list. Regards, Anthony Liguori
On 09/10/2009 06:54 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> On 09/10/2009 04:56 PM, Anthony Liguori wrote: >>> >>> The problem is patch volume. We often see hundreds of patches a >>> day. If typing a mail for each patch takes 2 minutes, that's >>> potentially hours spent just on sending these mails. >>> >> >> You exaggerate. The average rate is 13 patches per calendar day. > > On list or committed? There are a lot more on list than 13.. You certainly shouldn't ack patches you don't commit! > >> commit e09a5267adf0af25b55d2abaf06e288b2d9537ea >> Author: Dustin Kirkland <kirkland@canonical.com> >> Date: Thu Sep 3 12:31:33 2009 -0500 >> >> qemu-kvm: fix segfault when running kvm without /dev/kvm, falling >> back to non-accelerated mode >> >> qemu-kvm: fix segfault when running kvm without /dev/kvm, falling >> back >> to non-accelerated mode >> >> We're seeing segfaults on systems without access to /dev/kvm. It >> looks like the global kvm_allowed is being set just a little too >> late >> in vl.c. This patch moves the kvm initialization a bit higher in >> the >> vl.c main, just after options processing, and solves the segfaults. >> We're carrying this patch in Ubuntu 9.10 Alpha. Please apply >> upstream, or advise if and why this might not be the optimal >> solution. >> >> Signed-off-by: Dustin Kirkland <kirkland@canonical.com> >> >> Move the kvm_init() call a bit higher to fix a segfault when >> /dev/kvm is not available. The kvm_allowed global needs >> to be set correctly a little earlier. >> >> Signed-off-by: Dustin Kirkland <kirkland@canonical.com> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> >> There are many examples like this in the tree which is a pity. >> Others include parts of an email conversation. I'd like history to >> look better than this. > > This is goofy and is caused by improper patch submission. But when > people quote email threads in a commit message, I don't remove them. > It don't see it as a problem. From long experience, most commit messages need to be edited. People rarely write commit messages that can be understood a year later, and they don't know how 'git am' works.
On 09/10/2009 07:05 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> On 09/10/2009 04:56 PM, Anthony Liguori wrote: >>> >>> The problem is patch volume. We often see hundreds of patches a >>> day. If typing a mail for each patch takes 2 minutes, that's >>> potentially hours spent just on sending these mails. >>> >> >> You exaggerate. The average rate is 13 patches per calendar day. > FYI, in the past 14 hours, there were 92 patches posted to the list. Only 4 or 5 patchsets though. It's not that much effort to ack when committing. And of course, the average is way lower and I expect it to drop as the code base matures.
Avi Kivity wrote: > On 09/10/2009 06:54 PM, Anthony Liguori wrote: >> Avi Kivity wrote: >>> On 09/10/2009 04:56 PM, Anthony Liguori wrote: >>>> >>>> The problem is patch volume. We often see hundreds of patches a >>>> day. If typing a mail for each patch takes 2 minutes, that's >>>> potentially hours spent just on sending these mails. >>>> >>> >>> You exaggerate. The average rate is 13 patches per calendar day. >> >> On list or committed? There are a lot more on list than 13.. > > You certainly shouldn't ack patches you don't commit! But most spend time in staging. Acking patches that go to master, that's perfectly fine to do. The qemu-commits list does that and should CC the author directly so this should be happening. It's acking things that go into staging that I think would be difficult and not necessarily productive. >> This is goofy and is caused by improper patch submission. But when >> people quote email threads in a commit message, I don't remove them. >> It don't see it as a problem. > > From long experience, most commit messages need to be edited. People > rarely write commit messages that can be understood a year later, and > they don't know how 'git am' works. I don't like editing patches. I think it's unfair to the submitter to change their patch underneath of them. I'd suggest providing feedback on the list to people who write bad commit messages and ask them to write better ones. I try to limit the changes I make to resolving merge conflicts. Regards, Anthony Liguori
On 09/10/2009 07:22 PM, Anthony Liguori wrote: >> You certainly shouldn't ack patches you don't commit! > > > But most spend time in staging. What's the percentage of patches that make it to master? For me it's >90%. If it's too low we nned to fix that. > > Acking patches that go to master, that's perfectly fine to do. I think that's too late, especially as it often takes a week for master to be pushed. To a submitter, an ack means "no further action is required from you at this time" and it's good to provide it as early as possible. > The qemu-commits list does that and should CC the author directly so > this should be happening. It's too late, and doesn't help others who have an interest in the patch. > It's acking things that go into staging that I think would be > difficult and not necessarily productive. That is what I do and it doesn't seem to be troublesome. If I drop a patch from a queue I explicitly unack it. > >>> This is goofy and is caused by improper patch submission. But when >>> people quote email threads in a commit message, I don't remove >>> them. It don't see it as a problem. >> >> From long experience, most commit messages need to be edited. People >> rarely write commit messages that can be understood a year later, and >> they don't know how 'git am' works. > > I don't like editing patches. I think it's unfair to the submitter to > change their patch underneath of them. I'd suggest providing feedback > on the list to people who write bad commit messages and ask them to > write better ones. I try to limit the changes I make to resolving > merge conflicts. Editing the commit log is not changing the patch. I doubt you'll be able to get better commit messages - submitters have more immediate perspectives than maintainers (should have). I always try to make the log make sense a year from now (the code may change, but the commit log won't). Unfairly picking on Mark (who usually writes truly excellent changelogs, but this one is such a gem): > Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism > > Let's kick off this series with some of the more critical fixes. > > Signed-off-by: Mark McLoughlin<markmc@redhat.com> > What would you be thinking hunting the commit log for some change and coming up with this? (Mark, apologies for picking on you, it's truly unfair of me, but I can't help it)
Avi Kivity wrote: > On 09/10/2009 07:22 PM, Anthony Liguori wrote: >>> You certainly shouldn't ack patches you don't commit! >> >> >> But most spend time in staging. > > What's the percentage of patches that make it to master? For me it's > >90%. If it's too low we nned to fix that. Closer to 20% I'd say. This is largely due to multiple versions of the same series. If there's a way to improve this, that would make my life a lot easier. >> I don't like editing patches. I think it's unfair to the submitter >> to change their patch underneath of them. I'd suggest providing >> feedback on the list to people who write bad commit messages and ask >> them to write better ones. I try to limit the changes I make to >> resolving merge conflicts. > > Editing the commit log is not changing the patch. I doubt you'll be > able to get better commit messages - submitters have more immediate > perspectives than maintainers (should have). I always try to make the > log make sense a year from now (the code may change, but the commit > log won't). > > Unfairly picking on Mark (who usually writes truly excellent > changelogs, but this one is such a gem): > >> Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism >> >> Let's kick off this series with some of the more critical fixes. >> >> Signed-off-by: Mark McLoughlin<markmc@redhat.com> >> > > What would you be thinking hunting the commit log for some change and > coming up with this? Well the question is, should I/you edit this, or reject the patch requesting a better changelog? Certainly, the later is what akpm often does. Regards, Anthony Liguori
On 09/10/2009 07:38 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> On 09/10/2009 07:22 PM, Anthony Liguori wrote: >>>> You certainly shouldn't ack patches you don't commit! >>> >>> >>> But most spend time in staging. >> >> What's the percentage of patches that make it to master? For me it's >> >90%. If it's too low we nned to fix that. > > Closer to 20% I'd say. This is largely due to multiple versions of > the same series. If there's a way to improve this, that would make my > life a lot easier. Well, I'd say this is much more important than acking patches (though if you acked and unacked patches, it would be a lot more visible, and might shame the submitters into increased care). Why are there multiple series? If you have comments, clearly you wouldn't stage the patches. If others have comments, then you can simply wait a few days before staging (this is what I do). If testing fails, well, that's what testing is for. If the submitter puts out a new version, well, no good answer to that. Ask for an incremental? > Well the question is, should I/you edit this, or reject the patch > requesting a better changelog? > > Certainly, the later is what akpm often does. I'm happy to reject patches for whitespace but I will edit changelogs. My rationale is that many people don't care about that and I can't make them care; further the log is mostly for my own benefit - I spend quite a lot of time reading it when hunting regressions or preparing patchqueues for upstream.
On Thu, Sep 10, 2009 at 07:46:23PM +0300, Avi Kivity wrote: > On 09/10/2009 07:38 PM, Anthony Liguori wrote: > > Well the question is, should I/you edit this, or reject the patch > > requesting a better changelog? > > > > Certainly, the later is what akpm often does. > > I'm happy to reject patches for whitespace but I will edit changelogs. > My rationale is that many people don't care about that and I can't make > them care; further the log is mostly for my own benefit - I spend quite > a lot of time reading it when hunting regressions or preparing > patchqueues for upstream. I'd also add that anyone used to projects using SVN will probably be used to writing only a general explanation of the patch while the real commit message is left to whoever commits it. Maybe they ideally should be identical, but I expect that quite a few people _won't_ expect their email to appear 1:1 in the repository as log message (I usually feel quite embarrassed when my hastily written email by which I only wanted to get first comments ends up in a project's permanent history). I am sure it misses a lot of stuff and there might even be objections to some parts, but I wrote this draft of a "PATCHES" (or "CONTRIBUTING" or whatever) file that might help newcomers (and even some long-term developers might not know all the unwritten rules ;-). Suggested text: This is a (very) rough guide on how to submit patches to qemu, what is expected of you and what to expect from the process. Patches should go to the qemu-devel@nongnu.org list, subscription is possible via http://lists.nongnu.org/mailman/listinfo/qemu-devel The subject for any emails containing patches should start with [PATCH] so it is obvious that there is a patch included. Whenever you send a new patch or a new version of a patch, you should start a new thread - i.e. do _not_ reply to any email but write a new one. Patches are preferred inline (i.e. not as attachments - but be careful your mailer does not mangle them e.g. by adding line breaks). Emails generated via "git format-patch" are even better. Also be aware that emails are often used as-is as log messages, so try to write your emails in a way that is suitable for this, in particular they should in an understandable and not too verbose way describe what change was made and why. Also do not forget to add the appropriate Signed-off-by lines. Do not expect an immediate reply to your patches, reacting to patches simply takes some time. If there is no reaction and you checked that the patch was not already applied (there currently is no notification about this) try sending the patch once again, it might just have got lost or forgotten at some point.
On Thu, Sep 10, 2009 at 10:11:48AM -0300, Luiz Capitulino wrote: > On Thu, 10 Sep 2009 14:51:09 +0200 > Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > > For me that is simply too much effort (I am after all a FFmpeg/MPlayer developer > > in the first place) and I'll probably just drop all the non-trivial > > changes I made because they have a too high merging effort. > > Not sure if this is what you meant but, having different trees for > different subsystems is one of the best ways to get large scale > development and that's how git was designed to work. My point was: If you tell people to just use their own repository to "maintain" some part without actually having some appropriate "power" to get things accepted and directing patches to that part "through" them, then I think that instead of managing things by subsystems (good) you end up with a "wilderness" of forks (very, very bad IMO).
On Thu, 2009-09-10 at 19:35 +0300, Avi Kivity wrote: > Unfairly picking on Mark (who usually writes truly excellent changelogs, > but this one is such a gem): > > > Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism > > > > Let's kick off this series with some of the more critical fixes. > > > > Signed-off-by: Mark McLoughlin<markmc@redhat.com> > > > > What would you be thinking hunting the commit log for some change and > coming up with this? > > (Mark, apologies for picking on you, it's truly unfair of me, but I > can't help it) As you say, I normally try very hard with my changelogs, but I don't think the odd joke hurts much. Look back over the changelog to a similar, but even more concise commit message, a couple of weeks back that didn't go through Anthony. I was parodying that one :-) Anyway, you've cc-ed me now, so IMHO: - If one is so inclined, improving a commit message before pushing is perfectly fine - A simple "Applied, thanks" would be hugely appreciated - This "apply everything, test at length, reject problematic patches" appears to lead to a very batchy patch flow; there's a trade off to be made between trying to catch every regression before it hits the tree and the delay that effort introduces before the tree gets more widely tested by others Cheers, Mark.
On 09/10/2009 09:29 PM, Mark McLoughlin wrote: > On Thu, 2009-09-10 at 19:35 +0300, Avi Kivity wrote: > > >> Unfairly picking on Mark (who usually writes truly excellent changelogs, >> but this one is such a gem): >> >> >>> Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism >>> >>> Let's kick off this series with some of the more critical fixes. >>> >>> Signed-off-by: Mark McLoughlin<markmc@redhat.com> >>> >>> >> What would you be thinking hunting the commit log for some change and >> coming up with this? >> >> (Mark, apologies for picking on you, it's truly unfair of me, but I >> can't help it) >> > As you say, I normally try very hard with my changelogs, but I don't > think the odd joke hurts much. > Jokes are fine, but the commit log is sacred. > - This "apply everything, test at length, reject problematic patches" > appears to lead to a very batchy patch flow; there's a trade off to > be made between trying to catch every regression before it hits the > tree and the delay that effort introduces before the tree gets more > widely tested by others > For qemu.git I'd agree since it's undergoing a lot of churn. Unfortunately it also feeds qemu-kvm.git which I try very hard to keep regression-free (and finding and fixing regressions during a merge is quite horrible), so I'd really appreciate it if qemu.git quality didn't deteriorate. (and we're quite far from catching every regression btw). Anthony, how long are your test cycles?
On Thu, Sep 10, 2009 at 07:29:56PM +0100, Mark McLoughlin wrote: > On Thu, 2009-09-10 at 19:35 +0300, Avi Kivity wrote: > > Unfairly picking on Mark (who usually writes truly excellent changelogs, > > but this one is such a gem): > > > > > Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism > > > > > > Let's kick off this series with some of the more critical fixes. > > > > > > Signed-off-by: Mark McLoughlin<markmc@redhat.com> > > > > > > > What would you be thinking hunting the commit log for some change and > > coming up with this? > > > > (Mark, apologies for picking on you, it's truly unfair of me, but I > > can't help it) > > As you say, I normally try very hard with my changelogs, but I don't > think the odd joke hurts much. I don't mind jokes, but when I read that message I had not the slightest idea what it might do, so in that case it hurts exactly as much as "." as commit message hurts and not one iota less IMHO (note that I am not necessarily advocating to leave jokes out, but at least they should come with an explanation. Though considering an international community it might be better to avoid jokes in log messages completely). I'd also like to point out that the reference to "this series" is another reason I don't like it when log messages are created from emails unedited, such references are (probably sometimes) useful when sending a patch series but become completely meaningless in a log message since it is impossible to find out what it refers to.
Avi Kivity wrote: > For qemu.git I'd agree since it's undergoing a lot of churn. > Unfortunately it also feeds qemu-kvm.git which I try very hard to keep > regression-free (and finding and fixing regressions during a merge is > quite horrible), so I'd really appreciate it if qemu.git quality > didn't deteriorate. Or more accurately, you'd prefer if there were no bugs in qemu so that you only had to deal with the bugs that were introduced in qemu-kvm. That would be nice for you of course :-) But it's unrealistic to compare the two. $ git log --since='1 Month ago' --no-merges qemu-kvm/master --committer='Avi Kivity' --pretty=format:'%an' | wc -l 5 $ git log --since='1 Month ago' --no-merges qemu-kvm/master --committer='Marcelo Tosatti' --pretty=format:'%an' | wc -l 5 $ git log --since='1 Month ago' --no-merges origin/master --committer='Anthony Liguori' --pretty=format:'%an' | wc -l 251 So there are going to be at least 25x more regressions introduced from upstream qemu than what are introduced in qemu-kvm. > (and we're quite far from catching every regression btw). > > Anthony, how long are your test cycles? Depends on the number of regressions. I can usually get through testing in 4-5 hours when everything works. Everything usually doesn't work though. Regards, Anthony Liguori
On Thu, 2009-09-10 at 21:40 +0300, Avi Kivity wrote:
> (and we're quite far from catching every regression btw).
That's kind of my point. A lot of regressions are only found after
they've been pushed. Delaying a push delays finding those regressions.
Don't get me wrong - we certainly want to avoid regressions and doing
some testing before pushing is a good idea, but there is a balance to be
struck.
It's also the case that not all patches are equal. It should be possible
to short-cut the process for small patches, or regression fixes, or
patches from a trusted author who has done significant testing already.
Cheers,
Mark.
Hi, > Closer to 20% I'd say. This is largely due to multiple versions of the > same series. If there's a way to improve this, that would make my life a > lot easier. I think you should do smaller batches more frequently, i.e. commit 20 patches every day instead of >100 patches each week. Advantages: * Patches don't hang around in staging forever, which makes everybody happy. * Patch ack-ing via qemu-commits works reasonable. * Reduce your workload: You can ask patch submitters to deal with merge conflicts then without making the merge process unreasonable slow. Just drop anything which doesn't apply as-is, ask the submitter to rebase+resend, pick up the fresh patches the next day. * I'd suspect we'll also have less patch conflicts in the first place then. Disadvantages: * Might be more testing work (how automated is this btw?). >>> Subject: [Qemu-devel] [PATCH 01/19] Suppress more more kraxelism >>> >>> Let's kick off this series with some of the more critical fixes. >>> >>> Signed-off-by: Mark McLoughlin<markmc@redhat.com> >> >> What would you be thinking hunting the commit log for some change and >> coming up with this? I like that one, but it is a insider gag for qemu-commits readers and thus indeed not very useful when you'll read it a year or two in the future ... cheers, Gerd
Avi Kivity wrote: > On 09/10/2009 04:56 PM, Anthony Liguori wrote: >> >> The problem is patch volume. We often see hundreds of patches a day. >> If typing a mail for each patch takes 2 minutes, that's potentially >> hours spent just on sending these mails. >> > > You exaggerate. The average rate is 13 patches per calendar day. The > bulk of the patches are in patchsets which can be acked as a set, not > once per patch. > >> What I really need is some way to automatically generate these >> notifications. It's pretty easy to send a mail when a patch enters >> the queue but it's more difficult to send a mail when a patch is >> removed from the queue via a rebase. Often times, I remove patches >> from the queue simply because I'm not the right path for the patches >> to be committed from (like linux-user). > > I think more per-patch attention is needed, not less, for example see > this commit: > > commit e09a5267adf0af25b55d2abaf06e288b2d9537ea > Author: Dustin Kirkland <kirkland@canonical.com> > Date: Thu Sep 3 12:31:33 2009 -0500 > > qemu-kvm: fix segfault when running kvm without /dev/kvm, falling > back to non-accelerated mode > > qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back > to non-accelerated mode > > We're seeing segfaults on systems without access to /dev/kvm. It > looks like the global kvm_allowed is being set just a little too late > in vl.c. This patch moves the kvm initialization a bit higher in the > vl.c main, just after options processing, and solves the segfaults. > We're carrying this patch in Ubuntu 9.10 Alpha. Please apply > upstream, or advise if and why this might not be the optimal solution. > > Signed-off-by: Dustin Kirkland <kirkland@canonical.com> > > Move the kvm_init() call a bit higher to fix a segfault when > /dev/kvm is not available. The kvm_allowed global needs > to be set correctly a little earlier. > > Signed-off-by: Dustin Kirkland <kirkland@canonical.com> > Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> > > There are many examples like this in the tree which is a pity. Others > include parts of an email conversation. I'd like history to look better > than this. Even worse, I think this patch does not belong into upstream as it fixed a qemu-kvm-only bug. I think this was caused by Dustin CC'ing qemu, right? Did anyone test properly if the change has no side effects on upstream kvm (which has a different initialization scheme)? Jan
On (Thu) Sep 10 2009 [19:19:03], Reimar Döffinger wrote: > > I'd also add that anyone used to projects using SVN will probably be > used to writing only a general explanation of the patch while the real > commit message is left to whoever commits it. Maybe they ideally should > be identical, but I expect that quite a few people _won't_ expect their > email to appear 1:1 in the repository as log message (I usually feel > quite embarrassed when my hastily written email by which I only wanted > to get first comments ends up in a project's permanent history). So perhaps your "SUBMITTING_PATCHES" file should mention that the patch descriptions should be as good as possible, and mention: - what's being fixed - why it's being fixed the way it is - how does it solve the problem that's being fixed all of these reduce the load on maintainers and reviewers. If you took time out to write a patch and better some piece of code, you should take the time to tell why you spent so much time on it. > I am sure it misses a lot of stuff and there might even be objections to > some parts, but I wrote this draft of a "PATCHES" (or "CONTRIBUTING" or > whatever) file that might help newcomers (and even some long-term > developers might not know all the unwritten rules ;-). > Suggested text: One general comment: Please include line breaks between paragraphs. That makes things much easier to read. > This is a (very) rough guide on how to submit patches to qemu, what is expected > of you and what to expect from the process. > Patches should go to the qemu-devel@nongnu.org list, subscription is possible > via http://lists.nongnu.org/mailman/listinfo/qemu-devel > The subject for any emails containing patches should start with [PATCH] so it is > obvious that there is a patch included. > Whenever you send a new patch or a new version of a patch, you should start a new > thread - i.e. do _not_ reply to any email but write a new one. > Patches are preferred inline (i.e. not as attachments - but be careful your mailer > does not mangle them e.g. by adding line breaks). > Emails generated via "git format-patch" are even better. > Also be aware that emails are often used as-is as log messages, so try to write > your emails in a way that is suitable for this, in particular they should in an > understandable and not too verbose way describe what change was made and > why. You could add the points I mentioned above here. > Also do not forget to add the appropriate Signed-off-by lines. An example here wouldn't hurt. > Do not expect an immediate reply to your patches, reacting to patches simply > takes some time. If there is no reaction and you checked that the patch was > not already applied (there currently is no notification about this) try sending > the patch once again, it might just have got lost or forgotten at some > point. Please mention the commits-list where notifications are sent when patches get applied to the master branch of the git tree. Also mention Anthony's staging queue URL if someone really wants to know if their patch has been picked up by a bot for testing. In addition to the linefeed comment I gave above, the above list could be separated by bullets (since it is a list). Also, please make the text fit in 80 columns. And send it as a patch so that it's easy to apply. :-) Amit
Jan Kiszka wrote: > Avi Kivity wrote: > >> On 09/10/2009 04:56 PM, Anthony Liguori wrote: >> >>> The problem is patch volume. We often see hundreds of patches a day. >>> If typing a mail for each patch takes 2 minutes, that's potentially >>> hours spent just on sending these mails. >>> >>> >> You exaggerate. The average rate is 13 patches per calendar day. The >> bulk of the patches are in patchsets which can be acked as a set, not >> once per patch. >> >> >>> What I really need is some way to automatically generate these >>> notifications. It's pretty easy to send a mail when a patch enters >>> the queue but it's more difficult to send a mail when a patch is >>> removed from the queue via a rebase. Often times, I remove patches >>> from the queue simply because I'm not the right path for the patches >>> to be committed from (like linux-user). >>> >> I think more per-patch attention is needed, not less, for example see >> this commit: >> >> commit e09a5267adf0af25b55d2abaf06e288b2d9537ea >> Author: Dustin Kirkland <kirkland@canonical.com> >> Date: Thu Sep 3 12:31:33 2009 -0500 >> >> qemu-kvm: fix segfault when running kvm without /dev/kvm, falling >> back to non-accelerated mode >> >> qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back >> to non-accelerated mode >> >> We're seeing segfaults on systems without access to /dev/kvm. It >> looks like the global kvm_allowed is being set just a little too late >> in vl.c. This patch moves the kvm initialization a bit higher in the >> vl.c main, just after options processing, and solves the segfaults. >> We're carrying this patch in Ubuntu 9.10 Alpha. Please apply >> upstream, or advise if and why this might not be the optimal solution. >> >> Signed-off-by: Dustin Kirkland <kirkland@canonical.com> >> >> Move the kvm_init() call a bit higher to fix a segfault when >> /dev/kvm is not available. The kvm_allowed global needs >> to be set correctly a little earlier. >> >> Signed-off-by: Dustin Kirkland <kirkland@canonical.com> >> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >> >> There are many examples like this in the tree which is a pity. Others >> include parts of an email conversation. I'd like history to look better >> than this. >> > > Even worse, I think this patch does not belong into upstream as it fixed > a qemu-kvm-only bug. I think this was caused by Dustin CC'ing qemu, right? > > Did anyone test properly if the change has no side effects on upstream > kvm (which has a different initialization scheme)? > It doesn't break upstream qemu's -enable-kvm. Regards, Anthony Liguori
Anthony Liguori wrote: > Jan Kiszka wrote: >> Avi Kivity wrote: >> >>> On 09/10/2009 04:56 PM, Anthony Liguori wrote: >>> >>>> The problem is patch volume. We often see hundreds of patches a day. >>>> If typing a mail for each patch takes 2 minutes, that's potentially >>>> hours spent just on sending these mails. >>>> >>>> >>> You exaggerate. The average rate is 13 patches per calendar day. The >>> bulk of the patches are in patchsets which can be acked as a set, not >>> once per patch. >>> >>> >>>> What I really need is some way to automatically generate these >>>> notifications. It's pretty easy to send a mail when a patch enters >>>> the queue but it's more difficult to send a mail when a patch is >>>> removed from the queue via a rebase. Often times, I remove patches >>>> from the queue simply because I'm not the right path for the patches >>>> to be committed from (like linux-user). >>>> >>> I think more per-patch attention is needed, not less, for example see >>> this commit: >>> >>> commit e09a5267adf0af25b55d2abaf06e288b2d9537ea >>> Author: Dustin Kirkland <kirkland@canonical.com> >>> Date: Thu Sep 3 12:31:33 2009 -0500 >>> >>> qemu-kvm: fix segfault when running kvm without /dev/kvm, falling >>> back to non-accelerated mode >>> >>> qemu-kvm: fix segfault when running kvm without /dev/kvm, falling back >>> to non-accelerated mode >>> >>> We're seeing segfaults on systems without access to /dev/kvm. It >>> looks like the global kvm_allowed is being set just a little too late >>> in vl.c. This patch moves the kvm initialization a bit higher in the >>> vl.c main, just after options processing, and solves the segfaults. >>> We're carrying this patch in Ubuntu 9.10 Alpha. Please apply >>> upstream, or advise if and why this might not be the optimal solution. >>> >>> Signed-off-by: Dustin Kirkland <kirkland@canonical.com> >>> >>> Move the kvm_init() call a bit higher to fix a segfault when >>> /dev/kvm is not available. The kvm_allowed global needs >>> to be set correctly a little earlier. >>> >>> Signed-off-by: Dustin Kirkland <kirkland@canonical.com> >>> Signed-off-by: Anthony Liguori <aliguori@us.ibm.com> >>> >>> There are many examples like this in the tree which is a pity. Others >>> include parts of an email conversation. I'd like history to look better >>> than this. >>> >> Even worse, I think this patch does not belong into upstream as it fixed >> a qemu-kvm-only bug. I think this was caused by Dustin CC'ing qemu, right? >> >> Did anyone test properly if the change has no side effects on upstream >> kvm (which has a different initialization scheme)? >> > > It doesn't break upstream qemu's -enable-kvm. Yep, that's what I can confirm now too - after unbreaking -enable-kvm again. Jan
On Thu, Sep 10, 2009 at 8:19 PM, Reimar Döffinger <Reimar.Doeffinger@gmx.de> wrote: > On Thu, Sep 10, 2009 at 07:46:23PM +0300, Avi Kivity wrote: >> On 09/10/2009 07:38 PM, Anthony Liguori wrote: >> > Well the question is, should I/you edit this, or reject the patch >> > requesting a better changelog? >> > >> > Certainly, the later is what akpm often does. >> >> I'm happy to reject patches for whitespace but I will edit changelogs. >> My rationale is that many people don't care about that and I can't make >> them care; further the log is mostly for my own benefit - I spend quite >> a lot of time reading it when hunting regressions or preparing >> patchqueues for upstream. > > I'd also add that anyone used to projects using SVN will probably be > used to writing only a general explanation of the patch while the real > commit message is left to whoever commits it. Maybe they ideally should > be identical, but I expect that quite a few people _won't_ expect their > email to appear 1:1 in the repository as log message (I usually feel > quite embarrassed when my hastily written email by which I only wanted > to get first comments ends up in a project's permanent history). > I am sure it misses a lot of stuff and there might even be objections to > some parts, but I wrote this draft of a "PATCHES" (or "CONTRIBUTING" or > whatever) file that might help newcomers (and even some long-term > developers might not know all the unwritten rules ;-). > Suggested text: > > This is a (very) rough guide on how to submit patches to qemu, what is expected > of you and what to expect from the process. > Patches should go to the qemu-devel@nongnu.org list, subscription is possible > via http://lists.nongnu.org/mailman/listinfo/qemu-devel > The subject for any emails containing patches should start with [PATCH] so it is > obvious that there is a patch included. > Whenever you send a new patch or a new version of a patch, you should start a new > thread - i.e. do _not_ reply to any email but write a new one. > Patches are preferred inline (i.e. not as attachments - but be careful your mailer > does not mangle them e.g. by adding line breaks). > Emails generated via "git format-patch" are even better. > Also be aware that emails are often used as-is as log messages, so try to write > your emails in a way that is suitable for this, in particular they should in an > understandable and not too verbose way describe what change was made and > why. > Also do not forget to add the appropriate Signed-off-by lines. > Do not expect an immediate reply to your patches, reacting to patches simply > takes some time. If there is no reaction and you checked that the patch was > not already applied (there currently is no notification about this) try sending > the patch once again, it might just have got lost or forgotten at some > point. We had a discussion about this earlier this year: http://lists.gnu.org/archive/html/qemu-devel/2009-04/msg01184.html Since that we have switched to git, so a lot of it could be simplified. Instead of the lengthy formatting specification, we could just require that the patch should apply with "git-am" to git HEAD without any editing, merging or even apply.whitespace=fix.
On 09/12/2009 08:55 AM, Blue Swirl wrote: > We had a discussion about this earlier this year: > http://lists.gnu.org/archive/html/qemu-devel/2009-04/msg01184.html > > Since that we have switched to git, so a lot of it could be > simplified. Instead of the lengthy formatting specification, we could > just require that the patch should apply with "git-am" to git HEAD > without any editing, merging or even apply.whitespace=fix. > That doesn't tell people what information to include, and if they don't care how the log looks like, they'll do the minimum amount of work to make git am like it.
On 09/10/2009 10:31 PM, Anthony Liguori wrote: > Avi Kivity wrote: >> For qemu.git I'd agree since it's undergoing a lot of churn. >> Unfortunately it also feeds qemu-kvm.git which I try very hard to >> keep regression-free (and finding and fixing regressions during a >> merge is quite horrible), so I'd really appreciate it if qemu.git >> quality didn't deteriorate. > > Or more accurately, you'd prefer if there were no bugs in qemu so that > you only had to deal with the bugs that were introduced in qemu-kvm. Yes. Anyone who pulls qemu.git and develops or uses it would agree. > > That would be nice for you of course :-) But it's unrealistic to > compare the two. > $ git log --since='1 Month ago' --no-merges qemu-kvm/master > --committer='Avi Kivity' --pretty=format:'%an' | wc -l > 5 > $ git log --since='1 Month ago' --no-merges qemu-kvm/master > --committer='Marcelo Tosatti' --pretty=format:'%an' | wc -l > 5 > > $ git log --since='1 Month ago' --no-merges origin/master > --committer='Anthony Liguori' --pretty=format:'%an' | wc -l > 251 > > So there are going to be at least 25x more regressions introduced from > upstream qemu than what are introduced in qemu-kvm. Well, if we define a regression as something that blocks me from pushing (kvm-autotest), then both numbers are zero. Of course qemu.git is not run purely for my enjoyment, but a large number of patches are targeted at qemu-kvm.git. Further, the tests that I run (installing and booting some popular OSes) are really the basic minimum functionality, nothing fancy there. > >> (and we're quite far from catching every regression btw). >> >> Anthony, how long are your test cycles? > > Depends on the number of regressions. I can usually get through > testing in 4-5 hours when everything works. Everything usually > doesn't work though. We definitely need to improve this and the 80% reject rate. Can you start being a lot noisier about rejects? that will at least give some visibility into the problem.
On 09/10/2009 11:36 PM, Mark McLoughlin wrote: > On Thu, 2009-09-10 at 21:40 +0300, Avi Kivity wrote: > > >> (and we're quite far from catching every regression btw). >> > That's kind of my point. A lot of regressions are only found after > they've been pushed. Delaying a push delays finding those regressions. > That makes sense if the regression tests don't find any regressions. If they do, then pushing despite those regressions means everyone is now hitting the regressions, swearing, and trying to work around them. We get massive parallelism but little progress. > Don't get me wrong - we certainly want to avoid regressions and doing > some testing before pushing is a good idea, but there is a balance to be > struck. > > It's also the case that not all patches are equal. It should be possible > to short-cut the process for small patches, or regression fixes, or > patches from a trusted author who has done significant testing already. > I think reducing the batch size will also help. If the batch contains 100 patches there's a high likelihood of a regression in there. If you're testing 10-15 patches at a time it may actually work, and if not, you can easily guess who the offender is.
On Sun, Sep 13, 2009 at 6:44 PM, Avi Kivity <avi@redhat.com> wrote: > On 09/12/2009 08:55 AM, Blue Swirl wrote: >> >> We had a discussion about this earlier this year: >> http://lists.gnu.org/archive/html/qemu-devel/2009-04/msg01184.html >> >> Since that we have switched to git, so a lot of it could be >> simplified. Instead of the lengthy formatting specification, we could >> just require that the patch should apply with "git-am" to git HEAD >> without any editing, merging or even apply.whitespace=fix. >> > > That doesn't tell people what information to include, and if they don't care > how the log looks like, they'll do the minimum amount of work to make git am > like it. 1. Format of e-mail subject [clip] SP1.3: The summary must be suitable for changelog as is (start with a capital letter etc.) after deleting the bracketed text, white space after it and any text preceding it [clip] 2. Format of e-mail body [clip] SP2.2: The patch description must be selfstanding (not depend on other patches, other messages or background discussion) What information should people include? Description of the problem, step-by-step description how to reproduce it and the solution for the problem? Brief analysis of the alternative solutions considered so that if the commit is found buggy, corrective action is easier because we can try one of the alternatives instead of reverting? Link to hardware manual chapter where the correct behaviour is described?
On Sun, 2009-09-13 at 19:19 +0300, Avi Kivity wrote: > On 09/10/2009 11:36 PM, Mark McLoughlin wrote: > > On Thu, 2009-09-10 at 21:40 +0300, Avi Kivity wrote: > > > > > >> (and we're quite far from catching every regression btw). > >> > > That's kind of my point. A lot of regressions are only found after > > they've been pushed. Delaying a push delays finding those regressions. > > > > That makes sense if the regression tests don't find any regressions. Or if the regression tests take too long - rather than regression tests that take 5 hours to run, I'd prefer to see a subset of those which takes e.g. 30 minutes be used to sanity check a tree before pushing it. This would allow smaller batches to be pushed more regularly, while the more in-depth testing can be done less regularly on batches not much larger than are being pushed now. > If > they do, then pushing despite those regressions means everyone is now > hitting the regressions, swearing, and trying to work around them. We > get massive parallelism but little progress. Not all regressions are equal. I wouldn't mind some regressions in the tree so long as they are being tracked as "must be fixed" before the release. Clearly anything causing people to swear should be reverted. Assuming you can tell which patch caused the problem. > > Don't get me wrong - we certainly want to avoid regressions and doing > > some testing before pushing is a good idea, but there is a balance to be > > struck. > > > > It's also the case that not all patches are equal. It should be possible > > to short-cut the process for small patches, or regression fixes, or > > patches from a trusted author who has done significant testing already. > > > > I think reducing the batch size will also help. If the batch contains > 100 patches there's a high likelihood of a regression in there. If > you're testing 10-15 patches at a time it may actually work, and if not, > you can easily guess who the offender is. The batch size is determined by the length of time the testing takes, right? But agree, with a large batch, you're more likely to find at least one regression, to struggle to figure out which patch caused the regression and to go through several iterations before you have something to push. Cheers, Mark.
On 09/14/2009 10:49 AM, Mark McLoughlin wrote: > On Sun, 2009-09-13 at 19:19 +0300, Avi Kivity wrote: > >> On 09/10/2009 11:36 PM, Mark McLoughlin wrote: >> >>> On Thu, 2009-09-10 at 21:40 +0300, Avi Kivity wrote: >>> >>> >>> >>>> (and we're quite far from catching every regression btw). >>>> >>>> >>> That's kind of my point. A lot of regressions are only found after >>> they've been pushed. Delaying a push delays finding those regressions. >>> >>> >> That makes sense if the regression tests don't find any regressions. >> > Or if the regression tests take too long - rather than regression tests > that take 5 hours to run, I'd prefer to see a subset of those which > takes e.g. 30 minutes be used to sanity check a tree before pushing it. > I'm guessing the reason the tests take so long is lack of automation. My kvm-autotest takes 1.5-2 hours, and it runs everything serially. Once it starts running tests in parallel, latency can probably be reduces to 30 minutes on a fairly standard 8-core. >> If >> they do, then pushing despite those regressions means everyone is now >> hitting the regressions, swearing, and trying to work around them. We >> get massive parallelism but little progress. >> > Not all regressions are equal. I wouldn't mind some regressions in the > tree so long as they are being tracked as "must be fixed" before the > release. > > Clearly anything causing people to swear should be reverted. Assuming > you can tell which patch caused the problem. > Right, a regression that breaks some esoteric feature used by two people is not too bad. Of course, if we can detect it it's better to drop the patch and make the submitter fix it. The regressions I'm talking about are "common OS won't boot" things. >> I think reducing the batch size will also help. If the batch contains >> 100 patches there's a high likelihood of a regression in there. If >> you're testing 10-15 patches at a time it may actually work, and if not, >> you can easily guess who the offender is. >> > The batch size is determined by the length of time the testing takes, > right? > > But agree, with a large batch, you're more likely to find at least one > regression, to struggle to figure out which patch caused the regression > and to go through several iterations before you have something to push. > Exactly. Then I undergo exactly the same nightmare when merging into qemu-kvm. Even worse, fixes I apply during a merge are often of substandard quality and cause more regressions.
diff --git a/hw/mc146818rtc.c b/hw/mc146818rtc.c index 2022548..2b040a7 100644 --- a/hw/mc146818rtc.c +++ b/hw/mc146818rtc.c @@ -421,9 +421,10 @@ static void rtc_update_second2(void *opaque) } /* update ended interrupt */ + s->cmos_data[RTC_REG_C] |= REG_C_UF; if (s->cmos_data[RTC_REG_B] & REG_B_UIE) { - s->cmos_data[RTC_REG_C] |= 0x90; - rtc_irq_raise(s->irq); + s->cmos_data[RTC_REG_C] |= REG_C_IRQF; + rtc_irq_raise(s->irq); } /* clear update in progress bit */
The RTC emulation does not set the IRQ flags independent of the IRQ enable bits. The original MC146818A datasheet from 1984 notes: "flag bits in Register C [...] are set independent of the state of the corresponding enable bits in Register B" Similar sections can be found in newer documentation e.g. in rtc82885. Qemu and Bochs set the IRQ flags only if they are enabled, which breaks drivers polling on them. The following patch corrects this for the update-ended-flag in Qemu only. It does not fix the handling of the other flags. Signed-off-by: Bernhard Kauer <kauer@tudos.org>