Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove requests from fwd_list and put connections of requests freeing #960

Merged
merged 2 commits into from
Mar 19, 2018

Conversation

krizhanovsky
Copy link
Contributor

  1. Fix Crash in tfw_http_conn_msg_free() #956: remove req from fwd_list on error in tfw_http_resp_process().
  2. Dead loop in tfw_sg_wait_release() #959: print a warning and let the system crash if we can't remove a
    server grooup.
  3. Add req->fwd_list and req->nip_list warnings to make sure that the req
    is removed from the list when we go to code where we don't have a pointer
    to the server connection.
  4. Replace tfw_http_msg_free() by tfw_http_conn_msg_free() in couple of
    places to put connections on requests freeing to avoid memory leakage
    on connections reference counters.
  5. Code leanups all around.

…ss().

2. #959: print a warning and let the system crash if we can't remove a
   server grooup.
3. Add req->fwd_list and req->nip_list warnings to make sure that the req
   is removed from the list when we go to code where we don't have a pointer
   to the server connection.
4. Replace tfw_http_msg_free() by tfw_http_conn_msg_free() in couple of
   places to put connections on requests freeing to avoid memory leakage
   on connections reference counters.
5. Code leanups all around.
@@ -547,7 +547,8 @@ tfw_sg_wait_release(void)
TFW_WARN_NL("pending for server callbacks to complete "
"for 5s, %ld server groups still exist\n",
atomic64_read(&act_sg_n));
tend = jiffies + HZ * 5;
__WARN();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can cause bugs when a lot of server or server groups are destroyed on system shutdown. I've already prepared solution for this: 11568a7. As soon as #959 will be fixed, I'll create PR with the fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok. Should I rollback the change or will you just overwrite it by your patch?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll overwrite it. Anyway I'll need to rebase my branch to comply this PR.

bad_req = hmresp->req;
tfw_http_req_delist((TfwSrvConn *)conn, bad_req);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My understanding is that we get here in all of the outcomes from the parser or GFSM: PASS, BLOCK, POSTPONE. My concern is a tricky case when the server considers a response NOT transmitted successfully. That may happen in BLOCK or POSTPONE cases. Will the server attempt to retransmit the response after the connection is restored? If so, the pairing of requests and responses must be maintained, and I am not sure that this situation is handled well in either patched or unpatched code.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point, thanks @keshonok ! Issues #962 and #963 were created.

* TODO #687: this is the only place where req_qlock is used. Instead
* of competing for the lock from different softirqs, just process
* the next available response, set a flag for current softirq
* processing ret_queue and make the current softire retry from
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

softire -> softirq

@krizhanovsky krizhanovsky merged commit 56e03bb into master Mar 19, 2018
@krizhanovsky krizhanovsky deleted the ak-956 branch March 19, 2018 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants