|WARNING| [RFC,v3,7/7] regex/hs: add Hyperscan compile flag support

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: WARNING
https://dpdk.org/patch/169696

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-09-16

# DPDK Patch Review: Hyperscan RegEx PMD (RFC v3)

## Summary

This patch series introduces a new software regex PMD based on Intel Hyperscan. The review focuses on correctness bugs, coding style compliance, and API/documentation requirements per AGENTS.md guidelines.

---

## Patch 1/7: Driver skeleton and build integration

### Errors

None.

### Warnings

**Queue pair allocation without NUMA awareness**
In `hs_regex_configure()`, the queue pair array is allocated with `rte_zmalloc()` without a socket_id parameter:
```c
priv->qps = rte_zmalloc("hs_regex_qps",
                        sizeof(struct hs_regex_qp) * cfg->nb_queue_pairs,
                        RTE_CACHE_LINE_SIZE);
```
Queue pair control structures should use `rte_zmalloc_socket()` with `SOCKET_ID_ANY` or a device-specific socket to ensure proper NUMA placement. While `rte_zmalloc()` defaults to the calling lcore's socket, this is fragile if configure is called from an arbitrary control thread.

**Suggested fix:**
```c
priv->qps = rte_zmalloc_socket("hs_regex_qps",
                                sizeof(struct hs_regex_qp) * cfg->nb_queue_pairs,
                                RTE_CACHE_LINE_SIZE, SOCKET_ID_ANY);
```

---

## Patch 2/7: Device configure and queue pair setup

### Errors

**Resource leak on queue pair allocation failure**
In `hs_regex_configure()` at line 147, if queue pair allocation fails, `priv->nb_queue_pairs` is set to 0 but the function returns `-ENOMEM` immediately. If the caller retries configure with a smaller `nb_queue_pairs`, the code will skip the cleanup of `priv->rules` and `priv->db` because those blocks check `if (priv->rules)` and `if (priv->db)` before the new allocation. However, the comment claims reconfigure replaces these resources. If an earlier configure succeeded partially (allocated rules/db but failed on qps), those resources remain allocated but orphaned.

The pattern is:
1. First configure: allocate rules, db succeeds; qps allocation fails - nb_queue_pairs set to 0, return -ENOMEM. Rules/db still allocated.
2. Second configure: enters with rules/db non-NULL, frees them, then tries to allocate qps again.

Actually, re-reading the code: the cleanup of rules/db happens at the start of configure (lines 98-108), before the new allocation. So on retry, the old rules/db are freed. This is correct. However, if the qps allocation fails (line 147), `priv->nb_queue_pairs` is set to 0 but the function returns immediately. The caller has no queue pairs array but also no way to recover except calling configure again. This is acceptable behavior (caller retries), so not a leak. Withdraw this item.

### Warnings

**Non-const function pointer array in hs_regex_qp**
The `hs_regex_qp` structure contains `struct rte_regex_ops **ops`, which is a mutable array of pointers. This is not a function pointer array, so the non-const rule does not apply. Withdraw.

**Queue descriptor allocation without NUMA awareness**
In `hs_regex_qp_setup()` at line 318, the queue descriptor ring is allocated with `rte_zmalloc()` without a socket_id:
```c
qp->ops = rte_zmalloc("hs_regex_qp_ops",
                      sizeof(struct rte_regex_ops *) * nb_desc,
                      RTE_CACHE_LINE_SIZE);
```
Queue descriptor rings should use `rte_zmalloc_socket()` with `SOCKET_ID_ANY` or the device/queue socket to ensure NUMA-local allocation for performance.

**Suggested fix:**
```c
qp->ops = rte_zmalloc_socket("hs_regex_qp_ops",
                              sizeof(struct rte_regex_ops *) * nb_desc,
                              RTE_CACHE_LINE_SIZE, SOCKET_ID_ANY);
```

---

## Patch 3/7: Rule database update and compilation

### Errors

**Double-free on rule pattern realloc failure**
In `hs_regex_rule_db_update()` at lines 510-524, when `rte_realloc()` fails:
```c
struct hs_regex_rule *tmp = rte_realloc(priv->rules,
    new_cap * sizeof(struct hs_regex_rule), 0);
if (!tmp) {
    HS_LOG(ERR, "Failed to grow rules");
    rte_errno = ENOMEM;
    return i;
}
priv->rules = tmp;
```
If `rte_realloc()` fails, it returns NULL but leaves the original pointer (`priv->rules`) intact. The code correctly checks `if (!tmp)` and returns early, so `priv->rules` is not overwritten with NULL. The original allocation remains valid. No double-free occurs here. Withdraw.

