patches pending acks (or naks)

patches pending acks (or naks)

[Jan Beulich]

Co-REST-maintainers,

there are a couple of patches I'd like to ask for feedback on, and I'm
assuming (based on previous replies by him) Konrad is waiting for such
too before considering giving release-acks:

http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00667.html
http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00692.html 
Either
http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00260.html
or (my preference)
http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg01054.html

Thanks, Jan

Re: patches pending acks (or naks)

[Konrad Rzeszutek Wilk]

On December 11, 2014 6:39:07 AM EST, Jan Beulich <JBeulich@suse.com> wrote:
>Co-REST-maintainers,
>
>there are a couple of patches I'd like to ask for feedback on, and I'm
>assuming (based on previous replies by him) Konrad is waiting for such
>too before considering giving release-acks:
>

I believe these two already have Acks - it is just that they did not say 'for-xen-4.5'  in the title.

>http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00667.html

That
>http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00692.html
>

Release-Acked-by: Konrad Rzeszutek Wilk (Konrad.wilk@oracle.com)

For both above. The second is an obvious fix for large scale machines.

>Either
>http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00260.html
>or (my preference)
>http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg01054.html
>

Daniel's patch fixes the case for EFI map or large CPU count (to a certain extent). Jan's patch only fixes it for large CPU count. The memmap can be huge on small CPU count machines.

A proper fix would be to automatically adjust based on memmap and CPU but that could be too complex.

Perhaps putting both patches in is the right way?

>Thanks, Jan

Re: patches pending acks (or naks)

[Jan Beulich]

>>> On 11.12.14 at 16:18, <konrad.wilk@oracle.com> wrote:
> On December 11, 2014 6:39:07 AM EST, Jan Beulich <JBeulich@suse.com> wrote:
>>Either
>>http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00260.html 
>>or (my preference)
>>http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg01054.html 
>>
> 
> Daniel's patch fixes the case for EFI map or large CPU count (to a certain 
> extent). Jan's patch only fixes it for large CPU count. The memmap can be 
> huge on small CPU count machines.

The EFI memory map gets printed much later than when the ring
buffer gets set up with "conring_size=" present.

> A proper fix would be to automatically adjust based on memmap and CPU but 
> that could be too complex.

The problem isn't the complexity, but the chicken-and-egg problem
as far as CPU count is concerned. The memory map size would be
known early enough.

Jan

Re: patches pending acks (or naks)

[Konrad Rzeszutek Wilk]

On Thu, Dec 11, 2014 at 03:38:39PM +0000, Jan Beulich wrote:
> >>> On 11.12.14 at 16:18, <konrad.wilk@oracle.com> wrote:
> > On December 11, 2014 6:39:07 AM EST, Jan Beulich <JBeulich@suse.com> wrote:
> >>Either
> >>http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg00260.html 
> >>or (my preference)
> >>http://lists.xenproject.org/archives/html/xen-devel/2014-12/msg01054.html 
> >>
> > 
> > Daniel's patch fixes the case for EFI map or large CPU count (to a certain 
> > extent). Jan's patch only fixes it for large CPU count. The memmap can be 
> > huge on small CPU count machines.
> 
> The EFI memory map gets printed much later than when the ring
> buffer gets set up with "conring_size=" present.

Right..
> 
> > A proper fix would be to automatically adjust based on memmap and CPU but 
> > that could be too complex.
> 
> The problem isn't the complexity, but the chicken-and-egg problem
> as far as CPU count is concerned. The memory map size would be
> known early enough.

Let me explain my thought process:
 - There are existing knobs that can be used to change this (conring_size=X)
 - But we would like an awesome release where those knobs don't have to
   be used.
 - The patch you posted could be reworked to fully address memmap and CPU.
 - I don't know what your time constraints are for said patch and you
   might not have the energy to rework it.
 - I don't want to put pressure on you or Daniel on having to write new
   patches - especially as folks are going on vacation soon.

