Xilinx-CNS / tcpdirect

AMD TCPDirect ultra low latency kernel bypass TCP and UDP implementation for AMD Solarflare network adapters, to be used with corresponding versions of Onload®️ at https://github.com/Xilinx-CNS/onload. The stable branch is currently `v8_1`.
22 stars 17 forks source link

ON-15400: allowing setting log_level and log_format #8

Closed andresa-xilinx closed 3 days ago

andresa-xilinx commented 7 months ago

Originally, the customer reported that log_level and log_format could not be set using zf_attr_set_int(). They requested that an error be returned instead of allowing the operation to seemingly succeed without effect.

Upon inspecting the zf_attr_set_int and zf_attr_set_str code, as well as the unit test zfattrs.c, we found that the attributes log_level, log_format, and log_file never worked as intended. These are global attributes that were not only impossible to set programmatically but also kept resetting every time a new set of attributes was allocated within the app. Additionally, when zf_attrget was used, the local attributes within the set and the corresponding zflog variables were not synchronized.

The unit test did not show the expected headers and logs when the attributes were set in the set_test_attrs function in zfattrs.c. I have added specific tests to guarantee synchronization between local and global attributes across different instantiations of new attribute sets.

We see no reason to forbid the programmatic initialization of these parameters using zf_attr_set_int(). The existing unit test (zfattrs.c) works as expected after applying this patch.

andresa-xilinx commented 7 months ago

Before applying the patch:

andresa@sup-cap1:~/onload-patches/tcpdirect$  ZF_ATTR="emu=2;interface=enp129s0f0" ./build/gnu_x86_64-zf-devel/bin/zf_unit/zfattrs
1..100
# attr test bitmask 0x20cca
ok 1 - Data sent
ok 2 - Correct data received
# attr test bitmask 0x5ab26
ok 3 - Data sent
ok 4 - Correct data received
# attr test bitmask 0x323f4
ok 5 - Data sent
ok 6 - Correct data received
# attr test bitmask 0x38dbd
ok 7 - Data sent
ok 8 - Correct data received
# attr test bitmask 0x6927f
ok 9 - Data sent
ok 10 - Correct data received
# attr test bitmask 0x1a22a
ok 11 - Data sent
ok 12 - Correct data received
# attr test bitmask 0x2cfd4
ok 13 - Data sent
ok 14 - Correct data received
# attr test bitmask 0x4b20
ok 15 - Data sent
ok 16 - Correct data received
# attr test bitmask 0x50269
ok 17 - Data sent
ok 18 - Correct data received
# attr test bitmask 0x3348e
ok 19 - Data sent
ok 20 - Correct data received
# attr test bitmask 0x71b2f
ok 21 - Data sent
ok 22 - Correct data received
# attr test bitmask 0x1bf39
ok 23 - Data sent
ok 24 - Correct data received
# attr test bitmask 0x293d3
ok 25 - Data sent
ok 26 - Correct data received
...
# attr test bitmask 0x4627a
ok 95 - Data sent
ok 96 - Correct data received
# attr test bitmask 0xc369
ok 97 - Data sent
ok 98 - Correct data received
# attr test bitmask 0x5620d
ok 99 - Data sent
ok 100 - Correct data received
andresa-xilinx commented 7 months ago

After applying the patch, depending on the random seed, it'd output the first part of the tests either to stderr or to file_0.log (which can be found in the most recently created directory inside /tmp).

If first part of the test goes to file_0.log then output looks as follows.

andresa@sup-cap2:~/git/tcpdirect$ ZF_ATTR="emu=2;interface=enp129s0f0" ./build/gnu_x86_64-zf-devel/bin/zf_unit/zfattrs
1..100
# attr test bitmask 0x661b8
ok 1 - Data sent
ok 2 - Correct data received
ok 3 - Expected behaviour after allocating new set of attributes: log_file_name has not changed.
ok 4 - Expected behaviour after allocating new set of attributes: log_level has not changed.
ok 5 - Expected behaviour after allocating new set of attributes: log_format has not changed.
ok 6 - Data sent
ok 7 - Correct data received
ok 8 - Expected behaviour after duplicating attributes: log_file_name has not changed.
ok 9 - Expected behaviour after duplicating attributes: log_level has not changed.
ok 10 - Expected behaviour after duplicating attributes: log_format has not changed.
ok 11 - Data sent
ok 12 - Correct data received
# attr test bitmask 0x8b625
ok 13 - Data sent
ok 14 - Correct data received
# attr test bitmask 0x95d57
ok 15 - Data sent
ok 16 - Correct data received
...
# attr test bitmask 0x66c19
ok 95 - Data sent
ok 96 - Correct data received
# attr test bitmask 0x4b870
ok 97 - Data sent
ok 98 - Correct data received
# attr test bitmask 0xfc16c
ok 99 - Data sent
ok 100 - Correct data received
# attr test bitmask 0x5a44c
ok 101 - Data sent
ok 102 - Correct data received
# attr test bitmask 0xaf152
ok 103 - Data sent
ok 104 - Correct data received

