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

Check checksum on CH11 under KLH10 #2342

Merged
merged 1 commit into from
Nov 21, 2024

Conversation

bictorv
Copy link
Contributor

@bictorv bictorv commented Nov 20, 2024

There is some bug in the CH11 implementation for KLH10 which makes it corrupt packets - quite rarely, but making e.g. remote DUMPs (using the RTAPE protocol) unreliable. I've previously established that it must be the CH11, rather than cbridge or the RTAPE server, that is to blame. My hunch is that there is some synchronization problem in the CH11 implementation.

The CH11 implementation in KLH10 is based on the IMP implementation, and I did it some 20-25 years ago. It uses really hairy mechanisms for synchronization between the KLH10 end (dvch11) and the device process (dpchaos). Hacking this type of C code is far from my favourite thing to do, so I've been putting it off since there are so many other things to hack that are much more fun and rewarding.

This PR provides a workaround: when running on a KLH10 that uses CH11, ITS computes the checksum when a packet has been received, and thus discovers corrupt packets and simply drops them (after counting it which will appear in STATUS replies). The performance price is really low.

The underlying bug in KLH10s CH11 should be fixed at some point, but that's a separate issue (and not for this repo).

Side note: is "ADDI J,(B)" (significantly) faster than "ADD J,B" on a KLH10?

@larsbrinkhoff
Copy link
Member

larsbrinkhoff commented Nov 20, 2024

This triggers a HALT instruction on machines other than KS10.

EDIT: SIMH KS10 also fails, although V3 works. Strange. Anyway, the missing ] effectively removes the rest of the CHAOS code, so that's probably why.

@@ -2426,29 +2426,49 @@ BBLK
JRST [ AOS CHNLOS ;No buffers, lose this packet
JRST CHXBK1 ]
HRLI A,-%CPKDT ;Copy out the packet header
IFN KLH10P,[
Copy link
Member

Choose a reason for hiding this comment

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

This needs a closing ]. Or the line could just be removed because the comment doesn't need to be inside a conditional.

;; this code should be taken out again.
;; Note that the performance impact is very small.
;; /BV November 2024.
;; (Side note: is "ADDI J,(B)" faster than "ADD J,B"?)
Copy link
Member

Choose a reason for hiding this comment

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

On some machines, potentially yes. The reason being that the immediate operand can use the possibly streamlined 18-bit addressing circuitry. I believe it is a little faster on a KA10, or maybe a PDP-6. Certainly this type of optimization can be seen in early MIT code.

Copy link
Member

Choose a reason for hiding this comment

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

I don't think ITS source code comments are the right place for a Q and A, though.

Copy link
Member

@larsbrinkhoff larsbrinkhoff left a comment

Choose a reason for hiding this comment

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

Please remove conditional for the top comment, and the ADDI question. All commits can then be squashed together, leaving only one.

@bictorv
Copy link
Contributor Author

bictorv commented Nov 21, 2024

Anyway, the missing ] effectively removes the rest of the CHAOS code, so that's probably why.

I wish MIDAS would complain about unbalanced ]s. I don't think it's the first time this happens...

@larsbrinkhoff
Copy link
Member

I wish MIDAS would complain about unbalanced ]s.

It makes some half-hearted attempts. But in this case, I think it did complain:

Unterminated successful bracketed conditionals
The first was at 220-018 of file ITS

That's the line which .INSRTs NET, which in turn .INSRTs CHAOS. So hardly a great clue, but still, something.

@larsbrinkhoff larsbrinkhoff merged commit 4557f5d into master Nov 21, 2024
14 checks passed
@larsbrinkhoff larsbrinkhoff deleted the bv/klh10-ch11-checksum-check branch November 21, 2024 12:35
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.

3 participants