Em Wed, Sep 23, 2020 at 09:25:06AM +0800, Jin, Yao escreveu:
> On 9/23/2020 3:42 AM, Arnaldo Carvalho de Melo wrote:
> > Em Tue, Sep 22, 2020 at 11:19:17AM +0800, Jin Yao escreveu:
> > > - Update CascadelakeX events to v1.08.
> > > - Update CascadelakeX JSON metrics from TMAM 4.0.
> > > Other fixes:
> > > - Add NO_NMI_WATCHDOG metric constraint to Backend_Bound
> > > - Change 'MB/sec' to 'MB' in UNC_M_PMM_BANDWIDTH.
> > [acme@five perf]$ am /wb/1.patch
> > Applying: perf vendor events: Update CascadelakeX events to v1.08
> > error: patch fragment without header at line 283: @@ -213,14 +220,14 @@
> > Patch failed at 0001 perf vendor events: Update CascadelakeX events to v1.08
> > hint: Use 'git am --show-current-patch=diff' to see the failed patch
> > When you have resolved this problem, run "git am --continue".
> > If you prefer to skip this patch, run "git am --skip" instead.
> > To restore the original branch and stop patching, run "git am --abort".
> > [acme@five perf]$ git am --abort
> > [acme@five perf]$ set -o vi
> > [acme@five perf]$ patch -p1 < /wb/1.patch
> > patching file tools/perf/pmu-events/arch/x86/cascadelakex/cache.json
> > patching file tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
> > Hunk #7 FAILED at 87.
> > 1 out of 7 hunks FAILED -- saving rejects to file tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json.rej
> > patching file tools/perf/pmu-events/arch/x86/cascadelakex/frontend.json
> > patching file tools/perf/pmu-events/arch/x86/cascadelakex/memory.json
> > patching file tools/perf/pmu-events/arch/x86/cascadelakex/other.json
> > patching file tools/perf/pmu-events/arch/x86/cascadelakex/pipeline.json
> > patching file tools/perf/pmu-events/arch/x86/cascadelakex/uncore-memory.json
> > patching file tools/perf/pmu-events/arch/x86/cascadelakex/uncore-other.json
> > [acme@five perf]$
> > [acme@five perf]$ head tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json.rej
> > --- tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
> > +++ tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
> > @@ -87,86 +70,110 @@
> > "MetricName": "CLKS"
> > },
> > {
> > - "BriefDescription": "Total issue-pipeline slots (per-Physical Core)",
> > + "BriefDescription": "Total issue-pipeline slots (per-Physical Core till ICL; per-Logical Processor ICL onward)",
> > "MetricExpr": "4 * cycles",
> > "MetricGroup": "TopDownL1",
> > [acme@five perf]$ wc -l tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json.rej
> > 133 tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json.rej
> > [acme@five perf]$
> > Can you please check?
> I applied the patch on latest perf/core, it seemed OK.
> # git log --oneline
> 4cbdb1c21926 (HEAD -> perf/core) perf vendor events: Update CascadelakeX events to v1.08
> b1f815c479c1 (tag: perf-tools-tests-v5.10-2020-09-10, origin/perf/core) perf vendor events power9:
> Add hv_24x7 core level metric events
> f5a489dc8189 perf metricgroup: Pass pmu_event structure as a parameter for
> arch_get_runtimeparam() 560ccbc4a52c perf jevents: Add support for parsing
> perchip/percore events ...
> I strongly suspect that part of patch content is truncated by mail system.
> Let me resend the patch as attachment. Sorry about that!
Thanks, it now works, but then... You forgot to add the Cc: entries for
all the people in your actual e-mail Cc: list, and also the
Reviewed-by: from Andy, I had to do it all manually, so when I applied
your attachments with 'git am' I needed to go on and manually collect
all the Cc, Reviewed-by and Acked-by tags.
This complicates things, slows me down, doesn't scale. While I do all
this manual stuff normally, I don't think this can continue, and its not
something specific to you, submitters have to pay attention to these
details. Or tools.
Things like b4 help with this and probably have to take into account
attachments as well, that is why I'm adding Konstantin to the Cc: list
of this message.
Konstantin, is this case covered? I.e. patches that get botched and then
require attachments to be sent to then gets processed?
Thanks, applied.
- Arnaldo
> Thanks, it now works, but then... You forgot to add the Cc: entries for
> all the people in your actual e-mail Cc: list, and also the
> Reviewed-by: from Andy, I had to do it all manually, so when I applied
> your attachments with 'git am' I needed to go on and manually collect
> all the Cc, Reviewed-by and Acked-by tags.
For the event updates we should just use git pulls in my opinion.
They are just too large for the normal review procedures, and usually
don't really benefit much from community review anyways because they
are essentially hardware documentation.
Should just name intel / amd / etc. event list maintainers and you
could accept pulls from them.
-Andi
On Fri, Sep 25, 2020 at 03:05:27PM -0300, Arnaldo Carvalho de Melo wrote:
> Things like b4 help with this and probably have to take into account
> attachments as well, that is why I'm adding Konstantin to the Cc: list
> of this message.
>
> Konstantin, is this case covered? I.e. patches that get botched and then
> require attachments to be sent to then gets processed?
Hmm... it's complicated. The trouble with handling corner-cases is
unexpected ways this can affect other mail. For example, what do we do
when we see a patch in the body, but also a patch as an attachment --
should the attachment win, or did the developer mean something entirely
different ("this is the fixed patch -- I attached the previous version
for your reference").
I am working on a service that will automatically "explode" pull
requests into patch series, so this may help work around this particular
issue. For example, a developer would send a pull-request to the list
and cc "[email protected]" (or someone else can follow up with a cc
to that address). When the bot sees the cc, it will automatically
convert the pull request into patch series and send it to the same
recipients as on the original pull request.
This should help avoid the problem of terrible mail relays and nasty
mail clients.
B4 can already do most of that (see "b4 pr --explode"), so adding the
remaining bits should be easy enough. If this functionality is
interesting to you, I would be happy to have early beta testers.
-K
Em Fri, Sep 25, 2020 at 11:36:15AM -0700, Andi Kleen escreveu:
> > Thanks, it now works, but then... You forgot to add the Cc: entries for
> > all the people in your actual e-mail Cc: list, and also the
> > Reviewed-by: from Andy, I had to do it all manually, so when I applied
> > your attachments with 'git am' I needed to go on and manually collect
> > all the Cc, Reviewed-by and Acked-by tags.
>
> For the event updates we should just use git pulls in my opinion.
>
> They are just too large for the normal review procedures, and usually
> don't really benefit much from community review anyways because they
> are essentially hardware documentation.
>
> Should just name intel / amd / etc. event list maintainers and you
> could accept pulls from them.
That isn't that important, i.e. if I do it from e-mail manually or doing
a git pull.
What is important is that one person submits, another provides some
stamp, the stamp is collected, patch is applied and build tested, life
goes on.
b4 should fix this all up, but attachments, in this case, seems to be
needed due to the content having such long lines, right?
- Arnaldo
On Fri, Sep 25, 2020 at 11:36 AM Andi Kleen <[email protected]> wrote:
>
> > Thanks, it now works, but then... You forgot to add the Cc: entries for
> > all the people in your actual e-mail Cc: list, and also the
> > Reviewed-by: from Andy, I had to do it all manually, so when I applied
> > your attachments with 'git am' I needed to go on and manually collect
> > all the Cc, Reviewed-by and Acked-by tags.
>
> For the event updates we should just use git pulls in my opinion.
>
> They are just too large for the normal review procedures, and usually
> don't really benefit much from community review anyways because they
> are essentially hardware documentation.
>
> Should just name intel / amd / etc. event list maintainers and you
> could accept pulls from them.
>
> -Andi
Are the scripts to make the json available? Perhaps then Arnaldo could
generate the changes.
I believe Kajol Jain has some unmerged changes because there's a worry
about the interaction with the scripts and assumptions in jevents.
Having access to the scripts would mean that we could fix whatever
issues there would be.
Thanks,
Ian
Hi Arnaldo,
On 9/26/2020 2:05 AM, Arnaldo Carvalho de Melo wrote:
> Em Wed, Sep 23, 2020 at 09:25:06AM +0800, Jin, Yao escreveu:
>> On 9/23/2020 3:42 AM, Arnaldo Carvalho de Melo wrote:
>>> Em Tue, Sep 22, 2020 at 11:19:17AM +0800, Jin Yao escreveu:
>>>> - Update CascadelakeX events to v1.08.
>>>> - Update CascadelakeX JSON metrics from TMAM 4.0.
>
>>>> Other fixes:
>>>> - Add NO_NMI_WATCHDOG metric constraint to Backend_Bound
>>>> - Change 'MB/sec' to 'MB' in UNC_M_PMM_BANDWIDTH.
>
>>> [acme@five perf]$ am /wb/1.patch
>>> Applying: perf vendor events: Update CascadelakeX events to v1.08
>>> error: patch fragment without header at line 283: @@ -213,14 +220,14 @@
>>> Patch failed at 0001 perf vendor events: Update CascadelakeX events to v1.08
>>> hint: Use 'git am --show-current-patch=diff' to see the failed patch
>>> When you have resolved this problem, run "git am --continue".
>>> If you prefer to skip this patch, run "git am --skip" instead.
>>> To restore the original branch and stop patching, run "git am --abort".
>>> [acme@five perf]$ git am --abort
>>> [acme@five perf]$ set -o vi
>>> [acme@five perf]$ patch -p1 < /wb/1.patch
>>> patching file tools/perf/pmu-events/arch/x86/cascadelakex/cache.json
>>> patching file tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
>>> Hunk #7 FAILED at 87.
>>> 1 out of 7 hunks FAILED -- saving rejects to file tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json.rej
>>> patching file tools/perf/pmu-events/arch/x86/cascadelakex/frontend.json
>>> patching file tools/perf/pmu-events/arch/x86/cascadelakex/memory.json
>>> patching file tools/perf/pmu-events/arch/x86/cascadelakex/other.json
>>> patching file tools/perf/pmu-events/arch/x86/cascadelakex/pipeline.json
>>> patching file tools/perf/pmu-events/arch/x86/cascadelakex/uncore-memory.json
>>> patching file tools/perf/pmu-events/arch/x86/cascadelakex/uncore-other.json
>>> [acme@five perf]$
>
>>> [acme@five perf]$ head tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json.rej
>>> --- tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
>>> +++ tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json
>>> @@ -87,86 +70,110 @@
>>> "MetricName": "CLKS"
>>> },
>>> {
>>> - "BriefDescription": "Total issue-pipeline slots (per-Physical Core)",
>>> + "BriefDescription": "Total issue-pipeline slots (per-Physical Core till ICL; per-Logical Processor ICL onward)",
>>> "MetricExpr": "4 * cycles",
>>> "MetricGroup": "TopDownL1",
>>> [acme@five perf]$ wc -l tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json.rej
>>> 133 tools/perf/pmu-events/arch/x86/cascadelakex/clx-metrics.json.rej
>>> [acme@five perf]$
>
>>> Can you please check?
>
>> I applied the patch on latest perf/core, it seemed OK.
>
>> # git log --oneline
>> 4cbdb1c21926 (HEAD -> perf/core) perf vendor events: Update CascadelakeX events to v1.08
>> b1f815c479c1 (tag: perf-tools-tests-v5.10-2020-09-10, origin/perf/core) perf vendor events power9:
>> Add hv_24x7 core level metric events
>> f5a489dc8189 perf metricgroup: Pass pmu_event structure as a parameter for
>> arch_get_runtimeparam() 560ccbc4a52c perf jevents: Add support for parsing
>> perchip/percore events ...
>
>> I strongly suspect that part of patch content is truncated by mail system.
>
>> Let me resend the patch as attachment. Sorry about that!
>
> Thanks, it now works, but then... You forgot to add the Cc: entries for
> all the people in your actual e-mail Cc: list, and also the
> Reviewed-by: from Andy, I had to do it all manually, so when I applied
> your attachments with 'git am' I needed to go on and manually collect
> all the Cc, Reviewed-by and Acked-by tags.
>
Sorry for replying so late!
I realized I forgot to add CC/To list in the attached patches. Very sorry about that! :(
> This complicates things, slows me down, doesn't scale. While I do all
> this manual stuff normally, I don't think this can continue, and its not
> something specific to you, submitters have to pay attention to these
> details. Or tools.
>
Sorry for bringing troubles to you.
> Things like b4 help with this and probably have to take into account
> attachments as well, that is why I'm adding Konstantin to the Cc: list
> of this message.
>
Let me learn b4... but as Konstantin said in another thread, it's complicated too.
Anyway, for this case, I will take care in future. At least, I will check and add CC/To list to the
attached patch.
Thanks
Jin Yao
> Konstantin, is this case covered? I.e. patches that get botched and then
> require attachments to be sent to then gets processed?
>
> Thanks, applied.
>
> - Arnaldo
>