Skip to content

Commit

Permalink
Remove struct ndpi_packet_struct from struct ndpi_flow_struct (#1319
Browse files Browse the repository at this point in the history
)

There are no real reasons to embed `struct ndpi_packet_struct` (i.e. "packet")
in `struct ndpi_flow_struct` (i.e. "flow"). In other words, we can avoid
saving dissection information of "current packet" into the "flow" state,
i.e. in the flow management table.

The nDPI detection module processes only one packet at the time, so it is
safe to save packet dissection information in `struct ndpi_detection_module_struct`,
reusing always the same "packet" instance and saving a huge amount of memory.
Bottom line: we need only one copy of "packet" (for detection module),
not one for each "flow".

It is not clear how/why "packet" ended up in "flow" in the first place.
It has been there since the beginning of the GIT history, but in the original
OpenDPI code `struct ipoque_packet_struct` was embedded in
`struct ipoque_detection_module_struct`, i.e. there was the same exact
situation this commit wants to achieve.

Most of the changes in this PR are some boilerplate to update something
like "flow->packet" into something like "module->packet" throughout the code.
Some attention has been paid to update `ndpi_init_packet()` since we need
to reset some "packet" fields before starting to process another packet.

There has been one important change, though, in ndpi_detection_giveup().
Nothing changed for the applications/users, but this function can't access
"packet" anymore.
The reason is that this function can be called "asynchronously" with respect
to the data processing, i.e in context where there is no valid notion of
"current packet"; for example ndpiReader calls it after having processed all
the traffic, iterating the entire session table.

Mining LRU stuff seems a bit odd (even before this patch): probably we need
to rethink it, as a follow-up.
  • Loading branch information
IvanNardi authored Oct 5, 2021
1 parent f3fcf1e commit 730c236
Show file tree
Hide file tree
Showing 168 changed files with 516 additions and 506 deletions.
19 changes: 14 additions & 5 deletions python/ndpi.py
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,14 @@
/* NDPI_PROTOCOL_WIREGUARD */
uint8_t wireguard_stage;
uint32_t wireguard_peer_index[2];
/* NDPI_PROTOCOL_QUIC */
u_int8_t *quic_reasm_buf;
u_int32_t quic_reasm_buf_len;
/* NDPI_PROTOCOL_CSGO */
uint8_t csgo_strid[18],csgo_state,csgo_s2;
uint32_t csgo_id2;
};
struct ndpi_int_one_line_struct {
Expand Down Expand Up @@ -941,6 +949,9 @@
uint8_t direction_detect_disable:1, /* disable internal detection of packet direction */
_pad:7;
/* Current packet */
struct ndpi_packet_struct packet;
};
#define NDPI_CIPHER_SAFE 0
Expand Down Expand Up @@ -1163,15 +1174,13 @@
uint8_t ovpn_session_id[8];
uint8_t ovpn_counter;
/* Flow key used to search a match into the mining cache */
u_int32_t key_mining_cache;
/* NDPI_PROTOCOL_TINC */
uint8_t tinc_state;
struct tinc_cache_entry tinc_cache_entry;
/* NDPI_PROTOCOL_CSGO */
uint8_t csgo_strid[18],csgo_state,csgo_s2;
uint32_t csgo_id2;
/* internal structures to save functions calls */
struct ndpi_packet_struct packet;
struct ndpi_id_struct *src;
struct ndpi_id_struct *dst;
};
Expand Down
20 changes: 9 additions & 11 deletions python/ndpi_typestruct.py
Original file line number Diff line number Diff line change
Expand Up @@ -276,11 +276,9 @@ class CustomCategories(Structure):
("tinc_cache", POINTER(Cache)),
("proto_defaults", NDPIProtoDefaultsT * (ndpi.ndpi_wrap_ndpi_max_supported_protocols() +
ndpi.ndpi_wrap_ndpi_max_num_custom_protocols())),
("http_dont_dissect_response", c_uint8, 1),
("dns_dont_dissect_response", c_uint8, 1),
("direction_detect_disable", c_uint8, 1),
("disable_metadata_export", c_uint8, 1),
("hyperscan", c_void_p)
('_pad', c_uint8, 7),
('packet', NDPIPacketStruct),
]


Expand Down Expand Up @@ -408,6 +406,12 @@ class NDPIFlowUdpStruct(Structure):
('memcached_matches', c_uint8),
('wireguard_stage', c_uint8),
('wireguard_peer_index', c_uint32 * 2),
('quic_reasm_buf', POINTER(c_uint8)),
('quic_reasm_buf_len', c_uint32),
('csgo_strid', c_uint8 * 18),
('csgo_state', c_uint8),
('csgo_s2', c_uint8),
('csgo_id2', c_uint32),
]


