Re: Proposal to clarify mbuf handling rules
[ note: trimming -current from the CC: list ] Bosko Milekic writes: > 1)The mbuf should be marked read-only explicitly with a single > additional M_FLAG. > > #define M_RDONLY0x0x2000 > > 2)The flag should be ORed in in MEXT_ADD_REF() only if the ref_cnt is > equal to or greater than 2. This is unfortunate because an additional > check would have to be introduced. > > 3)The flag should be removed in _MEXTFREE only if that first > MEXT_IS_REF() evaluates true and if, upon returning from MEXT_REM_REF(), > the new ref_cnt is exactly 1. > > I'm pretty sure that this way, the subsystem will take care of the > read-onlyness itself pretty transparently, so that relevant code can > simply check for the M_RDONLY bit. (2) is questionable. Sounds good. By the way, MEXT_REM_REF() should be defined differently if INVARIANTS is defined so it KASSERT's the reference count is > 0. > As for the protocol routines that rely on the mbuf to be "read-only," > there may be other side-effects besides for this illegal sharing of > multiple-reference ext_bufs that could result from the possibility of > passing these "read-only mbufs" to them. This possibility should be > investigated. Not sure what you mean here.. got an example? > > Cleaning up this sounds like a good plan. It would be worth > > getting Ian and Bosko involved if possible. > > Sure. If I remember correctly, it was Ian who initially brought this > up. This is perhaps a 2-month old issue by now -- I, at the time, was > busy with the referencing stuff as well as the allocator > re-writing/playing around with (which I will have to continue once the > direction of SMP is further cleared up - at least for this part of the > code) - so I did not want to mix apples and oranges. I wonder if Ian has > some code, though. > > I have _some_ modifications regarding this already in my local tree but > have not yet been able to roll over a diff as my monitor is presently > broken (until the end of this week). In any case, how do you propose > coordinating the work? This seems like a fairly straightforward change. > I'm ready to put on hold whatever I'm doing right now in order > to do this, but only if that's okay with you guys - I want to make sure > that no efforts are being duplicated. Let's keep the discussion on freebsd-net. Here's a proposed sequence of steps, at least to get started.. 1. Implement your 1, 2, 3 above: add the flag and adjust the macros 2. Sprinkle code with const's and KASSERT()'s 3. Wait and see what blows up 4. Continue with my proposed changes -Archie ___ Archie Cobbs * Whistle Communications, Inc. * http://www.whistle.com To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposal to clarify mbuf handling rules
On Sun, 27 Aug 2000, David Malone wrote: [...] > (This is why the flag I was talking about in the other mail > would be useful for spotting other cases where the storage > may be writable, even if it's not a cluster). Thoughts: 1) The mbuf should be marked read-only explicitly with a single additional M_FLAG. #define M_RDONLY0x0x2000 2) The flag should be ORed in in MEXT_ADD_REF() only if the ref_cnt is equal to or greater than 2. This is unfortunate because an additional check would have to be introduced. 3) The flag should be removed in _MEXTFREE only if that first MEXT_IS_REF() evaluates true and if, upon returning from MEXT_REM_REF(), the new ref_cnt is exactly 1. I'm pretty sure that this way, the subsystem will take care of the read-onlyness itself pretty transparently, so that relevant code can simply check for the M_RDONLY bit. (2) is questionable. As for the protocol routines that rely on the mbuf to be "read-only," there may be other side-effects besides for this illegal sharing of multiple-reference ext_bufs that could result from the possibility of passing these "read-only mbufs" to them. This possibility should be investigated. > Cleaning up this sounds like a good plan. It would be worth > getting Ian and Bosko involved if possible. > > David. Sure. If I remember correctly, it was Ian who initially brought this up. This is perhaps a 2-month old issue by now -- I, at the time, was busy with the referencing stuff as well as the allocator re-writing/playing around with (which I will have to continue once the direction of SMP is further cleared up - at least for this part of the code) - so I did not want to mix apples and oranges. I wonder if Ian has some code, though. I have _some_ modifications regarding this already in my local tree but have not yet been able to roll over a diff as my monitor is presently broken (until the end of this week). In any case, how do you propose coordinating the work? This seems like a fairly straightforward change. I'm ready to put on hold whatever I'm doing right now in order to do this, but only if that's okay with you guys - I want to make sure that no efforts are being duplicated. Regards, Bosko. [EMAIL PROTECTED] To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposal to clarify mbuf handling rules
David Malone writes: > We were thinking it might be a good idea to have a flag associated > with mbufs which means they are writable, providing the reference > count is 1. Then we can provide a macro for checking writability. > This flag could be set on jumbo ethernet buffers, but not sendfile > buffers (for example). That's a good idea.. I forgot about things like sendfile, where the mbuf is read-only due to other reasons. So we need some kind of flag it seems. That's good -- it makes it even more obvious. -Archie ___ Archie Cobbs * Whistle Communications, Inc. * http://www.whistle.com To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
Re: Proposal to clarify mbuf handling rules
On Sun, Aug 27, 2000 at 02:25:55PM -0700, Archie Cobbs wrote:
> Each mbuf may be either a normal mbuf or a cluster mbuf (if the
> mbuf flags contains M_EXT). Cluster mbufs point to an entire page
> of memory, and this page of memory may be shared by more than one
> cluster mbuf (see m_copypacket()).
Clusters are currently 2048 bytes in size - which I don't think it
a page on the alpha or the i386. (Not that this is really important).
> his effectively makes the mbuf
> data read-only, because a change to one mbuf affects all of the
> mbufs, not just the one you're working on. There have been (and
> still are) several FreeBSD bugs because of this subtlety.
>
> A test for an mbuf being "read-only" is:
>
> if ((m->m_flags & M_EXT) != 0 && MEXT_IS_REF(m)) ...
You should also check that it's really a cluster you're looking
at:
if ((m->m_flags & M_EXT) != 0 &&
(MEXT_IS_REF(m) || (m)->m_ext.ext_free != NULL)) {
/* data is read only */
}
(This is why the flag I was talking about in the other mail
would be useful for spotting other cases where the storage
may be writable, even if it's not a cluster).
Cleaning up this sounds like a good plan. It would be worth
getting Ian and Bosko involved if possible.
David.
To Unsubscribe: send mail to [EMAIL PROTECTED]
with "unsubscribe freebsd-current" in the body of the message
Re: Proposal to clarify mbuf handling rules
On Sun, Aug 27, 2000 at 02:25:55PM -0700, Archie Cobbs wrote: > What do people think? If this is generally agreeable I'll try to > work on putting together a patch set for review. Myself and Ian Dowse have been talking about almost this issue recently in relation to sbcompress. At the moment sbcompress is too conservative about compressing mbuf chains, with the result that it is easily possible to run many machines out of mbuf clusters. (We've seen this problem with netscape and kioslave). At the moment sbcompress only compresses into mbufs, where it could also compress into clusters, providing they have a reference count of 1. However, this still means it can't compress into jumbo buffers associated with gigabit ethernet and the like. We were thinking it might be a good idea to have a flag associated with mbufs which means they are writable, providing the reference count is 1. Then we can provide a macro for checking writability. This flag could be set on jumbo ethernet buffers, but not sendfile buffers (for example). David. To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-current" in the body of the message
