fby3.5: cl: Modified IPMB architecture by rainlin024 · Pull Request #47 · facebook/OpenBIC

rainlin024 · 2021-11-08T06:52:51Z

Summary:

There were some threads creating with CMSIS-OS2 API and which required fixed memory space for CMSIS-OS2.
For Yv3.5, it requires IPMB TX and RX threads for four I2C buses and occupying too much memory to fix memory for maximum usage.
Modified OS API from CMSIS-OS2 API to Zephyr kernel API, including threads, mutex, message queue.
Removed memory reservation for CMSIS-OS2 stacks.
To reduce threads stack size, modified IPMB buffer from local variable to pointer and only allocate when process running.
Removed worker from common code, in that Zephyr kernel provided the service already.

Test plan:
-Build code: Pass
-Stress IPMB: Pass
Stressed get ME version for over thousands times.

facebook-github-bot · 2021-11-08T19:22:03Z

@GoldenBug has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

GoldenBug · 2021-11-09T01:58:37Z

  ipmi_msg_cfg *ptr_start = pnode;

-  osMutexAcquire(mutex_id[index], osWaitForever);
+  k_mutex_lock(&mutex_id[index], K_FOREVER);


Should this really wait forever? It seems better to use a timeout as documented in the Zephyr doc at https://docs.zephyrproject.org/latest/reference/kernel/synchronization/mutexes.html

For function insert node, find node and timeout handler, in timeout case, it risks a condition that BIC sending IPMB success but fail to record IPMB request package, which makes BIC take IPMB response as invalid package according to IPMI specification.
To avoid thread pending forever in mutex, we think we can add mutex status check. If BIC lock mutex fail due to one second timeout, will return directly and record a mutex lock fail in BIC console.
Is that make sense to you? Or do you think there is a better solution?

The main issue is to not hang forever. Having a one second timeout and recording the lock failure is sufficient to meet this requirement of not hanging the BMC if the mutex is unable to be acquired.

OK, we will fix it soon.

GoldenBug · 2021-11-09T01:58:56Z

  ipmi_msg_cfg *ptr_start = pnode;

-  osMutexAcquire(mutex_id[index], osWaitForever);
+  k_mutex_lock(&mutex_id[index], K_FOREVER);


Same as above...Shouldn't this timeout at some point?

Please see comment in insert_node case.

See above comment

Just for clarity, I agree with the same change as indicated in the insert_node case.