If the output goes to stderr it looks as follows. Note the messages indicating expected values (ok # ...) are interleaved with a very verbose tcpdirect output (see towards the end in the example for ok 1 and ok 2) .

andresa@sup-cap2:~/git/tcpdirect$ ZF_ATTR="emu=2;interface=enp129s0f0" ./build/gnu_x86_64-zf-devel/bin/zf_unit/zfattrs
1..100
# attr test bitmask 0x450c7
[zfattrs:196446]      3182509612261422   1000 | rxq_size=511 txq_size=511 evq_size=1014 rx_prefix_len=0
[zfattrs:196446]      3182509612733737 enp129s0f0/000  1000 | TCPL#00 | zftl_listen:
[zfattrs:196446]      3182509612815831 enp129s0f0/000  1000 | TCP#00 | zf_tcp_acquire
[zfattrs:196446]      3182509612866767 enp129s0f0/000  1000 | TCP#00 | zf_tcp_acquire
[zfattrs:196446]      3182509612901419 enp129s0f0/000  1000 | tcp_bind: bind to port 3001
[zfattrs:196446]      3182509612931540 enp129s0f0/000  1000 | tcp_connect to port 4001
[zfattrs:196446]      3182509612958255 enp129s0f0/000  1000 | TCP#00 | tcp_enqueue_flags: queuelen: 0
[zfattrs:196446]      3182509612996085 enp129s0f0/000  1000 | TCP#00 | tcp_enqueue_flags: queueing seg 0x7f25e8817580 2587162562:2587162563 pkt 200 SYN
[zfattrs:196446]      3182509613036588 enp129s0f0/000  1000 | TCP#00 | tcp_enqueue_flags: queuelen 1(after enqueued)
[zfattrs:196446]      3182509613071973 enp129s0f0/000  1000 | TCP#00 | tcp_do_transition: old_state 0x80 new_state 0x4, flags 0
[zfattrs:196446]      3182509613109143 enp129s0f0/000  1000 | zf_stack_busy_ref: busy_refcount = 1
[zfattrs:196446]      3182509613142443 enp129s0f0/000  1000 | TCP#00 | tcp_output: snd_right_edge 2587228097, cwnd 1, eff_right_edge 2587162563, ack 2587162562
[zfattrs:196446]      3182509613178528 enp129s0f0/000  1000 | TCP#00 | tcp_output: seg 0x7f25e8817580 2587162562:2587162563 pkt 200
[zfattrs:196446]      3182509613215134 enp129s0f0/000  1000 | TCP#00 | tcp_output: snd_right_edge 2587228097, cwnd 1, eff_right_edge 2587162563, seq 2587162562, lastack 2587162562, seg 0x7f25e8817580 2587162562:2587162563 pkt 200
[zfattrs:196446]      3182509613252925 enp129s0f0/000  1000 | TCP#00 | tcp_output_timers_common: rtseq 2587162562
[zfattrs:196446]      3182509613289528 enp129s0f0/000  1000 | TCP#00 | tcp_output_segment: seg 0x7f25e8817580 2587162562:2587162563 pkt 200
[zfattrs:196446]      3182509613337600 enp129s0f0/000  1000 | TCPL#00 | tcp_listen_input:
[zfattrs:196446]      3182509613373082 enp129s0f0/000  1000 | TCPL#00 | tcp_listen_input: TCP connection request 3001 -> 4001.
[zfattrs:196446]      3182509613410228 enp129s0f0/000  1000 | TCP#01 | zf_tcp_acquire
[zfattrs:196446]      3182509613444525 enp129s0f0/000  1000 | TCP#01 | tcp_do_transition: old_state 0x80 new_state 0x8, flags 0
[zfattrs:196446]      3182509613481897 enp129s0f0/000  1000 | zf_stack_busy_ref: busy_refcount = 2
[zfattrs:196446]      3182509613514661 enp129s0f0/000  1000 | TCP#01 | tcp_enqueue_flags: queuelen: 0
[zfattrs:196446]      3182509613549125 enp129s0f0/000  1000 | TCP#01 | tcp_enqueue_flags: queueing seg 0x7f25e8818140 2202796227:2202796228 pkt 202 SYN  ACK
[zfattrs:196446]      3182509613586416 enp129s0f0/000  1000 | TCP#01 | tcp_enqueue_flags: queuelen 1(after enqueued)
[zfattrs:196446]      3182509613622731 enp129s0f0/000  1000 | TCP#01 | tcp_output: snd_right_edge 2202861762, cwnd 1, eff_right_edge 2202796228, ack 2202796227
[zfattrs:196446]      3182509613657916 enp129s0f0/000  1000 | TCP#01 | tcp_output: seg 0x7f25e8818140 2202796227:2202796228 pkt 202
[zfattrs:196446]      3182509613694270 enp129s0f0/000  1000 | TCP#01 | tcp_output: snd_right_edge 2202861762, cwnd 1, eff_right_edge 2202796228, seq 2202796227, lastack 2202796227, seg 0x7f25e8818140 2202796227:2202796228 pkt 202
[zfattrs:196446]      3182509613729461 enp129s0f0/000  1000 | TCP#01 | tcp_output_timers_common: rtseq 2202796227
[zfattrs:196446]      3182509613763991 enp129s0f0/000  1000 | TCP#01 | tcp_output_segment: seg 0x7f25e8818140 2202796227:2202796228 pkt 202
[zfattrs:196446]      3182509613799567 enp129s0f0/000  1000 | TCP#00 | zf_stack_handle_tx_tcp: tx complete 200 idx 0 seg 0x7f25e8817580 2587162562:2587162563 pkt 200 in flight sendq 0-1-1
[zfattrs:196446]      3182509613836485 enp129s0f0/000  1000 | TCP#00 | zf_stack_handle_rx_tcp: seq 2202796227 (4) ack 746588224
[zfattrs:196446]      3182509613872688 enp129s0f0/000  1000 | TCP#00 | tcp_rx_flush
[zfattrs:196446]      3182509613909561 enp129s0f0/000  1000 | TCP#00 | tcp_output: snd_right_edge 2587228097, cwnd 1, eff_right_edge 2587162563, ack 2587162562
[zfattrs:196446]      3182509613944728 enp129s0f0/000  1000 | TCP#00 | tcp_input: state 0x4
[zfattrs:196446]      3182509613982164 enp129s0f0/000  1000 | TCP#00 | tcp_process: SYN-SENT: ackno 2587162563 pcb->snd_nxt 2587162563 unacked 2587162562
[zfattrs:196446]      3182509614020025 enp129s0f0/000  1000 | TCP#00 | tcp_do_transition: old_state 0x4 new_state 0x2, flags 0
[zfattrs:196446]      3182509614055667 enp129s0f0/000  1000 | TCP#00 | tcp_process: SYN-SENT --queuelen 0
[zfattrs:196446]      3182509614090546 enp129s0f0/000  1000 | TCP#00 | tcp_process: processed, current state 0x2
[zfattrs:196446]      3182509614129597 enp129s0f0/000  1000 | TCP#00 | tcp_send_empty_ack: sending ACK for 2202796228 rcv ann win 65535
[zfattrs:196446]      3182509614168516 enp129s0f0/000  1000 | TCP#01 | zf_stack_handle_tx_tcp: tx complete 202 idx 0 seg 0x7f25e8818140 2202796227:2202796228 pkt 202 in flight sendq 0-1-1
[zfattrs:196446]      3182509614206279 enp129s0f0/000  1000 | TCP#01 | zf_stack_handle_rx_tcp: seq 2587162563 (0) ack 746590336
[zfattrs:196446]      3182509614242722 enp129s0f0/000  1000 | TCP#01 | tcp_rx_flush
[zfattrs:196446]      3182509614292052 enp129s0f0/000  1000 | TCP#01 | tcp_output: snd_right_edge 2202861762, cwnd 1, eff_right_edge 2202796228, ack 2202796227
[zfattrs:196446]      3182509614329419 enp129s0f0/000  1000 | TCP#01 | tcp_input: state 0x8
[zfattrs:196446]      3182509614364352 enp129s0f0/000  1000 | TCP#01 | tcp_process: SYN-RCVD: ackno 2202796228 lastack 2202796227 pcb->snd_nxt 2202796228 unacked 2202796227
[zfattrs:196446]      3182509614400937 enp129s0f0/000  1000 | TCP#01 | tcp_do_transition: old_state 0x8 new_state 0x2, flags 0
[zfattrs:196446]      3182509614436852 enp129s0f0/000  1000 | TCP#01 | zf_tcp_acquire
[zfattrs:196446]      3182509614472682 enp129s0f0/000  1000 | TCP#01 | tcp_update_window: window update 2202796228+65535; snd_nxt=2202796228
[zfattrs:196446]      3182509614507673 enp129s0f0/000  1000 | TCP#01 | tcp_calculate_rtt_est: experienced rtt 1 ticks (90 msec).
[zfattrs:196446]      3182509614543940 enp129s0f0/000  1000 | TCP#01 | tcp_calculate_rtt_est: RTO 3 (270 milliseconds)
[zfattrs:196446]      3182509614579470 enp129s0f0/000  1000 | TCP#01 | tcp_handle_ack_common: pcb->rttest 0 rtseq 2202796227 ackno 2202796228
[zfattrs:196446]      3182509614613416 enp129s0f0/000  1000 | TCP#01 | tcp_handle_ack_new: slow start cwnd 1461
[zfattrs:196446]      3182509614649294 enp129s0f0/000  1000 | TCP#01 | tcp_handle_ack_new: ACK for 2202796228, has unacked
[zfattrs:196446]      3182509614685449 enp129s0f0/000  1000 | TCP#01 | tcp_handle_ack_new: unacked seg 0x7f25e8818140 2202796227:2202796228 pkt 202
[zfattrs:196446]      3182509614721925 enp129s0f0/000  1000 | TCP#01 | handle_acked_segment: removing seg 0x7f25e8818140 2202796227:2202796228 pkt 202 from pcb->unacked
[zfattrs:196446]      3182509614756894 enp129s0f0/000  1000 | TCP#01 | handle_acked_segment: queuelen 1 ...
[zfattrs:196446]      3182509614790919 enp129s0f0/000  1000 | TCP#01 | handle_acked_segment: 0 (after freeing acked seg)
[zfattrs:196446]      3182509614825922 enp129s0f0/000  1000 | TCP#01 | tcp_handle_ack_new: acked 1, new snd_buf 93440
[zfattrs:196446]      3182509614862122 enp129s0f0/000  1000 | TCP#01 | TCP connection established 3001 -> 4001.
[zfattrs:196446]      3182509614897319 enp129s0f0/000  1000 | TCP#01 | tcp_process (SYN_RCVD): cwnd 14600 ssthresh 65535
[zfattrs:196446]      3182509614933879 enp129s0f0/000  1000 | TCP#01 | tcp_process: processed, current state 0x2
[zfattrs:196446]      3182509614970270 enp129s0f0/000  1000 | TCP#01 | tcp_output: snd_right_edge 2202861763, cwnd 14600, eff_right_edge 2202810828, ack 2202796228
[zfattrs:196446]      3182509615007788 enp129s0f0/000  1000 | TCP#00 | tcp_write(pcb=0x7f25e8817500, data=0x7ffcac3804bc, len=4)
[zfattrs:196446]      3182509615046243 enp129s0f0/000  1000 | tcp_output_segment: 2587162563:2587162567 snd_right_edge 2587228098 pkt 2
ok 1 - Data sent
[zfattrs:196446]      3182509615090340 enp129s0f0/000  1000 | TCP#01 | zf_stack_handle_rx_tcp: seq 2587162563 (4) ack 746592448
[zfattrs:196446]      3182509615128385 enp129s0f0/000  1000 | TCP#00 | zf_stack_handle_tx_tcp: tx complete 2 idx 1 seg 0x7f25e8817598 2587162563:2587162567 pkt 2 in flight sendq 1-2-2
ok 2 - Correct data received
...
# attr test bitmask 0x6e0c5
ok 101 - Data sent
ok 102 - Correct data received
# attr test bitmask 0x916dd
ok 103 - Data sent
ok 104 - Correct data received
andresa-xilinx commented 6 months ago

With this commit log_file can be set programatically. The unit test shows log_file gets set correctly and it reports the desired output (either in stderr or in a file). Note that as zf_log_file is static, when the attrs get freed within the app the current behavior is not to change log_file value.

I would have expected that when attrs gets freed, log_file should return back to the original value (stderr). This is to prevent new attrs set not to accidentally inherit the previous value in log_file. Therefore, I have also included this change in behavior.

andresa-xilinx commented 6 months ago

Previous behavior was to log everything either to stderr or to a file. Now the app can choose among the two behaviors programatically as follows. See log messages in the console correspond to default behavior logging to stderr. Later we can see the listing of the unique directory and the different files created by different independent test iterations.

andresa@sup-cap2:~/onload-patches/tcpdirect$ ZF_ATTR="emu=2;interface=enp129s0f0" ./build/gnu_x86_64-zf-devel/bin/zf_unit/zfattrs
1..100
# attr test bitmask 0xb5f28
ok 1 - Data sent
ok 2 - Correct data received
# attr test bitmask 0x4ebb2
ok 3 - Data sent
ok 4 - Correct data received
# attr test bitmask 0x1c3f4
ok 5 - Data sent
ok 6 - Correct data received
# attr test bitmask 0xe1a75
  1000 | rxq_size=511 txq_size=1023 evq_size=2038 rx_prefix_len=0
ok 7 - Data sent
ok 8 - Correct data received
enp129s0f0/000  1000 | zf_stack_free()
# attr test bitmask 0xe6051
  1000 | rxq_size=1023 txq_size=1023 evq_size=2038 rx_prefix_len=0
ok 9 - Data sent
ok 10 - Correct data received
enp129s0f0/000  1000 | zf_stack_free()
# attr test bitmask 0xe1bcc
ok 11 - Data sent
ok 12 - Correct data received
# attr test bitmask 0x7e6fe
ok 13 - Data sent
ok 14 - Correct data received
# attr test bitmask 0xb34c4
ok 15 - Data sent
ok 16 - Correct data received
# attr test bitmask 0xcddf1
  1000 | rxq_size=511 txq_size=511 evq_size=1014 rx_prefix_len=0
ok 17 - Data sent
ok 18 - Correct data received
enp129s0f0/000  1000 | zf_stack_free()
# attr test bitmask 0x74e4c
ok 19 - Data sent
ok 20 - Correct data received
# attr test bitmask 0xbc299
...
# attr test bitmask 0x8d1c3
[zfattrs:624233]     10736589617248049   1000 | rxq_size=511 txq_size=511 evq_size=1014 rx_prefix_len=0
ok 91 - Data sent
ok 92 - Correct data received
[zfattrs:624233]     10736589617791237 enp129s0f0/000  1000 | zf_stack_free()
# attr test bitmask 0xa7b82
ok 93 - Data sent
ok 94 - Correct data received
# attr test bitmask 0x4adcf
ok 95 - Data sent
ok 96 - Correct data received
# attr test bitmask 0x415c5
  1000 | rxq_size=511 txq_size=511 evq_size=1014 rx_prefix_len=0
ok 97 - Data sent
ok 98 - Correct data received
enp129s0f0/000  1000 | zf_stack_free()
# attr test bitmask 0xdc1bc
ok 99 - Data sent
ok 100 - Correct data received
andresa@sup-cap2:~/onload-patches/tcpdirect$ ls /tmp/logfileBlqunM/ -l
total 44
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_0.log
-rw-r--r-- 1 andresa sflr 104 May  8 10:55 file_10.log
-rw-r--r-- 1 andresa sflr 105 May  8 10:55 file_11.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_12.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_13.log
-rw-r--r-- 1 andresa sflr 104 May  8 10:55 file_14.log
-rw-r--r-- 1 andresa sflr 183 May  8 10:55 file_15.log
-rw-r--r-- 1 andresa sflr 104 May  8 10:55 file_16.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_17.log
-rw-r--r-- 1 andresa sflr 104 May  8 10:55 file_18.log
-rw-r--r-- 1 andresa sflr 104 May  8 10:55 file_19.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_1.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_20.log
-rw-r--r-- 1 andresa sflr 183 May  8 10:55 file_21.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_22.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_2.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_3.log
-rw-r--r-- 1 andresa sflr 105 May  8 10:55 file_4.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_5.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_6.log
-rw-r--r-- 1 andresa sflr 105 May  8 10:55 file_7.log
-rw-r--r-- 1 andresa sflr 184 May  8 10:55 file_8.log
-rw-r--r-- 1 andresa sflr   0 May  8 10:55 file_9.log
andresa@sup-cap2:~/onload-patches/tcpdirect$ cat /tmp/logfileBlqunM/file_8.log
[zfattrs:624233]     10736588982406600   1000 | rxq_size=1023 txq_size=1023 evq_size=2038 rx_prefix_len=0
[zfattrs:624233]     10736588983238761 enp129s0f0/000  1000 | zf_stack_free()
abower-amd commented 6 months ago

I think the next step is a re-review by @pemberso-xilinx to see if he is happy with the changes, thanks!

andresa-xilinx commented 4 months ago

@ligallag-amd Yes, I also wondered the same about having getters and setters for global attributes. But see, there already is a single (pre existent) getter:

static uint64_t zf_log_level_get()

Which is not documented as such in the TCP Direct user's guide, nor do I think was the intention of that API. However, it is exactly that, a standard getter only used internally within TCP Direct.

I wonder what the implications would be with customers if we now change the API and forbid the setting/getting via the legacy functions. Or are you suggesting adding setters/getters and keep the previous setting/getting mechanism.

Re thread-safety, it'd perhaps make customers think it is thread safe, but remember by definition TCP Direct is not thread safe; customers know if they want to ensure thread safety they'd need to do that by themselves or move to Onload. Are you suggesting that it'd worth making it thread safe for this specific case?

ligallag-amd commented 4 months ago

@andresa-xilinx I was suggesting you add getters and setters to the public api and you get get rid of the get_attr/set_attr stuff. The static method you mention seems to only be used in a single file. I suppose doing this "locks in" the api in that logging is global and not per-stack - I don't know if we'd ever want to set logging per stack rather than globally.

Indeed, customers trying to set log_level, log_file, etc would probably encounter an error if we disallowed getting/setting from attr_set/get but I wonder how bad that'd be. It did nothing in the past perhaps it could still do nothing.

The trouble with thread safety in this context is that setting/getting separate attribute interferes with each other as there is this global taint. Take for example attrs a1 and a2, calling attr_set on a1 for log_file and attr_get on a2 for log_file will cause a rather nasty race despite those being entirely different attributes. This isn't the case for non-global attributes. Basically, we'd either need to be very explicit in our documentation that these attributes need to be synchronised on or we need to provide a way to synchronise on them.

pemberso-xilinx commented 3 months ago

The third commit seems to be doing more than just synchronizing as described. It looks like it is fixing comments from the second commit? I'd prefer clearer separation between the purpose of each commit. Possibly second and third commit should be merged?

andresa-xilinx commented 3 months ago

The third commit seems to be doing more than just synchronizing as described. It looks like it is fixing comments from the second commit? I'd prefer clearer separation between the purpose of each commit. Possibly second and third commit should be merged?

yes, agreed, I'll merge the two.

andresa-xilinx commented 3 months ago

Basically, we'd either need to be very explicit in our documentation that these attributes need to be synchronised on or we need to provide a way to synchronise on them.

@ligallag-amd I have documented as a known issue that global attributes need to be synchronized by the app:

https://xilinx.lightning.force.com/lightning/r/Knowledge__kav/ka04U000000y4I0QAI/view

Would this be enough?

andresa-xilinx commented 3 months ago

I know it's pedantic, but your second commit message isn't formatted properly. A line may be at most 72 characters. I also think you can remove: "Also, synchronizing global and local attributes when instantiating multiple attribute sets." - I don't believe you are doing any sort of synchronisation (no locking).

agreed.

I don't understand: "Now there is a single global initialization, and subsequent attribute sets instantiation won't overwrite values of a global attribute.". Where have you done this? To me, it looks like other attribute sets are capable of modifying the log level?

I meant before the initial values set at zf_init() were overwritten by zf_attr_alloc(...):

https://github.com/Xilinx-CNS/tcpdirect/pull/8/files#diff-9cb43605a7d0ab39a4c5c26d08f3e8de261d4ff17cb674059999a4b69c0d2663L140

By instantiation I meant during allocation of new set of attributes (e.g., see the unit test below), which now respect the current global values.

https://github.com/Xilinx-CNS/tcpdirect/pull/8/files#diff-a18c5e88dd663f03845ff5d519eaf2becfed1d9a4a78d05760bee14f250948b1R211

andresa-xilinx commented 3 months ago

I'd also like to see some test output where changing log_file fails. E.g. try setting it to a path that doesn't exist or you don't have permission to write to.

I've added testing for when log_file fails. Thanks for pointing this out.