Hi, you can read the numbers I posted as you like, but in the non-SMP case performance is not "seemingly" better, it IS better.
Before people think differently: the code i posted is _perfectly safe_ to use under the assumption that I used -- i.e. that you hold Giant when manipulating the free list -- on both non-SMP (where i tested it, and where it does improve performancw) and SMP systems (where I have no idea about performance, but i might have some numbers by tomorrow), under the assumption that I used -- i.e. that you hold Giant when manipulating the free list. The above assumption holds in the device drivers i patched, and also (at the time of this writing, and I suspect for quite a bit, although I and hopefully others are working on this) in all the network stack. And of course you can #ifdef the code out for the SMP case if you don't trust it so you won't pessimize the SMP case. Also: I am not violating _any_ interface. I am just holding apart a small set of buffers for future use, which is exactly the same thing that each and every network device driver in FreeBSD does when it links them in the receive ring -- no guarantee when/if they will be freed, certainly none of them is freed even if the system runs out of mbufs (unless the network interface is brought down, which i doubt it happens). This said, a detailed answer to your points: #1 I think your comment on the sleepers and mbuf exhaustion is not correct. I am only talking of holding apart a small set of buffers+clusters, in the order of ~50 blocks, and you can restrict the consumers of the free pool to those who do not want to sleep, so there is not need to special-case the code for the sleepers. Or, if you really want to special case it, the first sleeper which finds the free pool empty and needs to go to sleep will reset max_pool_size to 0 forcing the same behaviour we have now with correct handling of wakeup and such. #2 given that all i am asking for is defining an interface for this type of optimization, when the time comes that Giant goes, and assuming I am not retired yet, it will still change nothing for the non-SMP case, and for the SMP case we can run our performance tests and if it proves to be useful we can put in whatever is necessary. #3 easy to fix (assuming this is really a problem) -- just have the free pool organized as a list of pointers to the mbufs, rather than using the m_nextpkt field as the link field. Because the free list is short and fixed size you can even use an array to hold it. #4 same answer of #2: if/when... we can easily fix things in just one place. Until then, we are just pessimizing things for no good reason. In terms of API, since we both agree that we need one (and we do need one, otherwise driver writers will just go ahead and do their own, replicating work in each driver, as I am sure I^hthey will start doing now), may I suggest that we export two-- one where the caller is assumed to already hold a lock on the CPU it is running on, and one which also does the necessary locking upfront and calls the latter afterwards ? This way the former will be usable right now and efficiently from all contexts which run under Giant, and easy to upgrade when/if we need it. And, I am sorry to say... you raise some valid points, but without performance numbers to justify them, it is hard to tell how relevant they are in practice. I have made too many mistakes of this kind myself to believe my intuition. cheers luigi On Tue, Jul 02, 2002 at 09:03:54AM -0400, Bosko Milekic wrote: > > There are several problems with your "simple" code that make it simpler > > than what's in -CURRENT and in -STABLE and therefore yield [obviously] > > seemingly better times. > > > > 1) sleeping in the case of a system exhausted of mbufs and clusters is > > now broken because code that frees to this "combined" list will not > > generate wake ups; even if you build this functionality in, you would > > have to code the wakeup for a "special" case where one obtains both a > > cluster and an mbuf; it'll get complicated. > > > > 2) no per-CPU caches, you bring contention (in -CURRENT) from per-CPU > > back to global, which you probably don't care about now, but which will > > matter when Giant goes completely. > > > > 3) you violate write/read producer/consumer model. In -CURRENT, we try > > to not have the code doing the freeing write to any part of the mbuf, > > thus hopefully avoiding cache invalidation from occuring during a free > > that happens from a thread that doesn't usually write to the mbuf at > > all. > > Forgot, also, in -CURRENT: > > 4) breaks framework in mb_alloc that groups pages of mbufs and > clusters together allowing them to be freed in the future, if/when we > decide to implement lazy freeing; it would do the same thing if we > mvoed mbuf allocations to UMA, because grouping together mbufs and > clusters on a separate list will violate that scheme. > > > I think that because of (1) and (2), especially, and because the kernel > > in -CURRENT is the way it is right now, you're getting better > > performance numbers. Try exhausting the system and seeing how far you > > get. I suspect you'll eventually just hang (because you'll allocate a > > bunch of stuff violating the real interface and then won't be able to > > wakeup and Do The Right Thing). > > > > While, as I said before, I agree that we should have an interface that > > allocates both an mbuf and a cluster, I don't think that this is the > > right approach. I believe that the correct approach would be to couple > > the mbuf and cluster allocation under a single per-CPU lock/unlock > > operation, which isn't too difficult to do. > 1 To Unsubscribe: send mail to [EMAIL PROTECTED] with "unsubscribe freebsd-net" in the body of the message