Date: Fri, 9 Feb 2001 08:30:48 +0600 (ALMT) From: Boris Popov <bp@butya.kz> To: Ian Dowse <iedowse@maths.tcd.ie> Cc: freebsd-arch@freebsd.org, freebsd-net@freebsd.org Subject: Re: CFR: Sequential mbuf read/write extensions Message-ID: <Pine.BSF.4.21.0102090752290.24710-100000@lion.butya.kz> In-Reply-To: <200102081233.aa16167@salmon.maths.tcd.ie>
next in thread | previous in thread | raw e-mail | index | archive | help
On Thu, 8 Feb 2001, Ian Dowse wrote: > It may be beneficial to use separate structs for the build and > breakdown operations. The two cases have slightly different > requirements: the mb_count field is only useful when building, and > mb_pos is only strictly necessary when breaking down mbuf chains. > The main advantage of using separate structs is better compiler > type checking, especially in the arguments to functions that need > to break down one chain and build another. Yes, I've been thinking about it, because once I've managed to mix build and breakdown buffers :). The only (not essential) disadvantage is that it will require two init/done functions. > The i386 architecture is not fussy about alignment of multi-byte > types in memory operations. However other architectures are not > so forgiving. Some NIC drivers have to do magic to ensure that IP > packets are 4-byte aligned, but this will not help if you are using > a protocol that does not guarantee 4-byte alignment of 32- or 64-bit > quantities within the IP packet. Doing a > > mb_get_dword(...); > mb_get_byte(...); > mb_get_dword(...); > > will cause an alignment exception on the alpha, for example. No, in the current implementation mb_get* functions will work properly. But mb_put* will fail. This can be avoided by implementing alignment-safe set* macros (which can be written in two variants - first form is for aligned objects and second for bad aligned ones). > Someone suggested using numeric names to indicate the size of the > types rather than 'byte', 'word' etc. I'd agree with this too; the > text names are not intuitive unless you have used dos/windows for > too long :-) Maybe use names such as mb_get_uint32, so that it is > obvious what C type should be passed as an argument. Ok, I'd like type/numeric notation. It is definitely better than just mb_get32. > I wonder if 'mbdata' is the best name for the struct? I think I > had used something like 'mchain', but if separate build/breakdown > structs are used, maybe mbuild/mbreakdown or mbchain/mdchain? (the > NFS code uses the words 'build' and 'dissect' to refer to the two > operations). The main idea would just be to try and have the name > indicate what information is held by the struct. Good point and good names too. > Another useful 'put' function would be something that adds a number > of bytes of 'empty' space to the end of the chain, and sets up a > uio/iovec pointing to this space. e.g to read from a file to an > mbuf chain you could use: > > error = mb_put_muio(mbp, &uio, size, &iovp); > ... > error = VOP_READ(vp, &uio, flag, cred); > ... > FREE(iovp, M_TEMP); This can be added later when the code will be written. > For cases where there is a small (< MLEN) but relatively complex > data structure to be extracted from a chain, it may be useful to > have a function which just rearranges the mbufs to ensure that a > number of bytes become contiguous. It can make an in-mbuf pointer > to that space available. In most cases this will avoid having to > copy the data. Hmm, this can cause weird things if one have two or more such structures in the mbuf chain. Eg, at first point mbufs will be rearranged to place first structure properly but will misplace second structure. But in general case - yes, this is useful. > I wonder if these routines are the correct place for the endian > conversions? It certainly simplifies the code that must build and > parse requests, but requires duplication of each mb_get/mb_put > operation. I understand that there isn't currently code in the tree > for dealing with odd protocols that use little-endian format for > data transmitted on the network (smb is one of these?). sys/netncp is another example of the code which deals with little-endian formatted protocol (and mb* code was derived from sys/netncp/ncp_rq.c) I think it is good idea to provide functions for an in-place conversions because it makes code much more readable and reduces the size of generated code. Few additional functions is a good price for that. > Sometimes it is useful to have idempotent init() and free() functions. > For example, consider a function which builds a request and sends > it, but which must handle errors both before and after the mbuf > chain is sent off to the protocol. If mb_init simply NULL'd out > the mb_top pointer, then the code could look like this: [skip] > The pru_sosend() function takes over ownership of the mbuf chain, > so there is a need to just blank out the mbdata structure without > freeing the chain, and without performing any allocations. An init > function which cannot fail also simplifies the code. See callout_init() > in kern_timeout.c for similar code. Hmm, since so_send() can fail and some erros can be recovered by another call to so_send(), I'm just called m_copym() to duplicate the mbuf chain and give it to so_send(). > The mb_put_pstring function maybe belongs in the protocol-specific > code rather than here, since there are just too many different ways > of encoding strings. Different protocols are likely to encode > strings in different ways, with respect to length field type and > padding/alignment. The name 'pstring' associated with 'pascal' type string which is known as 'byte of length followed by data'. If this function doesn't suits to be general then it can be omitted (only netncp/nwfs code uses it). > Some of these mb_ functions return EBADRPC when not enough bytes > of data are found in the mbuf chain. It might be better to choose > a more generic return code, since these routines are not specific > to RPC. EBADRPC returned by all mb_get* functions to indicate that the format of reply is unexpected. > Ian Thanks for great review :) -- Boris Popov http://www.butya.kz/~bp/ To Unsubscribe: send mail to majordomo@FreeBSD.org with "unsubscribe freebsd-arch" in the body of the message
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?Pine.BSF.4.21.0102090752290.24710-100000>