Stable cvs version for 2.4

Stable cvs version for 2.4

[John Hall]

Hi,

We're currently using ARM Linux 2.4.18-rmk7, and following the 2.4 series of the kernel for our product that we're developing. I've been using cvs versions of the mtd subsystem since I've needed jffs2 on NAND flash, and I currently have code dating back to June. I've just done an update since there seem to be a lot of jffs2 changes.

Looking at some of the comments, there seem to be some fairly experimental things recently committed (e.g version 1.26 of fs/jffs2/read.c). My question is, is there a reasonably stable version I can use, and indeed is the current HEAD version suitable for 2.4, or is it 2.5 only?

I guess the answer to the first question is to stick with whatever's in the stock kernel unless I want to be helping to test the latest code, but I would appreciate advice anyway.

Regards,
John Hall

-- 
OptionExist Limited.
The Irwin Centre, Scotland Road, Dry Drayton, Cambridge.
Tel: +44 (0)1954 211244, Fax: +44 (0)1954 211565
http://www.optionexist.co.uk/

Re: Stable cvs version for 2.4

[David Woodhouse]

John.Hall@optionexist.co.uk said:
>  We're currently using ARM Linux 2.4.18-rmk7, and following the 2.4
> series of the kernel for our product that we're developing. I've been
> using cvs versions of the mtd subsystem since I've needed jffs2 on
> NAND flash, and I currently have code dating back to June. I've just
> done an update since there seem to be a lot of jffs2 changes.

The head of CVS as of yesterday (cvs up -D yesterday) should be fine. 
Don't use the stuff I committed at 1 o'clock this morning without testing it. 
That would not be advisable :)

--
dwmw2

Re: Stable cvs version for 2.4

[David Woodhouse]

dwmw2@infradead.org said:
>  The head of CVS as of yesterday (cvs up -D yesterday) should be fine.
>  Don't use the stuff I committed at 1 o'clock this morning without
> testing it.  That would not be advisable :)

The current code now seems to be passing my stress tests again. I think I 
may have ironed out the rbtree bugs. So anyone who was previously 
complaining that the performance went to pot after files got quite large, 
suck this one and see. Should now have O(log N) lookup and insertion costs 
instead of O(N), N obviously being the size of the file.

You may have to nab a copy of lib/rbtree.c from a vaguely recent 2.4 kernel 
if you're using ancient ones, or maybe just export its contents. I'm going 
to clean that up later; I don't like the existing one because it wastes 
sizeof(int) for a single bit of colour information which could easily be 
packed into the low bits of one of the pointers. :)

--
dwmw2

Re: Stable cvs version for 2.4

[Jasmine Strong]

On Wed, 4 Sep 2002, David Woodhouse wrote:

> to clean that up later; I don't like the existing one because it wastes
> sizeof(int) for a single bit of colour information which could easily be
> packed into the low bits of one of the pointers. :)

WARNING! WARNING!  there are architectures that will break if you do this.

Jas.

Re: Stable cvs version for 2.4

[David Woodhouse]

jasmine@hex.linuxgrrls.org said:
>  WARNING! WARNING!  there are architectures that will break if you do
> this. 

Architectures on which Linux runs? eCos needs its own rbtree implementation 
anyway, since the one I'm using is GPL'd.

--
dwmw2

Re: Stable cvs version for 2.4

[Jasmine Strong]

On Wed, 4 Sep 2002, David Woodhouse wrote:

>
> jasmine@hex.linuxgrrls.org said:
> >  WARNING! WARNING!  there are architectures that will break if you do
> > this.
>
> Architectures on which Linux runs?

Yes.  uCLinux, anyway.   Pointers should always be treated as opaque
unless you have a *really* good reason not to.

(Architectures that don't always use word-aligned pointers include m68k
and ARM Thumb.)

Jas.

Re: Stable cvs version for 2.4

[David Woodhouse]

jasmine@hex.linuxgrrls.org said:
>  Yes.  uCLinux, anyway.   Pointers should always be treated as opaque
> unless you have a *really* good reason not to.

I have 8000 reasons not to:

# grep node_frag /proc/slabinfo 
jffs2_node_frag     8028   8064     28   64   64    1

At a gratuitous extra 4 bytes per object, that's... ok, well it's only
wasting 32KiB which is less than I expected, but it still annoys me.

> (Architectures that don't always use word-aligned pointers include
> m68k and ARM Thumb.) 

Even when we're allocating from the slab and the object in question is a 
structure of size which is an even number?

Even so, I'd assume that we care even _more_ about that 32KiB on uCLinux, so
maybe it's worth adding 1 byte of padding to the structure there and
manually aligning the pointer after allocating, or something. We only need 
1 bit, after all. Or we could use the _top_ bit on uCLinux, perhaps?

--
dwmw2

RE: Stable cvs version for 2.4

[Allen Curtis]