Hence as a fallback I believe that Daniel's patch should go in and
Jan's patch can go in too. However if Jan (or somebody else) wants to
rework the v2 (or a new one) to address the memmap issue then that
patch would go in - instead of Daniel's or v2 of Jan patch.

> 
> Jan
> 

Re: patches pending acks (or naks)

[Jan Beulich]

>>> On 11.12.14 at 20:41, <konrad.wilk@oracle.com> wrote:
> On Thu, Dec 11, 2014 at 03:38:39PM +0000, Jan Beulich wrote:
>> >>> On 11.12.14 at 16:18, <konrad.wilk@oracle.com> wrote:
>> > A proper fix would be to automatically adjust based on memmap and CPU but 
>> > that could be too complex.
>> 
>> The problem isn't the complexity, but the chicken-and-egg problem
>> as far as CPU count is concerned. The memory map size would be
>> known early enough.
> 
> Let me explain my thought process:
>  - There are existing knobs that can be used to change this (conring_size=X)
>  - But we would like an awesome release where those knobs don't have to
>    be used.
>  - The patch you posted could be reworked to fully address memmap and CPU.

Not really, unless we separate parsing and printing of the ACPI
tables. Again, the CPU count is known only after that parsing.

>  - I don't know what your time constraints are for said patch and you
>    might not have the energy to rework it.
>  - I don't want to put pressure on you or Daniel on having to write new
>    patches - especially as folks are going on vacation soon.
> 
> Hence as a fallback I believe that Daniel's patch should go in and
> Jan's patch can go in too. However if Jan (or somebody else) wants to
> rework the v2 (or a new one) to address the memmap issue then that
> patch would go in - instead of Daniel's or v2 of Jan patch.

