Re: [PATCH next v1 2/3] printk: remove safe buffers
On 2021-03-29, Petr Mladek wrote: > I wonder if some console drivers rely on the fact that the write() > callback is called with interrupts disabled. > > IMHO, it would be a bug when any write() callback expects that > callers disabled the interrupts. Agreed. > Do you plan to remove the console-spinning stuff after offloading > consoles to the kthreads? Yes. Although a similar concept will be introduced to allow the threaded printers and the atomic consoles to compete. > Will you call console write() callback with irq enabled from the > kthread? No. That defeats the fundamental purpose of this entire rework excercise. ;-) > Anyway, we should at least add a comment why the interrupts are > disabled. I decided to move the local_irq_save/restore inside the console-spinning functions and added a comment for v2. John Ogness ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH next v1 2/3] printk: remove safe buffers
On 2021-03-29, John Ogness wrote: >> Will you call console write() callback with irq enabled from the >> kthread? > > No. That defeats the fundamental purpose of this entire rework > excercise. ;-) Sorry, I misread your question. The answer is "yes". We want to avoid a local_irq_save() when calling into console->write(). John Ogness ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Congratulations ($ 100,800,000.00)
Hello,i'm Mackenzie Scott,Ex-wife of Amazon founder i'm donating $4 billion to charities,individuals,universities across the Globe from my divorce funds,i'm donating part of it to provide immediate support to people suffering economically during the COVID-19 pandemic,i have a donation worth $100,800,000.00 Dollars for you,you can contact me for more information if you're interested. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Congratulations ($ 100,800,000.00)
Hello,i'm Mackenzie Scott,Ex-wife of Amazon founder i'm donating $4 billion to charities,individuals,universities across the Globe from my divorce funds,i'm donating part of it to provide immediate support to people suffering economically during the COVID-19 pandemic,i have a donation worth $100,800,000.00 Dollars for you,you can contact me for more information if you're interested. ___ kexec mailing list kexec@lists.infradead.org http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH next v1 2/3] printk: remove safe buffers
On Fri 2021-03-26 12:12:37, John Ogness wrote:
> On 2021-03-23, Petr Mladek wrote:
> >> --- a/kernel/printk/printk.c
> >> +++ b/kernel/printk/printk.c
> >> -
> >>if (seq != prb_next_seq(&printk_rb_static)) {
> >>pr_err("dropped %llu messages\n",
> >> prb_next_seq(&printk_rb_static) - seq);
> >> @@ -2666,7 +2631,6 @@ void console_unlock(void)
> >>size_t ext_len = 0;
> >>size_t len;
> >>
> >> - printk_safe_enter_irqsave(flags);
> >> skip:
> >>if (!prb_read_valid(prb, console_seq, &r))
> >>break;
> >> @@ -2711,6 +2675,8 @@ void console_unlock(void)
> >>printk_time);
> >>console_seq++;
> >>
> >> + printk_safe_enter_irqsave(flags);
> >
> > What is the purpose of the printk_safe context here, please?
>
> console_lock_spinning_enable() needs to be called with interrupts
> disabled. I should have just used local_irq_save().
>
> I could add local_irq_save() to console_lock_spinning_enable() and
> restore them at the end of console_lock_spinning_disable_and_check(),
> but then I would need to add a @flags argument to both functions. I
> think it is simpler to just do the disable/enable from the caller,
> console_unlock().
I see. I have missed it that all this code have to be called with
interrupts disabled.
OK, it is a must-to-have because of the spinning. But I wonder if some
console drivers rely on the fact that the write() callback is
called with interrupts disabled.
IMHO, it would be a bug when any write() callback expects that
callers disabled the interrupts.
Do you plan to remove the console-spinning stuff after offloading
consoles to the kthreads?
Will you call console write() callback with irq enabled from
the kthread?
Anyway, we should at least add a comment why the interrupts are
disabled.
> BTW, I could not find any sane way of disabling interrupts via a
> raw_spin_lock_irqsave() of @console_owner_lock because of the how it is
> used with lockdep. In particular for
> console_lock_spinning_disable_and_check().
I see. IMHO, we would need to explicitly call local_irq_save()/restore()
if we moved them to console_lock_spinning_enable()/disable_and_check().
I mean to do:
static void console_lock_spinning_enable(unsigned long *flags)
{
local_irq_save(*flags);
raw_spin_lock(&console_owner_lock);
console_owner = current;
raw_spin_unlock(&console_owner_lock);
/* The waiter may spin on us after setting console_owner */
spin_acquire(&console_owner_dep_map, 0, 0, _THIS_IP_);
}
...
Best Regards,
Petr
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
Re: [PATCH v14 01/11] x86: kdump: replace the hard-coded alignment with macro CRASH_ALIGN
On 2021/3/2 15:43, Baoquan He wrote:
> On 02/26/21 at 09:38am, Eric W. Biederman wrote:
>> chenzhou writes:
>>
>>> On 2021/2/25 15:25, Baoquan He wrote:
On 02/24/21 at 02:19pm, Catalin Marinas wrote:
> On Sat, Jan 30, 2021 at 03:10:15PM +0800, Chen Zhou wrote:
>> Move CRASH_ALIGN to header asm/kexec.h for later use. Besides, the
>> alignment of crash kernel regions in x86 is 16M(CRASH_ALIGN), but
>> function reserve_crashkernel() also used 1M alignment. So just
>> replace hard-coded alignment 1M with macro CRASH_ALIGN.
> [...]
>> @@ -510,7 +507,7 @@ static void __init reserve_crashkernel(void)
>> } else {
>> unsigned long long start;
>>
>> -start = memblock_phys_alloc_range(crash_size, SZ_1M,
>> crash_base,
>> +start = memblock_phys_alloc_range(crash_size,
>> CRASH_ALIGN, crash_base,
>>crash_base +
>> crash_size);
>> if (start != crash_base) {
>> pr_info("crashkernel reservation failed -
>> memory is in use.\n");
> There is a small functional change here for x86. Prior to this patch,
> crash_base passed by the user on the command line is allowed to be 1MB
> aligned. With this patch, such reservation will fail.
>
> Is the current behaviour a bug in the current x86 code or it does allow
> 1MB-aligned reservations?
Hmm, you are right. Here we should keep 1MB alignment as is because
users specify the address and size, their intention should be respected.
The 1MB alignment for fixed memory region reservation was introduced in
below commit, but it doesn't tell what is Eric's request at that time, I
guess it meant respecting users' specifying.
>>
>>> I think we could make the alignment unified. Why is the alignment system
>>> reserved and
>>> user specified different? Besides, there is no document about the 1MB
>>> alignment.
>>> How about adding the alignment size(16MB) in doc if user specified
>>> start address as arm64 does.
>> Looking at what the code is doing. Attempting to reserve a crash region
>> at the location the user specified. Adding unnecessary alignment
>> constraints is totally broken.
>>
>> I am not even certain enforcing a 1MB alignment makes sense. I suspect
>> it was added so that we don't accidentally reserve low memory on x86.
>> Frankly I am not even certain that makes sense.
>>
>> Now in practice there might be an argument for 2MB alignment that goes
>> with huge page sizes on x86. But until someone finds that there are
>> actual problems with 1MB alignment I would not touch it.
>>
>> The proper response to something that isn't documented and confusing is
>> not to arbitrarily change it and risk breaking users. Especially in
>> this case where it is clear that adding additional alignment is total
>> nonsense. The proper response to something that isn't clear and
>> documented is to dig in and document it, or to leave it alone and let it
> Sounds reasonable. Then adding document or code comment around looks
> like a good way to go further so that people can easily get why its
> alignment is different than other reservation.
Hi Baoquan & Eric,
Sorry for late reply, i missed it earlier.
Thanks for your explanation, i will just leave the 1MB alignment here as is.
I will introduce CRASH_ALIGN_SPECIFIED to help make function
reserve_crashkernel generic.
CRASH_ALIGN_SPECIFIED is used for user specified start address which is
distinct from
default CRASH_ALIGN.
Thanks,
Chen Zhou
>
>> be the next persons problem.
>>
>> In this case there is no reason for changing this bit of code.
>> All CRASH_ALIGN is about is a default alignment when none is specified.
>> It is not a functional requirement but just something so that things
>> come out nicely.
>>
>>
>> Eric
>>
> .
>
___
kexec mailing list
kexec@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/kexec