> At a gratuitous extra 4 bytes per object, that's... ok, well it's only
> wasting 32KiB which is less than I expected, but it still annoys me.
>
> > (Architectures that don't always use word-aligned pointers include
> > m68k and ARM Thumb.)
>
> Even when we're allocating from the slab and the object in question is a
> structure of size which is an even number?

How about we add 1 byte which contains up to 8 bit fields? Using bits in
pointer values sounds a lot like old Mac programming. Caution Will
Robinson....

RE: Stable cvs version for 2.4

[Jasmine Strong]

On Wed, 4 Sep 2002, Allen Curtis wrote:

> How about we add 1 byte which contains up to 8 bit fields?

Adding a byte will destroy the alignment.  you'd have to add a whole word.

We could have a #define to set whether we use the lsb of the address (on
aligned architectures) or an extra byte (on non-aligned architectures,
which won't care about the structure not being an exact multiple of words
anyway).

> Using bits in pointer values sounds a lot like old Mac programming.
> Caution Will Robinson....

I agree.

Jas.

Re: Stable cvs version for 2.4

[Steve Wahl]

On Wed, Sep 04, 2002 at 03:54:03PM +0100, Jasmine Strong wrote:
> 
> 
> On Wed, 4 Sep 2002, Allen Curtis wrote:
> 
> > How about we add 1 byte which contains up to 8 bit fields?
> 
> Adding a byte will destroy the alignment.  you'd have to add a whole word.
> 
> We could have a #define to set whether we use the lsb of the address (on
> aligned architectures) or an extra byte (on non-aligned architectures,
> which won't care about the structure not being an exact multiple of words
> anyway).

My $0.02 is: 

The #define is a good suggestion.  

But the assumption that using the lsb of the pointer will only break
on archetectures that don't care about alignment may be a bad one.

I haven't looked at the code (shame on me...), but I thought I heard
the word "tree" mentioned. :-) If this data structure is being used
for speed, I have to ask if the extra masking instructions are a good
trade off for the memory saved?

--> Steve

Re: Stable cvs version for 2.4

[David Woodhouse]

swahl@brecis.com said:
>  But the assumption that using the lsb of the pointer will only break
> on archetectures that don't care about alignment may be a bad one.

Theoretically yes, but in practice no. It is of course possible that some 
architectures will store in a 'pointer' something other than the actual 
address to be put on the bus when accessing the data in question.

Linux doesn't run on any of that hardware, thankfully. And if it becomes a 
problem, the pointer magic can be fixed then, and it'll be a drop in the 
ocean of all the other now-broken assumptions that we have to fix :)

> I haven't looked at the code (shame on me...), but I thought I heard
> the word "tree" mentioned. :-) If this data structure is being used
> for speed, I have to ask if the extra masking instructions are a good
> trade off for the memory saved? 

If you stick it in the rb_parent field, you don't have to mask it out 
during lookup or insertion operations, only during rebalancing. Either way, 
it's a lot faster than the linked list which I just replaced :)

--
dwmw2

Re: Stable cvs version for 2.4

[David Woodhouse]

jasmine@hex.linuxgrrls.org said:
> > How about we add 1 byte which contains up to 8 bit fields?
> Adding a byte will destroy the alignment.  you'd have to add a whole
> word. 

Er, what alignment?

Either the CPU or something else assumes and requires alignment, in which 
case we know that we can happily abuse the low-bit of the address.

Or it doesn't, in which case we can add our single byte and use it with 
impunity. And yes, Allen's right that if we're going to add a single byte 
to the structure we should just put the colour information into that 
instead of using it to align the pointer so that we can abuse the low bit 
of the pointer :)

--
dwmw2

Re: Stable cvs version for 2.4

[Jasmine Strong]

On Wed, 4 Sep 2002, David Woodhouse wrote:

> jasmine@hex.linuxgrrls.org said:
> > > How about we add 1 byte which contains up to 8 bit fields?
> > Adding a byte will destroy the alignment.  you'd have to add a whole
> > word.
>
> Er, what alignment?

The alignment of the structure.  Either it's a whole multiple of words or
it isn't.

> Either the CPU or something else assumes and requires alignment, in which
> case we know that we can happily abuse the low-bit of the address.

Yes, that's what I said.

> Or it doesn't, in which case we can add our single byte and use it with
> impunity.

I said that too...

> And yes, Allen's right that if we're going to add a single byte
> to the structure we should just put the colour information into that
> instead of using it to align the pointer so that we can abuse the low bit
> of the pointer :)

Yep.  I agree wholeheartedly.  I would be happiest with having
compile-time options for the three different methods-  on many
architectures writing a byte is much slower than writing a word, so on
those architectures it may be better to use an int (the current method).
On other architectures addresses are word aligned and one may abuse the
pointers with impunity (David's suggested method), and on others pointers
aren't always aligned and a byte write is nice and fast (the method using
a flags byte.)