GoldenBug · 2021-11-09T01:59:26Z


  while (1) {
-  	osMessageQueueGet(ipmb_txqueue[ipmb_cfg.index], (void *)&current_msg_tx, NULL, osWaitForever); // Wait for OS queue send interrupt
+    current_msg_tx = (struct ipmi_msg_cfg *) malloc(sizeof(struct ipmi_msg_cfg));


Shouldn't there be a check here to ensure the memory was allocated?

Will add if memory NULL check.

GoldenBug · 2021-11-09T01:59:41Z

  while (1) {
-  	osMessageQueueGet(ipmb_txqueue[ipmb_cfg.index], (void *)&current_msg_tx, NULL, osWaitForever); // Wait for OS queue send interrupt
+    current_msg_tx = (struct ipmi_msg_cfg *) malloc(sizeof(struct ipmi_msg_cfg));
+    k_msgq_get(&ipmb_txqueue[ipmb_cfg.index], (ipmi_msg_cfg *)current_msg_tx, K_FOREVER); // Wait for OS queue send interrupt


Should there be a timeout here?

In architecture design, Tx thread is only in charge of message sending and driver status checking.
So for mutex in Tx thread, we would like to keep it waiting for OS ISR at the moment there is another process requesting sending message by putting mqueue, instead of keeping polling mqueue. It helps a lot in reducing task switch and optimize BIC performance.

I'm OK with leaving it as it is to reduce the task switching and optimize performance for this instance.

GoldenBug · 2021-11-09T02:00:03Z


  		} else { // TODO: else if (ipmb_cfg.Inf == I3C_IF)
  			printf("IPMB %d using not support interface: %x\n", ipmb_cfg.index, ipmb_cfg.Inf);
+        free(current_msg_tx);


Should there be a check that current_msg_tx is not NULL before freeing it?

Will add if memory NULL check.

GoldenBug · 2021-11-09T02:03:17Z

  req_cfg.retries = 0;
  /* Blocks here until is able put message in tx queue */
-  if (osMessageQueuePut(ipmb_txqueue[index], &req_cfg, 0, osWaitForever) != osOK) {
+  if (k_msgq_put(&ipmb_txqueue[index], &req_cfg, K_FOREVER) != osOK) {


Timeout instead of forever?

Will add mqueue status check and return corresponding ipmb_error for project layer handling.

GoldenBug · 2021-11-09T02:03:26Z

 ipmb_error ipmb_send_response(ipmi_msg *resp, uint8_t index)
 {
-  osMutexAcquire(mutex_send_res, osWaitForever);
+  k_mutex_lock(&mutex_send_res, K_FOREVER);


Timeout vs. forever?

Will add timeout and mutex status check, returning ipmb_error_timeout in mutex timeout case.

GoldenBug · 2021-11-09T02:03:35Z

+  if (k_msgq_put(&ipmb_txqueue[index], &resp_cfg, K_FOREVER) != osOK) {
  	//free(resp_cfg);
-  	osMutexRelease(mutex_send_res);
+  	k_mutex_unlock(&mutex_send_res);


Timeout vs. forever?

Will add mqueue status check and return corresponding ipmb_error for project layer handling for line 576.

GoldenBug · 2021-11-09T02:03:45Z

      }

-      osMutexAcquire(mutex_id[index], osWaitForever);
+      k_mutex_lock(&mutex_id[index], K_FOREVER);


Timeout vs. Forever?

Please see comment in insert_node case.

Agree with making the same change as is mentioned in insert_node case comment.

GoldenBug · 2021-11-09T02:04:08Z

 #CONFIG_CMSIS_V2_MSGQ_MAX_DYNAMIC_SIZE=32768
 CONFIG_HEAP_MEM_POOL_SIZE=32768
-CONFIG_CMSIS_V2_MSGQ_MAX_DYNAMIC_SIZE=16384
+CONFIG_CMSIS_V2_MSGQ_MAX_DYNAMIC_SIZE=8000


Should this be set to 8192 to be consistent with the max sizes aligning with 4K boundaries?

OK, we will check if any side effect with setting to 8192 and fix it.

garnermic · 2021-11-10T15:45:45Z

I agree with the proposed changes and recommendations for changes (or in the case of the tx thread, leaving it as is). Please move forward with implementing these changes.

Summary: - There were some threads creating with CMSIS-OS2 API and which required fixed memory space for CMSIS-OS2. For Yv3.5, it requires IPMB TX and RX threads for four I2C buses and occupying too much memory to fix memory for maximum usage. Modified OS API from CMSIS-OS2 API to Zephyr kernel API, including threads, mutex, message queue. Removed memory reservation for CMSIS-OS2 stacks. - To reduce threads stack size, modified IPMB buffer from local variable to pointer and only allocate when process running. - Removed worker from common code, in that Zephyr kernel provided the service already. Test plan: -Build code: Pass -Stress IPMB: Pass Stressed get ME version for over thousands times.

facebook-github-bot · 2021-11-11T02:21:28Z

@rainlinWW has updated the pull request. You must reimport the pull request before landing.

rainlin024 · 2021-11-11T02:30:42Z

I will close this PR and PR new commit again.
Thanks for your review and suggestion.

modify modbus struct v1

facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Nov 8, 2021

GoldenBug suggested changes Nov 9, 2021

View reviewed changes

rainlin024 force-pushed the fby3.5-Modified_IPMB_architecture branch from f1647c2 to aefdfe8 Compare November 11, 2021 02:21

rainlin024 closed this Nov 11, 2021

rainlin024 deleted the fby3.5-Modified_IPMB_architecture branch November 30, 2021 03:35

HungYi-Li pushed a commit to HungYi-Li/OpenBIC that referenced this pull request May 13, 2024

Merge pull request facebook#47 from matt-lzy/modify_modbus_struct

75083d8

modify modbus struct v1

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fby3.5: cl: Modified IPMB architecture#47

fby3.5: cl: Modified IPMB architecture#47
rainlin024 wants to merge 1 commit into
facebook:mainfrom
rainlin024:fby3.5-Modified_IPMB_architecture

rainlin024 commented Nov 8, 2021

facebook-github-bot commented Nov 8, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

garnermic Nov 9, 2021

rainlin024 Nov 10, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

garnermic Nov 9, 2021

garnermic Nov 10, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

garnermic Nov 10, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

garnermic Nov 10, 2021

GoldenBug Nov 9, 2021

rainlin024 Nov 9, 2021

garnermic commented Nov 10, 2021

facebook-github-bot commented Nov 11, 2021

rainlin024 commented Nov 11, 2021

Labels

4 participants

Uh oh!

Conversation

rainlin024 commented Nov 8, 2021

facebook-github-bot commented Nov 8, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

garnermic commented Nov 10, 2021

facebook-github-bot commented Nov 11, 2021

rainlin024 commented Nov 11, 2021

Labels

4 participants