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.

Sign up for GitHub

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

Jump to bottom

LoongArch64: Fix PR #22817 introduced performance regression of ChaCha20 #23301

Closed
wants to merge 1 commit into from
Closed

LoongArch64: Fix PR #22817 introduced performance regression of ChaCha20 #23301

wants to merge 1 commit into from

Conversation

lrzlin
Copy link
Contributor

@lrzlin lrzlin commented Jan 14, 2024 •
edited

In that pull request, the input length check was moved forward, but the related ori $t3,$zero,64 instruction was missing, and it will cause input of any length down to the much slower scalar implementation. This fix that regression, and Fixes #23300.

@openssl-machine openssl-machine added the hold: cla required The contributor needs to submit a license agreement label Jan 14, 2024
@openssl-machine openssl-machine removed the hold: cla required The contributor needs to submit a license agreement label Jan 14, 2024
@xry111
Copy link
Contributor

xry111 commented Jan 15, 2024

LGTM. But let's add "Fixes #23300." to the description so GitHub will link this PR with the issue.

@lrzlin lrzlin mentioned this pull request Jan 15, 2024
Open
@mattcaswell mattcaswell added branch: master Merge to master branch approval: review pending This pull request needs review by a committer approval: otc review pending This pull request needs review by an OTC member triaged: bug The issue/pr is/fixes a bug severity: regression The issue/pr is a regression from previous released version branch: 3.2 Merge to openssl-3.2 and removed severity: regression The issue/pr is a regression from previous released version labels Jan 15, 2024
@github-actions github-actions bot added the severity: fips change The pull request changes FIPS provider sources label Jan 15, 2024
@lrzlin
This fix PR openssl#22817 introduced performance regression.
5729519 
In that pull request, the input length check was moved forward,
but the related ori instruction was missing, and it will cause
input of any length down to the much slower scalar implementation.

Fixes openssl#23300

CLA: trivial
@lrzlin
Copy link
Contributor Author

lrzlin commented Jan 15, 2024

LGTM. But let's add "Fixes #23300." to the description so GitHub will link this PR with the issue.

Added, seems need to be reviewed again.

@xry111
Copy link
Contributor

xry111 commented Jan 15, 2024

LGTM. But let's add "Fixes #23300." to the description so GitHub will link this PR with the issue.

Added, seems need to be reviewed again.

My approve is useless anyway :(. We need to collect 3 approves from the OpenSSL maintainers AFAIU.

@lrzlin
Copy link
Contributor Author

lrzlin commented Jan 15, 2024

My approve is useless anyway :(. We need to collect 3 approves from the OpenSSL maintainers AFAIU.

Oh thanks, I didn't know that before, in that case let's waiting the maintainers for reviewing it.

@xry111 xry111 mentioned this pull request Jan 15, 2024
Open
@github-actions github-actions bot removed the severity: fips change The pull request changes FIPS provider sources label Jan 15, 2024
@t8m
Copy link
Member

t8m commented Jan 15, 2024

My approve is useless anyway :(. We need to collect 3 approves from the OpenSSL maintainers AFAIU.

2 approves are sufficient.

@t8m t8m added tests: exempted The PR is exempt from requirements for testing severity: regression The issue/pr is a regression from previous released version labels Jan 15, 2024
@t8m t8m removed the approval: otc review pending This pull request needs review by an OTC member label Jan 15, 2024
@slontis
Copy link
Member

slontis commented Jan 15, 2024

t3 maps to r15, Wasnt that then an uninitialized value that could have been set to an arbitrary number (possibly greater than 64 also?)

@slontis slontis added approval: done This pull request has the required number of approvals and removed approval: review pending This pull request needs review by a committer labels Jan 15, 2024
@slontis
Copy link
Member

slontis commented Jan 15, 2024

My approve is useless anyway :(. We need to collect 3 approves from the OpenSSL maintainers AFAIU

More people that understand the code giving approvals is always welcome.

@lrzlin
Copy link
Contributor Author

lrzlin commented Jan 16, 2024

t3 maps to r15, Wasnt that then an uninitialized value that could have been set to an arbitrary number (possibly greater than 64 also?)

Yes, at least it's almost always greater than 64 on Gentoo loong64, however on AOSC Linux with some safety enhancement, the $t3 register's value will be set to zero at the beginning of this function.

@zhoumin2
Copy link
Contributor

We should add the missing initialization for $t3, so the fixing is straightforward. Thanks.

@mattcaswell
Copy link
Member

My approve is useless anyway

Not useless. Community approvals help give committers the confidence to also approve.

@openssl-machine
Copy link
Collaborator

24 hours has passed since 'approval: done' was set, but as this PR has been updated in that time the label 'approval: ready to merge' is not being automatically set. Please review the updates and set the label manually.

@t8m t8m added the cla: trivial One of the commits is marked as 'CLA: trivial' label Jan 17, 2024
@t8m
Copy link
Member

t8m commented Jan 17, 2024

Merged to the master and 3.2 branches. Thank you for your contribution.

@t8m t8m closed this Jan 17, 2024
openssl-machine pushed a commit that referenced this pull request Jan 17, 2024
@lrzlin @t8m
Fix performance regression of ChaCha20 on LoongArch64
9710285 
The regression was introduced in PR #22817.

In that pull request, the input length check was moved forward,
but the related ori instruction was missing, and it will cause
input of any length down to the much slower scalar implementation.

Fixes #23300

CLA: trivial

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23301)
openssl-machine pushed a commit that referenced this pull request Jan 17, 2024
@lrzlin @t8m
Fix performance regression of ChaCha20 on LoongArch64
96dd7b2 
The regression was introduced in PR #22817.

In that pull request, the input length check was moved forward,
but the related ori instruction was missing, and it will cause
input of any length down to the much slower scalar implementation.

Fixes #23300

CLA: trivial

Reviewed-by: Shane Lontis <shane.lontis@oracle.com>
Reviewed-by: Tomas Mraz <tomas@openssl.org>
(Merged from #23301)

(cherry picked from commit 9710285)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@t8m t8m t8m approved these changes

@xry111 xry111 xry111 approved these changes

@slontis slontis slontis approved these changes

Assignees
No one assigned
Labels
approval: done This pull request has the required number of approvals branch: master Merge to master branch branch: 3.2 Merge to openssl-3.2 cla: trivial One of the commits is marked as 'CLA: trivial' severity: regression The issue/pr is a regression from previous released version tests: exempted The PR is exempt from requirements for testing triaged: bug The issue/pr is/fixes a bug
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

LoongArch: chacha20 speed has huge performance degraded
7 participants
@lrzlin @xry111 @t8m @slontis @zhoumin2 @mattcaswell @openssl-machine