Re: [hatari-devel] DTA fix patch

[Eero Tamminen <oak@xxxxxxxxxxxxxx>]

Hi,

On 4/17/20 9:11 AM, Thomas Huth wrote:
> Am Tue, 14 Apr 2020 10:34:15 +0300
> schrieb Eero Tamminen <oak@xxxxxxxxxxxxxx>:
>
> > If you have time, could you check also my DTA fix
> > patch?
>
> Looks sane to me at a quick glance.
>
> > It seems to work fine, but I'm wondering e.g. whether Hatari should
> > still start cycling DTAs a bit earlier than at 32k.  Based on the
> > Hatari user's testing, 2k is enough, at that point TOS limits start
> > getting hit.
>
> Yeah, 32k sounds quite a lot already ... I'd maybe go with 4k or 8k
> first.

While real DTA is only 44 bytes, the internal one
is much larger (see below) => I'll go with 4K.


> Also, do you really want to grow that InternalDTA region
> exponentially when it is running out of entries? Adding 16 or 32
> entries at a time sounds more reasonable to me (I think it's rather
> unusual that a program really needs so many DTAs...).

Unused internal DTA takes something like 288
bytes, so last increment from 2K to 4K would
increase the allocation by 576KB.  Yeah, that's
too much.

Earlier used 256 count, would take 72KB for unused
entries, which sounds reasonable to me.

I'll start cache with that count, and increase its
size by same amount.


4K unused entries is a bit over 1MB of RAM, I
wonder how much it will be when all are in use,
with all the directory entry names cached?

I'm asking because as programs come and go,
their DTA addresses are likely to change too,
so terminated programs DTAs "leak" into cache.

I.e. cache will naturally grow until its cleaned
on emulation reset.


(Will commit the changes in day or two unless you have more comments.)


> A completely diffent topic, while reviewing your patch, I came accross
> this code in GemDOS_SNext() :
>
> 	if (!InternalDTAs[Index].bUsed)
> 	{
> 		/* Invalid handle, TOS returns ENMFIL
> 		 * (if Fsetdta() has been used by any process)
> 		 */
> 		Log_Printf(LOG_WARN, "GEMDOS Fsnext(): Invalid DTA\n");
> 		Regs[REG_D0] = GEMDOS_ENMFIL;
> 	}
>
> Shouldn't there be a "return true;" at the end of this if-statement?
> IMHO it doesn't make sense to continue with the function here?

You're right. I seem to have added that error path
in 2018 (commit bcbdec8b63), but without return,
it's a no-op.

I'll fix that too, thanks!



	- Eero