Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

alexk99 patches #9

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

alexk99 patches #9

wants to merge 9 commits into from

Conversation

alexk99
Copy link
Contributor

@alexk99 alexk99 commented Dec 22, 2016

const u32 tok = delta_ms * (car->cir / (BITS_PER_BYTE * MSEC_PER_SEC));

Раccчет tok не зависит от переменных, изменяемых внутри критической секции (cs).
Он зависит от delte_ms, которая рассчитывается вне cs и от cir - фактически константа.
Если в рассчет tok подставить формул delta_ms, пыполнить сокращения, то останется
(now - car->last) * (car->cir / (HZ * BITS_PER_BYTE))

Можно выссчитывать вот эту часть заранее, избавившись от лишнего деления (он ведь дорогие,
вроде дороже умножения).

car->cir_hz = (car->cir / (HZ * BITS_PER_BYTE))

В итоге получим
const u32 tok = (now - car->last) * car->cir_hz;

Конечно, добавится умножение в выводе статистике, она же должна много реже выводиться.

критическую секцию можно еще упростить, вынести статистику во вне и переведя ее на atomic операции.
Правда в случае включенного RATE_ESTIMATOR укоратить cs вроде не получается, но вот я бы у себя
его выключил. Это ведь тоже только статистика, правильно?

переменная struct ratelimit_stat.first нигде не используется.
просто удалил.

const u32 tok = delta_ms * (car->cir / (BITS_PER_BYTE * MSEC_PER_SEC));

Раccчет tok не зависит от переменных, изменяемых внутри критической секции (cs).
Он зависит от delte_ms, которая рассчитывается вне cs и от cir - фактически константа.
Если в рассчет tok подставить формул delta_ms, пыполнить сокращения, то останется
(now - car->last) * (car->cir / (HZ * BITS_PER_BYTE))

Можно выссчитывать вот эту часть заранее, избавившись от лишнего деления (он ведь дорогие,
вроде дороже умножения).

 car->cir_hz = (car->cir / (HZ * BITS_PER_BYTE))

В итоге получим
 const u32 tok = (now - car->last) * car->cir_hz;

Конечно, добавится умножение в выводе статистике, она же должна много реже выводиться.

2)
критическую секцию можно еще упростить, вынести статистику во вне и переведя ее на atomic операции.
Правда в случае включенного RATE_ESTIMATOR укоратить cs вроде не получается, но вот я бы у себя
его выключил. Это ведь тоже только статистика, правильно?

3)
переменная struct ratelimit_stat.first нигде не используется.
просто удалил.
@aabc
Copy link
Owner

aabc commented Dec 23, 2016

Спасибо! Выгладит всё корректно. Небольшие комментарии:

cir_hz назван не правильно, так как переменная не имеет отношения к Hz, там вместо bits/s становится bits/jiffies/8. Может лучше оставить старое название.

Не красиво выглядят "игры" с #ifndef RATE_ESTIMATOR #ifdef RATE_ESTIMATOR. Наверное, лучше сделать так:

#ifdef RATE_ESTIMATOR
		if (!match)
			rate_estimator(&ent->stat, now / RATEST_JIFFIES, len);
#endif

		spin_unlock(&ent->lock_bh);

		if (match) {
			atomic64_add(len, &ent->stat.red_bytes);
			atomic_inc(&ent->stat.red_pkt);
		} else {
			atomic64_add(len, &ent->stat.green_bytes);
			atomic_inc(&ent->stat.green_pkt);
		}

Надеюсь вы понимаете, что выигрыш по скорости на современных процах будет минимальный.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 23, 2016

Можно оставить старое, не принципиально.
C rate-estimator получилось отлично, я просто не смотрел, что он делает. Не думал двигать его.
Если выигрышь есть, а его цена час времени упрощения кода, то почему нет.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 23, 2016

код стал попроще и я разглядел в нем еще идею для проверки. первые тесты вроде проходят. сейчас сделаю набор тестов c iperf'ом, чтобы поточнее. Сразу понимаю вопрос а нафига? никто, вроде, и так не замечает влияние модуля в линуксе. Мне хочется получить реально быструю процедуру прохождения пакетами ведра, чтобы многопоточный код, в моем случае dpdk, где pps будет высоким и воркеры будут сильно конкурировать за доступ к ведру, не стал узким местом.

