You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@trafficserver.apache.org by shaygalon <gi...@git.apache.org> on 2016/02/27 00:54:18 UTC
[GitHub] trafficserver pull request: TS-4233 AARCH64 48b va freelist fix
GitHub user shaygalon opened a pull request:
https://github.com/apache/trafficserver/pull/502
TS-4233 AARCH64 48b va freelist fix
Tested on linux aarch64 Cavium ThunderX platform, 4.2 kernel configured with 64K pages.
Crashes during freelist operations when trying to access pointer after sign extension.
Freelist pointer versioning sign extends upper 16 bits. Does not work well for 48b va on aarch64.
Patch clears the upper 16 bits instead.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/shaygalon/trafficserver master
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/trafficserver/pull/502.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #502
----
commit f304c50a2c00d7d711015c981b2007dacdf159b7
Author: Shay Gal-On <sg...@cavium.com>
Date: 2016-02-26T23:46:01Z
TS-4233 AARCH64 48b va freelist fix
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by shaygalon <gi...@git.apache.org>.
Github user shaygalon commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-203657654
For the 64K page size, a multiple of kernel page size would be fine and more future proof. It would be better to have it as a variable and not a constant, and assign sysconf(_SC_PAGESIZE). However, I was trying to minimize impact of changes. 64K is of particular interest as that is the default page size for arm64 kernels for redhat/centos/fedora etc.
For locking - absolutely, again I was trying to minimize impact, and not add more dependencies. Since this is highly platform dependent and only affects performance and not correctness, left it wrapped in a macro. Perhaps you would like to add a configure test that will check the timing for a variety of locking mechanisms, and choose the best one.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-219108516
There are now merge conflicts on this branch, before we can test on the CI, this needs to be rebased.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-219186514
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-199877909
@PSUdaemon to review, the thought it as that at least the freelist bit mask is a clearer solution for all platforms.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #502: AARCH64 fixes tested on Cavium ThunderX
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the issue:
https://github.com/apache/trafficserver/pull/502
Ping on this? Do we still want to move along with this?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver issue #502: AARCH64 fixes tested on Cavium ThunderX
Posted by zsmith928 <gi...@git.apache.org>.
Github user zsmith928 commented on the issue:
https://github.com/apache/trafficserver/pull/502
@zwoop @shaygalon can we assist here? We have on-demand CAvium ThunderX servers now via our platform and would be happy to give you guys direct access if its helps to unblock this issue.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by PSUdaemon <gi...@git.apache.org>.
Github user PSUdaemon commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-202990616
This kinda feels like it should be three separate PR's.
f304c50a2c00d7d711015c981b2007dacdf159b7 - seems probably ok, simply because if it's wrong it won't affect many people. Ideally I think CK++ should supersede this though.
9d13995ce3a5e26ef80e54b629daba538c0ec87f - I think I see what you are doing here, but I don't know enough about the cache internals to know if this is OK. @SolidWallOfCode thought it seemed ok, but was concerned about HostDB interaction. Also seems like maybe we can say this should be rounded up to a multiple of page size rather than this special case treatment.
96656663b07f6da8204f3edbb392bf3d55b05398 - Can you explain the need for this? Seems like we should not write our own spinlock implementation. Perhaps we can borrow one of the 8 implementations libck has. And this also seems like a one off case that can be made more generic. I don't really like the #ifdef around everything. I think it would be better to have a mutex macro that we can override with whatever method we want. For example, cohort locks might be better for us.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-218525666
Can one of the admins verify this patch?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-219190157
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-218782620
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-219194930
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by atsci <gi...@git.apache.org>.
Github user atsci commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-219191877
Can one of the admins verify this patch? Only approve PRs which have been reviewed.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by zwoop <gi...@git.apache.org>.
Github user zwoop commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-220376328
.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] trafficserver pull request: AARCH64 fixes tested on Cavium Thunder...
Posted by shaygalon <gi...@git.apache.org>.
Github user shaygalon commented on the pull request:
https://github.com/apache/trafficserver/pull/502#issuecomment-199982414
For x86 you actually have to sign extend the lower bit otherwise on AMD systems you would be addressing the wrong address... The meaning of the bits >48 is architecture dependent.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---