On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> The added test items cover both cases where bitmap buf of the printed
> result is greater than and less than 4KB.
> And it also covers the case where offset for bitmap_print_to_buf is
> non-zero which will happen when printed buf is larger than one page
> in sysfs bin_attribute.
More test cases is always a good thing, thanks!
Reviewed-by: Andy Shevchenko <[email protected]>
> Signed-off-by: Barry Song <[email protected]>
> ---
> -v7:
> minor cleanup according to Andy's comments
>
> lib/test_bitmap.c | 150 ++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 150 insertions(+)
>
> diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
> index 4ea73f5aed41..eb8ebaf12865 100644
> --- a/lib/test_bitmap.c
> +++ b/lib/test_bitmap.c
> @@ -19,6 +19,7 @@
> KSTM_MODULE_GLOBALS();
>
> static char pbl_buffer[PAGE_SIZE] __initdata;
> +static char print_buf[PAGE_SIZE * 2] __initdata;
>
> static const unsigned long exp1[] __initconst = {
> BITMAP_FROM_U64(1),
> @@ -156,6 +157,20 @@ static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,
> return true;
> }
>
> +static bool __init
> +__check_eq_str(const char *srcfile, unsigned int line,
> + const char *exp_str, const char *str,
> + unsigned int len)
> +{
> + bool eq;
> +
> + eq = strncmp(exp_str, str, len) == 0;
> + if (!eq)
> + pr_err("[%s:%u] expected %s, got %s\n", srcfile, line, exp_str, str);
> +
> + return eq;
> +}
> +
> #define __expect_eq(suffix, ...) \
> ({ \
> int result = 0; \
> @@ -173,6 +188,7 @@ static bool __init __check_eq_clump8(const char *srcfile, unsigned int line,
> #define expect_eq_pbl(...) __expect_eq(pbl, ##__VA_ARGS__)
> #define expect_eq_u32_array(...) __expect_eq(u32_array, ##__VA_ARGS__)
> #define expect_eq_clump8(...) __expect_eq(clump8, ##__VA_ARGS__)
> +#define expect_eq_str(...) __expect_eq(str, ##__VA_ARGS__)
>
> static void __init test_zero_clear(void)
> {
> @@ -660,6 +676,139 @@ static void __init test_bitmap_cut(void)
> }
> }
>
> +struct test_bitmap_print {
> + const unsigned long *bitmap;
> + unsigned long nbits;
> + const char *mask;
> + const char *list;
> +};
> +
> +static const unsigned long small_bitmap[] __initconst = {
> + BITMAP_FROM_U64(0x3333333311111111ULL),
> +};
> +
> +static const char small_mask[] __initconst = "33333333,11111111\n";
> +static const char small_list[] __initconst = "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61\n";
> +
> +static const unsigned long large_bitmap[] __initconst = {
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> + BITMAP_FROM_U64(0x3333333311111111ULL), BITMAP_FROM_U64(0x3333333311111111ULL),
> +};
> +
> +static const char large_mask[] __initconst = "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111,"
> + "33333333,11111111,33333333,11111111\n";
> +
> +static const char large_list[] __initconst = /* more than 4KB */
> + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
> + "49,252-253,256,260,264,268,272,276,280,284,288-289,292-293,296-297,300-301,304-305,308-309,312-313,316-317,320,3"
> + "24,328,332,336,340,344,348,352-353,356-357,360-361,364-365,368-369,372-373,376-377,380-381,384,388,392,396,400,4"
> + "04,408,412,416-417,420-421,424-425,428-429,432-433,436-437,440-441,444-445,448,452,456,460,464,468,472,476,480-4"
> + "81,484-485,488-489,492-493,496-497,500-501,504-505,508-509,512,516,520,524,528,532,536,540,544-545,548-549,552-5"
> + "53,556-557,560-561,564-565,568-569,572-573,576,580,584,588,592,596,600,604,608-609,612-613,616-617,620-621,624-6"
> + "25,628-629,632-633,636-637,640,644,648,652,656,660,664,668,672-673,676-677,680-681,684-685,688-689,692-693,696-6"
> + "97,700-701,704,708,712,716,720,724,728,732,736-737,740-741,744-745,748-749,752-753,756-757,760-761,764-765,768,7"
> + "72,776,780,784,788,792,796,800-801,804-805,808-809,812-813,816-817,820-821,824-825,828-829,832,836,840,844,848,8"
> + "52,856,860,864-865,868-869,872-873,876-877,880-881,884-885,888-889,892-893,896,900,904,908,912,916,920,924,928-9"
> + "29,932-933,936-937,940-941,944-945,948-949,952-953,956-957,960,964,968,972,976,980,984,988,992-993,996-997,1000-"
> + "1001,1004-1005,1008-1009,1012-1013,1016-1017,1020-1021,1024,1028,1032,1036,1040,1044,1048,1052,1056-1057,1060-10"
> + "61,1064-1065,1068-1069,1072-1073,1076-1077,1080-1081,1084-1085,1088,1092,1096,1100,1104,1108,1112,1116,1120-1121"
> + ",1124-1125,1128-1129,1132-1133,1136-1137,1140-1141,1144-1145,1148-1149,1152,1156,1160,1164,1168,1172,1176,1180,1"
> + "184-1185,1188-1189,1192-1193,1196-1197,1200-1201,1204-1205,1208-1209,1212-1213,1216,1220,1224,1228,1232,1236,124"
> + "0,1244,1248-1249,1252-1253,1256-1257,1260-1261,1264-1265,1268-1269,1272-1273,1276-1277,1280,1284,1288,1292,1296,"
> + "1300,1304,1308,1312-1313,1316-1317,1320-1321,1324-1325,1328-1329,1332-1333,1336-1337,1340-1341,1344,1348,1352,13"
> + "56,1360,1364,1368,1372,1376-1377,1380-1381,1384-1385,1388-1389,1392-1393,1396-1397,1400-1401,1404-1405,1408,1412"
> + ",1416,1420,1424,1428,1432,1436,1440-1441,1444-1445,1448-1449,1452-1453,1456-1457,1460-1461,1464-1465,1468-1469,1"
> + "472,1476,1480,1484,1488,1492,1496,1500,1504-1505,1508-1509,1512-1513,1516-1517,1520-1521,1524-1525,1528-1529,153"
> + "2-1533,1536,1540,1544,1548,1552,1556,1560,1564,1568-1569,1572-1573,1576-1577,1580-1581,1584-1585,1588-1589,1592-"
> + "1593,1596-1597,1600,1604,1608,1612,1616,1620,1624,1628,1632-1633,1636-1637,1640-1641,1644-1645,1648-1649,1652-16"
> + "53,1656-1657,1660-1661,1664,1668,1672,1676,1680,1684,1688,1692,1696-1697,1700-1701,1704-1705,1708-1709,1712-1713"
> + ",1716-1717,1720-1721,1724-1725,1728,1732,1736,1740,1744,1748,1752,1756,1760-1761,1764-1765,1768-1769,1772-1773,1"
> + "776-1777,1780-1781,1784-1785,1788-1789,1792,1796,1800,1804,1808,1812,1816,1820,1824-1825,1828-1829,1832-1833,183"
> + "6-1837,1840-1841,1844-1845,1848-1849,1852-1853,1856,1860,1864,1868,1872,1876,1880,1884,1888-1889,1892-1893,1896-"
> + "1897,1900-1901,1904-1905,1908-1909,1912-1913,1916-1917,1920,1924,1928,1932,1936,1940,1944,1948,1952-1953,1956-19"
> + "57,1960-1961,1964-1965,1968-1969,1972-1973,1976-1977,1980-1981,1984,1988,1992,1996,2000,2004,2008,2012,2016-2017"
> + ",2020-2021,2024-2025,2028-2029,2032-2033,2036-2037,2040-2041,2044-2045,2048,2052,2056,2060,2064,2068,2072,2076,2"
> + "080-2081,2084-2085,2088-2089,2092-2093,2096-2097,2100-2101,2104-2105,2108-2109,2112,2116,2120,2124,2128,2132,213"
> + "6,2140,2144-2145,2148-2149,2152-2153,2156-2157,2160-2161,2164-2165,2168-2169,2172-2173,2176,2180,2184,2188,2192,"
> + "2196,2200,2204,2208-2209,2212-2213,2216-2217,2220-2221,2224-2225,2228-2229,2232-2233,2236-2237,2240,2244,2248,22"
> + "52,2256,2260,2264,2268,2272-2273,2276-2277,2280-2281,2284-2285,2288-2289,2292-2293,2296-2297,2300-2301,2304,2308"
> + ",2312,2316,2320,2324,2328,2332,2336-2337,2340-2341,2344-2345,2348-2349,2352-2353,2356-2357,2360-2361,2364-2365,2"
> + "368,2372,2376,2380,2384,2388,2392,2396,2400-2401,2404-2405,2408-2409,2412-2413,2416-2417,2420-2421,2424-2425,242"
> + "8-2429,2432,2436,2440,2444,2448,2452,2456,2460,2464-2465,2468-2469,2472-2473,2476-2477,2480-2481,2484-2485,2488-"
> + "2489,2492-2493,2496,2500,2504,2508,2512,2516,2520,2524,2528-2529,2532-2533,2536-2537,2540-2541,2544-2545,2548-25"
> + "49,2552-2553,2556-2557\n";
> +
> +static const struct test_bitmap_print test_print[] __initconst = {
> + { small_bitmap, sizeof(small_bitmap) * BITS_PER_BYTE, small_mask, small_list },
> + { large_bitmap, sizeof(large_bitmap) * BITS_PER_BYTE, large_mask, large_list },
> +};
> +
> +static void __init test_bitmap_print_buf(void)
> +{
> + int i;
> +
> + for (i = 0; i < ARRAY_SIZE(test_print); i++) {
> + const struct test_bitmap_print *t = &test_print[i];
> + int n;
> +
> + n = bitmap_print_to_buf(false, print_buf, t->bitmap, t->nbits,
> + 0, 2 * PAGE_SIZE);
> + expect_eq_uint(strlen(t->mask) + 1, n);
> + expect_eq_str(t->mask, print_buf, n);
> +
> + n = bitmap_print_to_buf(true, print_buf, t->bitmap, t->nbits,
> + 0, 2 * PAGE_SIZE);
> + expect_eq_uint(strlen(t->list) + 1, n);
> + expect_eq_str(t->list, print_buf, n);
> +
> + /* test bitmap_print_to_buf by non-zero offset */
> + if (strlen(t->list) > PAGE_SIZE) {
> + n = bitmap_print_to_buf(true, print_buf, t->bitmap, t->nbits,
> + PAGE_SIZE, PAGE_SIZE);
> + expect_eq_uint(strlen(t->list) + 1 - PAGE_SIZE, n);
> + expect_eq_str(t->list + PAGE_SIZE, print_buf, n);
> + }
> + }
> +}
> +
> static void __init selftest(void)
> {
> test_zero_clear();
> @@ -672,6 +821,7 @@ static void __init selftest(void)
> test_mem_optimisations();
> test_for_each_set_clump8();
> test_bitmap_cut();
> + test_bitmap_print_buf();
> }
>
> KSTM_MODULE_LOADERS(test_bitmap);
> --
> 2.25.1
>
--
With Best Regards,
Andy Shevchenko
On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > The added test items cover both cases where bitmap buf of the printed
> > result is greater than and less than 4KB.
> > And it also covers the case where offset for bitmap_print_to_buf is
> > non-zero which will happen when printed buf is larger than one page
> > in sysfs bin_attribute.
>
> More test cases is always a good thing, thanks!
Generally yes. But in this case... I believe, Barry didn't write that
huge line below by himself. Most probably he copy-pasted the output of
his bitmap_print_buf() into the test. If so, this code tests nothing,
and just enforces current behavior of snprintf.
> Reviewed-by: Andy Shevchenko <[email protected]>
>
> > Signed-off-by: Barry Song <[email protected]>
> > ---
> > -v7:
> > minor cleanup according to Andy's comments
[...]
> > +static const char large_list[] __initconst = /* more than 4KB */
> > + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
I don't like this behavior of the code: each individual line is not a
valid bitmap_list. I would prefer to split original bitmap and print
list representation of parts in a compatible format; considering a
receiving part of this splitting machinery.
Thanks,
Yury
> > + "49,252-253,256,260,264,268,272,276,280,284,288-289,292-293,296-297,300-301,304-305,308-309,312-313,316-317,320,3"
> > + "24,328,332,336,340,344,348,352-353,356-357,360-361,364-365,368-369,372-373,376-377,380-381,384,388,392,396,400,4"
> > + "04,408,412,416-417,420-421,424-425,428-429,432-433,436-437,440-441,444-445,448,452,456,460,464,468,472,476,480-4"
> > + "81,484-485,488-489,492-493,496-497,500-501,504-505,508-509,512,516,520,524,528,532,536,540,544-545,548-549,552-5"
> > + "53,556-557,560-561,564-565,568-569,572-573,576,580,584,588,592,596,600,604,608-609,612-613,616-617,620-621,624-6"
> > + "25,628-629,632-633,636-637,640,644,648,652,656,660,664,668,672-673,676-677,680-681,684-685,688-689,692-693,696-6"
> > + "97,700-701,704,708,712,716,720,724,728,732,736-737,740-741,744-745,748-749,752-753,756-757,760-761,764-765,768,7"
> > + "72,776,780,784,788,792,796,800-801,804-805,808-809,812-813,816-817,820-821,824-825,828-829,832,836,840,844,848,8"
> > + "52,856,860,864-865,868-869,872-873,876-877,880-881,884-885,888-889,892-893,896,900,904,908,912,916,920,924,928-9"
> > + "29,932-933,936-937,940-941,944-945,948-949,952-953,956-957,960,964,968,972,976,980,984,988,992-993,996-997,1000-"
> > + "1001,1004-1005,1008-1009,1012-1013,1016-1017,1020-1021,1024,1028,1032,1036,1040,1044,1048,1052,1056-1057,1060-10"
> > + "61,1064-1065,1068-1069,1072-1073,1076-1077,1080-1081,1084-1085,1088,1092,1096,1100,1104,1108,1112,1116,1120-1121"
> > + ",1124-1125,1128-1129,1132-1133,1136-1137,1140-1141,1144-1145,1148-1149,1152,1156,1160,1164,1168,1172,1176,1180,1"
> > + "184-1185,1188-1189,1192-1193,1196-1197,1200-1201,1204-1205,1208-1209,1212-1213,1216,1220,1224,1228,1232,1236,124"
> > + "0,1244,1248-1249,1252-1253,1256-1257,1260-1261,1264-1265,1268-1269,1272-1273,1276-1277,1280,1284,1288,1292,1296,"
> > + "1300,1304,1308,1312-1313,1316-1317,1320-1321,1324-1325,1328-1329,1332-1333,1336-1337,1340-1341,1344,1348,1352,13"
> > + "56,1360,1364,1368,1372,1376-1377,1380-1381,1384-1385,1388-1389,1392-1393,1396-1397,1400-1401,1404-1405,1408,1412"
> > + ",1416,1420,1424,1428,1432,1436,1440-1441,1444-1445,1448-1449,1452-1453,1456-1457,1460-1461,1464-1465,1468-1469,1"
> > + "472,1476,1480,1484,1488,1492,1496,1500,1504-1505,1508-1509,1512-1513,1516-1517,1520-1521,1524-1525,1528-1529,153"
> > + "2-1533,1536,1540,1544,1548,1552,1556,1560,1564,1568-1569,1572-1573,1576-1577,1580-1581,1584-1585,1588-1589,1592-"
> > + "1593,1596-1597,1600,1604,1608,1612,1616,1620,1624,1628,1632-1633,1636-1637,1640-1641,1644-1645,1648-1649,1652-16"
> > + "53,1656-1657,1660-1661,1664,1668,1672,1676,1680,1684,1688,1692,1696-1697,1700-1701,1704-1705,1708-1709,1712-1713"
> > + ",1716-1717,1720-1721,1724-1725,1728,1732,1736,1740,1744,1748,1752,1756,1760-1761,1764-1765,1768-1769,1772-1773,1"
> > + "776-1777,1780-1781,1784-1785,1788-1789,1792,1796,1800,1804,1808,1812,1816,1820,1824-1825,1828-1829,1832-1833,183"
> > + "6-1837,1840-1841,1844-1845,1848-1849,1852-1853,1856,1860,1864,1868,1872,1876,1880,1884,1888-1889,1892-1893,1896-"
> > + "1897,1900-1901,1904-1905,1908-1909,1912-1913,1916-1917,1920,1924,1928,1932,1936,1940,1944,1948,1952-1953,1956-19"
> > + "57,1960-1961,1964-1965,1968-1969,1972-1973,1976-1977,1980-1981,1984,1988,1992,1996,2000,2004,2008,2012,2016-2017"
> > + ",2020-2021,2024-2025,2028-2029,2032-2033,2036-2037,2040-2041,2044-2045,2048,2052,2056,2060,2064,2068,2072,2076,2"
> > + "080-2081,2084-2085,2088-2089,2092-2093,2096-2097,2100-2101,2104-2105,2108-2109,2112,2116,2120,2124,2128,2132,213"
> > + "6,2140,2144-2145,2148-2149,2152-2153,2156-2157,2160-2161,2164-2165,2168-2169,2172-2173,2176,2180,2184,2188,2192,"
> > + "2196,2200,2204,2208-2209,2212-2213,2216-2217,2220-2221,2224-2225,2228-2229,2232-2233,2236-2237,2240,2244,2248,22"
> > + "52,2256,2260,2264,2268,2272-2273,2276-2277,2280-2281,2284-2285,2288-2289,2292-2293,2296-2297,2300-2301,2304,2308"
> > + ",2312,2316,2320,2324,2328,2332,2336-2337,2340-2341,2344-2345,2348-2349,2352-2353,2356-2357,2360-2361,2364-2365,2"
> > + "368,2372,2376,2380,2384,2388,2392,2396,2400-2401,2404-2405,2408-2409,2412-2413,2416-2417,2420-2421,2424-2425,242"
> > + "8-2429,2432,2436,2440,2444,2448,2452,2456,2460,2464-2465,2468-2469,2472-2473,2476-2477,2480-2481,2484-2485,2488-"
> > + "2489,2492-2493,2496,2500,2504,2508,2512,2516,2520,2524,2528-2529,2532-2533,2536-2537,2540-2541,2544-2545,2548-25"
> > + "49,2552-2553,2556-2557\n";
> > +
> > +static const struct test_bitmap_print test_print[] __initconst = {
> > + { small_bitmap, sizeof(small_bitmap) * BITS_PER_BYTE, small_mask, small_list },
> > + { large_bitmap, sizeof(large_bitmap) * BITS_PER_BYTE, large_mask, large_list },
> > +};
> > +
> > +static void __init test_bitmap_print_buf(void)
> > +{
> > + int i;
> > +
> > + for (i = 0; i < ARRAY_SIZE(test_print); i++) {
> > + const struct test_bitmap_print *t = &test_print[i];
> > + int n;
> > +
> > + n = bitmap_print_to_buf(false, print_buf, t->bitmap, t->nbits,
> > + 0, 2 * PAGE_SIZE);
> > + expect_eq_uint(strlen(t->mask) + 1, n);
> > + expect_eq_str(t->mask, print_buf, n);
> > +
> > + n = bitmap_print_to_buf(true, print_buf, t->bitmap, t->nbits,
> > + 0, 2 * PAGE_SIZE);
> > + expect_eq_uint(strlen(t->list) + 1, n);
> > + expect_eq_str(t->list, print_buf, n);
> > +
> > + /* test bitmap_print_to_buf by non-zero offset */
> > + if (strlen(t->list) > PAGE_SIZE) {
> > + n = bitmap_print_to_buf(true, print_buf, t->bitmap, t->nbits,
> > + PAGE_SIZE, PAGE_SIZE);
> > + expect_eq_uint(strlen(t->list) + 1 - PAGE_SIZE, n);
> > + expect_eq_str(t->list + PAGE_SIZE, print_buf, n);
> > + }
> > + }
> > +}
> > +
> > static void __init selftest(void)
> > {
> > test_zero_clear();
> > @@ -672,6 +821,7 @@ static void __init selftest(void)
> > test_mem_optimisations();
> > test_for_each_set_clump8();
> > test_bitmap_cut();
> > + test_bitmap_print_buf();
> > }
> >
> > KSTM_MODULE_LOADERS(test_bitmap);
> > --
> > 2.25.1
> >
>
> --
> With Best Regards,
> Andy Shevchenko
>
On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <[email protected]> wrote:
> On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > The added test items cover both cases where bitmap buf of the printed
> > > result is greater than and less than 4KB.
> > > And it also covers the case where offset for bitmap_print_to_buf is
> > > non-zero which will happen when printed buf is larger than one page
> > > in sysfs bin_attribute.
> >
> > More test cases is always a good thing, thanks!
>
> Generally yes. But in this case... I believe, Barry didn't write that
> huge line below by himself. Most probably he copy-pasted the output of
> his bitmap_print_buf() into the test. If so, this code tests nothing,
> and just enforces current behavior of snprintf.
I'm not sure I got what you are telling me. The big line is to test
strings that are bigger than 4k.
...
> > > +static const char large_list[] __initconst = /* more than 4KB */
> > > + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > > + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > > + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
>
> I don't like this behavior of the code: each individual line is not a
> valid bitmap_list. I would prefer to split original bitmap and print
> list representation of parts in a compatible format; considering a
> receiving part of this splitting machinery.
I agree that split is not the best here, but after all it's only 1
line and this is on purpose.
--
With Best Regards,
Andy Shevchenko
On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <[email protected]> wrote:
> > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > The added test items cover both cases where bitmap buf of the printed
> > > > result is greater than and less than 4KB.
> > > > And it also covers the case where offset for bitmap_print_to_buf is
> > > > non-zero which will happen when printed buf is larger than one page
> > > > in sysfs bin_attribute.
> > >
> > > More test cases is always a good thing, thanks!
> >
> > Generally yes. But in this case... I believe, Barry didn't write that
> > huge line below by himself. Most probably he copy-pasted the output of
> > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > and just enforces current behavior of snprintf.
>
> I'm not sure I got what you are telling me. The big line is to test
> strings that are bigger than 4k.
I'm trying to say that human are not able to verify correctness of
this line. The test is supposed to check bitmap_print_to_buf(), but
reference output itself is generated by bitmap_print_to_buf(). This
test will always pass by design, even if there's an error somewhere
in the middle, isn't it?
>
> ...
>
> > > > +static const char large_list[] __initconst = /* more than 4KB */
> > > > + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > > > + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
> >
> > I don't like this behavior of the code: each individual line is not a
> > valid bitmap_list. I would prefer to split original bitmap and print
> > list representation of parts in a compatible format; considering a
> > receiving part of this splitting machinery.
>
> I agree that split is not the best here, but after all it's only 1
> line and this is on purpose.
What I see is that bitmap_print_to_buf() is called many times, and
each time it returns something that is not a valid bitmap list string.
If the caller was be able to concatenate all the lines returned by
bitmap_print_to_buf(), he'd probably get correct result. But in such
case, why don't he use scnprintf("pbl") directly?
If there exists a real case where scnprintf("pbl") output is too long
(or execution time is too slow) that we need to split, the right
approach is to split the original bitmap, not an output of
scnprintf("pbl").
And I still don't understand, what prevents the higher levels from
calling the scnprintf() directly in this specific case?
> -----Original Message-----
> From: Yury Norov [mailto:[email protected]]
> Sent: Friday, July 16, 2021 11:24 AM
> To: Andy Shevchenko <[email protected]>
> Cc: Andy Shevchenko <[email protected]>; Song Bao Hua (Barry
> Song) <[email protected]>; Greg Kroah-Hartman
> <[email protected]>; Andrew Morton <[email protected]>;
> Linux Kernel Mailing List <[email protected]>; Dave Hansen
> <[email protected]>; Rasmus Villemoes <[email protected]>; Rafael
> J. Wysocki <[email protected]>; Randy Dunlap <[email protected]>;
> Alexander Gordeev <[email protected]>; Stefano Brivio
> <[email protected]>; Ma, Jianpeng <[email protected]>; Valentin
> Schneider <[email protected]>; Peter Zijlstra (Intel)
> <[email protected]>; Daniel Bristot de Oliveira <[email protected]>;
> Guodong Xu <[email protected]>; tangchengchang
> <[email protected]>; Zengtao (B) <[email protected]>;
> yangyicong <[email protected]>; [email protected]; Linuxarm
> <[email protected]>
> Subject: Re: [PATCH v7 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases
>
> On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> > On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <[email protected]> wrote:
> > > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > > The added test items cover both cases where bitmap buf of the printed
> > > > > result is greater than and less than 4KB.
> > > > > And it also covers the case where offset for bitmap_print_to_buf is
> > > > > non-zero which will happen when printed buf is larger than one page
> > > > > in sysfs bin_attribute.
> > > >
> > > > More test cases is always a good thing, thanks!
> > >
> > > Generally yes. But in this case... I believe, Barry didn't write that
> > > huge line below by himself. Most probably he copy-pasted the output of
> > > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > > and just enforces current behavior of snprintf.
> >
> > I'm not sure I got what you are telling me. The big line is to test
> > strings that are bigger than 4k.
>
> I'm trying to say that human are not able to verify correctness of
> this line. The test is supposed to check bitmap_print_to_buf(), but
> reference output itself is generated by bitmap_print_to_buf(). This
> test will always pass by design, even if there's an error somewhere
> in the middle, isn't it?
So would it be better to compare the output of bitmap_print_to_buf()
with the output of the direct scnprintf("pbl")? In this case, we have
to assume scnprintf("pbl") is always right.
>
> >
> > ...
> >
> > > > > +static const char large_list[] __initconst = /* more than 4KB */
> > > > > +
> "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,6
> 8,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > > +
> "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,15
> 6,160-161,164-165,168-169,172-173,176-1"
> > > > > +
> "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-22
> 9,232-233,236-237,240-241,244-245,248-2"
> > >
> > > I don't like this behavior of the code: each individual line is not a
> > > valid bitmap_list. I would prefer to split original bitmap and print
> > > list representation of parts in a compatible format; considering a
> > > receiving part of this splitting machinery.
> >
> > I agree that split is not the best here, but after all it's only 1
> > line and this is on purpose.
>
> What I see is that bitmap_print_to_buf() is called many times, and
> each time it returns something that is not a valid bitmap list string.
> If the caller was be able to concatenate all the lines returned by
> bitmap_print_to_buf(), he'd probably get correct result. But in such
> case, why don't he use scnprintf("pbl") directly?
I still don't understand what you mean, for a sys ABI like list, its
main users are random "cat" commands:
$ cat /sys/devices/system/cpu/cpu0/topology/thread_siblings_list
0-1
Users "cat" the file to get a human-readable list. Users who are
"catting" sysfs have no bitmap to run scnprintf("pbl").
>
> If there exists a real case where scnprintf("pbl") output is too long
> (or execution time is too slow) that we need to split, the right
> approach is to split the original bitmap, not an output of
> scnprintf("pbl").
The whole patchset is printing bitmap to a list or bitmask
so that users can "cat" the files to get a human-readable list.
But due to the limit of sysfs attribute, the list will be
trimmed if the printed result is more than 4KB.
>
> And I still don't understand, what prevents the higher levels from
> calling the scnprintf() directly in this specific case?
Again, I don't understand what you mean. Higher levels like those drivers
can, for sure, call scnprintf() directly, then we have to copy this kind
of code here and there. With a cpumap API, those drivers can simply call
an API.
Anyway, it is really difficult for me to understand the approach
you are expecting. I'd really appreciate you can post some pseudo
code for the modification of drivers to make it clear. In my personal
opinion, the current approach from tian tao for patch1-3 is the best
choice from the perspective of the API's users - sysfs bin_attribute.
Thanks
Barry
On Thu, Jul 15, 2021 at 04:23:32PM -0700, Yury Norov wrote:
> On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> > On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <[email protected]> wrote:
> > > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > > The added test items cover both cases where bitmap buf of the printed
> > > > > result is greater than and less than 4KB.
> > > > > And it also covers the case where offset for bitmap_print_to_buf is
> > > > > non-zero which will happen when printed buf is larger than one page
> > > > > in sysfs bin_attribute.
> > > >
> > > > More test cases is always a good thing, thanks!
> > >
> > > Generally yes. But in this case... I believe, Barry didn't write that
> > > huge line below by himself. Most probably he copy-pasted the output of
> > > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > > and just enforces current behavior of snprintf.
> >
> > I'm not sure I got what you are telling me. The big line is to test
> > strings that are bigger than 4k.
>
> I'm trying to say that human are not able to verify correctness of
> this line. The test is supposed to check bitmap_print_to_buf(), but
> reference output itself is generated by bitmap_print_to_buf(). This
> test will always pass by design, even if there's an error somewhere
> in the middle, isn't it?
Then please manually check it to verify it is correct or not. Once we
have it verified, that's fine, it will remain static in this test for
always going forward.
That's what "oracles" are for, there is nothing wrong with this test
case or "proof" that I can see.
> >
> > ...
> >
> > > > > +static const char large_list[] __initconst = /* more than 4KB */
> > > > > + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > > + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > > > > + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
> > >
> > > I don't like this behavior of the code: each individual line is not a
> > > valid bitmap_list. I would prefer to split original bitmap and print
> > > list representation of parts in a compatible format; considering a
> > > receiving part of this splitting machinery.
> >
> > I agree that split is not the best here, but after all it's only 1
> > line and this is on purpose.
>
> What I see is that bitmap_print_to_buf() is called many times,
That is not what the above list shows at all, it's one long string all
together, only split up to make it easier for us to work with.
> and
> each time it returns something that is not a valid bitmap list string.
> If the caller was be able to concatenate all the lines returned by
> bitmap_print_to_buf(), he'd probably get correct result. But in such
> case, why don't he use scnprintf("pbl") directly?
I do not understand the objection here at all. This series is fixing a
real problem that people are having and your complaining about test
strings is _VERY_ odd.
If you have an alternate solution, please propose it, otherwise I will
be taking this series in the next few days.
thanks,
greg k-h
On Wed, Jul 21, 2021 at 01:40:36PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 15, 2021 at 04:23:32PM -0700, Yury Norov wrote:
> > On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> > > On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <[email protected]> wrote:
> > > > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > > > The added test items cover both cases where bitmap buf of the printed
> > > > > > result is greater than and less than 4KB.
> > > > > > And it also covers the case where offset for bitmap_print_to_buf is
> > > > > > non-zero which will happen when printed buf is larger than one page
> > > > > > in sysfs bin_attribute.
> > > > >
> > > > > More test cases is always a good thing, thanks!
> > > >
> > > > Generally yes. But in this case... I believe, Barry didn't write that
> > > > huge line below by himself. Most probably he copy-pasted the output of
> > > > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > > > and just enforces current behavior of snprintf.
> > >
> > > I'm not sure I got what you are telling me. The big line is to test
> > > strings that are bigger than 4k.
> >
> > I'm trying to say that human are not able to verify correctness of
> > this line. The test is supposed to check bitmap_print_to_buf(), but
> > reference output itself is generated by bitmap_print_to_buf(). This
> > test will always pass by design, even if there's an error somewhere
> > in the middle, isn't it?
>
> Then please manually check it to verify it is correct or not. Once we
> have it verified, that's fine, it will remain static in this test for
> always going forward.
>
> That's what "oracles" are for, there is nothing wrong with this test
> case or "proof" that I can see.
>
> > >
> > > ...
> > >
> > > > > > +static const char large_list[] __initconst = /* more than 4KB */
> > > > > > + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > > > + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > > > > > + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
> > > >
> > > > I don't like this behavior of the code: each individual line is not a
> > > > valid bitmap_list. I would prefer to split original bitmap and print
> > > > list representation of parts in a compatible format; considering a
> > > > receiving part of this splitting machinery.
> > >
> > > I agree that split is not the best here, but after all it's only 1
> > > line and this is on purpose.
> >
> > What I see is that bitmap_print_to_buf() is called many times,
>
> That is not what the above list shows at all, it's one long string all
> together, only split up to make it easier for us to work with.
>
> > and
> > each time it returns something that is not a valid bitmap list string.
> > If the caller was be able to concatenate all the lines returned by
> > bitmap_print_to_buf(), he'd probably get correct result. But in such
> > case, why don't he use scnprintf("pbl") directly?
>
> I do not understand the objection here at all. This series is fixing a
> real problem that eeople are having
I explicitly asked about an example of this problem. Barry answered in
a great length, but the key points are:
https://lore.kernel.org/lkml/[email protected]/
> > So, the root problem here is that some machines have so many CPUs that
> > their cpumask text representation may not fit into the full page in the
> > worst case. Is my understanding correct? Can you share an example of
> > such configuration?
>
> in my understanding, I have not found any machine which has really
> caused the problem till now.
> [...]
>
> This doesn't really happen nowadays as the maximum
> NR_CPUS is 8196 for X86_64 and 4096 for ARM64 since 8196 * 9 / 32 = 2305
> is still smaller than 4KB page size.
If it's not true, can you or Barry please share such an example?
> and your complaining about test
> strings is _VERY_ odd.
The test itself is bad, but it's a minor issue.
My main complain is that the bitmap part of this series introduces a
function that requires O(N^2) of CPU time and O(N) of memory to just
print a string. The existing snprintf does this in O(N) and O(1)
respectively. Additionally to that, the proposed function has some
flaws in design.
> If you have an alternate solution, please propose it, otherwise I will
> be taking this series in the next few days.
I think I suggested a better solution in the thread for v4:
https://lore.kernel.org/lkml/YMu2amhrdGZHJ5mY@yury-ThinkPad/
> kasprintf() does everything you need. Why don't you use it directly in
> your driver?
I'm not that familiar to sysfs internals to submit a patch, but the
idea in more details is like this:
cpulist_read(...)
{
if (bitmap_string == NULL)
bitmap_string = kasprintf(bitmap, ...);
}
Where bitmap_string pointer may be stored in struct file, struct kobject,
struct bit_attribute or where it's convenient.
If it's completely impossible to store a pointer, and the bug is real
and urgent, then the best solution I see is to move bitmap_get_print_buf()
to the driver/base/node.c code, because it's not optimal for general use.
Which I already suggested here:
https://lkml.org/lkml/2021/7/16/763
Can you or Barry tell, would one of those alternative solutions work for you?
Thanks,
Yury
On Thu, Jul 22, 2021 at 10:09:27AM -0700, Yury Norov wrote:
> On Wed, Jul 21, 2021 at 01:40:36PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 15, 2021 at 04:23:32PM -0700, Yury Norov wrote:
> > > On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> > > > On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <[email protected]> wrote:
> > > > > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > > > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > > > > The added test items cover both cases where bitmap buf of the printed
> > > > > > > result is greater than and less than 4KB.
> > > > > > > And it also covers the case where offset for bitmap_print_to_buf is
> > > > > > > non-zero which will happen when printed buf is larger than one page
> > > > > > > in sysfs bin_attribute.
> > > > > >
> > > > > > More test cases is always a good thing, thanks!
> > > > >
> > > > > Generally yes. But in this case... I believe, Barry didn't write that
> > > > > huge line below by himself. Most probably he copy-pasted the output of
> > > > > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > > > > and just enforces current behavior of snprintf.
> > > >
> > > > I'm not sure I got what you are telling me. The big line is to test
> > > > strings that are bigger than 4k.
> > >
> > > I'm trying to say that human are not able to verify correctness of
> > > this line. The test is supposed to check bitmap_print_to_buf(), but
> > > reference output itself is generated by bitmap_print_to_buf(). This
> > > test will always pass by design, even if there's an error somewhere
> > > in the middle, isn't it?
> >
> > Then please manually check it to verify it is correct or not. Once we
> > have it verified, that's fine, it will remain static in this test for
> > always going forward.
> >
> > That's what "oracles" are for, there is nothing wrong with this test
> > case or "proof" that I can see.
> >
> > > >
> > > > ...
> > > >
> > > > > > > +static const char large_list[] __initconst = /* more than 4KB */
> > > > > > > + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > > > > + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > > > > > > + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
> > > > >
> > > > > I don't like this behavior of the code: each individual line is not a
> > > > > valid bitmap_list. I would prefer to split original bitmap and print
> > > > > list representation of parts in a compatible format; considering a
> > > > > receiving part of this splitting machinery.
> > > >
> > > > I agree that split is not the best here, but after all it's only 1
> > > > line and this is on purpose.
> > >
> > > What I see is that bitmap_print_to_buf() is called many times,
> >
> > That is not what the above list shows at all, it's one long string all
> > together, only split up to make it easier for us to work with.
> >
> > > and
> > > each time it returns something that is not a valid bitmap list string.
> > > If the caller was be able to concatenate all the lines returned by
> > > bitmap_print_to_buf(), he'd probably get correct result. But in such
> > > case, why don't he use scnprintf("pbl") directly?
> >
> > I do not understand the objection here at all. This series is fixing a
> > real problem that eeople are having
>
> I explicitly asked about an example of this problem. Barry answered in
> a great length, but the key points are:
>
> https://lore.kernel.org/lkml/[email protected]/
>
> > > So, the root problem here is that some machines have so many CPUs that
> > > their cpumask text representation may not fit into the full page in the
> > > worst case. Is my understanding correct? Can you share an example of
> > > such configuration?
> >
> > in my understanding, I have not found any machine which has really
> > caused the problem till now.
>
> > [...]
> >
> > This doesn't really happen nowadays as the maximum
> > NR_CPUS is 8196 for X86_64 and 4096 for ARM64 since 8196 * 9 / 32 = 2305
> > is still smaller than 4KB page size.
>
>
> If it's not true, can you or Barry please share such an example?
So for a 4k page size, if you have every-other-cpu-enabled on x86, it
will overflow this, right?
And I have heard of systems much bigger than this as well. Why do you
not think that large number of CPUs are not around?
> > and your complaining about test
> > strings is _VERY_ odd.
>
> The test itself is bad, but it's a minor issue.
>
> My main complain is that the bitmap part of this series introduces a
> function that requires O(N^2) of CPU time and O(N) of memory to just
> print a string. The existing snprintf does this in O(N) and O(1)
> respectively. Additionally to that, the proposed function has some
> flaws in design.
Can you propose a better solution?
And is O(N^2) even an issue for this? Have you run it to determine the
cpu load for such a tiny thing? Why optimize something that no one has
even tried yet?
> > If you have an alternate solution, please propose it, otherwise I will
> > be taking this series in the next few days.
>
> I think I suggested a better solution in the thread for v4:
>
> https://lore.kernel.org/lkml/YMu2amhrdGZHJ5mY@yury-ThinkPad/
>
> > kasprintf() does everything you need. Why don't you use it directly in
> > your driver?
>
> I'm not that familiar to sysfs internals to submit a patch, but the
> idea in more details is like this:
>
> cpulist_read(...)
> {
> if (bitmap_string == NULL)
> bitmap_string = kasprintf(bitmap, ...);
>
> }
>
> Where bitmap_string pointer may be stored in struct file, struct kobject,
> struct bit_attribute or where it's convenient.
No, we are not storing strings in a kobject, sorry.
thanks,
greg k-h
On Thu, Jul 22, 2021 at 07:47:28PM +0200, Greg Kroah-Hartman wrote:
> On Thu, Jul 22, 2021 at 10:09:27AM -0700, Yury Norov wrote:
> > On Wed, Jul 21, 2021 at 01:40:36PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jul 15, 2021 at 04:23:32PM -0700, Yury Norov wrote:
> > > > On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> > > > > On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <[email protected]> wrote:
> > > > > > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > > > > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > > > > > The added test items cover both cases where bitmap buf of the printed
> > > > > > > > result is greater than and less than 4KB.
> > > > > > > > And it also covers the case where offset for bitmap_print_to_buf is
> > > > > > > > non-zero which will happen when printed buf is larger than one page
> > > > > > > > in sysfs bin_attribute.
> > > > > > >
> > > > > > > More test cases is always a good thing, thanks!
> > > > > >
> > > > > > Generally yes. But in this case... I believe, Barry didn't write that
> > > > > > huge line below by himself. Most probably he copy-pasted the output of
> > > > > > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > > > > > and just enforces current behavior of snprintf.
> > > > >
> > > > > I'm not sure I got what you are telling me. The big line is to test
> > > > > strings that are bigger than 4k.
> > > >
> > > > I'm trying to say that human are not able to verify correctness of
> > > > this line. The test is supposed to check bitmap_print_to_buf(), but
> > > > reference output itself is generated by bitmap_print_to_buf(). This
> > > > test will always pass by design, even if there's an error somewhere
> > > > in the middle, isn't it?
> > >
> > > Then please manually check it to verify it is correct or not. Once we
> > > have it verified, that's fine, it will remain static in this test for
> > > always going forward.
> > >
> > > That's what "oracles" are for, there is nothing wrong with this test
> > > case or "proof" that I can see.
> > >
> > > > >
> > > > > ...
> > > > >
> > > > > > > > +static const char large_list[] __initconst = /* more than 4KB */
> > > > > > > > + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > > > > > + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > > > > > > > + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
> > > > > >
> > > > > > I don't like this behavior of the code: each individual line is not a
> > > > > > valid bitmap_list. I would prefer to split original bitmap and print
> > > > > > list representation of parts in a compatible format; considering a
> > > > > > receiving part of this splitting machinery.
> > > > >
> > > > > I agree that split is not the best here, but after all it's only 1
> > > > > line and this is on purpose.
> > > >
> > > > What I see is that bitmap_print_to_buf() is called many times,
> > >
> > > That is not what the above list shows at all, it's one long string all
> > > together, only split up to make it easier for us to work with.
> > >
> > > > and
> > > > each time it returns something that is not a valid bitmap list string.
> > > > If the caller was be able to concatenate all the lines returned by
> > > > bitmap_print_to_buf(), he'd probably get correct result. But in such
> > > > case, why don't he use scnprintf("pbl") directly?
> > >
> > > I do not understand the objection here at all. This series is fixing a
> > > real problem that eeople are having
> >
> > I explicitly asked about an example of this problem. Barry answered in
> > a great length, but the key points are:
> >
> > https://lore.kernel.org/lkml/[email protected]/
> >
> > > > So, the root problem here is that some machines have so many CPUs that
> > > > their cpumask text representation may not fit into the full page in the
> > > > worst case. Is my understanding correct? Can you share an example of
> > > > such configuration?
> > >
> > > in my understanding, I have not found any machine which has really
> > > caused the problem till now.
> >
> > > [...]
> > >
> > > This doesn't really happen nowadays as the maximum
> > > NR_CPUS is 8196 for X86_64 and 4096 for ARM64 since 8196 * 9 / 32 = 2305
> > > is still smaller than 4KB page size.
> >
> >
> > If it's not true, can you or Barry please share such an example?
>
> So for a 4k page size, if you have every-other-cpu-enabled on x86, it
> will overflow this, right?
>
> And I have heard of systems much bigger than this as well. Why do you
> not think that large number of CPUs are not around?
I asked a question: is it urgent?, and I've got an answer: not urgent.
> > > and your complaining about test
> > > strings is _VERY_ odd.
> >
> > The test itself is bad, but it's a minor issue.
> >
> > My main complain is that the bitmap part of this series introduces a
> > function that requires O(N^2) of CPU time and O(N) of memory to just
> > print a string. The existing snprintf does this in O(N) and O(1)
> > respectively. Additionally to that, the proposed function has some
> > flaws in design.
>
> Can you propose a better solution?
Yes. Fix sysfs to let bin_attr store a pointer to relevant data.
Meanwhile, use this O(N^2) hack locally.
> And is O(N^2) even an issue for this?
If it's in lib/bitmap than yes, because it's exposed to the whole
kernel.
Thanks,
Yury
On Thu, Jul 22, 2021 at 11:27:05AM -0700, Yury Norov wrote:
> On Thu, Jul 22, 2021 at 07:47:28PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 22, 2021 at 10:09:27AM -0700, Yury Norov wrote:
...
> > And I have heard of systems much bigger than this as well. Why do you
> > not think that large number of CPUs are not around?
>
> I asked a question: is it urgent?, and I've got an answer: not urgent.
Sometimes that kind of answers may include grain of politeness.
--
With Best Regards,
Andy Shevchenko
On Thu, Jul 22, 2021 at 11:27:05AM -0700, Yury Norov wrote:
> On Thu, Jul 22, 2021 at 07:47:28PM +0200, Greg Kroah-Hartman wrote:
> > On Thu, Jul 22, 2021 at 10:09:27AM -0700, Yury Norov wrote:
> > > On Wed, Jul 21, 2021 at 01:40:36PM +0200, Greg Kroah-Hartman wrote:
> > > > On Thu, Jul 15, 2021 at 04:23:32PM -0700, Yury Norov wrote:
> > > > > On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> > > > > > On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <[email protected]> wrote:
> > > > > > > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > > > > > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > > > > > > The added test items cover both cases where bitmap buf of the printed
> > > > > > > > > result is greater than and less than 4KB.
> > > > > > > > > And it also covers the case where offset for bitmap_print_to_buf is
> > > > > > > > > non-zero which will happen when printed buf is larger than one page
> > > > > > > > > in sysfs bin_attribute.
> > > > > > > >
> > > > > > > > More test cases is always a good thing, thanks!
> > > > > > >
> > > > > > > Generally yes. But in this case... I believe, Barry didn't write that
> > > > > > > huge line below by himself. Most probably he copy-pasted the output of
> > > > > > > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > > > > > > and just enforces current behavior of snprintf.
> > > > > >
> > > > > > I'm not sure I got what you are telling me. The big line is to test
> > > > > > strings that are bigger than 4k.
> > > > >
> > > > > I'm trying to say that human are not able to verify correctness of
> > > > > this line. The test is supposed to check bitmap_print_to_buf(), but
> > > > > reference output itself is generated by bitmap_print_to_buf(). This
> > > > > test will always pass by design, even if there's an error somewhere
> > > > > in the middle, isn't it?
> > > >
> > > > Then please manually check it to verify it is correct or not. Once we
> > > > have it verified, that's fine, it will remain static in this test for
> > > > always going forward.
> > > >
> > > > That's what "oracles" are for, there is nothing wrong with this test
> > > > case or "proof" that I can see.
> > > >
> > > > > >
> > > > > > ...
> > > > > >
> > > > > > > > > +static const char large_list[] __initconst = /* more than 4KB */
> > > > > > > > > + "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,68,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > > > > > > + "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,156,160-161,164-165,168-169,172-173,176-1"
> > > > > > > > > + "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-229,232-233,236-237,240-241,244-245,248-2"
> > > > > > >
> > > > > > > I don't like this behavior of the code: each individual line is not a
> > > > > > > valid bitmap_list. I would prefer to split original bitmap and print
> > > > > > > list representation of parts in a compatible format; considering a
> > > > > > > receiving part of this splitting machinery.
> > > > > >
> > > > > > I agree that split is not the best here, but after all it's only 1
> > > > > > line and this is on purpose.
> > > > >
> > > > > What I see is that bitmap_print_to_buf() is called many times,
> > > >
> > > > That is not what the above list shows at all, it's one long string all
> > > > together, only split up to make it easier for us to work with.
> > > >
> > > > > and
> > > > > each time it returns something that is not a valid bitmap list string.
> > > > > If the caller was be able to concatenate all the lines returned by
> > > > > bitmap_print_to_buf(), he'd probably get correct result. But in such
> > > > > case, why don't he use scnprintf("pbl") directly?
> > > >
> > > > I do not understand the objection here at all. This series is fixing a
> > > > real problem that eeople are having
> > >
> > > I explicitly asked about an example of this problem. Barry answered in
> > > a great length, but the key points are:
> > >
> > > https://lore.kernel.org/lkml/[email protected]/
> > >
> > > > > So, the root problem here is that some machines have so many CPUs that
> > > > > their cpumask text representation may not fit into the full page in the
> > > > > worst case. Is my understanding correct? Can you share an example of
> > > > > such configuration?
> > > >
> > > > in my understanding, I have not found any machine which has really
> > > > caused the problem till now.
> > >
> > > > [...]
> > > >
> > > > This doesn't really happen nowadays as the maximum
> > > > NR_CPUS is 8196 for X86_64 and 4096 for ARM64 since 8196 * 9 / 32 = 2305
> > > > is still smaller than 4KB page size.
> > >
> > >
> > > If it's not true, can you or Barry please share such an example?
> >
> > So for a 4k page size, if you have every-other-cpu-enabled on x86, it
> > will overflow this, right?
> >
> > And I have heard of systems much bigger than this as well. Why do you
> > not think that large number of CPUs are not around?
>
> I asked a question: is it urgent?, and I've got an answer: not urgent.
Just because people can not publically state that they are running Linux
on bigger boxes than this, does NOT mean that they are not running Linux
on bigger boxes than this.
So sometimes, you just have to trust that if someone says "hey, this is
a problem over here", that maybe it really is a problem.
> > > > and your complaining about test
> > > > strings is _VERY_ odd.
> > >
> > > The test itself is bad, but it's a minor issue.
> > >
> > > My main complain is that the bitmap part of this series introduces a
> > > function that requires O(N^2) of CPU time and O(N) of memory to just
> > > print a string. The existing snprintf does this in O(N) and O(1)
> > > respectively. Additionally to that, the proposed function has some
> > > flaws in design.
> >
> > Can you propose a better solution?
>
> Yes. Fix sysfs to let bin_attr store a pointer to relevant data.
Why? It changes all the time and should be generated dynamically.
> Meanwhile, use this O(N^2) hack locally.
Who else uses this that it matters?
> > And is O(N^2) even an issue for this?
>
> If it's in lib/bitmap than yes, because it's exposed to the whole
> kernel.
Who else will use it that it matters for speed?
And did you measure the speed of this?
thanks,
greg k-h
> -----Original Message-----
> From: Greg Kroah-Hartman [mailto:[email protected]]
> Sent: Friday, July 23, 2021 6:45 AM
> To: Yury Norov <[email protected]>
> Cc: Andy Shevchenko <[email protected]>; Andy Shevchenko
> <[email protected]>; Song Bao Hua (Barry Song)
> <[email protected]>; Andrew Morton <[email protected]>;
> Linux Kernel Mailing List <[email protected]>; Dave Hansen
> <[email protected]>; Rasmus Villemoes <[email protected]>; Rafael
> J. Wysocki <[email protected]>; Randy Dunlap <[email protected]>;
> Alexander Gordeev <[email protected]>; Stefano Brivio
> <[email protected]>; Ma, Jianpeng <[email protected]>; Valentin
> Schneider <[email protected]>; Peter Zijlstra (Intel)
> <[email protected]>; Daniel Bristot de Oliveira <[email protected]>;
> Guodong Xu <[email protected]>; tangchengchang
> <[email protected]>; Zengtao (B) <[email protected]>;
> yangyicong <[email protected]>; [email protected]; Linuxarm
> <[email protected]>
> Subject: Re: [PATCH v7 4/4] lib: test_bitmap: add bitmap_print_to_buf test cases
>
> On Thu, Jul 22, 2021 at 11:27:05AM -0700, Yury Norov wrote:
> > On Thu, Jul 22, 2021 at 07:47:28PM +0200, Greg Kroah-Hartman wrote:
> > > On Thu, Jul 22, 2021 at 10:09:27AM -0700, Yury Norov wrote:
> > > > On Wed, Jul 21, 2021 at 01:40:36PM +0200, Greg Kroah-Hartman wrote:
> > > > > On Thu, Jul 15, 2021 at 04:23:32PM -0700, Yury Norov wrote:
> > > > > > On Fri, Jul 16, 2021 at 12:32:45AM +0300, Andy Shevchenko wrote:
> > > > > > > On Thu, Jul 15, 2021 at 11:48 PM Yury Norov <[email protected]>
> wrote:
> > > > > > > > On Thu, Jul 15, 2021 at 03:09:39PM +0300, Andy Shevchenko wrote:
> > > > > > > > > On Thu, Jul 15, 2021 at 11:58:56PM +1200, Barry Song wrote:
> > > > > > > > > > The added test items cover both cases where bitmap buf of
> the printed
> > > > > > > > > > result is greater than and less than 4KB.
> > > > > > > > > > And it also covers the case where offset for bitmap_print_to_buf
> is
> > > > > > > > > > non-zero which will happen when printed buf is larger than
> one page
> > > > > > > > > > in sysfs bin_attribute.
> > > > > > > > >
> > > > > > > > > More test cases is always a good thing, thanks!
> > > > > > > >
> > > > > > > > Generally yes. But in this case... I believe, Barry didn't write
> that
> > > > > > > > huge line below by himself. Most probably he copy-pasted the output
> of
> > > > > > > > his bitmap_print_buf() into the test. If so, this code tests nothing,
> > > > > > > > and just enforces current behavior of snprintf.
> > > > > > >
> > > > > > > I'm not sure I got what you are telling me. The big line is to test
> > > > > > > strings that are bigger than 4k.
> > > > > >
> > > > > > I'm trying to say that human are not able to verify correctness of
> > > > > > this line. The test is supposed to check bitmap_print_to_buf(), but
> > > > > > reference output itself is generated by bitmap_print_to_buf(). This
> > > > > > test will always pass by design, even if there's an error somewhere
> > > > > > in the middle, isn't it?
> > > > >
> > > > > Then please manually check it to verify it is correct or not. Once we
> > > > > have it verified, that's fine, it will remain static in this test for
> > > > > always going forward.
> > > > >
> > > > > That's what "oracles" are for, there is nothing wrong with this test
> > > > > case or "proof" that I can see.
> > > > >
I have manually verified the test case from multiple
different sides. The purpose is verifying large_bitmap,
large_mask and large_list in the test case are consistent.
What I have done includes:
1. snprintf(%*pbl) from large_bitmap to an intermediate buffer - vbf,
and strcmp(vbf, large_list)
2. snprintf(%*pb) from large_bitmap to an intermediate buffer - vbf,
and strcmp(vbf, large_mask)
3. bitmap_parselist from large_list to an intermediate bitmap - vb,
and bitmap_equal(vb, large_bitmap)
4. bitmap_parse from large_mask to an intermediate bitmap - vb,
and bitmap_equal(vb, large_bitmap)
diff --git a/lib/test_bitmap.c b/lib/test_bitmap.c
index eb8ebaf12865..3efedc86a1b9 100644
--- a/lib/test_bitmap.c
+++ b/lib/test_bitmap.c
@@ -781,10 +781,45 @@ static const struct test_bitmap_print
test_print[] __initconst = {
{ large_bitmap, sizeof(large_bitmap) * BITS_PER_BYTE, large_mask, large_list },
};
+#define NEW_API_VERIFIED
+
+#ifdef NEW_API_VERIFIED
+#define VBF_SIZE (2 * PAGE_SIZE)
+static char vbf[VBF_SIZE];
+static int vb_bits = sizeof(large_bitmap) * BITS_PER_BYTE;
+DECLARE_BITMAP(vbmap, sizeof(large_bitmap) * BITS_PER_BYTE);
+#endif
+
static void __init test_bitmap_print_buf(void)
{
int i;
+#ifdef NEW_API_VERIFIED
+ snprintf(vbf, VBF_SIZE, "%*pbl\n", vb_bits, large_bitmap);
+ if (strcmp(vbf, large_list))
+ printk("%s WRONG large bitmap list, print pbl verified\n", __func__);
+ else
+ printk("%s CORRECT large bitmap list, print pbl verified\n", __func__);
+
+ snprintf(vbf, VBF_SIZE, "%*pb\n", vb_bits, large_bitmap);
+ if (strcmp(vbf, large_mask))
+ printk("%s WRONG large bitmap mask, print pb verified\n", __func__);
+ else
+ printk("%s CORRECT large bitmap mask, print pb verified\n", __func__);
+
+ bitmap_parselist(large_list, vbmap, vb_bits);
+ if (bitmap_equal(vbmap, large_bitmap, vb_bits))
+ printk("%s CORRECT large bitmap mask, parselist verified\n", __func__);
+ else
+ printk("%s WRONG large bitmap mask, parselist verified\n", __func__);
+
+ bitmap_parse(large_mask, sizeof(large_mask), vbmap, vb_bits);
+ if (bitmap_equal(vbmap, large_bitmap, vb_bits))
+ printk("%s CORRECT large bitmap mask, parselist verified\n", __func__);
+ else
+ printk("%s WRONG large bitmap mask, parselist verified\n", __func__);
+#endif
+
for (i = 0; i < ARRAY_SIZE(test_print); i++) {
const struct test_bitmap_print *t = &test_print[i];
int n;
Those cases are all good??
[ 1.490355] test_bitmap: loaded.
[ 1.494449] test_bitmap: parselist: 14: input is '0-2047:128/256'
OK, Time: 8384
[ 1.507611] test_bitmap_print_buf CORRECT large bitmap list, print pbl verified
[ 1.508415] test_bitmap_print_buf CORRECT large bitmap mask, print pb verified
[ 1.510337] test_bitmap_print_buf CORRECT large bitmap mask, parselist verified
[ 1.510770] test_bitmap_print_buf CORRECT large bitmap mask, parse verified
[ 1.512833] test_bitmap: all 1945 tests passed
> > > > > > >
> > > > > > > ...
> > > > > > >
> > > > > > > > > > +static const char large_list[] __initconst = /* more than
> 4KB */
> > > > > > > > > > +
> "0,4,8,12,16,20,24,28,32-33,36-37,40-41,44-45,48-49,52-53,56-57,60-61,64,6
> 8,72,76,80,84,88,92,96-97,100-101,104-1"
> > > > > > > > > > +
> "05,108-109,112-113,116-117,120-121,124-125,128,132,136,140,144,148,152,15
> 6,160-161,164-165,168-169,172-173,176-1"
> > > > > > > > > > +
> "77,180-181,184-185,188-189,192,196,200,204,208,212,216,220,224-225,228-22
> 9,232-233,236-237,240-241,244-245,248-2"
> > > > > > > >
> > > > > > > > I don't like this behavior of the code: each individual line is
> not a
> > > > > > > > valid bitmap_list. I would prefer to split original bitmap and
> print
> > > > > > > > list representation of parts in a compatible format; considering
> a
> > > > > > > > receiving part of this splitting machinery.
> > > > > > >
> > > > > > > I agree that split is not the best here, but after all it's only
> 1
> > > > > > > line and this is on purpose.
> > > > > >
> > > > > > What I see is that bitmap_print_to_buf() is called many times,
> > > > >
> > > > > That is not what the above list shows at all, it's one long string all
> > > > > together, only split up to make it easier for us to work with.
> > > > >
> > > > > > and
> > > > > > each time it returns something that is not a valid bitmap list string.
> > > > > > If the caller was be able to concatenate all the lines returned by
> > > > > > bitmap_print_to_buf(), he'd probably get correct result. But in such
> > > > > > case, why don't he use scnprintf("pbl") directly?
> > > > >
> > > > > I do not understand the objection here at all. This series is fixing
> a
> > > > > real problem that eeople are having
> > > >
> > > > I explicitly asked about an example of this problem. Barry answered in
> > > > a great length, but the key points are:
> > > >
> > > >
> https://lore.kernel.org/lkml/[email protected]
> m/
> > > >
> > > > > > So, the root problem here is that some machines have so many
> CPUs that
> > > > > > their cpumask text representation may not fit into the full
> page in the
> > > > > > worst case. Is my understanding correct? Can you share an example
> of
> > > > > > such configuration?
> > > > >
> > > > > in my understanding, I have not found any machine which has really
> > > > > caused the problem till now.
> > > >
> > > > > [...]
> > > > >
> > > > > This doesn't really happen nowadays as the maximum
> > > > > NR_CPUS is 8196 for X86_64 and 4096 for ARM64 since 8196 * 9 /
> 32 = 2305
> > > > > is still smaller than 4KB page size.
> > > >
> > > >
> > > > If it's not true, can you or Barry please share such an example?
> > >
> > > So for a 4k page size, if you have every-other-cpu-enabled on x86, it
> > > will overflow this, right?
> > >
> > > And I have heard of systems much bigger than this as well. Why do you
> > > not think that large number of CPUs are not around?
> >
> > I asked a question: is it urgent?, and I've got an answer: not urgent.
>
> Just because people can not publically state that they are running Linux
> on bigger boxes than this, does NOT mean that they are not running Linux
> on bigger boxes than this.
>
> So sometimes, you just have to trust that if someone says "hey, this is
> a problem over here", that maybe it really is a problem.
>
> > > > > and your complaining about test
> > > > > strings is _VERY_ odd.
> > > >
> > > > The test itself is bad, but it's a minor issue.
> > > >
> > > > My main complain is that the bitmap part of this series introduces a
> > > > function that requires O(N^2) of CPU time and O(N) of memory to just
> > > > print a string. The existing snprintf does this in O(N) and O(1)
> > > > respectively. Additionally to that, the proposed function has some
> > > > flaws in design.
> > >
> > > Can you propose a better solution?
> >
> > Yes. Fix sysfs to let bin_attr store a pointer to relevant data.
>
> Why? It changes all the time and should be generated dynamically.
>
> > Meanwhile, use this O(N^2) hack locally.
>
> Who else uses this that it matters?
>
> > > And is O(N^2) even an issue for this?
> >
> > If it's in lib/bitmap than yes, because it's exposed to the whole
> > kernel.
>
> Who else will use it that it matters for speed?
>
My understanding is that nobody else will print bitmap to a human-readable
mask or list except things like sysfs ABI for userspace. So I have been
arguing performance wouldn't be a concern here.
Those kernel modules who care about performance will directly run bit
operations like and/or/andnot etc on bitmap binaries rather than on a
list or a mask.
On the other hand, Yury's comment on providing a bitmap_max_string(),
which can estimate the max size of the mask or the list according to
bitmap size, might be able to help set a relatively more precise size
for the ABI sysfs file if people care about the file size of the sysfs
entry.
int bitmap_max_string_mask(int nbits)
{
/* each 32bits need 9 bytes like "ffffffff,"
return DIV_ROUND_UP(nbits, 32) * 9;
}
int bitmap_max_string_list(int nbits)
{
...
}
Perhaps this could be an incremental patch after the current patchset
settle down.
Though I'm not quite sure it can really apply to bin_attribute, maybe
we can set the ret value to bin_attribute->bsize in some way?
But, not quite sure bin_attribute->bsize can use a dynamic value since
the size is needed while sysfs file is created:
int sysfs_add_file_mode_ns(struct kernfs_node *parent,
const struct attribute *attr, bool is_bin,
umode_t mode, kuid_t uid, kgid_t gid, const void *ns)
{
struct lock_class_key *key = NULL;
const struct kernfs_ops *ops;
struct kernfs_node *kn;
loff_t size;
if (!is_bin) {
...
size = PAGE_SIZE;
} else {
struct bin_attribute *battr = (void *)attr;
if (battr->mmap)
ops = &sysfs_bin_kfops_mmap;
...
size = battr->size;
}
kn = __kernfs_create_file(parent, attr->name, mode & 0777, uid, gid,
size, ops, (void *)attr, ns, key);
...
return 0;
}
> And did you measure the speed of this?
>
> thanks,
>
> greg k-h
Thanks
Barry
On Wed, Jul 28, 2021 at 12:08 PM Song Bao Hua (Barry Song)
<[email protected]> wrote:
> > From: Greg Kroah-Hartman [mailto:[email protected]]
> > Sent: Friday, July 23, 2021 6:45 AM
...
> +static int vb_bits = sizeof(large_bitmap) * BITS_PER_BYTE;
> +DECLARE_BITMAP(vbmap, sizeof(large_bitmap) * BITS_PER_BYTE);
Just side note: we have BITS_PER_TYPE() macro :-)
--
With Best Regards,
Andy Shevchenko