Skip to content

Fixing potential integer overflow on sequence counter - #729

Merged
ksivaman merged 3 commits into
NVIDIA:mainfrom
shamisp:topic/overflow-fix
Apr 4, 2024
Merged

Fixing potential integer overflow on sequence counter#729
ksivaman merged 3 commits into
NVIDIA:mainfrom
shamisp:topic/overflow-fix

Conversation

@shamisp

Copy link
Copy Markdown
Contributor

Current implementation may potential cause hangs or data corruption

@shamisp
shamispforce-pushed the topic/overflow-fix branch 4 times, most recently from d17dc7b to f346513CompareMarch 23, 2024 05:04
Current implementation may potential cause hangs or data corruption
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>

@deepakn94deepakn94 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM, minor comment.

}

// Return true if producer > consumer, otherwise false while preventing integer overflow
// If we expect that produce will be 2B+ messages behind consumer

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Nit: "produce" -> "producer"

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

fixed

Addressing reviewers comments
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
@shamisp
shamispforce-pushed the topic/overflow-fix branch from a760748 to 9fe5e76CompareApril 2, 2024 14:24
@ptrendxptrendx assigned ptrendx and unassigned ptrendxApr 2, 2024
@ptrendx
ptrendx self-requested a review April 2, 2024 17:21
@ksivaman

Copy link
Copy Markdown
Member

/te-ci pytorch

@ksivamanksivaman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Got the greenlight from Anton.

@ksivaman
ksivaman merged commit e1e2b76 into NVIDIA:mainApr 4, 2024
shamisp added a commit to shamisp/TransformerEngine that referenced this pull request Apr 4, 2024
* Fixing potential integer overflow on sequence counter
Current implementation may potential cause hangs or data corruption
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
* Fixing typo in comments
Addressing reviewers comments
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
---------
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
Co-authored-by: Kirthi Shankar Sivamani <ksivamani@nvidia.com>
shamisp added a commit to shamisp/TransformerEngine that referenced this pull request Apr 5, 2024
* Fixing potential integer overflow on sequence counter
Current implementation may potential cause hangs or data corruption
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
* Fixing typo in comments
Addressing reviewers comments
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
---------
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
Co-authored-by: Kirthi Shankar Sivamani <ksivamani@nvidia.com>
pggPL pushed a commit to pggPL/TransformerEngine that referenced this pull request May 23, 2024
* Fixing potential integer overflow on sequence counter
Current implementation may potential cause hangs or data corruption
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
* Fixing typo in comments
Addressing reviewers comments
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
---------
Signed-off-by: Pasha (Pavel) Shamis <pasharesearch@gmail.com>
Co-authored-by: Kirthi Shankar Sivamani <ksivamani@nvidia.com>
Signed-off-by: Pawel Gadzinski <pgadzinski@nvidia.com>
Sign up for freeto 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

@shamisp@ksivaman@deepakn94@ptrendx