|WARNING| [RFC,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/168541

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-08-28

# Review of DPDK Hyperscan RegEx PMD Patch Series

## Patch 1/7: Driver Skeleton and Build Integration

### Errors

**Resource leak on configure failure (patch 2)**
In `hs_regex_configure()` (patch 2), if `hs_regex_rule_db_import()` fails after allocating `priv->qps`, the function frees `qps` and sets `nb_queue_pairs` to 0, then reverts `dev_state` to `CREATED`. However, this leaves `priv->max_matches` and `priv->nb_groups` set to their configured values from earlier in the function, creating inconsistent state. These should also be reset on failure.

**Use of `volatile` for device state (patch 6)**
The `dev_state` field in `struct hs_regex_priv` is read and written by control-plane operations (`configure`, `start`, `stop`) and checked in the fast path (`enqueue_burst`). If different threads call these operations concurrently (which the documentation says is not supported per-QP but doesn't explicitly prohibit at the device level), this is a data race. Use `rte_atomic_*` for `dev_state` if concurrent access is possible. If the single-producer/single-consumer model guarantees only one thread touches the device at a time, document this explicitly in the header.

**Missing error check on `rte_hash_create()` (patch 3)**
In `hs_regex_rule_db_update()`, after calling `rte_hash_create()`, the code checks `if (!priv->rule_id_hash)` and logs an error, then returns `-ENOMEM`. However, if this allocation fails partway through processing a multi-rule update, the function has already added earlier rules to `priv->rules` without adding their IDs to the hash. Subsequent calls will not detect duplicates for those rules, corrupting the duplicate-detection invariant. Either create the hash before processing any rules, or roll back `priv->rules` additions on hash creation failure.

**Missing cleanup of `rule_id_hash` on partial rule update failure (patch 3)**
If `hs_regex_rule_db_update()` fails partway through (e.g., pattern allocation failure for rule `i`), it returns `i` to indicate how many rules were processed. However, the hash table has already been populated with the `rule_id` values for rules `0..i-1` via `rte_hash_add_key()`. If the application retries with a corrected rule list, those IDs will be flagged as duplicates even though the rules were not fully added. The function should either delete the hash entries for partially processed rules on failure, or document that partial failures require reconfiguration.

**Queue pair setup can fail silently if `priv->qps` is NULL (patch 2)**
In `hs_regex_qp_setup()`, the function checks `if (priv->qps == NULL)` and logs an error, then returns `-EINVAL`. However, earlier in the function, `qp_id` bounds checking assumes `priv->nb_queue_pairs` is the limit. If `configure()` failed and set `nb_queue_pairs` to 0 (as happens on import failure in patch 2), but left a stale `qps` pointer (though the current code frees it), or if `configure()` was never called (`nb_queue_pairs` is 0, `qps` is NULL), the bounds check `if (qp_id >= priv->nb_queue_pairs)` will reject `qp_id=0` because `nb_queue_pairs=0`, but the error message says "max 0" which is confusing. The NULL check should come before the bounds check to provide a clearer error message.

**Scratch allocation rollback on partial failure does not free the database (patch 3)**
In `hs_regex_rule_db_compile_activate()`, if scratch allocation fails for QP `i`, the function unwinds by freeing scratch for QPs `0..i-1`, then calls `hs_free_database(priv->db)` and sets `priv->db = NULL`, returning `-ENOMEM`. This is correct. However, in `hs_regex_rule_db_import()`, the same rollback pattern is used: free scratch for `0..i-1`, free the database, set `priv->db = NULL`. Both are correct. No issue here.

**`db_compiled` not reset on database free in `configure()` (patch 2)**
In `hs_regex_configure()`, when freeing an existing database:
```c
if (priv->db) {
    hs_free_database(priv->db);
    priv->db = NULL;
    priv->db_compiled = 0;
}
```
This correctly resets `db_compiled`. No issue here.

**Missing NULL check on `qp_conf` parameter (patch 2)**
In `hs_regex_qp_setup()`, the code reads `qp_conf->nb_desc` without checking if `qp_conf` is NULL:
```c
nb_desc = (qp_conf && qp_conf->nb_desc) ? qp_conf->nb_desc :
                       HS_REGEX_DEFAULT_NB_DESC;
```
This is safe--the code uses the ternary operator to check `qp_conf` before dereferencing. No issue here.

**Auto-start in enqueue_burst without explicit start (patch 7)**
In `hs_regex_enqueue_burst()`, if `dev_state != STARTED` but `db_compiled` is true, the code sets `dev_state = STARTED` and logs a notice. This auto-start behavior is not documented in the API and bypasses the application's explicit control over the device lifecycle. If the application expects to call `rte_regexdev_start()` after configuring additional settings, this silent auto-start may cause incorrect behavior. Either remove the auto-start (require explicit start), or document it clearly in the .rst file as a PMD-specific behavior.

### Warnings

**Missing release notes entry for xstats (patch 5)**
Patch 5 adds per-queue-pair xstats but does not update `doc/guides/rel_notes/release_26_11.rst` to mention this feature. While not strictly required for an internal implementation detail, xstats are user-visible and worth documenting.

**Hardcoded scratch allocation in queue_pair_setup (patch 2)**
In `hs_regex_qp_setup()`, scratch is allocated via `hs_alloc_scratch(priv->db, &qp->scratch)` only if `priv->db` exists at the time of setup. If the database is compiled or imported after queue pair setup, the scratch remains NULL, causing enqueue to fail with "qp %u has no scratch". The documentation should clarify that queue pair setup must follow database compilation, or `qp_setup` should be callable multiple times to refresh scratch when the database changes.

**Queue pair setup does not validate `nb_desc` upper bound before rounding (patch 2)**
The code rounds `nb_desc` up to the next power of two, capped at `HS_REGEX_MAX_NB_DESC`. If the user passes `nb_desc > HS_REGEX_MAX_NB_DESC`, it is silently capped:
```c
if (aligned > HS_REGEX_MAX_NB_DESC)
    aligned = HS_REGEX_MAX_NB_DESC;
```
This is acceptable, but the warning log only mentions rounding to power-of-two, not capping. Consider logging when capping occurs.

**Missing documentation of thread-safety model at device level (patch 4)**
The code documents single-producer/single-consumer per queue pair, but does not clarify whether concurrent calls to `configure()`, `start()`, `stop()`, etc., from different threads are safe. If they are not, state this explicitly in the .rst file.

**`dev_dump` does not print queue pair IDs or per-QP stats (patch 6)**
The dump function prints aggregated enqueued/dequeued/matches but does not print per-QP stats. For debugging, per-QP breakdowns are more useful than aggregates. Consider adding a loop to print per-QP stats.

**Missing bounds check on match array write (patch 4)**
In `hs_match_cb()`:
```c
if (op->nb_matches < ctx->max_matches) {
    struct rte_regexdev_match *m = &op->matches[op->nb_matches];
    ...
}
```
This assumes `op->matches` is allocated for at least `ctx->max_matches` entries. If the application allocated fewer, this is an out-of-bounds write. However, this is an application bug (API contract), not a PMD bug. The PMD cannot validate the application's allocation. No issue here.

**Enqueue processes `nb_ops` even when ring is full (patch 4)**
The code clamps `nb_ops` to `free_space`:
```c
free_space = qp->nb_desc - qp->count;
if (nb_ops > free_space)
    nb_ops = free_space;
```
Then loops over `nb_ops`. If `free_space` is 0, the loop does nothing and returns 0. This is correct--enqueue honors backpressure. No issue here.

**Dequeue does not clear pointers after consuming ops (patch 4)**
The dequeue loop copies `qp->ops[qp->head]` into `ops[i]` then advances `qp->head`. It does not zero `qp->ops[qp->head]` after copying. This is safe (ring reuse), but if a stale op pointer remains in the ring and the application frees the op, a later enqueue could overwrite the stale pointer. Since enqueue always writes before advancing tail, and dequeue consumes in order, the stale pointer is never read. No issue here.

**Close does not check if queues are drained (patch 6)**
`hs_regex_close()` frees QP resources without checking if `qp->count > 0`. If the application closes the device while ops are pending, those ops are leaked (not returned to the application). Document that the application must drain all queues before close, or add a warning log if `qp->count > 0`.

**Missing validation of `rule_db_len` upper bound (patch 3)**
In `hs_regex_rule_db_import()`, the code checks `if (!rule_db || rule_db_len == 0)` but does not check if `rule_db_len` is absurdly large. Hyperscan's `hs_deserialize_database()` may allocate memory proportional to `rule_db_len`. Consider capping `rule_db_len` or checking for overflow when casting to `size_t`.

**Missing Doxygen for PMD-private flags (patch 7)**
The header defines `HS_REGEX_RULE_SINGLEMATCH_F` etc. but provides no Doxygen comments. These flags are part of the PMD's user-facing API (applications must know about them to use them). Add Doxygen.

**`hs_regex_dev_destroy()` calls `hs_regex_close()` which may log errors (patch 6)**
In `hs_regex_dev_destroy()`, the code calls `hs_regex_close(dev)` before freeing `priv`. If `close()` is called on a device that was never started, it logs `"Device not started, cannot stop"` when trying to stop. However, `close()` handles the `STARTED` state by calling `stop()` first, and for other states, `stop()` returns an error. The code in `close()` only calls `stop()` if `dev_state == STARTED`, so it will not log the "cannot stop" error for `CONFIGURED` or `CREATED` states. No issue here.

**Enqueue does not check mbuf refcnt before linearizing (patch 4)**
`rte_pktmbuf_linearize(mbuf)` may fail if the mbuf is cloned (refcnt > 1) or if the first segment is too small. The code checks the return value and sets `RTE_REGEX_OPS_RSP_RESOURCE_LIMIT_REACHED_F` on failure, which is correct. No issue here.

**Missing check for zero-length patterns after trimming (patch 3)**
In `hs_regex_rule_db_update()`, the code checks `if (!rules[i].pcre_rule || rules[i].pcre_rule_len == 0)` before allocation. However, it does not trim trailing whitespace or validate that the pattern is non-empty after copying. Hyperscan may accept zero-length patterns if `HS_FLAG_ALLOWEMPTY` is set, so this is not necessarily an error. No issue here.

**`rule_db_export` does not document alignment requirements (patch 3)**
The function copies the serialized database to `rule_db` via `memcpy()`. If Hyperscan requires the buffer to be aligned (e.g., for mmap), this is not documented. Check Hyperscan documentation for alignment requirements.

### Info

**Consider using `rte_malloc_socket()` for NUMA-aware allocations**
Allocations like `priv->rules`, `priv->qps`, and per-QP scratch could use `rte_malloc_socket()` with the device's socket ID for better NUMA locality. Currently they use `SOCKET_ID_ANY`.

**Queue pair ring uses modulo arithmetic with power-of-two size**
The code ensures `nb_desc` is a power of two, then uses `% nb_desc` in head/tail arithmetic. This is correct, but `& (nb_desc - 1)` would be faster (single AND vs. division). Since the code already rounds to power-of-two, consider using bitmask instead of modulo.

**Enqueue processes ops one at a time without batching**
Each op triggers a separate `hs_scan()` call. If Hyperscan supports batch scanning (multiple buffers in one call), batching could improve throughput. Check Hyperscan API for batch support.

**`hs_match_cb` increments `total_matches` even when match array is full**
This is intentional--`total_matches` tracks the actual match count for statistics, while `nb_matches` is capped by `max_matches`. The documentation correctly notes that `nb_actual_matches` reflects the true count. No issue here.

**`hs_regex_info_get` does not use `dev` parameter**
The function casts `dev` to `(void)dev` to suppress unused warnings. Since `info_get` is stateless, this is acceptable. However, if per-device capabilities differ in the future (e.g., based on Hyperscan version), the function should read from `priv`. No issue now.

**Missing documentation of serialized database portability**
The .rst file mentions import/export but does not document that serialized Hyperscan databases are tied to the CPU architecture and Hyperscan version. Warn users not to use serialized databases across different systems or Hyperscan versions.

**`hs_regex_dump` prints all rules without pagination**
If `nb_rules` is 1,000,000, this will produce a very long dump. Consider printing only a summary (first N rules, total count) or adding a parameter to limit output.

## Patch 2/7: Configure and Queue Pair Setup

### Errors

**Missing rollback of `max_matches` and `nb_groups` on import failure**
As noted in Patch 1 review, if `hs_regex_rule_db_import()` fails after `configure()` has set `priv->max_matches` and `priv->nb_groups`, these fields are not reset when rolling back to `CREATED` state. Add:
```c
priv->max_matches = 0;
priv->nb_groups = 0;
```
before `return ret;` in the failure path.

**Queue pair setup does not validate `qp_id` against `nb_queue_pairs` before checking `qps` NULL**
The bounds check `if (qp_id >= priv->nb_queue_pairs)` assumes `nb_queue_pairs` is valid, but if `configure()` never succeeded (or failed and set `nb_queue_pairs = 0`), the bounds check produces a confusing error. Reorder checks:
```c
if (priv->qps == NULL) {
    HS_LOG(ERR, "qp_setup: device not configured");
    return -EINVAL;
}
if (qp_id >= priv->nb_queue_pairs) {
    ...
}
```

## Patch 3/7: Rule Database Update and Compilation

### Errors

**Hash table created inside loop without rollback on subsequent failure**
In `hs_regex_rule_db_update()`, if the hash table is created successfully after processing rule 0, but rule 1 fails (e.g., pattern allocation), the function returns 1, leaving the hash table allocated but only