Possible bug in tk_frame of NicheLite for M52233DEMO

[flashtoo]

I'm not sure where the best place to post this is so I thought I'd dump it here.

In the tk_frame function provided by the NicheLite stack in the M52233DEMO, the function deducts 68 bytes for the frame. However, the best I can tell, the frame is 48 bytes so it should deduct 49 bytes so the last byte is still in the stack.  This is a waste of 19 bytes per task. Not much of a loss but none the less a what seems to be a small bug.  Or, if it really is 68 bytes, then the last byte will be overlayed on the first byte of the nexts task stack.  The following task would have to use up all it's stack to hit this however...

flashtoo

Message Edited by flashtoo on 2007-03-1907:36 AM

[wyliek]

Hi there

Have you tried subtracting the 49 bytes instead of 68 and if so did it work?

Could you post some code for how you think tk_switch and tk_frame should look? I'd be very interested to see and perhaps give them a try.

cheers

[mccPaul]

Hi Kyle,

This assembles, but I haven't run it as the version of that stack I have checked out today doesn't run yet so I'll test it later. I noticed a couple of things in the original implementation:

first, it clobbers the current status register setting to disable interrupts, no so bad, but then it restores them by setting the sr interrupt mask to 0. This may not be what you want if you are being careful with interrupt levels and priorities. My version preserves the current sr.

