On Tue, Dec 15, 2009 at 01:28:01PM -0700, Jerry Jelinek wrote:
> Ed,
>
> Thanks for reviewing this.  My responses to your comments are
> in-line.
>
> Edward Pilatowicz wrote:
> >On Tue, Dec 15, 2009 at 08:39:12AM -0700, Jerry Jelinek wrote:
> >>I have an initial code review for the fix for bug:
> >>
> >>6768950 panic[cpu1]/thread=ffffff084ce0b3e0: syscall_asm_amd64.s:480
> >>        lwp ffffff0756a8cdc0, pcb_rupdate != 0
> >>
> >>There is a webrev at:
> >>
> >>http://cr.opensolaris.org/~gjelinek/webrev.6768950/
> >>
> >>The code changes in the sn1 and solaris10 brands are basically
> >>identical.  I know there is a lot of common code there but I
> >>didn't want to clutter up this bug fix with the unrelated changes
> >>necessary to make the code common.  I'll be addressing that with
> >>a separate fix.
> >>
> >>My initial testing of these changes looks good but I still need
> >>to run more extensive tests.
> >>
...
> >- prior to these changes V_URET_ADDR wasn't always set, so the different
> >  brand syscall callbacks would get the userland return address from
> >  their syscall specific locations (registers, interrupt stack, etc).  but
> >  now since V_URET_ADDR is always set, perhaps the callback handlers could
> >  be made more consistent by always getting the value from the stack (ie,
> >  via V_URET_ADDR)?
> >
> >- so following up with the last comment (and getting more into potential
> >  comminization work) it seems to me like it might be benificial to move
> >  all the syscall mechanism specific handling code out of the actual brand
> >  callbacks and into BRAND_CALLBACK.  you've already started doing this by
> >  having BRAND_CALLBACK be aware of how to get the userland return
> >  address.  (prior to that it didn't have any dependancy upon the
> >  different syscall mechanisms, except when deciding which brand callback
> >  to invoke.)  continuing down that path we could move all the syscall
> >  specific handling code into BRAND_CALLBACK.  then each brand would only
> >  deliver a single callback which would take one parameter, the syscall
> >  number.  it would return one value, a userland return address.  then
> >  BRAND_CALLBACK could handle all the different syscall specific return
> >  paths.  this would also be benificial in the future since if a new
> >  syscall mechanism was introduced, we wouldn't have to update any actual
> >  brand callbacks, just BRAND_CALLBACK.  thoughts?
>
> For these last two I agree that there are some good opportunities here and
> I was torn between doing a bunch more clean up on this and deferring that
> work to the fix for:
>
> 6900207 code can be shared between solaris10 and ipkg brands
>
> Since bug 6768950 is serious and I'd like to get the fix done sooner
> rather than later, I'd like to defer some of these other changes to 6900207.
> I was about to start on that anyway so once 6768950 is done I'm going to
> immediately start work on a bunch of ideas I have for making the code shared
> and simpler.  I was also going to roll a fix for:
>
> 6887823 brandz on x86 should ignore %gs on 64-bit kernels
>
> into that same set of cleanup.  I definitely agree with your comments here
> but I'm worried about the fix for 6768950 taking too long.
>

sounds good.
ed
_______________________________________________
zones-discuss mailing list
zones-discuss@opensolaris.org

Reply via email to