Expand Down Expand Up @@ -735,15 +739,9 @@ class NDPIFlowStructStack(Structure):
('starcraft_udp_stage', c_uint8, 3),
('ovpn_session_id', c_uint8 * 8),
('ovpn_counter', c_uint8),
('key_mining_cache', c_uint32),
('tinc_state', c_uint8),
('TincCacheEntry', TincCacheEntry),
('csgo_strid', c_uint8 * 18),
('csgo_state', c_uint8),
('csgo_s2', c_uint8),
('csgo_id2', c_uint32),
('kxun_counter', c_uint16),
('iqiyi_counter', c_uint16),
('packet', NDPIPacketStruct),
('src', POINTER(NDPIIdStruct)),
('dst', POINTER(NDPIIdStruct))
]
Expand Down
8 changes: 6 additions & 2 deletions src/include/ndpi_typedefs.h
Original file line number Diff line number Diff line change
Expand Up @@ -1175,6 +1175,9 @@ struct ndpi_detection_module_struct {
MMDB_s mmdb_city, mmdb_as;
u_int8_t mmdb_city_loaded, mmdb_as_loaded;
#endif

/* Current packet */
struct ndpi_packet_struct packet;
};

#endif /* NDPI_LIB_COMPILATION */
Expand Down Expand Up @@ -1423,12 +1426,13 @@ struct ndpi_flow_struct {
u_int8_t ovpn_session_id[8];
u_int8_t ovpn_counter;

/* Flow key used to search a match into the mining cache */
u_int32_t key_mining_cache;

/* NDPI_PROTOCOL_TINC */
u_int8_t tinc_state;
struct tinc_cache_entry tinc_cache_entry;

/* internal structures to save functions calls */
struct ndpi_packet_struct packet;
struct ndpi_id_struct *src;
struct ndpi_id_struct *dst;
};
Expand Down
Loading

9 comments on commit 730c236

@vel21ripn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This commit completely kills the nDPI multithreading feature that ndpi-netfilter uses successfully.
"struct ndpi_packet_struct" cannot be static for all threads!
The number of such structures must be no less than the number of cores / threads.
I totally agree that "struct ndpi_packet_struct" is not needed for every flow. But making it static is the worst possible option!
This commit needs to be rewritten.

@IvanNardi
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vel21ripn, I don't know ndpi-netfilter (sorry) but I think you get it wrong.
This change does NOT make struct ndpi_packet_struct static for all threads.
It does make it static for each struct ndpi_detection_module_struct.
Since you can't (AFAIK) safely use the same detection module across multiple threads, in multiple threads/cores scenario you need multiple detection modules anyway, regardless of this specific change.
Am I missing something obvious?

@aouinizied
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@vel21ripn @IvanNardi nDPI detection module is not thread-safe as stated here.
The fact that you are using 1 detection module shared across threads is specific to your implementation.
Is there something that prevents you from using one detection module per thread as done within ndpiReader and ndpiSimpleIntegration?

Zied

@vel21ripn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The following scheme has worked so far: initialized the structure of the ndpi_init_detection_module() module,
specified the list of detected protocols ndpi_set_protocol_detection_bitmask2(), executed ndpi_finalize_initialization().
After that, several threads could work in parallel with different flows.
I did not rewrite ahocorasick in vain, since there was common data. I deliberately moved into a separate structure AC_MATCH_t everything that is modified during the processing of the package.
Can you please tell me what data of the ndpi_detection_module_struct structure are modified during the operation of ndpi_detection_process_packet()?

@utoni
Copy link
Collaborator

@utoni utoni commented on 730c236 Oct 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ndpi_detection_module_struct is not meant to be thread-safe. It is as simple as that.
Even if it worked for you before 730c236, invalid API usage of a library can have unforeseen consequences at any time.
The real issue I see here is that there is no documentation available that states what is allowed in MT environments and what not.

@IvanNardi
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you please tell me what data of the ndpi_detection_module_struct structure are modified during the operation of ndpi_detection_process_packet()?

All the LRU caches are updated at runtime

@vel21ripn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@IvanNardi
All the LRU caches are updated at runtime
Thanks.
It's not a problem. It has very simple code. It is very easy to make this code reentrant.
Are there any other common data besides lru and libcache?
Ndpi-netfilter uses libcache with locks.

@vel21ripn
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we replace "& ndpi_struct.packet" with an inline function, then there is no problem with re-entering the ndpi_detection_process_packet() code.
I can prepare such a PR. I also remember about the need to fix the LRU cache code.
I just now noticed example/ndpiSimpleIntegration.c. This is a very good starting point for testing multi-threaded use of libndpi.

@IvanNardi
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please, continue the discussion on #1344

Please sign in to comment.