Date: Thu, 08 Feb 2001 12:33:50 +0000 From: Ian Dowse <iedowse@maths.tcd.ie> To: Boris Popov <bp@butya.kz> Cc: freebsd-arch@freebsd.org, freebsd-net@freebsd.org, iedowse@maths.tcd.ie Subject: Re: CFR: Sequential mbuf read/write extensions Message-ID: <200102081233.aa16167@salmon.maths.tcd.ie> In-Reply-To: Your message of "Tue, 06 Feb 2001 17:50:52 %2B0600." <Pine.BSF.4.21.0102061703030.82511-100000@lion.butya.kz>
next in thread | previous in thread | raw e-mail | index | archive | help
In message <Pine.BSF.4.21.0102061703030.82511-100000@lion.butya.kz>, Boris Popo v writes: > Before starting import process for smbfs, I would like to >introduce new API which greatly simplifies process of packaging data into >mbufs and fetching it back (in fact, similar API already presented in the >tree, but it is private to the netncp code and it will be really nice to >share it). Hi Boris, These mbuf chain manipulation primitives look great! I was playing around with some similar code myself a while ago, so I'll just mention a few of the general issues that may be worth thinking about. I don't have any strong opinions about what approaches are best, so please don't take anything I say too seriously unless you agree with it :-) 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. 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. 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. 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. 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); 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. 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?). 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: mb_init(&mb); if (mb_add_xxx(...) != 0) goto out; ...->pru_sosend(..., mb.mb_top, ...); mb_init(&mb); ... if (error) goto out; out: mb_free(&mb); return (error); 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. 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. 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. Ian 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? <200102081233.aa16167>