Jas.

Re: Stable cvs version for 2.4

[David Woodhouse]

jasmine@hex.linuxgrrls.org said:
> > > Adding a byte will destroy the alignment.  you'd have to add a whole
> > > word.
> > Er, what alignment?
>  The alignment of the structure.  Either it's a whole multiple of
> words or it isn't.

Sorry, I was making the unstated assumption that we would have a #define 
and add the byte only in the case where it was required because the 
allocator would _already_ be giving us unaligned structures.

So we never need to add a word instead of a byte to preserve the alignment, 
because in the case where the alignment needs preserving, we didn't need to 
add the byte in the first place.

(And now I'm making the assumption that "aligned structures" == "pointers 
without the low bit set", but that's not unreasonable in the real world.) 

But anyway, having seen that it's not as huge a saving as I thought, and
having seen that I can probably save the 4 bytes of 'size' with less pain,
I may not bother with it at all for a while.

--
dwmw2

RE: Stable cvs version for 2.4

[Allen Curtis]

> Yep.  I agree wholeheartedly.  I would be happiest with having
> compile-time options for the three different methods-  on many
> architectures writing a byte is much slower than writing a word, so on
> those architectures it may be better to use an int (the current method).
> On other architectures addresses are word aligned and one may abuse the
> pointers with impunity (David's suggested method), and on others pointers
> aren't always aligned and a byte write is nice and fast (the method using
> a flags byte.)

Just pick a method that will work for all. No sense in cluttering the code
with conditional compile stuff because we can not choose an implementation.
How about we make the flags element either a byte or a word depending on
architecture. This would only result in a structure redefinition and not
affect the rest of the code.

Re: Stable cvs version for 2.4

[David Woodhouse]

acurtis@onz.com said:
>  Just pick a method that will work for all. No sense in cluttering the
> code with conditional compile stuff because we can not choose an
> implementation. How about we make the flags element either a byte or a
> word depending on architecture. This would only result in a structure
> redefinition and not affect the rest of the code. 

If we're going to do that, we might as well just use a byte and not even 
have the #define.

In the common case, on the architectures where stuff will be aligned, the 
compiler will automatically pad that out so it takes 32 bits anyway, and 
there's no difference. In the case where the compiler doesn't pad, we gain 
three bytes per object.

If we're not going to avoid those 32 bits in the common case, it's not 
really worth the effort.

--
dwmw2

Re: Stable cvs version for 2.4

[Jasmine Strong]

On Wed, 4 Sep 2002, David Woodhouse wrote:

> At a gratuitous extra 4 bytes per object, that's... ok, well it's only
> wasting 32KiB which is less than I expected, but it still annoys me.

Is there nowhere that's not a pointer that you can stuff it?

> > (Architectures that don't always use word-aligned pointers include
> > m68k and ARM Thumb.)
>
> Even when we're allocating from the slab and the object in question is a
> structure of size which is an even number?

If you can ensure that the region you are allocating from is word-aligned,
that the structures within it are always word aligned, and the code will
never run on architectures with weird addressing schemes, fine.

> Even so, I'd assume that we care even _more_ about that 32KiB on uCLinux, so

Yes, I'd agree, but...

> maybe it's worth adding 1 byte of padding to the structure there and
> manually aligning the pointer after allocating, or something. We only need
> 1 bit, after all. Or we could use the _top_ bit on uCLinux, perhaps?

Depends on the architecture, again.  Problem is that treating pointers as
non-opaque objects will necessarily limit portability.

Jas.

Re: Stable cvs version for 2.4

[David Woodhouse]

jasmine@hex.linuxgrrls.org said:
>  Is there nowhere that's not a pointer that you can stuff it? 

struct jffs2_node_frag
{
	rb_node_t rb;
	struct jffs2_full_dnode *node; /* NULL for holes */
	uint32_t size;
	uint32_t ofs; /* Don't really need this, but optimisation */
};

Not really. 'size' and 'ofs' are within the file -- so if you open a file,
seek to 0xFFFFFFFD and write a byte, you can quite happily end up with a
hole frag with size=0xFFFFFFFD and a subsequent data frag with the same ofs.
So I can't abuse the high or low bits like I do with the physical offsets on
the flash in the jffs2_raw_node_ref.

And although I thought at the time that 'size' was an optimisation I don't 
really think it is any more.

Although actually perhaps it is. Must investigate that possibility too -- no
reason why I can't assume that 'frag->size' is always equivalent to
'frag_next(frag)->ofs - frag->ofs', as long as I stick in an end-marker to
show the size of the last frag, if I can stick 0xFFFFFFFF in the 'node'
pointer to mark that or something. Or if you'd shoot me for that too I could
allocate something at boot and use _that_ address as the 'end marker' in the
knowledge that it's not ambiguous.

--
dwmw2