thank you, Ben.
there's one more "finding", I guess we just mark it as intentional (inside
coverity itself), right ?
693#ifdef FIFTYONEDEGREES_H_PATTERN_INCLUDED
CID 1403837 (#1 of 1): Calling risky function (DC.WEAK_CRYPTO)dont_call:
random should not be used for security-related applications, because linear
congruential algorithms are too easy to break.
Use a compliant random number generator, such as /dev/random or /dev/urandom
on Unix-like systems, and CNG (Cryptography API: Next Generation) on
Windows.
694 _51d_lru_seed = random();
695 if (global_51degrees.cache_size) {
696 _51d_lru_tree = lru64_new(global_51degrees.cache_size);
697 }
698#endif
чт, 24 окт. 2019 г. в 13:50, Ben Shillito <b...@51degrees.com>:
> Hi,
>
>
>
> This is intentional, as the calling method would return early if the htx
> is null (this is assuming that smp_prefetch_htx will not return a different
> result the second time?):
>
> htx = smp_prefetch_htx(smp, chn, 1);
>
> if (!htx)
>
> return 0;
>
> Rather than doing the null check again, I will instead change the method
> to take the htx which has already been checked. This way coverity should
> not have an issue with it, we avoid having an extra null check, and we
> remove the second call to smp_prefetch_htx which was unnecessary.
>
>
>
> I will submit this as a separate patch to the other change.
>
>
>
> Thanks,
>
>
>
> Ben Shillito
> Developer
>
> [image: 51Degrees] <https://51degrees.com/>
>
> O: +44 1183 287152 <callto:441183287152>
> E: b...@51degrees.com <b...@51degrees.com?subject=Your%20Email>
> @51Degrees <http://twitter.com/51Degreesmobi>
> 51Degrees <https://www.linkedin.com/company/2171864>
>
> [image: Find out More] <https://51degrees.com/emailsig.aspx>
>
>
>
> *From:* Илья Шипицин [mailto:chipits...@gmail.com]
> *Sent:* 24 October 2019 08:04
> *To:* Ben Shillito <b...@51degrees.com>
> *Cc:* Willy Tarreau <w...@1wt.eu>; HAProxy <haproxy@formilux.org>
> *Subject:* Re: Possible optimization to 51d in multithread
>
>
>
> Hello, Ben.
>
>
>
> since you are going to touch 51d code, can you please review the following
> coverity finding (it thinks there might be null pointer dereference) ?
>
>
>
> 239
> // No need to null check as this has already been carried out in the
>
> 240 // calling method
>
>
>
> 2. returned_null: smp_prefetch_htx returns NULL (checked 35 out of 36
> times). [show details
> <https://scan6.coverity.com/eventId=28590715-1&modelId=28590715-0&fileInstanceId=94351345&filePath=%2Fsrc%2Fhttp_fetch.c&fileStart=172&fileEnd=294>
> ]
>
>
>
> 3. var_assigned: Assigning: htx = NULL return value from smp_prefetch_htx.
>
> 241 htx = smp_prefetch_htx(smp, chn, 1);
>
> 242
>
>
>
> 4. Condition i < global_51degrees.header_count, taking true branch.
>
> 243 for (i = 0; i < global_51degrees.header_count; i++) {
>
> 244 name.ptr = (global_51degrees.header_names + i)->area;
>
> 245 name.len = (global_51degrees.header_names + i)->data;
>
> 246 ctx.blk = NULL;
>
> 247
>
>
>
> CID 1403847 (#1 of 1): Dereference null return value (NULL_RETURNS)5.
> dereference: Dereferencing a pointer that might be NULL htx when calling
> http_find_header. [show details
> <https://scan6.coverity.com/eventId=28590715-16&modelId=28590715-1&fileInstanceId=94351414&filePath=%2Fsrc%2Fhttp_htx.c&fileStart=54&fileEnd=129>
> ]
>
> 248 if (http_find_header(htx, name, &ctx, 1)) {
>
> 249 ws->importantHeaders[ws->importantHeadersCount
> ].header = ws->dataSet->httpHeaders + i;
>
> 250 ws->importantHeaders[ws->importantHeadersCount
> ].headerValue = ctx.value.ptr;
>
> 251 ws->importantHeaders[ws->importantHeadersCount
> ].headerValueLength = ctx.value.len;
>
> 252 ws->importantHeadersCount++;
>
> 253 }
>
> 254 }
>
> 255}
>
> 256#endif
>
>
>
> ср, 23 окт. 2019 г. в 14:40, Ben Shillito <b...@51degrees.com>:
>
> Hi Willy,
>
> Thanks for letting me know. I will get the work for this scheduled in and
> get a patch over to you.
>
> I agree it looks like a fairly small change for what I'm sure is a
> considerable performance increase for large systems.
>
> Good to see this level of attention to detail when it comes to the
> performance of HAProxy.
>
> Thanks,
>
> Ben Shillito
> Developer
> O: +44 1183 287152
> E: b...@51degrees.com
> T: @51Degrees
>
> -----Original Message-----
> From: Willy Tarreau [mailto:w...@1wt.eu]
> Sent: 23 October 2019 07:04
> To: Ben Shillito <b...@51degrees.com>
> Cc: HAProxy <haproxy@formilux.org>
> Subject: Possible optimization to 51d in multithread
>
> Hi Ben,
>
> after Brian reported the thread performance regression affecting the
> pattern matchings in haproxy when relying on the LRU cache, I had a look at
> other users of the LRU cache and found that 51d.c is using it with a lock
> as well and may also suffer from a lack of linearity with threads.
>
> You may want to have a look at this patch I just committed to make the
> pattern LRU cache per thread :
>
> 403bfbb130 ("BUG/MEDIUM: pattern: make the pattern LRU cache
> thread-local and lockless")
>
> It's quite straightforward, and if you want to do the same on 51d.c, you
> just have to make your lru_tree pointer thread_local and allocate it for
> each thread in a small callback registered to be called after threads are
> initialized. Same for the call to lru64_destroy(). Then you can remove the
> lru lock and gain a lot of scalability.
>
> I'll let you have a look because I'd rather not break somehing non-
> obvious in your code and also because you know better than me how to
> benchmark the changes using the real lib and database, but if you need some
> help, just let me know.
>
> Given that it's quite small and simple a change, I'm fine with merging
> such a patch for 2.1 even a bit late (but only this, no other changes that
> are not bugfixes please).
>
> Willy
> This email and any attachments are confidential and may also be
> privileged. If you are not the named recipient, please notify the sender
> immediately and do not disclose, use, store or copy the information
> contained herein. This is an email from 51Degrees.mobi Limited, 5 Charlotte
> Close, Reading. RG47BY. T: +44 118 328 7152; E: i...@51degrees.com;
> 51Degrees.mobi Limited t/as 51Degrees.
>
> This email and any attachments are confidential and may also be
> privileged. If you are not the named recipient, please notify the sender
> immediately and do not disclose, use, store or copy the information
> contained herein. This is an email from 51Degrees.mobi Limited, 5 Charlotte
> Close, Reading. RG47BY. T: +44 118 328 7152; E: i...@51degrees.com;
> 51Degrees.mobi Limited t/as 51Degrees.
>