@aabc
Copy link
Owner

aabc commented Dec 23, 2016

У вас патчи меняющие смысл кода. Например, не будет ли вредной race condition в последнем случае (e2f5d7f)? Вероятно, надо обосновать в комментах или дескрипшене патча почему это не критично. Иначе, код выглядит на первый взгляд как ошибочный и через время у людей может возникать желание его исправлять.

А на второй взгляд, car->last читается до spin_lock, разве это не ошибка? Пакет учитывается в tc, а затем проверяется без лока, когда на него уже мог повлиять другой пакет - следовательно, наш пакет может отброситься из-за того, что другой пакет превысил ведерко.

(Да и код с локом нельзя назвать lockless.)

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 23, 2016

Например, не будет ли вредной race condition в последнем случае

race какой переменной?

А на второй взгляд, car->last читается до spin_lock, разве это не ошибка?

Она читается до лока в оригинальном коде.

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

Да, тут у меня ошибка в случае racе переменной tc. Точно, могут отброситься оба пакета, хотя один из них еще мог бы пройти.

(Да и код с локом нельзя назвать lockless.)

В общем случае да. Но если большинству транзакций не нужны локи, то эти случаи и будут lockless. Идея в том, в этом алгоритме можно попробовать не на каждый пакет делать пополнения ведра, в dpdk библиотеке предлагают делать это по истечении заданного время, например. Вторая идея в том, что большая часть пакетов не должна отбрасываться как только скорость стабилизировалась,
поэтом хочется для них сделать прохождение без локов, если это возможно, конечно.

Я еще подумаю на вторым патчем. Для этого и сделал пометку, debug. Идея была начать обсуждение.

А первый патч не меняет логику совсем. Еще раз его проглядел, результат должен быть один в один с оригинальным кодом.

@aabc
Copy link
Owner

aabc commented Dec 23, 2016

Если не на каждом делать пополнение ведерка или наоборот 1 пакет может влиять на другие (из-за race condition), то это уже другой алгоритм. А люди ожидают от ipt-ratelimit алгоритм Cisco RED-like.

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

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 23, 2016 via email

@aabc
Copy link
Owner

aabc commented Dec 23, 2016

Вода вытекала каждую миллисекунду, а стала каждый jiffy. Это могло повлиять на вид пропускаемого трафика.

@aabc
Copy link
Owner

aabc commented Dec 23, 2016

А на второй взгляд, car->last читается до spin_lock, разве это не ошибка?

Она читается до лока в оригинальном коде.

(Только сейчас заметил этот комментарий.) Да, надо проанализировать это.

@aabc
Copy link
Owner

aabc commented Dec 23, 2016

Да, надо проанализировать это.

Баг. Токены могут вытекать быстрее чем нужно.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 23, 2016 via email

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 23, 2016

ошибся, воду читать как мсек. но суть таже

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 23, 2016

Да, надо проанализировать это.

я тоже к чужому коду отношусь порой критичнее, чем к своему. Т.е. так как чтение last показалось в моем коде, то сразу нашелся повод подумать сильнее. Ну это нормально ))

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 23, 2016

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

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

Допустим, пришло пять пакетов. Не важно один и несколько потоков.

Оригинальный алогоритм для каждого пакета пытается выполнить два шага (пока не берем в рассчет переполнение корзины):
1) добавить рамер пакета в корзину;
2) если накопились токены (прошло время) вычесть из корзины накопленные токены;

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

Для для таких случаев можно написать алгоритм только с вычитанием, назвав его условно fast path.
Отличия от предыдущего патча в том, что теперь я отслеживаю конфликты гонок с помощью СAS (compare and swap),
гарантированно соблюдая условие достаточности токенов для каждого отдельного пакета.
На этом этапе можно не думать о пополнениях корзины, даже если и пора. Это можно сделать позже, накопления
не пропадут, т.к. время last пока не меняется.

Пробуя пройти fast path для 5ого последний пакета, замечаем, что в корзине переполнение
и идем по медленному пути с критической секцией. Здесь все внутри секции, никаких сюрпризов.

