Re: [Bro-Dev] Performance Issues after the fe7e1ee commit?
Hi Jon, Thanks for looking into this! I tried this again after pulling from master and at least for our case it is looking good! ### old # /hostname/bro/bin/bro --version /hostname/bro/bin/bro version 2.5-519 # time /hostname/bro/bin/bro -r 20180606-1242-prodfilers.pcap master.bro real0m32.772s user0m28.821s sys 0m3.873s ### new # /hostname/bro-devel/bin/bro --version /hostname/bro-devel/bin/bro version 2.5-658 # time /hostname/bro-devel/bin/bro -r 20180606-1242-prodfilers.pcap master.bro real0m34.684s user0m31.502s sys 0m4.266s Thanks again! --Tim -Original Message- From: Jon Siwek [mailto:jsi...@corelight.com] Sent: Thursday, June 07, 2018 6:06 PM To: Robin Sommer Cc: McMullan, Tim ; ; Wallior, Julien ; Trejo, Devin Subject: Re: [Bro-Dev] Performance Issues after the fe7e1ee commit? On Thu, Jun 7, 2018 at 12:21 PM Robin Sommer wrote: > > Hmm, I'm still seeing much larger runtimes on that M57 trace using our > test configuration, even with yesterday's change: > > ; Master, pre-Broker > # zcat 2009-M57-day11-18.trace.gz | time bro -r - tests/m57-long.bro > 73.72user 7.90system 1:06.53elapsed 122%CPU (0avgtext+0avgdata > 199092maxresident) > > ; Current master > # zcat 2009-M57-day11-18.trace.gz | time bro -r - tests/m57-long.bro > 2191.59user 1606.69system 12:39.92elapsed 499%CPU (0avgtext+0avgdata > 228400maxresident) > > Bro must still be blocking somewhere when reading from trace files. Thanks, if you pull master changes [1] again there's likely some improvement. You can also try comparing the timing of: # zcat 2009-M57-day11-18.trace.gz | time bro -r - tests/m57-long.bro Known::use_host_store=F Known::use_service_store=F Known::use_cert_store=F With that, I get the same timings as from before pre-Broker. At least a good chunk of the difference when using data stores is that, for every query, Bro will immediately block until getting a response back from the data store thread/actor. Not doing this type of synchronous data store access when reading pcaps leads to the possibility of results differing between runs (and I recall such differences being likely from experience in running the unit test suite). - Jon [1] https://github.com/bro/broker/commit/9b56fea4999d4e11a5cd2caaafd934759015fab5 IMPORTANT: The information contained in this email and/or its attachments is confidential. If you are not the intended recipient, please notify the sender immediately by reply and immediately delete this message and all its attachments. Any review, use, reproduction, disclosure or dissemination of this message or any attachment by an unintended recipient is strictly prohibited. Neither this message nor any attachment is intended as or should be construed as an offer, solicitation or recommendation to buy or sell any security or other financial instrument. Neither the sender, his or her employer nor any of their respective affiliates makes any warranties as to the completeness or accuracy of any of the information contained herein or that this message or any of its attachments is free of viruses. ___ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
Re: [Bro-Dev] Performance Issues after the fe7e1ee commit?
On Fri, Jun 8, 2018 at 12:16 PM Jon Siwek wrote: > Only thing I'm thinking is that your test system maybe > does a poorer job of scheduling the right thread to run in order for > the FlushPendingQueries() spin-loop to actually finish. Actually, realized you still had bad timing with data stores off, so it would more likely be a problem with the AdvanceTime() code path. There's some mutex locking going on there, but w/o data stores involved there shouldn't be anyone competing for them. - Jon ___ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
Re: [Bro-Dev] Performance Issues after the fe7e1ee commit?
Ok, I'll dig around a bit more and also double-check that I didn't change any settings in the meantime. Robin On Fri, Jun 08, 2018 at 12:40 -0500, you wrote: > On Fri, Jun 8, 2018 at 12:16 PM Jon Siwek wrote: > > > Only thing I'm thinking is that your test system maybe > > does a poorer job of scheduling the right thread to run in order for > > the FlushPendingQueries() spin-loop to actually finish. > > Actually, realized you still had bad timing with data stores off, so > it would more likely be a problem with the AdvanceTime() code path. > There's some mutex locking going on there, but w/o data stores > involved there shouldn't be anyone competing for them. > > - Jon > -- Robin Sommer * ICSI/LBNL * ro...@icir.org * www.icir.org/robin ___ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
Re: [Bro-Dev] Performance Issues after the fe7e1ee commit?
On Fri, Jun 8, 2018 at 10:23 AM Robin Sommer wrote: > > > # zcat 2009-M57-day11-18.trace.gz | time bro -r - tests/m57-long.bro > > Known::use_host_store=F Known::use_service_store=F > > Known::use_cert_store=F > > That indeed gets it way down, though still not back to the same level > on my box: > > 170.49user 58.14system 1:55.94elapsed 197%CPU > > (pre-master: 73.72user 7.90system 1:06.53elapsed 122%CPU) Just double-checking: same configure/build flags were used between the two? Here's the more precise results I got from running on a macOS and Linux system: # Linux master (b51e6f3) --build-type=debug $ export BROPATH=$BROPATH:../testing/external/scripts; time bro -r 2009-M57-day11-18.trace test-all-policy testing-setup real0m32.572s user0m40.926s sys 0m7.873s # Linux master (b51e6f3) --build-type=debug $ export BROPATH=$BROPATH:../testing/external/scripts; time bro -r 2009-M57-day11-18.trace test-all-policy testing-setup Known::use_host_store=F Known::use_cert_store=F Known::use_service_store=F real0m25.603s user0m23.311s sys 0m7.952s # Linux pre-broker (7a6f502) --build-type=debug $ export BROPATH=$BROPATH:../testing/external/scripts; time bro -r 2009-M57-day11-18.trace test-all-policy testing-setup real0m24.785s user0m21.230s sys 0m8.341s # macOS master (b51e6f3) --build-type=debug $ export BROPATH=$BROPATH:../testing/external/scripts; time bro -r 2009-M57-day11-18.trace test-all-policy testing-setup.bro real0m38.775s user0m42.365s sys 0m8.950s # macOS master (b51e6f3) --build-type=debug $ export BROPATH=$BROPATH:../testing/external/scripts; time bro -r 2009-M57-day11-18.trace test-all-policy testing-setup.bro Known::use_host_store=F Known::use_cert_store=F Known::use_service_store=F real0m32.975s user0m33.774s sys 0m4.409s # macOS pre-broker (7a6f502) --build-type=debug $ export BROPATH=$BROPATH:../testing/external/scripts; time bro -r 2009-M57-day11-18.trace test-all-policy testing-setup.bro real0m30.785s user0m31.840s sys 0m3.788s > Are there more places where Bro's waiting for Broker in pcap mode? Not that I can think of. > Yeah, I remember that discussion. It's the trade-off between > performance and consistency. Where's the code that's doing this > blocking? I just know that Manager::AdvanceTime() and also Manager::TrackStoreQuery() -> FlushPendingQueries() can block/spin waiting on actor/threading in pcap mode. > Would it be possible to not block right away, but sync up > with Broker when events are flushed the next time? (Like we had > discussed before as a general mechanism for making async operations > consistent) I think the way to make an async operation most consistent is to model it as a synchronous operation? That's what I'm doing now at least, and if I try moving FlushPendingQueries() to later (before the AdvanceTime() call) in an attempt to possibly have more queries in the queue/buffer, I get the external test suites failing. At least on my own test systems, I don't have much performance to recover by going this route anyway, so maybe we need to dig more into why your results are different. Only thing I'm thinking is that your test system maybe does a poorer job of scheduling the right thread to run in order for the FlushPendingQueries() spin-loop to actually finish. - Jon ___ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
Re: [Bro-Dev] Performance Issues after the fe7e1ee commit?
On Thu, Jun 07, 2018 at 17:05 -0500, you wrote: > Thanks, if you pull master changes [1] again there's likely some improvement. Yeah, a little bit, not much though. > # zcat 2009-M57-day11-18.trace.gz | time bro -r - tests/m57-long.bro > Known::use_host_store=F Known::use_service_store=F > Known::use_cert_store=F That indeed gets it way down, though still not back to the same level on my box: 170.49user 58.14system 1:55.94elapsed 197%CPU (pre-master: 73.72user 7.90system 1:06.53elapsed 122%CPU) Are there more places where Bro's waiting for Broker in pcap mode? > With that, I get the same timings as from before pre-Broker. At least > a good chunk of the difference when using data stores is that, for > every query, Bro will immediately block until getting a response back > from the data store thread/actor. Yeah, I remember that discussion. It's the trade-off between performance and consistency. Where's the code that's doing this blocking? Would it be possible to not block right away, but sync up with Broker when events are flushed the next time? (Like we had discussed before as a general mechanism for making async operations consistent) Robin -- Robin Sommer * ICSI/LBNL * ro...@icir.org * www.icir.org/robin ___ bro-dev mailing list bro-dev@bro.org http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
[Bro-Dev] SMB Analyzer code factorization
Hi all,
As I looked into SMBv1 analyzer, I found that most of the files
describing SMB messages have code duplication. According to the SMB
specification ([MS-CIFS]), SMB messages are composed of a fixed-length
header (defined as SMB_Header in smb1-protocol.pac for Bro) and then of
two "blocks" : the parameter block (section 2.2.3.2) and the data block
(section 2.2.3.3). Every message of the protocol I saw in the
specification or encountered in trafic always followed that scheme.
However, in Bro's SMBv1 analyzer, each .pac file describing a particular
type of message reimplement the two blocks common fields.
The SMB PDU is defined as such :
type SMB_PDU(...) = record {
header: SMB_Header(...)
message: case msg_len of {
35 -> no_msg: SMB_No_Message;
default -> msg: SMB_Message(...);
};
};
SMB_Message is just a switch between SMB_Message_request and
SMB_Message_Response. Then these two are defined as a switch on command
type :
type SMB_Message_Request(...) = case command of {
SMB_COM_READ_ANDX -> read_andx: SMB1_read_andx_request(...);
...
};
And then every command is implemented like this one :
type SMB_read_andx_request(...) = record {
word_count: uint8
... specific fields ...
byte_count: uint16;
... specific fields ...
};
I feel like it would be a good idea to abstract the two blocks : it
would factorize code. Also, there is currently no check on the value of
word_count field or byte_count field for the rest of the payload. This
could lead to protocol violations from BinPAC if you have a byte_count
set to 0 and then a following field trying to parse a SMB_String for
example (see SMB1_transaction_request record in smb1-com-transaction.pac
for an example of that).
A solution could be to change SMB_Message_* to something closer to what
the specification describe by dividing every message in two half
structures :
type SMB_Message_Request(...) = record {
parameter_block: SMB_Request_Parameters(...);
data_block: SMB_Request_Data(...);
};
type SMB_Request_Parameters(...) = record {
word_count: uint8;
# Maybe a check on word_count here? Some messages seems to have a
predefined value for this field
words: SMB_Request_Words(...);
};
type SMB_Request_Data(...) = record {
byte_count: uint16;
data_or_not: case byte_count of {
0 -> none: empty;
default -> bytes: SMB_Request_Bytes(...);
};
};
And then SMB_Request_Words and SMB_Request_Bytes would be switch on
different messages types :
type SMB_Request_Words(...) = case command of {
SMB_COM_READ_ANDX -> words_read_and_x:
SMB1_Words_read_andx_request(...);
...
};
type SMB_Request_Bytes(...) = case command of {
SMB_COM_READ_ANDX -> bytes_read_and_x:
SMB1_Bytes_read_andx_request(...);
...
};
Of course, it means splitting every existing message and re factoring a
lot of code of the analyzer, but it would be easier then to address
problems such as the one quoted as example above. What do you think of it ?
Regards,
signature.asc
Description: OpenPGP digital signature
___
bro-dev mailing list
bro-dev@bro.org
http://mailman.icsi.berkeley.edu/mailman/listinfo/bro-dev