Which memmap issue? You confirmed in your reply that you understand
that the memmap gets printed late enough for the change in v2 to
take effect. Plus those are info-level messages, and hence don't get
printed at all by default. And if somebody alters the log levels, (s)he
can surely be expected to also adjust the ring size. (The log level
aspect is actually another argument against Daniel's patch.)

Jan

Re: patches pending acks (or naks)

[Konrad Rzeszutek Wilk]

On Fri, Dec 12, 2014 at 09:45:17AM +0000, Jan Beulich wrote:
> >>> On 11.12.14 at 20:41, <konrad.wilk@oracle.com> wrote:
> > On Thu, Dec 11, 2014 at 03:38:39PM +0000, Jan Beulich wrote:
> >> >>> On 11.12.14 at 16:18, <konrad.wilk@oracle.com> wrote:
> >> > A proper fix would be to automatically adjust based on memmap and CPU but 
> >> > that could be too complex.
> >> 
> >> The problem isn't the complexity, but the chicken-and-egg problem
> >> as far as CPU count is concerned. The memory map size would be
> >> known early enough.
> > 
> > Let me explain my thought process:
> >  - There are existing knobs that can be used to change this (conring_size=X)
> >  - But we would like an awesome release where those knobs don't have to
> >    be used.
> >  - The patch you posted could be reworked to fully address memmap and CPU.
> 
> Not really, unless we separate parsing and printing of the ACPI
> tables. Again, the CPU count is known only after that parsing.

Right, the logic of increasing the buffer based on CPU count is an
excellent addition.
> 
> >  - I don't know what your time constraints are for said patch and you
> >    might not have the energy to rework it.
> >  - I don't want to put pressure on you or Daniel on having to write new
> >    patches - especially as folks are going on vacation soon.
> > 
> > Hence as a fallback I believe that Daniel's patch should go in and
> > Jan's patch can go in too. However if Jan (or somebody else) wants to
> > rework the v2 (or a new one) to address the memmap issue then that
> > patch would go in - instead of Daniel's or v2 of Jan patch.
> 
> Which memmap issue? You confirmed in your reply that you understand
> that the memmap gets printed late enough for the change in v2 to
> take effect. Plus those are info-level messages, and hence don't get

Correct. The count of memmap entries can be high even with an
small amount of CPUs. Meaning your patch would not modify the
size of the circular buffer in such case (and we would lose some
of the memmap entries being printed). Daniel's patch would
provide a cushion by expanding the default size, however ..

> printed at all by default. And if somebody alters the log levels, (s)he
> can surely be expected to also adjust the ring size. (The log level
> aspect is actually another argument against Daniel's patch.)

... your point about the need to use 'loglvl' points out that
Daniel's patch does not fix the all-generic case.

/me puts on the Xen 4.6 todo 'adjust log buffer based on memmap size'

And your patch is:
Release-Acked-by: Konrad Rzeszutek Wilk <konrad.wilk@oracle.com>

Re: patches pending acks (or naks)

[Jan Beulich]

>>> On 12.12.14 at 17:41, <konrad.wilk@oracle.com> wrote:
> On Fri, Dec 12, 2014 at 09:45:17AM +0000, Jan Beulich wrote:
>> >>> On 11.12.14 at 20:41, <konrad.wilk@oracle.com> wrote:
>> > On Thu, Dec 11, 2014 at 03:38:39PM +0000, Jan Beulich wrote:
>> >> >>> On 11.12.14 at 16:18, <konrad.wilk@oracle.com> wrote:
>> >> > A proper fix would be to automatically adjust based on memmap and CPU but 
>> >> > that could be too complex.
>> >> 
>> >> The problem isn't the complexity, but the chicken-and-egg problem
>> >> as far as CPU count is concerned. The memory map size would be
>> >> known early enough.
>> > 
>> > Let me explain my thought process:
>> >  - There are existing knobs that can be used to change this (conring_size=X)
>> >  - But we would like an awesome release where those knobs don't have to
>> >    be used.
>> >  - The patch you posted could be reworked to fully address memmap and CPU.
>> 
>> Not really, unless we separate parsing and printing of the ACPI
>> tables. Again, the CPU count is known only after that parsing.
> 
> Right, the logic of increasing the buffer based on CPU count is an
> excellent addition.
>> 
>> >  - I don't know what your time constraints are for said patch and you
>> >    might not have the energy to rework it.
>> >  - I don't want to put pressure on you or Daniel on having to write new
>> >    patches - especially as folks are going on vacation soon.
>> > 
>> > Hence as a fallback I believe that Daniel's patch should go in and
>> > Jan's patch can go in too. However if Jan (or somebody else) wants to
>> > rework the v2 (or a new one) to address the memmap issue then that
>> > patch would go in - instead of Daniel's or v2 of Jan patch.
>> 
>> Which memmap issue? You confirmed in your reply that you understand
>> that the memmap gets printed late enough for the change in v2 to
>> take effect. Plus those are info-level messages, and hence don't get
> 
> Correct. The count of memmap entries can be high even with an
> small amount of CPUs. Meaning your patch would not modify the
> size of the circular buffer in such case (and we would lose some
> of the memmap entries being printed).

I'm not following - the patch doesn't make the ring size dependent
on CPU count, that was the case already before. It only allows the
permanent ring buffer to be allocated quite a bit earlier thus
avoiding messages from getting overwritten.

> Daniel's patch would
> provide a cushion by expanding the default size, however ..
> 
>> printed at all by default. And if somebody alters the log levels, (s)he
>> can surely be expected to also adjust the ring size. (The log level
>> aspect is actually another argument against Daniel's patch.)
> 
> ... your point about the need to use 'loglvl' points out that
> Daniel's patch does not fix the all-generic case.
> 
> /me puts on the Xen 4.6 todo 'adjust log buffer based on memmap size'

I'm not convinced this is a good idea, or else we should account for
possible other high volume sources too. Plus you'd still not be in the
position to auto-size the ring buffer in a way accounting for both
CPU count and memmap size.

Jan