Сделаем код чище: Что можно исправить в ядре Linux

    Наверняка многие хотели бы попробовать что-то изменить в ядре Linux к лучшему, но не знают с чего начать. Я хочу описать несколько проблем, исправить которые под силу каждому, и на примере показать путь от нахождения проблемы до опубликования её исправления в списке рассылки. По ходу повествования читатель познакомится с некоторыми вспомогательными утилитами.

    Из года в год, из драйвера в драйвер кочуют одни и те же тривиальные проблемы, часто связанные с незнанием каких-то стандартных практик, утилит или расширений, существующих в ядре Linux.

    Вот краткий список такого рода проблем:



    Опечатки и описки



    Опечатки и описки в документации и комментариях — дело не редкое. Один человек, а именно Lucas De Marchi, разработал специальную утилиту codespell, чтобы отлавливать такие опечатки.

    Нижеследующий пример всё о себе расскажет сам.

    Клонируем ядро:
    mkdir ~/devel
    cd ~/devel
    git clone git://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git
    cd ~/devel/linux-next
    

    Обратите внимание, что работать мы будем с самым свежим, то есть с деревом linux-next.

    Совершаем тестовый запуск codespell:
    $ codespell.py drivers/staging/unisys
    drivers/staging/unisys/include/guestlinuxdebug.h:138: doesnt  ==> doesn't
    

    Теперь исправляем:
    $ codespell.py -w -i 3 drivers/staging/unisys
                                                             * doesnt show, so we
            doesnt  ==> doesn't (Y/n) y
    FIXED: drivers/staging/unisys/include/guestlinuxdebug.h
    

    Смотрим, что получилось:
    --- a/drivers/staging/unisys/include/guestlinuxdebug.h
    +++ b/drivers/staging/unisys/include/guestlinuxdebug.h
    @@ -135,7 +135,7 @@ enum event_pc {                     /* POSTCODE event identifier tuples */
     #define POSTCODE_SEVERITY_ERR DIAG_SEVERITY_ERR
     #define POSTCODE_SEVERITY_WARNING DIAG_SEVERITY_WARNING
     #define POSTCODE_SEVERITY_INFO DIAG_SEVERITY_PRINT     /* TODO-> Info currently
    -                                                        * doesnt show, so we
    +                                                        * doesn't show, so we
                                                             * set info=warning */
     /* example call of POSTCODE_LINUX_2(VISOR_CHIPSET_PC, POSTCODE_SEVERITY_ERR);
      * Please also note that the resulting postcode is in hex, so if you are
    



    Собственная реализация вывода



    Не столько проблема, сколько улучшение читаемости кода и микрооптимизация: часто значительное уменьшение расходуемой памяти на стеке при вызове функции, уменьшение количества передаваемых спецификаторов в vsnprintf().

    Ранее я описал специальные расширения спецификатора %p в ядре, теперь очередь за применением полученных знаний.


    В качестве простоты возьмём шаблон
    %02x[-: ]%02x[-: ]%02x. Он позволяет находить передачу нескольких байт через стек, которую можно заменить расширением %*ph[CDN].

    Поищем в коде:
    $ git grep -n -i -e '%02x[-: ]%02x[-: ]%02x' drivers/staging/unisys
    
    drivers/staging/unisys/virtpci/virtpci.c:1313:                                  "[%d:%d] VNic:%02x:%02x:%02x:%02x:%02x:%02x num_rcv_bufs:%d mtu:%d",
    


    Что будем делать далее, я опишу на примере ниже. А пока переходим к следующим проблемным местам.


    Собственная реализация алгоритмов



    Вот, возьмём, к примеру, drivers/staging/fbtft/fbtft-bus.c, строки 99-100:
    for (i = 0; i < pad; i++)
      *buf++ = 0x000;
    

    pad определена как u16 и может быть в диапазоне от 0 до 3, то есть от 0 до 6 байт. Как мы знаем, memset() жутко оптимизированная функция, особенно на малых размерах. Почему не применить?

    Или ещё пример из того же драйвера, а именно drivers/staging/fbtft/fbtft-core.c, строки 1091-1096:
      /* make debug message */
      msg[0] = '\0';
      for (j = 0; j < i; j++) {
          snprintf(str, 128, " %02X", buf[j]);
          strcat(msg, str);
      }
    

    Вот не знали люди, что в ядре есть bin2hex(), не говоря уже о том, что strcat() совершенно лишний — snprintf() добавляет терминирующий '\0'.

    Попробуйте модифицировать самостоятельно.

    Кто-то уже увидел как можно ещё упростить?
    На самом деле буфер нужен для вывода дампа в шестнадцатиричном виде, поэтому удаляем цикл, переменную msg и заменяем это всё либо спецификатором %*ph с передаваемой длиной i, либо вызовом print_hex_bytes().

    Обратите внимание далее по коду есть подобное, можно за компанию и его оптимизировать: строки 1192-1202.



    Определение существующих констант и типов данных



    Возвращаясь к драйверу unisys, запустим такую команду:
    $ git grep -n MAX_MACADDR_LEN drivers/staging/unisys/
    drivers/staging/unisys/common-spar/include/channels/iochannel.h:190:#ifndef MAX_MACADDR_LEN
    drivers/staging/unisys/common-spar/include/channels/iochannel.h:191:#define MAX_MACADDR_LEN 6   /* number of bytes…
    drivers/staging/unisys/common-spar/include/channels/iochannel.h:192:#endif
    …и так далее…
    


    Однако стоит отметить, что константа длины MAC адреса давным давно определена в ядре как ETH_ALEN. Уверен, мейнтейнеры с радостью примут от вас патч, заменяющий их определение стандартным ядерным.


    Пример исправления



    Переходим плавно к практике. Выше мы нашли место, где при выводе нескольких байт используется передача каждого из них через стек.

    Если мы посмотрим в код, то увидим следующее:
    str_pos += scnprintf(vbuf + str_pos, len - str_pos,
          "[%d:%d] VNic:%02x:%02x:%02x:%02x:%02x:%02x num_rcv_bufs:%d mtu:%d",
          tmpvpcidev->bus_no,
          tmpvpcidev->device_no,
          tmpvpcidev->net.mac_addr[0],
          tmpvpcidev->net.mac_addr[1],
          tmpvpcidev->net.mac_addr[2],
          tmpvpcidev->net.mac_addr[3],
          tmpvpcidev->net.mac_addr[4],
          tmpvpcidev->net.mac_addr[5],
          tmpvpcidev->net.num_rcv_bufs,
          tmpvpcidev->net.mtu);
    

    А это оказывается кто-то так выводит MAC адрес! Что ж, мы с лёгкостью можем использовать специальное расширение спецификатора — %pM.

    Давайте заменим и посмотрим на результат:
    --- a/drivers/staging/unisys/virtpci/virtpci.c
    +++ b/drivers/staging/unisys/virtpci/virtpci.c
    @@ -1310,15 +1310,10 @@ static ssize_t info_debugfs_read(struct file *file, char __user *buf,
                                            tmpvpcidev->scsi.max.cmd_per_lun);
                    } else {
                            str_pos += scnprintf(vbuf + str_pos, len - str_pos,
    -                                       "[%d:%d] VNic:%02x:%02x:%02x:%02x:%02x:%02x num_rcv_bufs:%d mtu:%d",
    +                                       "[%d:%d] VNic:%pM num_rcv_bufs:%d mtu:%d",
                                            tmpvpcidev->bus_no,
                                            tmpvpcidev->device_no,
    -                                       tmpvpcidev->net.mac_addr[0],
    -                                       tmpvpcidev->net.mac_addr[1],
    -                                       tmpvpcidev->net.mac_addr[2],
    -                                       tmpvpcidev->net.mac_addr[3],
    -                                       tmpvpcidev->net.mac_addr[4],
    -                                       tmpvpcidev->net.mac_addr[5],
    +                                       tmpvpcidev->net.mac_addr,
                                            tmpvpcidev->net.num_rcv_bufs,
                                            tmpvpcidev->net.mtu);
                    }
    

    Вроде бы неплохо — на 5 строк и переменных на стеке меньше. Стоит всё же откомпилировать результат. Подробно я не буду описывать как это делается, лишь укажу, что потребуется включить драйвер в конфигурации с помощью опций:
    CONFIG_STAGING=y
    CONFIG_UNISYSPAR=y
    CONFIG_UNISYS_VIRTPCI=m

    Сохраняем наше изменение в дереве с помощью git commit -a -s и форматируем в виде патча.
    $ git format-patch HEAD~1
    0001-staging-unisys-print-MAC-address-via-pM.patch
    

    Далее, воспользуемся замечательным скриптом get_maintainter.pl, чтобы узнать кого необходимо информировать персонально.
    $ scripts/get_maintainer.pl --git-min-percent=67 --nor --norolestats 00*
    Benjamin Romer <benjamin.romer@unisys.com>
    David Kershner <david.kershner@unisys.com>
    Greg Kroah-Hartman <gregkh@linuxfoundation.org>
    sparmaintainer@unisys.com
    devel@driverdev.osuosl.org
    linux-kernel@vger.kernel.org
    

    Отправляем наш патч по адресам:
    $ git send-email --cc-cmd 'scripts/get_maintainer.pl --git-min-percent=67 --nor --nol --norolestats' 00*
    0001-staging-unisys-print-MAC-address-via-pM.patch
    Who should the emails be sent to (if any)? devel@driverdev.osuosl.org, sparmaintainer@unisys.com
    Message-ID to be used as In-Reply-To for the first email (if any)? 
    …
    Send this email? ([y]es|[n]o|[q]uit|[a]ll): y
    

    Вот и письмецо.

    UPDATE: Уже в ядре: 9a836c0a6310e6e9.

    Similar posts

    Ads
    AdBlock has stolen the banner, but banners are not teeth — they will be back

    More

    Comments 29

      0
      Насколько мощная машина нужна для сборки ядра в приемлемое время?
        +4
        На стационарнике i5-3470 в 5 потоков собирается минут за 10. Но у меня все необходимое включено в ядро, без модулей :)
          +1
          Если модули лишние поотключать — любой не нетбук до 5 лет выдержки.
          У меня на A10-5850 в 6 потоков —
          real 3m25.969s
          user 11m33.297s
          sys 0m45.940s
          Но я думаю в 2.5 реально уложиться — еще много всего не выкинул лишнего из модулей, да и диск очень старый.
          +1
          За удобную отправку в нужные ML и мейнтернерам огромное спасибо! В свое время намучился вручную через консольные мейл-клиенты отправлять, так как некоторые GUI клиенты портили патчи.
            +22
              /* make debug message */
              msg[0] = '\0';
              for (j = 0; j < i; j++) {
                  snprintf(str, 128, " %02X", buf[j]);
                  strcat(msg, str);
              }
            


            Алгоритм маляра Шлемиэля даже в ядре? Хотя это скорее всего никому не нужный драйвер. Одно дело если бы такая ошибка была в fs/ и другое — в каком-то драйвере, которым пользуются только экстремалы.
              +4
              Алгоритм маляра Шлемиэля

              Это 5! Не слышал раньше.
                +1
                Почитайте Джоэля, местами очень познавательно :-)
              +1
              --git-min-percent=67

              Это хорошо, да. А то, бывает, поправишь где-нибудь три строчки, а любители get_maintainer.pl начинают тебя после этого в cc: включать.
              В списке рассылки qemu-devel однажды поднималась эта тема, с пожеланиями включить --no-git-fallback по умолчанию.
                +6
                Не забываем патч прогнать через checkpatch.pl
                  +2
                  А это оказывается кто-то так выводит MAC адрес! Что ж, мы с лёгкостью можем использовать специальное расширение спецификатора — %pM.


                  А где это можно использовать в ядре? Везде? Вот, например, в /arch/powerpc/boot/devtree.c можно? Или там еще какие-нибудь библиотеки не загружены?
                    +2
                    Вы задали хороший вопрос, возможно я напишу заметку о бардаке, который творится в arch/. Но если кратко, то arch/*/boot содержит для каждой архитектуры собственные библиотеки, т.к. на момент работы того кода основная часть ядра ещё не декомпрессирована.
                    0
                    В доках есть православный метод работы с linux-next — www.kernel.org/doc/man-pages/linux-next.html
                      0
                      Забыл написать сразу. Для тех, кто хочет познакомиться с ядром поближе, есть The Eudyptula Challenge, который на хабре почему-то освещен совсем слабо. 20 заданий, 2 из первых 18 (последние 2 не видел) — отправить тривиальный патч в staging — 1 найти с помощью checkpatch.pl, другой через sparse.

                      Так что checkpatch.pl и sparse можно использовать не только для проверки своих патчей, но и для нахождения фатальных недостатков в drivers/staging.
                      +3
                      Блин, вот смотрю на всех вас. Энтузиасты, полны энергии, амбиций и времени. И завидно становится.
                      Молодцы, и спасибо за работу.
                        0
                        Вот со временем, не совсем так. Бывает из-за этого энтузиазма все выходные дома за ноутом просидишь. Но бывает это меня спасало от походов по магазинам с женой — «Я очень занят, разбираюсь с очень плохой багой у нас, времени нет, иди с подружками сама», а сам в Qt колупаюсь, патчик готовлю под пиво :)
                          +1
                          Меня всего то на пару месяцев на OpenSuse хватило, а потом ребёнок родился и защита :(
                          С тех пор уже кроме мелких поделок для себя — ничего не делал.
                            0
                            Поймал себя на том, что задумался, как иначе-то можно выходные проводить.
                            За десктопом, разве что, лучше.
                              0
                              Да, иногда два 23' монитора не хватает дома, но привык работать и с ноутбучным 15ым.
                          +1
                          И из этой статьи можно поискать плохие куски кода: PVS-Studio покопался во внутренностях Linux
                            +1
                            Ну вот я нашел одно такое место, написал тем кто создавал/правил файл, у них почта давно протухла. Написал в мэйл-лист, там молчат. И чего делать?
                              0
                              Ответ не обязательно придет сразу, это же опенсорс. Мейнтейнер вашего куска кода может быть в отпуске или в тюрьме. А если исправление касалось очень старого драйвера — возможно сам драйвер попахивает и пора поднимать вопрос о его исключении? Для оперативного обсуждения можно попробовать в IRC (тоже не факт что ответят сразу).
                                0
                                Мейнтейнер в тюрме! Аж заинтриговало его детище.
                                  0
                                    0
                                    ReiserFS, я промахнулся с добавлением комментария, там была ссылочка, которая при редактировании пропала.

                                    P.S. Опередили:)
                                  0
                                  Во-первых, может потребоваться какое-то время на ответ, конкретное время зависит от подсистемы, в которую вы отправили патч. Различные исправления кодстайла, мелкие чистки кода и другие некритические изменения, часто, имеют меньший приоритет, по сравнению с исправлениями багов, так что нет ничего удивительного, что вам не отвечают сразу.
                                  Во-вторых, перед тем как отправить патч стоило почитать правила, которым он должен удовлетворять, как минимум должен быть Signed-off-by и описание изменений. Правила описаны тут.
                                    0
                                    Добро пожаловать в клуб любителей SCSI! Вот www.spinics.net/lists/linux-rdma/msg19739.html ничего не напоминает? Посмотрите на дату :) Да, а по какаим адресам?
                                      0
                                      Открыл дерево коммитов, поискал адреса в файле MAINTAINERS. Но как я понял, идея бесперспективная? Раз и вам год не ответили.
                                        0
                                        Первая версия была в августе 2011 :-). Подсистема SCSI — самая консервативная в ядре. Туда удаётся пробить патчи, правда. Но это сопряжено с бешеными трудностями, так что это hard path. Вы посмотрите лучше на мой патч как образец и изменения (всё уходит в одну строку), и его оформления (поле темы, тело письма), а затем найдите что-нибудь в drivers/staging. Не зря же я про staging писал. Грег очень лояльный мейнтейнер, только обязательно проверяйте изменения перед оправкой как минимум компилированием и checkpatch.pl, иначе рассердите его!
                                    0
                                    мимо

                                    Only users with full accounts can post comments. Log in, please.