Af xdp fixes 8780 v2 - #16157
Af xdp fixes 8780 v2#16157Aboussejra wants to merge 4 commits into
Conversation
…ltname used in suricata config WriteLinuxTunables built the sysfs path from the configured interface name. When an altname is used in the config, sysfs only exposes the real netdev name and writing gro_flush_timeout/napi_defer_hard_irqs failed. Resolve the real name via if_indextoname before building the path. Ticket: OISF#8780
The XSK queue id counter was global, so with two capture interfaces the second interface's threads got queue ids continuing after the first interface's, and xsk_socket__create failed for queues that do not exist on that device. Make the counter per interface config and assign ids with a single atomic fetch-and-add at thread init. Ticket: OISF#8780
UMEM size was hardcoded to XSK ring default descriptors * 2 with default frame size. Add umem-frames and frame-size options to the af-xdp section so buffer memory can be tuned per interface. Ticket: OISF#8780
Expose xsk_socket stats in stats.log: capture.afxdp.rx_dropped, rx_invalid_descs, rx_ring_full and fill_ring_empty. Helps diagnose whether kernel drops come from. Ticket: OISF#8780
|
NOTE: This PR may contain new authors. |
| #define DEFAULT_NAPI_HARD_IRQS 2 | ||
| #define DEFAULT_RX_RING_SIZE XSK_RING_CONS__DEFAULT_NUM_DESCS | ||
| #define DEFAULT_UMEM_FRAMES (XSK_RING_PROD__DEFAULT_NUM_DESCS * 2) | ||
| #define DEFAULT_FRAME_SIZE XSK_UMEM__DEFAULT_FRAME_SIZE |
There was a problem hiding this comment.
Just a thought: For an easier user addoption, couldn't the frame size be deduced from the configured MTU size (and fail if MTU is over 4k?)
4k seems to be a limit if hugepages are not used. There could be then some extra logic to detect if hugepages are used (and therefore larger MTU would be allowed).
lukashino
left a comment
lukashino
left a comment
There was a problem hiding this comment.
No terrible blockers found, more like nits and comments to clarify before the merge.
After that, it is on track to get the approve from my side.
|
|
||
| /* The configured interface may be an alternative name (e.g. "mon0"), but | ||
| * sysfs only exposes a directory for the real netdev name. Resolve it | ||
| * through the ifindex. */ |
There was a problem hiding this comment.
The last sentence seems redundant/too verbose to me.
|
|
||
| /* The queue numbering counter is per interface and shared by that interface's threads | ||
| * which all init in parallel, so claim the index with a single atomic fetch and add. */ | ||
| ptv->xsk.queue.queue_num = SC_ATOMIC_ADD(afxdpconfig->queue_idx, 1); |
There was a problem hiding this comment.
I miss the initialization step here, you are adding the 1 but you only declared it and never initiliazed it to zero (as it was done before).
Also I think the comment, in this case, can be removed or fit it into a single line.
There was a problem hiding this comment.
e.g. init can be done in ParseAFXDPConfig
| /* Size of one UMEM frame in bytes. Must be a power of two. */ | ||
| if (SCConfGetChildValueIntWithDefault(if_root, if_default, "frame-size", &conf_val_int) == 1) { | ||
| if (conf_val_int > 0) { | ||
| aconf->frame_size = (uint32_t)conf_val_int; |
There was a problem hiding this comment.
As the code comment above suggest, the power of two constrain could be verified here. Or is it enforced elsewhere?
| ptv->xsk.cfg.rx_size = NUM_FRAMES_CONS; | ||
| ptv->xsk.cfg.tx_size = NUM_FRAMES_PROD; | ||
| ptv->xsk.cfg.rx_size = afxdpconfig->rx_ring_size; | ||
| ptv->xsk.cfg.tx_size = XSK_RING_PROD__DEFAULT_NUM_DESCS; |
There was a problem hiding this comment.
If IPS mode is not supported, cannot we set the tx_size to 0? Or completely disable the TX rings?
There was a problem hiding this comment.
Indeed ! I'll add that as a dedicated commit !
|
@lukashino Reworked discussed points in v3 #16208 |
2 participants
Link to ticket: https://redmine.openinfosecfoundation.org/issues/8780
Describe changes:
Series of fixes for AF_XDP capture mode, tested on suricata 8.0.6 base:
Provide values to any of the below to override the defaults.
link to the pull request in the respective
_BRANCHvariable.SV_REPO=
SV_BRANCH=
SU_REPO=
SU_BRANCH=