From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.droids-corp.org (zoll.droids-corp.org [94.23.50.67]) by dpdk.org (Postfix) with ESMTP id 1AC428E81 for ; Mon, 23 Nov 2015 10:10:10 +0100 (CET) Received: from was59-1-82-226-113-214.fbx.proxad.net ([82.226.113.214] helo=[192.168.0.10]) by mail.droids-corp.org with esmtpsa (TLS1.2:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.84) (envelope-from ) id 1a0n97-0007HI-M4; Mon, 23 Nov 2015 10:10:50 +0100 Message-ID: <5652D7E9.7050202@6wind.com> Date: Mon, 23 Nov 2015 10:10:01 +0100 From: Olivier MATZ User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Icedove/31.7.0 MIME-Version: 1.0 To: Declan Doherty , dev@dpdk.org References: <1447176763-19303-1-git-send-email-declan.doherty@intel.com> <1447441090-8129-1-git-send-email-declan.doherty@intel.com> <1447441090-8129-7-git-send-email-declan.doherty@intel.com> <564F3BC5.90308@6wind.com> <564F57CE.2030203@intel.com> In-Reply-To: <564F57CE.2030203@intel.com> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Subject: Re: [dpdk-dev] [PATCH v7 06/10] mbuf_offload: library to support attaching offloads to a mbuf X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: patches and discussions about DPDK List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 23 Nov 2015 09:10:10 -0000 Hi Declan, On 11/20/2015 06:26 PM, Declan Doherty wrote: >> The new files are called rte_mbuf_offload, but from what I understand, >> it is more like a mbuf metadata api. What you call "offload operation" >> is not called because an offload is attached, but because you call >> rte_cryptodev_enqueue_burst() on it. > > Maybe rte_mbuf_offload_metadata would be a better name, if not a bit > more long winded :) The idea of this API set is to give a generic > framework for attaching the the offload operation meta data to a mbuf > which will be retrieved at a later point, when the particular offload > burst function is called. I guess as we only have a single offload > device at the moment the API may look a little over the top! Indeed, not sure rte_mbuf_offload_metadata is better. I'm still not convinced that offload should appear in the name, it is a bit confusing with hardware offloads (ol_flags). Also, it suggests that a work is delegated to another entity, but for instance in this case it is just used as a storage area: ol = rte_pktmbuf_offload_alloc(pool, RTE_PKTMBUF_OL_CRYPTO); rte_crypto_op_attach_session(&ol->op.crypto, session); ol->op.crypto.digest.data = rte_pktmbuf_append(m, digest_len); ol->op.crypto.digest.phys_addr = ...; /* ... */ rte_pktmbuf_offload_attach(m, ol); ret = rte_cryptodev_enqueue_burst(dev_id, qp_id, &m, 1); Do you have some other examples in mind that would use this API? >>> +/** Rearms rte_mbuf_offload default parameters */ >>> +static inline void >>> +__rte_pktmbuf_offload_reset(struct rte_mbuf_offload *ol, >>> + enum rte_mbuf_ol_op_type type) >>> +{ >>> + ol->m = NULL; >>> + ol->type = type; >>> + >>> + switch (type) { >>> + case RTE_PKTMBUF_OL_CRYPTO: >>> + __rte_crypto_op_reset(&ol->op.crypto); break; >>> + default: >>> + break; >>> + } >>> +} >> >> Would it work if several OL are registered? > > I can't see any reason why it wouldn't Sorry, I read it to quickly. I thought it was a rte_pktmbuf_offload_detach() function. By the way there is no such function? >> Also, what is not clear to me is how the offload structure is freed. >> For instance, I think that calling rte_pktmbuf_free(m) on a mbuf >> that has a offload attached would result in a leak. >> >> It would mean that it is not allowed to call any function that free or >> reassemble a mbuf when an offload is attached. > > We just need to walk the chain of offloads calling > rte_pktmbuf_offload_free(), before freeing the mbuf, which will be an > issue with any externally attached meta data. In the case of > reassembling I don't see why we would just move the chain to the head mbuf. Walking the chain of offload + adding the initialization will probably have a little cost that should be evaluated. The freeing is probably not the only problem: - packet duplication: are the offload infos copied? If no, it means that the copy is not exactly a copy - if you chain 2 mbufs that both have offload info attached, how does it behave? - if you prepend a segment to your mbuf, you need to copy the mbuf offload pointer, and also parse the list of offload to update the ol->m pointer of each element. >> It seems that these offload structures are only used to pass crypto >> info to the cryptodev. Would it be a problem to have an API like this? >> >> rx_burst(port, q, mbuf_tab, crypto_tab, n); >> > > I really dislike this option, there's no direct linkage between mbuf and > offload operation. > >> Or even: >> >> rx_burst(port, q, crypto_tab, n); >> >> with each *cryto_tab pointing to a mbuf >> > > I looked at this but it would really hamstring any pipelining > applications which might want to attach multiple offloads to a mbuf at a > point in the pipeline for processing at later steps. As far as I can see, there is already a way to chain several crypto operations in the crypto structure. Another option is to use the mbuf offload API (or the m->userdata pointer which already works for that) only in the application: - the field is completely ignored by the mbuf api and the dpdk driver - it is up to the application to initialize it and free it -> it can only be used when passing mbuf from a part of the app to another, so it perfectly matches the pipeline use case. Example: app_core1: /* receive a mbuf */ crypto = alloc() crypto->xxx = yyy: /* ... */ m->userdata = crypto; enqueue(m, app_core2); app_core2: m = dequeue(); crypto = m->userdata; m->userdata = NULL; /* api is tx_burst(port, q, mbuf_tab, crypto_tab, n) */ tx_burst(port, q, &m, &crypto, 1); Regards, Olivier