И последний шаг. Fast path могут пересекаться с другими fast path, они thread safe за счет CAS.
Но, fast path одного потока не должен пересекаться с CS slow path другого.
Для этого добавляем exlusive/shared (или read/write) блокировку вместо spinlock'а.
Shared блокировки дешевые, не должны влиять на производительно быстрых секций. Правда exlusive
дороже, чем обычная. Предполагаем, что в итоге суммарная польза от одновременной работы быстрых,
путей превысет намного потерю от более дорогих, но редких exclusive (write) блокировок.

P.S.
Снова пока не думал про estimator.
@aabc
Copy link
Owner

aabc commented Dec 23, 2016

Сделать полностью lockless (или waitless) интересная задача, надо подумать...

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 23, 2016

полностью lockless не выйдет, т.к. в случае переполнения корзины никуда не деться, надо добавлять в корзину накопившиеся токены и одновременно обновлять last time. Для двух операций, нужен dcas.

Если я ничего не упустил в своем последнем патче, должно быть уже хорошо. Думаю о том, как сделать тесты. Т.к. на живой сети совершенно не понятно сколько там параллельных воркеров linux запускает для двух iperf'ов на два разные dst ip. Хочу сделать синтетический, с 6 настоящими тредами.
И замерить по времени разные варианты оригинальный и последний. Может вообще, оригинальный ничуть не медленнее.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 25, 2016

Сделал тесты реализации последнего патча, назовем ее fast/slow (FS) версией.

Делал два теста: один на корректносить, второй на производительность.

Тест на корректность проверял, что решения для тестового набора пакетов принимаются
одинаково исходной реализацией и FS версией. Тут все ок. Алгоритмы принимают решения
одинаково.

С производительностью все совсем не очевидно.
RW блокировки, точнее сказать Write блокировки оказались намного медленнее, чем я ожидал.
pthread реализация этих блокировок вообще очень тормозная, FS версия работала в !3 раза медленней
оригинальной. Переписал тест под dpdk там своя реализация и для спинлоков и для rw блокировок.
FS стал работать намного быстрее, но все равно медленнее, чем оригинальная. Тут я понял, что мой синтический тест, в отличие от реального iperf, не имеет никакой обратной связи с потерями пакетов, и поток пакетов просто постоянно попадает в Slow path. Увеличил cir примерно до той скорости с которой сыпятся тестовые пакеты и тогда fs версия стала работать быстрее на 10-20% чем оригинальная.

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

Это тест для 4 ядер, интересно, что будет с ростом числа ядер.
Т.е. 4 потока трафика проходило через корзину.

процента red от общего числа пакетов -- profit
2%-6% -- profit 20%
14% -- profit 4%-10%
21% -- profit 5%
23% -- profit -3% -- 5%
33% -- profit -2%
47% -- profit -7%
59% -- profit -28%

Есть разброс в результатах, по-хорошему, его бы автоматизировать, сделать сотни прогонов и построить нормальную табличку.

Если интересны все эти размышления, то поделитесь пожалуйста статистикой, какие у реальных пользователей green/red счетчики.

@alexk99 alexk99 closed this Dec 25, 2016
@alexk99 alexk99 reopened this Dec 25, 2016
@aabc
Copy link
Owner

aabc commented Dec 25, 2016

RW блокировки, точнее сказать Write блокировки оказались намного медленнее, чем я ожидал.