Second, (I haven't had time to properly investigate this) the discrepancy in struct size and reserved space in the tk_frame function may be to reserve space for the registers that are pushed in tk_switch.

Code:

/*************************************************** * tk_switch * * Called with the new task pointer as the first * address on the stack. All registers are copied * to the stack. Interrupts are masked on entry * and returned to previous state on exit. * */ _tk_switch:tk_switch: move.w %sr, %d1     /* Preserve sr */ move.w #0x2700, %sr    /* Disable interrupts */            link %a6, #-44     /* Push a6 and reserve space for the other registers */          /* a6 = sp - 4, sp = sp - 48 */           movem.l %d2-%d7/%a1-%a5, (%sp)  /* Push d2 to d7 and a1 to a5 onto the stack */          /* Now we have stacked all the registers that matter */           move.l 12(%a6), %d0    /* Get the task pointer that was passed to the function */          /* This will be the first 4 bytes on the stack after the */          /* return address */           movea.l (tk_cur), %a1    /* Get a pointer to the current task */ move.l %a7, tk_fp(%a1)    /* Save sp (a7) in the task struct */ move.l %d0, tk_cur     /* Install new (passed) task */ movea.l %d0, %a1     /* Address new task */ movea.l tk_fp(%a1), %a7    /* Install new task's stack */ movem.l (%sp), %d2-%d7/%a1-%a5  /* Pop registers from the new stack */ lea  44(%sp), %a6    /* Adjust a6 based on the new task's sp */ unlk %a6       /* Pop a6 from the new task's stack */  move.w %d1, %sr     /* Restore old sr */ rts         /* Return in new task context */

 
Let me know if you try it.

Cheers,

Paul.

[flashtoo]

Paul,

I think there is a bug in the code snippet for tk_switch you proviced.  Where you get the passed variable, I beleive the offset should be 8(%a6) not 12(%a6).  The original stack pointer contains task pointer (there is no return value to push). The JSR decrement the stack by 4 and push the PC. The LINK decrements the stack by 4 and puts A6 onto it. It then puts store this address into A6 and modifies A7. So the parameter is an offset of 8 from A6.

FlashToo

[mccPaul]

Hi FlashToo,

Well spotted - bad counting on my part!

Paul.

[flashtoo]

Paul, I tried this code, it seems like it works.

I assume you modifed tk_frame too as it also has the interrupt problem. And, for some reason, it  reserves 20 bytes on the stack but does not use them... 

While as mentioned above, the 68 bytes reserved in tk_frame does not seem to be needed, it does however, need to be a multiple of 4 to keep alignment so 52 would be the correct number I think.

FlashToo

[flashtoo]

Paul,

Thanks for the improved code. I'll try it and get back...

Here is another issue (perhaps should be a new thread)..

The linker file that comes with the demo should have an ALIGN statement before the start of the stack.  Since this memory is going to be used for stack space for the main task, it needs to be long work aligned to insure good performace.  It also can cause address error exceptions.

Code:

  .               = ALIGN(0x04);  #Add this line  ___SP_END       = .;  .               = . + (0x1000);  ___SP_INIT      = .;

 
For those short on memory, I also found that I could reduce this stack down to about 0x300 bytes. However, I have not done a lot of testing on this value...

FlashToo

[bkatt]

>... it clobbers the current status register setting to disable interrupts, no so bad, but then it restores them by setting the sr interrupt mask to 0...

I'm not convinced that either of these routines should be disabling interrupts. The frame function is not special in any way except it happens to be coded in assembly instead of C. The switch function does change the stack pointer, but does it atomically and both stacks seem completely valid.

Also, I see no reason to preserve the status register in the switch -- this is cooperative multitasking, and what does it mean for a task to yield with some or all of the interrupts disabled?

What I would like to see is something pushed on the stack in the frame function so that if the main program of a task should return, the task would terminate normally. "main" in a C program behaves this way, and the starting function of a pthread also does this. But if you return from a niche task, kaboom.

[mccPaul]

Good point - I can't think of a good reason to disable interrupts.

Paul

[flashtoo]

Interesting. So paraphrasing, if I may, you are saying, why turn off interrupts at all? They should just keep running assuming your interrupt preserves all registers used. And, if we don't turn off interrupts, we don't need to push/pull the status register. 

In 99% of the time I would agree with you.  But I suppose one could have a time critical task that could not be interrupted. It would disable interrupts during the task initialization and would expect them to stay that way.  In this case, we would need to push/pull the SR.   Perhaps this should be a #define TASKS_CHANGE_SR or something like that...

FlashToo

[flashtoo]

Here is my reworked tk_frame. 

1. Correctly save/restore SR
2. Make proper use of the Frame Pointer (A6)
3. Allocate only needed bytes on the stack (in link command)
4. Changed space from "RTOS frame" from 68 to 52 bytes.

FlashToo

Code:

_tk_frame:  move.w   SR, D1          /* Preserve sr */  move.w   #0x2700, SR     /* Disable interrupts */   link      A6,#-4       /* Create frame and get local memory */     movem.l  D2,(SP)       /* Save D2 */  move.l   8(A6),A0        /* get passed task */  move.l   tk_stack(A0),D2 /* get it's stack base */  add.l    tk_size(A0),D2  /* add stack size - top of new stack */  move.l   #52,D0  sub.l    D0,D2           /* deduct room for frame */  move.l   12(A6),D0       /* get passed function pointer */  move.l   D2,A0           /* address stack (FP) for new task */  move.l   D0,48(A0)       /* Install it in task's stack */  move.l   D2,D0           /* return task->fp */  movem.l  (SP),D2       /* restore D2 */  unlk     A6     move.w   D1, SR          /* Restore old sr */  rts

 

[flashtoo]

And while we are at it, here is an improved tk_getsp(). If you really want the stack pointer at time of call you have to remove the stack used to call this subroutine.

FlashToo

Code:

_tk_getsp:   move.l  A7,D0            /* return stack pointer */// add what calling this function uses too   add.l   #4,D0
   rts

[mjbcswitzerland]

Hi Flashtoo

Normally Ethernet frames are padded to be at least 60 bytes in length. Can this be a possible reason for what you see?

Regards

[mccPaul]

Hi,

Mark - this is task switching code in NicheLite and the frame is the task frame, not an Ethernet frame.

It does look like there is a small leak in the tk_frame function, and also the tk_switch function would appear to be much more efficient if it used two movem instructions to push and pop all the registers to and from the task stack instead of 24 move instructions to achieve the same thing.

Cheers,

Paul.

[mjbcswitzerland]

Ooops. I thought tk_frame was something to do with tx frame...

Cheers

Mark