feat(nfapi): OAI nFAPI Delay Management implementation#242
Conversation
|
CI Build: #529 | Not performing CI due to the absence of one of the following mandatory labels:
|
There was a problem hiding this comment.
Pull request overview
This PR adds OAI nFAPI Delay Management for NR, including a VNF autonomous timing thread and updated PNF/VNF timing/jitter reporting, and also improves stability around PRACH handling and thread naming.
Changes:
- Implement VNF-side autonomous timing thread and EWMA-based slot-ahead delay controller driven by
TIMING_INFO. - Add PNF-side RFC3550-style jitter calculation and richer timing window checks; extend NR timing info fields (including renames for TX_DATA timing/jitter fields).
- Improve PRACH segmented packet handling in the O-RAN FHI path and relax hard asserts on queue-length conditions during delay situations.
Reviewed changes
Copilot reviewed 20 out of 20 changed files in this pull request and generated 9 comments.
Show a summary per file
| File | Description |
|---|---|
| radio/fhi_72/oaioran.c | Adds PRACH segmentation support during PRACH read/decompression and relaxes strict asserts on delay conditions. |
| nfapi/open-nFAPI/vnf/src/vnf_p7.c | Adds NR timing extraction, EWMA delay-management controller, and wrap-safe time computations; refactors UL node sync handling. |
| nfapi/open-nFAPI/vnf/src/vnf_p7_interface.c | Adjusts socket IP_TOS and initializes new timing/mutex fields for P7 connection nodes. |
| nfapi/open-nFAPI/vnf/src/vnf_interface.c | Uses user-configured timing parameters instead of hardcoded defaults during PHY allocation. |
| nfapi/open-nFAPI/vnf/public_inc/nfapi_vnf_interface.h | Widens timing_window type and adds timing config fields to VNF config. |
| nfapi/open-nFAPI/vnf/inc/vnf_p7.h | Introduces delay-management constants/state, mutex/condvar fields, and timing stats types. |
| nfapi/open-nFAPI/pnf/src/pnf_p7.c | Implements RFC3550 jitter tracking, wrap-safe time diffs, timing-window enforcement, and periodic/aperiodic timing-info send logic changes. |
| nfapi/open-nFAPI/pnf/src/pnf_p7_interface.c | Initializes PNF config defaults for timing-info sending and improves cleanup of allocated buffers. |
| nfapi/open-nFAPI/pnf/public_inc/nfapi_pnf_interface.h | Adds per-message receive timestamps into the PNF slot buffer for timing calculations. |
| nfapi/open-nFAPI/pnf/inc/pnf_p7.h | Adds jitter state, timing trigger fields, and buffer sizing constants for PNF P7 processing. |
| nfapi/open-nFAPI/nfapi/src/nfapi_nr_p7.c | Updates packing/unpacking for renamed NR timing-info TX_DATA fields. |
| nfapi/open-nFAPI/nfapi/public_inc/nfapi_nr_interface_scf.h | Renames NR timing-info TX_DATA fields to match updated pack/unpack and usage. |
| nfapi/open-nFAPI/nfapi/public_inc/nfapi_interface.h | Widens NR timing_window TLV from uint8 to uint16. |
| nfapi/open-nFAPI/fapi/src/nr_fapi_p5.c | Packs/unpacks widened timing-window TLV and adds TLVs for per-message timing offsets. |
| nfapi/oai_integration/socket/socket_vnf.c | Sets IP_TOS for VNF socket to prioritize timing-sensitive traffic. |
| nfapi/oai_integration/socket/socket_pnf.c | Allocates RX/reassembly buffers and sets IP_TOS for PNF socket; adjusts slot handling behavior. |
| nfapi/oai_integration/nfapi_vnf.h | Widens timing window types and adds timing offset configuration fields. |
| nfapi/oai_integration/nfapi_vnf.c | Adds autonomous VNF timing thread, timing-info configuration wiring, offset TLV wiring, and adjusts message ordering. |
| nfapi/oai_integration/nfapi_pnf.c | Wires timing offsets and timing-info configuration from VNF into PNF P7 behavior; gates slot indications under WLS. |
| common/utils/system.c | Avoids aborting on pthread_setname_np() failure; logs error instead. |
| uint64_t pnf_timehr_to_us(pnf_p7_t* pnf_p7, uint32_t time_hr) | ||
| { | ||
| uint32_t sec = TIMEHR_SEC(time_hr); | ||
| uint32_t usec = TIMEHR_USEC(time_hr); | ||
| // Convert to 64-bit microseconds (relative, will wrap at 4096 seconds) | ||
| return (uint64_t)sec * 1000000ULL + (uint64_t)usec; | ||
| } |
There was a problem hiding this comment.
Resolved — pnf_timehr_to_us() explicitly discards the unused parameter with (void)pnf_p7;, so there's no -Wunused-parameter warning.
| /* Function Declaration */ | ||
| // Extract timing info points from a timing_info message | ||
| // Returns the number of valid stats extracted (0-8) | ||
| int vnf_nr_extract_timing_info(const nfapi_nr_timing_info_t *ind, | ||
| nfapi_vnf_p7_connection_info_t *p7_info, | ||
| vnf_timing_stats_t *out_stats); |
There was a problem hiding this comment.
Resolved — the comment now reads "Returns 1 if at least one valid timing sample was extracted, 0 otherwise", which matches the implementation.
| static int32_t ceil_div_pos_i32(int32_t num, int32_t den) | ||
| { | ||
| if (den <= 0) | ||
| return 0; | ||
|
|
||
| if (num <= 0) | ||
| return 0; | ||
|
|
||
| return (num + den - 1) / den; | ||
| } | ||
|
|
||
| static int32_t abs_i32(int32_t v) | ||
| { | ||
| return v < 0 ? -v : v; | ||
| } | ||
| static inline int32_t p7_max_i32(int32_t a, int32_t b) | ||
| { | ||
| return a > b ? a : b; | ||
| } |
There was a problem hiding this comment.
Resolved — ceil_div_pos_i32() and p7_max_i32() are marked __attribute__((unused)), so no -Wunused-function warning.
There was a problem hiding this comment.
why don't you remove the altogether instead?
| static inline int p7_ewma_effectively_zero_i32( | ||
| int32_t value, | ||
| int32_t denom) | ||
| { |
There was a problem hiding this comment.
Resolved — p7_ewma_effectively_zero_i32() is marked __attribute__((unused)).
There was a problem hiding this comment.
as above -- rather remove that code if not used.
| pthread_mutex_lock(&p7_con->mutex); | ||
| p7_con->timing_info_accum_count = 0; | ||
| p7_con->timing_info_accum_worst_late = INT32_MIN; | ||
| pthread_mutex_unlock(&p7_con->mutex); | ||
|
|
||
| vnf_nr_delay_management(p7_con, &aggregated_stats); |
There was a problem hiding this comment.
The EWMA state and last-adjustment markers here are only accessed inside vnf_nr_delay_management on the timing-info handler thread, so they aren't shared. The one field shared with the autonomous timing thread, slot_ahead, is now accessed atomically on both sides (__atomic_load/__atomic_store, relaxed), so no lock is needed.
There was a problem hiding this comment.
dangerous -- please put the lock. You make the assumption that not something else worth protecting would be added later. So unless there is a really good reason, just leave it inside.
There was a problem hiding this comment.
Agreed, thanks. Reverted to the mutex: vnf_nr_delay_management runs under p7_con->mutex, and the timing thread now reads slot_ahead under the same lock — so any field added here stays protected.
| p7_info->sync_slot_counter++; | ||
| } | ||
|
|
||
| int target_ind_dec = (sfnslot_dec + p7_info->slot_ahead) % max_sfnslotdec; |
There was a problem hiding this comment.
This read is an atomic load (__atomic_load_n(&p7_info->slot_ahead, __ATOMIC_RELAXED)), and the writes in vnf_nr_delay_management are now atomic too.
| nfapi_vnf_p7_connection_info_t *p7_info = NULL; | ||
|
|
||
| while (1) { | ||
| if (nr_start_resp_received) { |
There was a problem hiding this comment.
This is no longer volatile — the poll uses __atomic_load_n(&nr_start_resp_received, __ATOMIC_ACQUIRE).
| int nr_start_resp_cb(nfapi_vnf_config_t *config, int p5_idx, nfapi_nr_start_response_scf_t *resp) { | ||
| UNUSED(config); | ||
| NFAPI_TRACE(NFAPI_TRACE_INFO, "[VNF] Received NFAPI_START_RESP idx:%d phy_id:%d\n", p5_idx, resp->header.phy_id); | ||
| nr_start_resp_received = 1; |
There was a problem hiding this comment.
And the store pairs with __atomic_store_n(&nr_start_resp_received, 1, __ATOMIC_RELEASE).
8fc8c91 to
44a7d01
Compare
|
CI Build: #530 | Not performing CI due to the absence of one of the following mandatory labels:
|
44a7d01 to
d60ebf6
Compare
29b1323 to
54e3c17
Compare
| int iptos_value = 0; | ||
| int iptos_value = 184; |
There was a problem hiding this comment.
Where does this value of 184 come from?
If it's supposed to be 'IPTOS_DSCP_EF', include <netinet/ip.h> and use the #define.
If it's not, just add a small comment on what '184' is doing.
There was a problem hiding this comment.
Done — replaced the magic 184 with IPTOS_DSCP_EF; the macro is provided by <netinet/ip.h>, now included via the shared socket_common.h.
|
|
||
| // configure the UDP socket options | ||
| int iptos_value = 0; | ||
| int iptos_value = 184; |
There was a problem hiding this comment.
Same as above, you can even include <netinet/ip.h> in socket_common.h, since it would be used by both VNF and PNF
There was a problem hiding this comment.
Done — as suggested, #include <netinet/ip.h> is now in socket_common.h (shared by both VNF and PNF), and this uses IPTOS_DSCP_EF.
| if (sched_response.TX_req.Number_of_PDUs > 0) | ||
| oai_nfapi_tx_data_req(&sched_response.TX_req); | ||
|
|
||
| if (sched_response.DL_req.dl_tti_request_body.nPDUs > 0) | ||
| oai_nfapi_dl_tti_req(&sched_response.DL_req); | ||
|
|
||
| if (sched_response.UL_tti_req.n_pdus > 0) | ||
| oai_nfapi_ul_tti_req(&sched_response.UL_tti_req); | ||
|
|
||
| if (sched_response.TX_req.Number_of_PDUs > 0) | ||
| oai_nfapi_tx_data_req(&sched_response.TX_req); | ||
|
|
There was a problem hiding this comment.
Is there a reason to change the message order here?
in SCF222.10.04 Figure 2-21 you can see it is in the order that was defined, and in SCF225.3.0 Section 2.1.3.5 it states that the message order defined in FAPI also applies for nFAPI.
There was a problem hiding this comment.
You're right. This reordering was a leftover from an earlier experiment while validating the timing approach and isn't actually needed — I've reverted it to the SCF spec order (DL_TTI, then UL_TTI, then TX_DATA last). Same behaviour, fewer changes.
| p7_vnf->config->send_p7_msg = &vnf_nr_send_p7_msg; | ||
| NFAPI_TRACE(NFAPI_TRACE_INFO, "[VNF] Creating VNF NFAPI P7 start thread %s\n", __FUNCTION__); | ||
| threadCreate(&vnf_p7_start_pthread, &vnf_nr_start_p7_thread, p7_vnf->config, "vnf_p7_thread", -1, OAI_PRIORITY_RT); | ||
| threadCreate(&vnf_p7_start_pthread, &vnf_nr_start_p7_thread, p7_vnf->config, "vnf_p7_thread", 14, OAI_PRIORITY_RT); |
There was a problem hiding this comment.
You should avoid pinning to specific cores, if it's really needed, add a new parameter to the config file for the use to be able to set the core to pin to ( default -1 ), similarly to the pinning of the nvIPC thread.
There was a problem hiding this comment.
I removed the core pinning entirely rather than making it configurable: testing showed the default (no pin, -1) behaves the same, since the process is already CPU-isolated at the deployment level. So no config parameter is needed — the thread is created with -1 (upstream default) and the branch no longer touches openair2/.
| #ifndef ENABLE_WLS | ||
| // Start VNF autonomous timing thread | ||
| pthread_t t; | ||
| threadCreate(&t, &vnf_timing_thread, p7_vnf, "vnf_timing", 15, OAI_PRIORITY_RT_MAX); |
There was a problem hiding this comment.
Same as above, don't pin to a specific core.
There was a problem hiding this comment.
Same — this thread is now created with -1 (no pinning).
| if(p7_vnf->thread_started == 0) { | ||
| pthread_t vnf_p7_thread; | ||
| threadCreate(&vnf_p7_thread, &configure_nr_p7_vnf, p7_vnf, "vnf_p7_thread", -1, OAI_PRIORITY_RT); | ||
| threadCreate(&vnf_p7_thread, &configure_nr_p7_vnf, p7_vnf, "vnf_p7_thread", 14, OAI_PRIORITY_RT); |
| */ | ||
|
|
||
| int iptos_value = 0; | ||
| int iptos_value = 184; |
There was a problem hiding this comment.
As above, you can include <netinet/ip.h> and use IPTOS_DSCP_EF
There was a problem hiding this comment.
Done — replaced with IPTOS_DSCP_EF and added #include <netinet/ip.h> in this file.
f46c8dd to
8e513e1
Compare
|
Validation: 1687346, c9bb67a, 00019ca, 3b47305, dfc4a75, 567d0e6, fdcf301, 1faf9fe, eed749e, 41491f7, c16fd2a, 8e513e1 Please use 'git commit -S' to sign your commits. |
b45cfd6 to
0716848
Compare
|
CI Build: #668 | Failed on the following stages: |
pthread_setname_np() can fail (observed ret 2 / ENOENT) on some setups; the previous AssertFatal aborted the whole softmodem during bring-up. Log the error (LOG_E) and continue instead so a thread-name failure is reported but non-fatal. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Shouldn't have duplicate slot increment at PNF side Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Rename tx_data_request_* timing fields to tx_data_* in NR timing-related structures and handling paths. This removes naming ambiguity and aligns with current SCF 225 terminology used by timing info exchange. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
0716848 to
ab9048c
Compare
|
CI Build: #688 | Failed on the following stages: |
|
Hi Robert @rorsc , I have a question. I find it quite strange that without modifying the code, sometimes the build succeeds and sometimes it fails. Also, what optimizations or changes do I still need to make in order to be considered for merging into the develop branch? So far, the most successful test result I’ve had is this one #691 which passed the highest number of checks. |
The Gitlab of Eurecom has a problem, which seems to be high crawler traffic (maybe for AI, but who knows). So while oai is on duranta in github, some parts are still on Gitlab (e.g., flexric). If you inspect the logs, you'll see that it typically fails on getting something from gitlab. The Eurecom IT has been informed but was unable to resolve the problems yet. For FlexRIC specifically, we are in the process of migrating it to duranta for that very reason, see #277. Apart from that, a lot of tests run over the air, so you can always have interference, or stray UEs connecting, or other problems. Some CI tests are also a bit flaky. See #72. |
SCF 225 allows timing_window up to 30000us, which exceeds uint8 capacity. Promote timing_window-related fields and pack/unpack paths to uint16 so configuration values are represented consistently across VNF/PNF interfaces. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Add DL_TTI/UL_TTI/UL_DCI/TX_DATA per-message timing-offset TLVs to the PARAM response and CONFIG request (pack/unpack in nr_fapi_p5.c, PNF/VNF config structs) so the VNF can tell the PNF how far ahead each message type is sent. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
9276b73 to
e3be287
Compare
96f167a to
634ea03
Compare
|
CI Build: #781 | Failed on the following stages: |
634ea03 to
c30e973
Compare
|
CI Build: #789 | Failed on the following stages: |
Measure per-message P7 arrival timing against the expected window (timing check, slot-diff, in-window tests), accumulate worst-case late/early margins and jitter per message type, and pack them into the timing-info report (pnf_nr_pack_and_send_timing_info). Reset slot_start on start/stop and count late DL/UL/DCI/TX requests. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Pre-allocate an rx message buffer and a reassembly buffer (sizes in pnf_p7.h) and reassemble P7 messages fragmented at MTU boundaries in the message pump; free the buffers on config destroy. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Parse the PNF timing-info report (vnf_nr_handle_timing_info / vnf_nr_extract_timing_info) and aggregate worst-late and jitter across message types into per-connection state for the pacing logic to consume. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Add a self-paced VNF timing thread that derives the numerology, sleeps to each slot boundary (clock_nanosleep), publishes sfn/slot under lock and issues MAC slot indications, catching up in bounded bursts under load and yielding on extreme lag. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Add UL node-sync handling (vnf_nr_handle_ul_node_sync) with proportional micro-steering of the slot phase and a drift monitor, plus the slot-time helpers used to compute the dynamic sleep target. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Expose timing_window and periodic/aperiodic timing mode and period through the VNF config, add tx_data connection routing and set the RT thread priorities used to set up the P7 timing path. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Add vnf_nr_delay_management(): an EWMA controller that tracks mean lateness and jitter deviation from the aggregated timing info and adjusts slot_ahead (increase by ceil((EWMA+Dev)/slot_dur) when late, decrease one slot with >=4-sigma early margin), gated to one timing_info_period, wired into vnf_nr_handle_timing_info. Without this commit the pipeline still runs, with a fixed non-adaptive slot_ahead. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Cast byte elements to uint32_t before left-shifting in pulls32() to prevent undefined behavior when the result sets the sign bit of a 32-bit signed integer. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
Track the timing thread handle and add terminate checks at all blocking points (initial poll loop, cond_wait, main loop). Introduce stop_vnf_timing_thread() helper called from both stop_nr_nfapi_vnf() and pnf_disconnection_indication_cb() to ensure the timing thread is joined before the P7 connection info is freed. Prevents heap-use-after-free crash when PNF disconnects or softmodem exits while the timing thread is still running. Signed-off-by: Ming-Hong HSU, BMW Lab@NTUST <m11302209@gapps.ntust.edu.tw>
c30e973 to
bc293fd
Compare
|
CI Build: #793 | Failed on the following stages: |
This Pull Request introduces the clean, production-ready OAI nFAPI Delay Management features (VNF autonomous timing thread, adaptive EWMA delay-management controller, etc.) along with the NULL pointer stability bugfix.