Да, линуксе rwlocks очень медленные поэтому их не рекомендуют использовать. (Навскидку нагуглилось https://lwn.net/Articles/364583/) Про DPDK не знаю. Поэтому никакой реализацией с rwlocks не ускорить то что уже есть.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 25, 2016

в хороших реализациях r-блокировки реализованы как lockless в отсутствии w блокировок. в dpdk, похоже, с ними все хорошо. но 20 процентов в лучших случаях - конечно маловато. я надеялся будет лучше. но все равно интересный опыт. заодно баг нашли в вашей реализации.

так что насчет статистики? не сложно сделать выборку по десятку другому пользователей на предмет red счетчиков? Попутно, а в чем смысл дополнительной логики с extended сir по сравнению с классическим tbf? пользователь может понять разницу? можно просто линк на какую-нибудь публикацию, если такие есть. Спасибо.

@aabc
Copy link
Owner

aabc commented Dec 25, 2016

Мне не отчитываются по статистике. Но могу глянуть по одному юзеру позже.

extended сir это RED-like фишка циски.

Вам её не обязательно реализовывать, есть еще стандартные алгоритмы (RFC2697, RFC2698). https://www.freebsd.org/cgi/man.cgi?query=ng_car&apropos=0&sektion=4&manpath=FreeBSD+11-current&arch=default&format=html (Я смотрю там появился "Traffic shaping with RED", раньше этого не было.)

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 25, 2016 via email

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 25, 2016

Мне не отчитываются по статистике. Но могу глянуть по одному юзеру позже.

под пользователями я имел ввиду не пользователей модуля, а пользователей-абонентов сети,
предполагая, что у вас есть доступ к своей сети.

Вам её не обязательно реализовывать, есть еще стандартные алгоритмы (RFC2697, RFC2698).

они сложнее, цисковский как раз золотая середина.

ответ на свой вопрос я нашел в описании red, вроде не новый алгоритм, но только вот сейчас прочитал про реальные ситуации, где он помогает: https://en.wikipedia.org/wiki/TCP_global_synchronization

@aabc
Copy link
Owner

aabc commented Dec 25, 2016

под пользователями я имел ввиду не пользователей модуля, а пользователей-абонентов сети,
предполагая, что у вас есть доступ к своей сети.

Статистика у пользователей модуля, а не пользователей сети. Или я не понял что вам нужно.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 25, 2016 via email

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 26, 2016

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

я все время путаю, что в текущем коде обратная логика, refill корзины - это вычитание, pkt_len - это добавление. В пред. комментарии у меня наоборот.

@aabc
Copy link
Owner

aabc commented Dec 26, 2016

Второй cas нужен так как от tc надо отнимать до 0, но не больше (наполнять ведерко до максимума, но не больше). Поэтому просто atomic вычитание сделать нельзя.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 26, 2016

ну да, без кода перед глазами я забыл про это, да нужен. cas. Получается 3 cas подряд. Надо подумать, каждое ли из этих 3 состояний корректно, так лучше всего размышлять о таких алгоритмах, как o state machine с переходами.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 26, 2016

кстати, вспомнил, что я в самом начале размышлений о tbf искал lock-less реализации и нашел одну. она мне тогда показалось неправильной, плюс на go. решил забить, но теперь мне кажется по-другому. возможно, будет интересно взглянуь https://github.com/tsenart/tb

@aabc
Copy link
Owner

aabc commented Dec 26, 2016

Но могу глянуть по одному юзеру позже.

К сожалению, выяснилось, что у него больше нет нужной информации.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 26, 2016

К сожалению, выяснилось, что у него больше нет нужной информации.

ок, спасибо. не беда, может кто на наге поделится.

@aabc
Copy link
Owner

aabc commented Dec 27, 2016

		u32 tok;

		spin_lock(&ent->lock_bh);
		tok = (now - car->last) * car->cir;
		car->tc += len; /* перенести ниже этого фрагмента */
		if (tok) {
			car->tc -= min(tok, car->tc);
			car->last = now; /* апдейтить выше */
		}
		/* код выше переделать на CAS */
		/* spin_lock(&ent->lock_bh) передвинуть сюда */
		/* car->tc += len;	 передвинуть сюда */

Можно и не весь алгоритм сразу заменять на cas, а скажем начать только с апдейта last и tc. В этом случае: а) не важно какому треду досталось это подсчитать, так как и раньше было не детерминировано кто первый войдет в cs, б) какое-то время все токены у одного треда, это важно только в ситуации когда бочёнок заполнился и влияет только на какой ещё пакет дропнется, а это (почти) та же самая не детерминированость.

