Linux kernel mirror (for testing) git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git
kernel os linux
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

Revert "printk: extend console_lock for per-console locking"

This reverts commit 8e274732115f63c1d09136284431b3555bd5cc56.

The testing of 5.19 release candidates revealed missing synchronization
between early and regular console functionality.

It would be possible to start the console kthreads later as a workaround.
But it is clear that console lock serialized console drivers between
each other. It opens a big area of possible problems that were not
considered by people involved in the development and review.

printk() is crucial for debugging kernel issues and console output is
very important part of it. The number of consoles is huge and a proper
review would take some time. As a result it need to be reverted for 5.19.

Link: https://lore.kernel.org/r/YrBdjVwBOVgLfHyb@alley
Signed-off-by: Petr Mladek <pmladek@suse.com>
Link: https://lore.kernel.org/r/20220623145157.21938-5-pmladek@suse.com

+56 -220
-15
include/linux/console.h
··· 16 16 17 17 #include <linux/atomic.h> 18 18 #include <linux/types.h> 19 - #include <linux/mutex.h> 20 19 21 20 struct vc_data; 22 21 struct console_font_op; ··· 154 155 u64 seq; 155 156 unsigned long dropped; 156 157 struct task_struct *thread; 157 - bool blocked; 158 - 159 - /* 160 - * The per-console lock is used by printing kthreads to synchronize 161 - * this console with callers of console_lock(). This is necessary in 162 - * order to allow printing kthreads to run in parallel to each other, 163 - * while each safely accessing the @blocked field and synchronizing 164 - * against direct printing via console_lock/console_unlock. 165 - * 166 - * Note: For synchronizing against direct printing via 167 - * console_trylock/console_unlock, see the static global 168 - * variable @console_kthreads_active. 169 - */ 170 - struct mutex lock; 171 158 172 159 void *data; 173 160 struct console *next;
+56 -205
kernel/printk/printk.c
··· 224 224 static int nr_ext_console_drivers; 225 225 226 226 /* 227 - * Used to synchronize printing kthreads against direct printing via 228 - * console_trylock/console_unlock. 229 - * 230 - * Values: 231 - * -1 = console kthreads atomically blocked (via global trylock) 232 - * 0 = no kthread printing, console not locked (via trylock) 233 - * >0 = kthread(s) actively printing 234 - * 235 - * Note: For synchronizing against direct printing via 236 - * console_lock/console_unlock, see the @lock variable in 237 - * struct console. 238 - */ 239 - static atomic_t console_kthreads_active = ATOMIC_INIT(0); 240 - 241 - #define console_kthreads_atomic_tryblock() \ 242 - (atomic_cmpxchg(&console_kthreads_active, 0, -1) == 0) 243 - #define console_kthreads_atomic_unblock() \ 244 - atomic_cmpxchg(&console_kthreads_active, -1, 0) 245 - #define console_kthreads_atomically_blocked() \ 246 - (atomic_read(&console_kthreads_active) == -1) 247 - 248 - #define console_kthread_printing_tryenter() \ 249 - atomic_inc_unless_negative(&console_kthreads_active) 250 - #define console_kthread_printing_exit() \ 251 - atomic_dec(&console_kthreads_active) 252 - 253 - /* 254 227 * Helper macros to handle lockdep when locking/unlocking console_sem. We use 255 228 * macros instead of functions so that _RET_IP_ contains useful information. 256 229 */ ··· 268 295 static bool panic_in_progress(void) 269 296 { 270 297 return unlikely(atomic_read(&panic_cpu) != PANIC_CPU_INVALID); 271 - } 272 - 273 - /* 274 - * Tracks whether kthread printers are all blocked. A value of true implies 275 - * that the console is locked via console_lock() or the console is suspended. 276 - * Writing to this variable requires holding @console_sem. 277 - */ 278 - static bool console_kthreads_blocked; 279 - 280 - /* 281 - * Block all kthread printers from a schedulable context. 282 - * 283 - * Requires holding @console_sem. 284 - */ 285 - static void console_kthreads_block(void) 286 - { 287 - struct console *con; 288 - 289 - for_each_console(con) { 290 - mutex_lock(&con->lock); 291 - con->blocked = true; 292 - mutex_unlock(&con->lock); 293 - } 294 - 295 - console_kthreads_blocked = true; 296 - } 297 - 298 - /* 299 - * Unblock all kthread printers from a schedulable context. 300 - * 301 - * Requires holding @console_sem. 302 - */ 303 - static void console_kthreads_unblock(void) 304 - { 305 - struct console *con; 306 - 307 - for_each_console(con) { 308 - mutex_lock(&con->lock); 309 - con->blocked = false; 310 - mutex_unlock(&con->lock); 311 - } 312 - 313 - console_kthreads_blocked = false; 314 298 } 315 299 316 300 /* ··· 2603 2673 down_console_sem(); 2604 2674 console_suspended = 0; 2605 2675 console_unlock(); 2676 + 2677 + /* 2678 + * While suspended, new records may have been added to the 2679 + * ringbuffer. Wake up the kthread printers to print them. 2680 + */ 2681 + wake_up_klogd(); 2682 + 2606 2683 pr_flush(1000, true); 2607 2684 } 2608 2685 ··· 2628 2691 /* If trylock fails, someone else is doing the printing */ 2629 2692 if (console_trylock()) 2630 2693 console_unlock(); 2631 - else { 2632 - /* 2633 - * If a new CPU comes online, the conditions for 2634 - * printer_should_wake() may have changed for some 2635 - * kthread printer with !CON_ANYTIME. 2636 - */ 2637 - wake_up_klogd(); 2638 - } 2694 + 2695 + /* Wake kthread printers. Some may have become usable. */ 2696 + wake_up_klogd(); 2639 2697 } 2640 2698 return 0; 2641 2699 } ··· 2650 2718 down_console_sem(); 2651 2719 if (console_suspended) 2652 2720 return; 2653 - console_kthreads_block(); 2654 2721 console_locked = 1; 2655 2722 console_may_schedule = 1; 2656 2723 } ··· 2671 2740 up_console_sem(); 2672 2741 return 0; 2673 2742 } 2674 - if (!console_kthreads_atomic_tryblock()) { 2675 - up_console_sem(); 2676 - return 0; 2677 - } 2678 2743 console_locked = 1; 2679 2744 console_may_schedule = 0; 2680 2745 return 1; ··· 2679 2752 2680 2753 int is_console_locked(void) 2681 2754 { 2682 - return (console_locked || atomic_read(&console_kthreads_active)); 2755 + return console_locked; 2683 2756 } 2684 2757 EXPORT_SYMBOL(is_console_locked); 2685 2758 ··· 2723 2796 * Check if the given console is currently capable and allowed to print 2724 2797 * records. 2725 2798 * 2726 - * Requires holding the console_lock. 2799 + * Requires the console_lock. 2727 2800 */ 2728 2801 static inline bool console_is_usable(struct console *con) 2729 2802 { ··· 2736 2809 static void __console_unlock(void) 2737 2810 { 2738 2811 console_locked = 0; 2739 - 2740 - /* 2741 - * Depending on whether console_lock() or console_trylock() was used, 2742 - * appropriately allow the kthread printers to continue. 2743 - */ 2744 - if (console_kthreads_blocked) 2745 - console_kthreads_unblock(); 2746 - else 2747 - console_kthreads_atomic_unblock(); 2748 - 2749 - /* 2750 - * New records may have arrived while the console was locked. 2751 - * Wake the kthread printers to print them. 2752 - */ 2753 - wake_up_klogd(); 2754 - 2755 2812 up_console_sem(); 2756 2813 } 2757 2814 ··· 2753 2842 * 2754 2843 * @handover will be set to true if a printk waiter has taken over the 2755 2844 * console_lock, in which case the caller is no longer holding the 2756 - * console_lock. Otherwise it is set to false. A NULL pointer may be provided 2757 - * to disable allowing the console_lock to be taken over by a printk waiter. 2845 + * console_lock. Otherwise it is set to false. 2758 2846 * 2759 2847 * Returns false if the given console has no next record to print, otherwise 2760 2848 * true. 2761 2849 * 2762 - * Requires the console_lock if @handover is non-NULL. 2763 - * Requires con->lock otherwise. 2850 + * Requires the console_lock. 2764 2851 */ 2765 - static bool __console_emit_next_record(struct console *con, char *text, char *ext_text, 2766 - char *dropped_text, bool *handover) 2852 + static bool console_emit_next_record(struct console *con, char *text, char *ext_text, 2853 + char *dropped_text, bool *handover) 2767 2854 { 2768 - static atomic_t panic_console_dropped = ATOMIC_INIT(0); 2855 + static int panic_console_dropped; 2769 2856 struct printk_info info; 2770 2857 struct printk_record r; 2771 2858 unsigned long flags; ··· 2772 2863 2773 2864 prb_rec_init_rd(&r, &info, text, CONSOLE_LOG_MAX); 2774 2865 2775 - if (handover) 2776 - *handover = false; 2866 + *handover = false; 2777 2867 2778 2868 if (!prb_read_valid(prb, con->seq, &r)) 2779 2869 return false; ··· 2780 2872 if (con->seq != r.info->seq) { 2781 2873 con->dropped += r.info->seq - con->seq; 2782 2874 con->seq = r.info->seq; 2783 - if (panic_in_progress() && 2784 - atomic_fetch_inc_relaxed(&panic_console_dropped) > 10) { 2875 + if (panic_in_progress() && panic_console_dropped++ > 10) { 2785 2876 suppress_panic_printk = 1; 2786 2877 pr_warn_once("Too many dropped messages. Suppress messages on non-panic CPUs to prevent livelock.\n"); 2787 2878 } ··· 2802 2895 len = record_print_text(&r, console_msg_format & MSG_FORMAT_SYSLOG, printk_time); 2803 2896 } 2804 2897 2805 - if (handover) { 2806 - /* 2807 - * While actively printing out messages, if another printk() 2808 - * were to occur on another CPU, it may wait for this one to 2809 - * finish. This task can not be preempted if there is a 2810 - * waiter waiting to take over. 2811 - * 2812 - * Interrupts are disabled because the hand over to a waiter 2813 - * must not be interrupted until the hand over is completed 2814 - * (@console_waiter is cleared). 2815 - */ 2816 - printk_safe_enter_irqsave(flags); 2817 - console_lock_spinning_enable(); 2898 + /* 2899 + * While actively printing out messages, if another printk() 2900 + * were to occur on another CPU, it may wait for this one to 2901 + * finish. This task can not be preempted if there is a 2902 + * waiter waiting to take over. 2903 + * 2904 + * Interrupts are disabled because the hand over to a waiter 2905 + * must not be interrupted until the hand over is completed 2906 + * (@console_waiter is cleared). 2907 + */ 2908 + printk_safe_enter_irqsave(flags); 2909 + console_lock_spinning_enable(); 2818 2910 2819 - /* don't trace irqsoff print latency */ 2820 - stop_critical_timings(); 2821 - } 2822 - 2911 + stop_critical_timings(); /* don't trace print latency */ 2823 2912 call_console_driver(con, write_text, len, dropped_text); 2913 + start_critical_timings(); 2824 2914 2825 2915 con->seq++; 2826 2916 2827 - if (handover) { 2828 - start_critical_timings(); 2829 - *handover = console_lock_spinning_disable_and_check(); 2830 - printk_safe_exit_irqrestore(flags); 2831 - } 2917 + *handover = console_lock_spinning_disable_and_check(); 2918 + printk_safe_exit_irqrestore(flags); 2832 2919 skip: 2833 2920 return true; 2834 - } 2835 - 2836 - /* 2837 - * Print a record for a given console, but allow another printk() caller to 2838 - * take over the console_lock and continue printing. 2839 - * 2840 - * Requires the console_lock, but depending on @handover after the call, the 2841 - * caller may no longer have the console_lock. 2842 - * 2843 - * See __console_emit_next_record() for argument and return details. 2844 - */ 2845 - static bool console_emit_next_record_transferable(struct console *con, char *text, char *ext_text, 2846 - char *dropped_text, bool *handover) 2847 - { 2848 - /* 2849 - * Handovers are only supported if threaded printers are atomically 2850 - * blocked. The context taking over the console_lock may be atomic. 2851 - */ 2852 - if (!console_kthreads_atomically_blocked()) { 2853 - *handover = false; 2854 - handover = NULL; 2855 - } 2856 - 2857 - return __console_emit_next_record(con, text, ext_text, dropped_text, handover); 2858 2921 } 2859 2922 2860 2923 /* ··· 2878 3001 2879 3002 if (con->flags & CON_EXTENDED) { 2880 3003 /* Extended consoles do not print "dropped messages". */ 2881 - progress = console_emit_next_record_transferable(con, &text[0], 2882 - &ext_text[0], NULL, handover); 3004 + progress = console_emit_next_record(con, &text[0], 3005 + &ext_text[0], NULL, 3006 + handover); 2883 3007 } else { 2884 - progress = console_emit_next_record_transferable(con, &text[0], 2885 - NULL, &dropped_text[0], handover); 3008 + progress = console_emit_next_record(con, &text[0], 3009 + NULL, &dropped_text[0], 3010 + handover); 2886 3011 } 2887 3012 if (*handover) 2888 3013 return false; ··· 2999 3120 if (oops_in_progress) { 3000 3121 if (down_trylock_console_sem() != 0) 3001 3122 return; 3002 - if (!console_kthreads_atomic_tryblock()) { 3003 - up_console_sem(); 3004 - return; 3005 - } 3006 3123 } else 3007 3124 console_lock(); 3008 3125 ··· 3081 3206 console_lock(); 3082 3207 console->flags |= CON_ENABLED; 3083 3208 console_unlock(); 3209 + 3210 + /* Wake the newly enabled kthread printer. */ 3211 + wake_up_klogd(); 3212 + 3084 3213 __pr_flush(console, 1000, true); 3085 3214 } 3086 3215 EXPORT_SYMBOL(console_start); ··· 3286 3407 3287 3408 newcon->dropped = 0; 3288 3409 newcon->thread = NULL; 3289 - newcon->blocked = true; 3290 - mutex_init(&newcon->lock); 3291 3410 3292 3411 if (newcon->flags & CON_PRINTBUFFER) { 3293 3412 /* Get a consistent copy of @syslog_seq. */ ··· 3586 3709 console_unlock(); 3587 3710 } 3588 3711 3589 - /* 3590 - * Print a record for a given console, not allowing another printk() caller 3591 - * to take over. This is appropriate for contexts that do not have the 3592 - * console_lock. 3593 - * 3594 - * See __console_emit_next_record() for argument and return details. 3595 - */ 3596 - static bool console_emit_next_record(struct console *con, char *text, char *ext_text, 3597 - char *dropped_text) 3598 - { 3599 - return __console_emit_next_record(con, text, ext_text, dropped_text, NULL); 3600 - } 3601 - 3602 3712 static bool printer_should_wake(struct console *con, u64 seq) 3603 3713 { 3604 3714 short flags; ··· 3593 3729 if (kthread_should_stop() || !printk_kthreads_available) 3594 3730 return true; 3595 3731 3596 - if (con->blocked || 3597 - console_kthreads_atomically_blocked()) { 3732 + if (console_suspended) 3598 3733 return false; 3599 - } 3600 3734 3601 3735 /* 3602 3736 * This is an unsafe read from con->flags, but a false positive is ··· 3615 3753 struct console *con = data; 3616 3754 char *dropped_text = NULL; 3617 3755 char *ext_text = NULL; 3756 + bool handover; 3618 3757 u64 seq = 0; 3619 3758 char *text; 3620 3759 int error; ··· 3665 3802 if (error) 3666 3803 continue; 3667 3804 3668 - error = mutex_lock_interruptible(&con->lock); 3669 - if (error) 3670 - continue; 3805 + console_lock(); 3671 3806 3672 - if (con->blocked || 3673 - !console_kthread_printing_tryenter()) { 3674 - /* Another context has locked the console_lock. */ 3675 - mutex_unlock(&con->lock); 3807 + if (console_suspended) { 3808 + up_console_sem(); 3676 3809 continue; 3677 3810 } 3678 3811 3679 - /* 3680 - * Although this context has not locked the console_lock, it 3681 - * is known that the console_lock is not locked and it is not 3682 - * possible for any other context to lock the console_lock. 3683 - * Therefore it is safe to read con->flags. 3684 - */ 3685 - 3686 - if (!__console_is_usable(con->flags)) { 3687 - console_kthread_printing_exit(); 3688 - mutex_unlock(&con->lock); 3812 + if (!console_is_usable(con)) { 3813 + __console_unlock(); 3689 3814 continue; 3690 3815 } 3691 3816 ··· 3686 3835 * which can conditionally invoke cond_resched(). 3687 3836 */ 3688 3837 console_may_schedule = 0; 3689 - console_emit_next_record(con, text, ext_text, dropped_text); 3838 + console_emit_next_record(con, text, ext_text, dropped_text, &handover); 3839 + if (handover) 3840 + continue; 3690 3841 3691 3842 seq = con->seq; 3692 3843 3693 - console_kthread_printing_exit(); 3694 - 3695 - mutex_unlock(&con->lock); 3844 + __console_unlock(); 3696 3845 } 3697 3846 3698 3847 con_printk(KERN_INFO, con, "printing thread stopped\n");