**Resource leak on compilation failure (scratch cleanup incomplete)**
In `hs_regex_rule_db_compile_activate()` at lines 638-649, if `hs_alloc_scratch()` fails for queue pair `i`, the code correctly frees all previously allocated scratch spaces (loop over `j < i`) and frees the database. However, the code does NOT set `priv->db_compiled = 0` before returning. The function body at line 656 sets `priv->db_compiled = 1` only on success, so the flag remains 0 (its initial value or previous state). Actually, looking at line 226 in patch 2, the flag is zeroed during configure: `priv->db_compiled = 0;`. So if compile fails, the flag stays 0. No issue. Withdraw.

**Use of `free()` on Hyperscan-allocated buffer without error check**
In `hs_regex_rule_db_export()` at line 783, the code calls `free(buf)` where `buf` is allocated by Hyperscan's `hs_serialize_database()`. The comment correctly notes this is Hyperscan's malloc, not rte_malloc. However, the code does not check if `rule_db` copy succeeded before freeing. If `memcpy()` could fail (it can't in practice, but defensively), the buffer would be freed and the error lost. In this case, `memcpy()` has no failure mode for valid pointers, so this is fine. Withdraw.

**Missing error check on `snprintf()` return value in hash name**
In `hs_regex_rule_db_update()` at line 347:
```c
snprintf(hash_name, sizeof(hash_name), "hs_rule_ids_%u", dev->data->dev_id);
```
`snprintf()` can truncate if the name exceeds `RTE_HASH_NAMESIZE`. The return value is not checked. If `dev_id` is very large, the name may be truncated, but rte_hash will still work (the name is used for lookups, and truncation produces a valid string). This is not a correctness bug. Withdraw.

### Warnings

**Missing validation of `hs_compile_ext_multi()` output before use**
In `hs_regex_rule_db_compile_activate()` at line 616, `hs_compile_ext_multi()` is called and its return value is checked. On failure, the function logs the error and returns. On success, `priv->db` is non-NULL. The subsequent scratch allocation loop (lines 625-649) dereferences `priv->db` without re-checking. Since the Hyperscan API guarantees `db` is valid on `HS_SUCCESS`, this is acceptable. Withdraw.

---

## Patch 4/7: Enqueue and dequeue burst paths

### Errors

**64-bit counter may overflow in `hs_match_ctx.total_matches`**
In the `hs_match_cb()` function at line 69, `ctx->total_matches` is a `uint64_t` incremented on every match. In `hs_regex_enqueue_burst()` at line 901, this counter is added to `qp->qp_matches`, which is also `uint64_t`. With 64-bit counters, overflow is practically impossible (would require 2^64 matches), so this is not a concern. Withdraw.

**Potential NULL dereference in dequeue if `priv->qps` is NULL**
In `hs_regex_dequeue_burst()` at line 924, the code checks `if (unlikely(priv->qps == NULL)) return 0;` before accessing `qp = &priv->qps[qp_id];`. This is correct. Withdraw.

**Missing bounds check on `qp->count` decrement in dequeue**
In `hs_regex_dequeue_burst()` at line 939:
```c
for (i = 0; i < nb_ops; i++) {
    ops[i] = qp->ops[qp->head];
    qp->head = (qp->head + 1) & (qp->nb_desc - 1);
    qp->count--;
}
```
The code computes `avail = qp->count;` and then `nb_ops = min(nb_ops, avail);`, so the loop will decrement `qp->count` exactly `nb_ops` times, and `nb_ops <= avail`. The decrement cannot underflow. Withdraw.

**Integer overflow in ring pointer arithmetic**
In `hs_regex_enqueue_burst()` at line 1014 and `hs_regex_dequeue_burst()` at line 939, the ring uses `(qp->tail + 1) & (qp->nb_desc - 1)` and `(qp->head + 1) & (qp->nb_desc - 1)`. The value `qp->nb_desc` is a `uint16_t` power of two (ensured by the rounding in `hs_regex_qp_setup()`). The mask `(qp->nb_desc - 1)` is guaranteed to be valid for wrapping. No overflow occurs. Withdraw.

---

## Patch 5/7: Extended statistics

### Errors

None.

### Warnings

None.

---

## Patch 6/7: Start, stop, close, and device dump

### Errors

**Double-free on repeated `hs_regex_close()` calls**
In `hs_regex_close()` at line 866, the function frees `priv->qps` and sets it to NULL (line 888). It also frees `priv->db` and sets it to NULL (line 893). If the function is called twice without an intervening configure, the second call will skip the frees (due to the NULL checks) and succeed harmlessly. No double-free occurs. Withdraw.

**Resource leak on `hs_regex_dev_destroy()` if close was never called**
In `hs_regex_dev_destroy()` at line 1369, the function calls `hs_regex_close(dev);` before freeing `priv`. If `hs_regex_close()` was never called by the application, this ensures resources are released. This is correct. Withdraw.

### Warnings

None.

---

## Patch 7/7: Hyperscan compile flag support

### Errors

**Auto-start in enqueue without state machine validation**
In `hs_regex_enqueue_burst()` at lines 1026-1037, if the device is not started but a database is compiled, the code auto-starts the device by setting `priv->dev_state = HS_REGEX_DEV_STARTED`. However, this bypasses the validation in `hs_regex_start()` which checks that the device is in `HS_REGEX_DEV_CONFIGURED` or `HS_REGEX_DEV_STOPPED` state. If the device is in `HS_REGEX_DEV_CREATED` (not configured), the auto-start will proceed anyway. The code checks `priv->db_compiled`, which is only true if configure+compile succeeded, so the device must be at least configured. However, if the device is `HS_REGEX_DEV_STOPPED` and then auto-started, this is acceptable. If it's `HS_REGEX_DEV_CREATED`, `db_compiled` will be false (configure zeros it), so the else branch is taken. No issue. Withdraw.

### Warnings

**Documentation describes auto-start behavior but does not warn about EBUSY on explicit start after auto-start**
The documentation at line 185 states that if `start()` is called after auto-start, it will fail with `-EBUSY`. However, the `hs_regex_start()` function at line 806 checks `if (priv->dev_state == HS_REGEX_DEV_STARTED)` and returns `-EBUSY`. This is consistent with the documentation. No issue. Withdraw.

---

## General Observations

### Coding Style

**Implicit comparison on `rte_pktmbuf_linearize()` return value**
In `hs_regex_enqueue_burst()` at line 1058:
```c
if (rte_pktmbuf_linearize(mbuf) != 0) {
```
This is an explicit comparison against 0, which is the required DPDK style. No issue.

**Explicit NULL checks throughout**
The code consistently uses `if (ptr == NULL)` rather than `if (!ptr)`. This complies with DPDK style. No issues.

**Use of `likely()` and `unlikely()` in fast path**
The enqueue and dequeue functions use `unlikely()` for error paths (e.g., `if (unlikely(dev == NULL))`). These are hints, not comparisons, so they don't affect the explicit comparison rule. No issues.

### API and Documentation

**Release notes correctly updated**
Patch 1 adds a detailed release notes entry describing the new PMD, its capabilities, and key features. This complies with the documentation requirement.

**Feature matrix updated**
The `hs.ini` file lists supported features (`Run time compilation`, `x86`). This is correct.

**Documentation matches code**
The `hs.rst` file describes the PMD's behavior, limitations, and API usage. Cross-checking against the code:
- "Up to 64 queue pairs" matches `HS_REGEX_MAX_QUEUE_PAIRS` (64).
- "Up to 1,000,000 rules" matches `HS_REGEX_MAX_RULES` (1000000).
- "Per-queue-pair scratch space" is implemented in `hs_regex_qp` structure.
- "Synchronous scan in enqueue" is confirmed by the code calling `hs_scan()` directly in the enqueue loop.
- "Multi-segment mbufs are linearized" is implemented at line 1058.
- "Multi-process mode is not supported" is enforced by the probe function check at line 1398.

Documentation is accurate.

**API tags correct**
No new public API is added (this is a PMD, not a library). The `RTE_PMD_REGISTER_VDEV` macro is used correctly. No `__rte_experimental` or `__rte_internal` tags are needed.

---

## Summary of Findings

### Errors

None.

### Warnings

1. **Queue pair allocation without NUMA awareness** (Patch 1/7): Use `rte_zmalloc_socket()` instead of `rte_zmalloc()` for `priv->qps`.
2. **Queue descriptor allocation without NUMA awareness** (Patch 2/7): Use `rte_zmalloc_socket()` instead of `rte_zmalloc()` for `qp->ops`.

### Info

None.

---

## Conclusion

The patch series is well-structured and implements a complete Hyperscan-based regex PMD. The code is mostly correct, with proper resource management, explicit comparisons, and accurate documentation. The two warnings regarding NUMA-aware allocation are minor performance considerations rather than correctness bugs, but should be addressed for optimal NUMA placement of queue pair control structures and descriptor rings.

The auto-start behavior in patch 7 is a design choice that improves compatibility with applications that skip the explicit `start()` call. The documentation clearly describes this behavior and its implications.