Во время worst case сочетаются одновременно три условия: а) пакеты нужного трафика есть на всех тредах, (скажем их 8), б) бочёнок полностью заполнен, в) подошло время добавления токенов. Следствия: 1) Никто не гарантировал, что это время настанет именно сейчас, а не на миллисекунду позже - значит все 8 пакетов могли спокойно дропнуться сейчас. 2) "Справедливо" обработается пакет в треде, который вычисляет токены, а остальные 7 могут успеть дропнутся пока идут вычисления между подсчетом времени и токенов. Контраргумент к последнему - всё равно правильно вычислится кол-во места в бочёнке, только лишь с задержкой на время обработки оного пакета (в который может попасть 8 пакетов из-за паралеллизма), а так как эта задержка случайна (см п.1), то она и не важна. 3) Чем чаще будет апдейтиться last и tc, тем (по идее) выше будет cache pollution. Возражение: всё равно та же самая cache line апдейтиться при добавлении skb->len. CMPXCHG в других 7 тредах не будет делать записи в память (и тем самым делать cache collision) так как время last и now у них, скорее всего, уже будет совпадать.

В итоге тут даже неточностей не будет, но и RED не будет. Его просто так не получить за счет ошибок.

Речь не об ошибках, а о вероятностности. RED и не будет получен, но влияние того, что пакет дропается "не в том" треде влияет не больше чем эффект RED(-like). Нет большого смысла сохранять строгую последовательность выполнения cs, так как пакеты и так распределяются между тредами вероятностно, плюс, RED-like алгоритм их всё равно дропает не упорядочено.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 27, 2016

да, я понимаю о чем вы, последовательность событий в этом алгоритме совершенно не важна.

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

@aabc
Copy link
Owner

aabc commented Dec 27, 2016

Я изменял свой пост по ходу написания много раз. Сейчас последняя версия. ;-)

@aabc
Copy link
Owner

aabc commented Dec 27, 2016

Посмотрел, пока странно почему у вас last не atomic_t.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 27, 2016

atomic_t - это синтаксический сахар, внутри там long. лень было исправлять, переделывать инициализатор

#ifdef CONFIG_64BIT
typedef struct {
long counter;
} atomic64_t;
#endif

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 27, 2016

сейчас поправлю, будет смотреться правильнее, конечно

@aabc
Copy link
Owner

aabc commented Dec 27, 2016

Я имел ввиду, конечно, не только тип, а почему со значением работа не как с atomic64_t, нет atomic64_read при чтении.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 27, 2016

да, да.. правлю.. но на x86 чтения из памяти всегда атомарные, я это имел ввиду, когда сказал, что было лень. я знал, что будет работать. но правильно конечно делать load

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 27, 2016

поправил все атомики и небольшой баг. Собираюсь perfomance тесты сегодня сделать.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 27, 2016

Почти ровно в раза быстрее lock-less версия получилась. Теперь можно в страивать в the_router :)

@aabc
Copy link
Owner

aabc commented Dec 27, 2016

Поздравляю! А (многие) люди ещё хотят полисинг не per IP, а per subnet. Я начинал делать, но забросил из-за недостатка времени. Очередное поле для замедления вычислений.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 27, 2016

Я сейчас думаю о минимально базовых вещах, необходимых для ipoe bras. Поэтому пока всякие редкие фичи вне поля интересов. Да, со временем у всех беда. Без особой надежды, конечно, спрашиваю, но может вам интересно будет поучаствовать в разработке софтового браса на dpdk? Я хочу сделать коммечерский продукт. Сейчас половину своего времени трачу на него. И хочется двигаться быстрее. Временное окно небольшое, год, другой и кто-нибудь займет место на рынке.

@aabc
Copy link
Owner

aabc commented Dec 27, 2016

Заняться full-time или в свободное время?

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 27, 2016

Бюджета освоить у меня, к сожалению, нет. Поэтому фулл тайм, только если сами захотите, если вдруг как у меня есть источники дохода, не требующие постоянного внимания.

@alexk99
Copy link
Contributor Author

alexk99 commented Dec 27, 2016

Т.е. другими словами я партнеров ищу. Вложить время и умения сейчас с надеждой, что взлетит )

@aabc
Copy link
Owner

aabc commented Dec 27, 2016

Понятно, но нет пока свободного времени с моей нынешней работой (последний год), в этом и проблема пока не решенная.

@aabc aabc force-pushed the master branch 3 times, most recently from a8d91df to 640851a Compare April 24, 2020 06:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants