From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from dpdk.org (dpdk.org [92.243.14.124]) by inbox.dpdk.org (Postfix) with ESMTP id 20BE9A31F3 for ; Sat, 19 Oct 2019 21:48:04 +0200 (CEST) Received: from [92.243.14.124] (localhost [127.0.0.1]) by dpdk.org (Postfix) with ESMTP id 3EB6D1BF1E; Sat, 19 Oct 2019 21:48:03 +0200 (CEST) Received: from EUR03-AM5-obe.outbound.protection.outlook.com (mail-eopbgr30043.outbound.protection.outlook.com [40.107.3.43]) by dpdk.org (Postfix) with ESMTP id EA62C1BECD for ; Sat, 19 Oct 2019 21:48:01 +0200 (CEST) ARC-Seal: i=1; a=rsa-sha256; s=arcselector9901; d=microsoft.com; cv=none; b=VUOeatPaz7xH9KJgLeeB8HQiz0Ys/gJ6VJx2/PW1HNMjIkWEC4i81/grd1V6HrX2rm2c4sNol4DMNFIKSnW2/WwSF0LesIXIs+gYyz3RlYreeWJOrgUuiGWNEtY83MaHjjS9UJ2YKKTV7DRNdZhcx7/53kTACSHmd3728q4bjN3ifS6fvJ+g/iZN541RCFA9FEqY//mC+hqsQwv7Ktb6SQIUN/vTbPMsQjgImH7fWSpT/qeNHhLF7e7mAUvgVo+tez0hOAt6vB1Q2LKiriFOECvoYwtSccwfx0Uy4LDbzAJia7ohLMK8rjS0DAccgSeb6N1VrtUEQitCji1tvHiAPA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=microsoft.com; s=arcselector9901; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=MDWscY/oVehzYMOTe1lVDxDB0B8eRAdohkHppNYbRJw=; b=DR7Dy+r1lYaiYrM4QYJNs/Y81KrReajDqXDj0jsbgAFi82ZE4PGYWpHR2bvzNqVl9CrHOIkgf9I8FPqt/KDiR3I/CZX5ULRMZFqSRB1FD/7gF0/eDZwcKfHfEiop9T4pmJj6VXe704VZtc8DJoxGuMxO6s3TS9+Rv3YwjdjD0I+nWHIuVRgKur0gUnXjh7yJM6z3GLQHcgyzsv9XQj5OOojZBe6dUlQmrG3fo0MV/kOKD0S4Z5t5S9wQJHX6Xr/rx5ZM4A5kwaaiMoioumakW4NpFb4j7z011Z2Xq995yv+a+yWl7UTAlSTpASPx83xy2iABqBg25Rvz3u2Q+gjmCA== ARC-Authentication-Results: i=1; mx.microsoft.com 1; spf=pass smtp.mailfrom=mellanox.com; dmarc=pass action=none header.from=mellanox.com; dkim=pass header.d=mellanox.com; arc=none DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=Mellanox.com; s=selector2; h=From:Date:Subject:Message-ID:Content-Type:MIME-Version:X-MS-Exchange-SenderADCheck; bh=MDWscY/oVehzYMOTe1lVDxDB0B8eRAdohkHppNYbRJw=; b=Ijyu5H7T6zka2WcEPy8Ak7kH+y8JMLnkvgJ2VWnwjReYbhKiVrOhqDxVxyNsaq4SnPXnzblEkbRJ4RXa8pB1uD+KrT7yCfKMfD4e4zRd8MjNFNYPR3pKveBtB6LrgF4zqNnv8+oLFPT6zhX4WMP6nM+OMLZ/jUGkR5FbwX+Up9Y= Received: from AM4PR05MB3265.eurprd05.prod.outlook.com (10.171.188.154) by AM4PR05MB3267.eurprd05.prod.outlook.com (10.171.188.28) with Microsoft SMTP Server (version=TLS1_2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id 15.20.2347.21; Sat, 19 Oct 2019 19:48:00 +0000 Received: from AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::edab:529f:d14e:d3b]) by AM4PR05MB3265.eurprd05.prod.outlook.com ([fe80::edab:529f:d14e:d3b%7]) with mapi id 15.20.2347.026; Sat, 19 Oct 2019 19:48:00 +0000 From: Slava Ovsiienko To: Olivier Matz CC: "dev@dpdk.org" , Matan Azrad , Raslan Darawsheh , Thomas Monjalon , Yongseok Koh Thread-Topic: [PATCH v2] ethdev: extend flow metadata Thread-Index: AQHVhZWNhabx0gMeL0inCxF4J+ZF1adiS/Tg Date: Sat, 19 Oct 2019 19:47:59 +0000 Message-ID: References: <20190704232122.19477-1-yskoh@mellanox.com> <1570723359-24650-1-git-send-email-viacheslavo@mellanox.com> <20191018092219.z5yoldgnicltypjr@platinum> In-Reply-To: <20191018092219.z5yoldgnicltypjr@platinum> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: authentication-results: spf=none (sender IP is ) smtp.mailfrom=viacheslavo@mellanox.com; x-originating-ip: [95.164.10.10] x-ms-publictraffictype: Email x-ms-office365-filtering-correlation-id: ce853bc0-ba6b-4fc2-cb5d-08d754cd3f25 x-ms-office365-filtering-ht: Tenant x-ms-traffictypediagnostic: AM4PR05MB3267:|AM4PR05MB3267: x-ld-processed: a652971c-7d2e-4d9b-a6a4-d149256f461b,ExtAddr,ExtFwd x-ms-exchange-transport-forked: True x-microsoft-antispam-prvs: x-ms-oob-tlc-oobclassifiers: OLM:10000; x-forefront-prvs: 01952C6E96 x-forefront-antispam-report: SFV:NSPM; SFS:(10009020)(4636009)(396003)(346002)(39840400004)(136003)(366004)(376002)(54094003)(13464003)(199004)(189003)(6506007)(486006)(25786009)(256004)(316002)(7696005)(26005)(52536014)(14444005)(478600001)(561944003)(6436002)(5660300002)(76176011)(55016002)(54906003)(53546011)(33656002)(66066001)(30864003)(11346002)(71190400001)(66556008)(102836004)(71200400001)(66476007)(66446008)(64756008)(229853002)(476003)(6916009)(74316002)(76116006)(14454004)(81166006)(107886003)(7736002)(9686003)(66946007)(99286004)(186003)(81156014)(86362001)(8676002)(2906002)(4326008)(6116002)(3846002)(6246003)(8936002)(305945005)(446003)(21314003)(309714004); DIR:OUT; SFP:1101; SCL:1; SRVR:AM4PR05MB3267; H:AM4PR05MB3265.eurprd05.prod.outlook.com; FPR:; SPF:None; LANG:en; PTR:InfoNoRecords; A:1; MX:1; received-spf: None (protection.outlook.com: mellanox.com does not designate permitted sender hosts) x-ms-exchange-senderadcheck: 1 x-microsoft-antispam: BCL:0; x-microsoft-antispam-message-info: W8P+WziO6EmcxuCpQAYhl65KAHJ7XpBx1kfSlzQorxBmLQbMJLZ7rX4GC3oErqFgNmRQCoPYSIaE1fNMyrzhv5vENifjteRwqLIleihinuHBDjIBwyb/p7oyaUrS2GewPy78anVrEVj4jRF1dbDpDlp10vaGE4hVmQf+CFmosrOS1bcSBApybwnlha2MmifzXujnRiqaKLPuqT8Vt38wxmbB7xXUV3oPSBGZpwwhORCw/8qyp9OuoP+ePO5vpwW9Jj/EMo0y2Z1KTqmtnEGv7rYTX61wpIWDZOHWNh02UQN1wctCr+PJtYhLVMbZdJdIGmL1ZTxR1dm00uVOChhRhYGAlHQttUPPHKxK3xKR0HVohpK1uNuK95kvrG1MMaAr7rQPbqj5bO9ZEKz84nafPE+KB2mFdSZxcsuJdM+B82I= Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: quoted-printable MIME-Version: 1.0 X-OriginatorOrg: Mellanox.com X-MS-Exchange-CrossTenant-Network-Message-Id: ce853bc0-ba6b-4fc2-cb5d-08d754cd3f25 X-MS-Exchange-CrossTenant-originalarrivaltime: 19 Oct 2019 19:47:59.4508 (UTC) X-MS-Exchange-CrossTenant-fromentityheader: Hosted X-MS-Exchange-CrossTenant-id: a652971c-7d2e-4d9b-a6a4-d149256f461b X-MS-Exchange-CrossTenant-mailboxtype: HOSTED X-MS-Exchange-CrossTenant-userprincipalname: gT08wle5m9wKYSqcdunrrR9y7VcSSgEU/C+ESpHDdkYic+Sw3SNI2y7+xSierLn4RsVPJJESEx7tkVfdRID96aehn4fQv5JkR0pMeoE1uaw= X-MS-Exchange-Transport-CrossTenantHeadersStamped: AM4PR05MB3267 Subject: Re: [dpdk-dev] [PATCH v2] ethdev: extend flow metadata X-BeenThere: dev@dpdk.org X-Mailman-Version: 2.1.15 Precedence: list List-Id: DPDK patches and discussions List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dev-bounces@dpdk.org Sender: "dev" Hi, Olivier Thank you for your comment (and for the dynamic mbuf patch, btw). Please, s= ee below. > -----Original Message----- > From: Olivier Matz > Sent: Friday, October 18, 2019 12:22 > To: Slava Ovsiienko > Cc: dev@dpdk.org; Matan Azrad ; Raslan > Darawsheh ; Thomas Monjalon > ; Yongseok Koh > Subject: Re: [PATCH v2] ethdev: extend flow metadata >=20 > Hi Viacheslav, >=20 > Few comments on the dynamic mbuf part below. >=20 [snip] > > @@ -12,10 +12,18 @@ > > #include > > #include > > #include > > +#include > > +#include > > #include "rte_ethdev.h" > > #include "rte_flow_driver.h" > > #include "rte_flow.h" > > > > +/* Mbuf dynamic field name for metadata. */ int > > +rte_flow_dynf_metadata_offs =3D -1; > > + > > +/* Mbuf dynamic field flag bit number for metadata. */ uint64_t > > +rte_flow_dynf_metadata_mask; > > + > > /** > > * Flow elements description tables. > > */ > > @@ -153,8 +161,41 @@ struct rte_flow_desc_data { > > MK_FLOW_ACTION(DEC_TCP_SEQ, sizeof(rte_be32_t)), > > MK_FLOW_ACTION(INC_TCP_ACK, sizeof(rte_be32_t)), > > MK_FLOW_ACTION(DEC_TCP_ACK, sizeof(rte_be32_t)), > > + MK_FLOW_ACTION(SET_META, sizeof(struct > rte_flow_action_set_meta)), > > }; > > > > +int > > +rte_flow_dynf_metadata_register(void) > > +{ > > + int offset; > > + int flag; > > + > > + static const struct rte_mbuf_dynfield desc_offs =3D { > > + .name =3D MBUF_DYNF_METADATA_NAME, > > + .size =3D MBUF_DYNF_METADATA_SIZE, > > + .align =3D MBUF_DYNF_METADATA_ALIGN, > > + .flags =3D MBUF_DYNF_METADATA_FLAGS, > > + }; > > + static const struct rte_mbuf_dynflag desc_flag =3D { > > + .name =3D MBUF_DYNF_METADATA_NAME, > > + }; >=20 > I don't see think we need #defines. > You can directly use the name, sizeof() and __alignof__() here. > If the information is used externally, the structure shall be made global= non- > static. The intention was to gather all dynamic fields definitions in one place=20 (in rte_mbuf_dyn.h). It would be easy to see all fields in one sight (some might be shared, some might be mutual exclusive, estimate mbuf space, required by various features, etc.). So, we can't just fill structure field= s with simple sizeof() and alignof() instead of definitions (the field parame= ters must be defined once). I do not see the reasons to make table global. I would prefer the definitio= ns. - the definitions are compile time processing (table fields are runtime), it provides code optimization and better performance. > > + > > + offset =3D rte_mbuf_dynfield_register(&desc_offs); > > + if (offset < 0) > > + goto error; > > + flag =3D rte_mbuf_dynflag_register(&desc_flag); > > + if (flag < 0) > > + goto error; > > + rte_flow_dynf_metadata_offs =3D offset; > > + rte_flow_dynf_metadata_mask =3D (1ULL << flag); > > + return 0; > > + > > +error: > > + rte_flow_dynf_metadata_offs =3D -1; > > + rte_flow_dynf_metadata_mask =3D 0ULL; > > + return -rte_errno; > > +} > > + > > static int > > flow_err(uint16_t port_id, int ret, struct rte_flow_error *error) { > > diff --git a/lib/librte_ethdev/rte_flow.h > > b/lib/librte_ethdev/rte_flow.h index 391a44a..a27e619 100644 > > --- a/lib/librte_ethdev/rte_flow.h > > +++ b/lib/librte_ethdev/rte_flow.h > > @@ -27,6 +27,8 @@ > > #include > > #include > > #include > > +#include > > +#include > > > > #ifdef __cplusplus > > extern "C" { > > @@ -417,7 +419,8 @@ enum rte_flow_item_type { > > /** > > * [META] > > * > > - * Matches a metadata value specified in mbuf metadata field. > > + * Matches a metadata value. > > + * > > * See struct rte_flow_item_meta. > > */ > > RTE_FLOW_ITEM_TYPE_META, > > @@ -1213,9 +1216,17 @@ struct rte_flow_item_icmp6_nd_opt_tla_eth { > > #endif > > > > /** > > - * RTE_FLOW_ITEM_TYPE_META. > > + * @warning > > + * @b EXPERIMENTAL: this structure may change without prior notice > > * > > - * Matches a specified metadata value. > > + * RTE_FLOW_ITEM_TYPE_META > > + * > > + * Matches a specified metadata value. On egress, metadata can be set > > + either by > > + * mbuf tx_metadata field with PKT_TX_METADATA flag or > > + * RTE_FLOW_ACTION_TYPE_SET_META. On ingress, > > + RTE_FLOW_ACTION_TYPE_SET_META sets > > + * metadata for a packet and the metadata will be reported via mbuf > > + metadata > > + * dynamic field with PKT_RX_DYNF_METADATA flag. The dynamic mbuf > > + field must be > > + * registered in advance by rte_flow_dynf_metadata_register(). > > */ > > struct rte_flow_item_meta { > > rte_be32_t data; > > @@ -1813,6 +1824,13 @@ enum rte_flow_action_type { > > * undefined behavior. > > */ > > RTE_FLOW_ACTION_TYPE_DEC_TCP_ACK, > > + > > + /** > > + * Set metadata on ingress or egress path. > > + * > > + * See struct rte_flow_action_set_meta. > > + */ > > + RTE_FLOW_ACTION_TYPE_SET_META, > > }; > > > > /** > > @@ -2300,6 +2318,43 @@ struct rte_flow_action_set_mac { > > uint8_t mac_addr[RTE_ETHER_ADDR_LEN]; }; > > > > +/** > > + * @warning > > + * @b EXPERIMENTAL: this structure may change without prior notice > > + * > > + * RTE_FLOW_ACTION_TYPE_SET_META > > + * > > + * Set metadata. Metadata set by mbuf tx_metadata field with > > + * PKT_TX_METADATA flag on egress will be overridden by this action. > > +On > > + * ingress, the metadata will be carried by mbuf metadata dynamic > > +field > > + * with PKT_RX_DYNF_METADATA flag if set. The dynamic mbuf field > > +must be > > + * registered in advance by rte_flow_dynf_metadata_register(). > > + * > > + * Altering partial bits is supported with mask. For bits which have > > +never > > + * been set, unpredictable value will be seen depending on driver > > + * implementation. For loopback/hairpin packet, metadata set on Rx/Tx > > +may > > + * or may not be propagated to the other path depending on HW > capability. > > + * > > + * RTE_FLOW_ITEM_TYPE_META matches metadata. > > + */ > > +struct rte_flow_action_set_meta { > > + rte_be32_t data; > > + rte_be32_t mask; > > +}; > > + > > +/* Mbuf dynamic field offset for metadata. */ extern int > > +rte_flow_dynf_metadata_offs; > > + > > +/* Mbuf dynamic field flag mask for metadata. */ extern uint64_t > > +rte_flow_dynf_metadata_mask; > > + > > +/* Mbuf dynamic field pointer for metadata. */ #define > > +RTE_FLOW_DYNF_METADATA(m) \ > > + RTE_MBUF_DYNFIELD((m), rte_flow_dynf_metadata_offs, uint32_t > *) > > + > > +/* Mbuf dynamic flag for metadata. */ #define PKT_RX_DYNF_METADATA > > +(rte_flow_dynf_metadata_mask) > > + >=20 > I wonder if helpers like this wouldn't be better, because they combine th= e > flag and the field: >=20 > /** > * Set metadata dynamic field and flag in mbuf. > * > * rte_flow_dynf_metadata_register() must have been called first. > */ > __rte_experimental > static inline void rte_mbuf_dyn_metadata_set(struct rte_mbuf *m, > uint32_t metadata) { > *RTE_MBUF_DYNFIELD(m, rte_flow_dynf_metadata_offs, > uint32_t *) =3D metadata; > m->ol_flags |=3D rte_flow_dynf_metadata_mask; } Setting flag looks redundantly. What if driver just replaces the metadata and flag is already set? The other option - the flags (for set of fields) might be set in combinatio= ns. mbuf field is supposed to be engaged in datapath, performance is very critical, adding one more abstraction layer seems not to be relevant. Also, metadata is not feature of mbuf. It should have rte_flow prefix. > /** > * Get metadata dynamic field value in mbuf. > * > * rte_flow_dynf_metadata_register() must have been called first. > */ > __rte_experimental > static inline int rte_mbuf_dyn_metadata_get(const struct rte_mbuf *m, > uint32_t *metadata) { > if ((m->ol_flags & rte_flow_dynf_metadata_mask) =3D=3D 0) > return -1; What if metadata is 0xFFFFFFFF ? The checking of availability might embrace larger code block,=20 so this might be not the best place to check availability. > *metadata =3D *RTE_MBUF_DYNFIELD(m, rte_flow_dynf_metadata_offs, > uint32_t *); > return 0; > } >=20 > /** > * Delete the metadata dynamic flag in mbuf. > * > * rte_flow_dynf_metadata_register() must have been called first. > */ > __rte_experimental > static inline void rte_mbuf_dyn_metadata_del(struct rte_mbuf *m) { > m->ol_flags &=3D ~rte_flow_dynf_metadata_mask; } >=20 Sorry, I do not see the practical usecase for these helpers. In my opinion = it is just some kind of obscuration. They do replace the very simple code and introduce some risk of performance= impact. >=20 > > /* > > * Definition of a single action. > > * > > @@ -2533,6 +2588,32 @@ enum rte_flow_conv_op { }; > > > > /** > > + * Check if mbuf dynamic field for metadata is registered. > > + * > > + * @return > > + * True if registered, false otherwise. > > + */ > > +__rte_experimental > > +static inline int > > +rte_flow_dynf_metadata_avail(void) { > > + return !!rte_flow_dynf_metadata_mask; } >=20 > _registered() instead of _avail() ? Accepted, sounds better. >=20 > > + > > +/** > > + * Register mbuf dynamic field and flag for metadata. > > + * > > + * This function must be called prior to use SET_META action in order > > +to > > + * register the dynamic mbuf field. Otherwise, the data cannot be > > +delivered to > > + * application. > > + * > > + * @return > > + * 0 on success, a negative errno value otherwise and rte_errno is s= et. > > + */ > > +__rte_experimental > > +int > > +rte_flow_dynf_metadata_register(void); > > + > > +/** > > * Check whether a flow rule can be created on a given port. > > * > > * The flow rule is validated for correctness and whether it could be > > accepted diff --git a/lib/librte_mbuf/rte_mbuf_dyn.h > > b/lib/librte_mbuf/rte_mbuf_dyn.h index 6e2c816..4ff33ac 100644 > > --- a/lib/librte_mbuf/rte_mbuf_dyn.h > > +++ b/lib/librte_mbuf/rte_mbuf_dyn.h > > @@ -160,4 +160,12 @@ int rte_mbuf_dynflag_lookup(const char *name, > > */ > > #define RTE_MBUF_DYNFIELD(m, offset, type) ((type)((uintptr_t)(m) + > > (offset))) > > > > +/** > > + * Flow metadata dynamic field definitions. > > + */ > > +#define MBUF_DYNF_METADATA_NAME "flow-metadata" > > +#define MBUF_DYNF_METADATA_SIZE sizeof(uint32_t) #define > > +MBUF_DYNF_METADATA_ALIGN __alignof__(uint32_t) #define > > +MBUF_DYNF_METADATA_FLAGS 0 >=20 > If this flag is only to be used in rte_flow, it can stay in rte_flow. > The name should follow the function name conventions, I suggest > "rte_flow_metadata". The definitions: MBUF_DYNF_METADATA_NAME,=20 MBUF_DYNF_METADATA_SIZE, MBUF_DYNF_METADATA_ALIGN are global. rte_flow proposes only minimal set tyo check and access the metadata. By knowing the field names applications would have the more flexibility in processing the fields, for example it allows to optimi= ze the handling of multiple dynamic fields . The definition of metadata size a= llows to generate optimized code: #if MBUF_DYNF_METADATA_SIZE =3D=3D sizeof(uint32) *RTE_MBUF_DYNFIELD(m) =3D get_metadata_32bit() #else *RTE_MBUF_DYNFIELD(m) =3D get_metadata_64bit() #endif MBUF_DYNF_METADATA_FLAGS flag is not used by rte_flow, this flag is related exclusively to dynamic mbuf " Reserved for future use= , must be 0". Would you like to drop this definition? >=20 > If the flag is going to be used in several places in dpdk (rte_flow, pmd,= app, > ...), I wonder if it shouldn't be defined it in rte_mbuf_dyn.c. I mean: >=20 > =3D=3D=3D=3D > /* rte_mbuf_dyn.c */ > const struct rte_mbuf_dynfield rte_mbuf_dynfield_flow_metadata =3D { > ... > }; In this case we would make this descriptor global. It is no needed, because there Is no supposed any usage, but by rte_flow_dynf_metadata_register() only. The=20 > int rte_mbuf_dynfield_flow_metadata_offset =3D -1; const struct > rte_mbuf_dynflag rte_mbuf_dynflag_flow_metadata =3D { > ... > }; > int rte_mbuf_dynflag_flow_metadata_bitnum =3D -1; >=20 > int rte_mbuf_dyn_flow_metadata_register(void) > { > ... > } >=20 > /* rte_mbuf_dyn.h */ > extern const struct rte_mbuf_dynfield rte_mbuf_dynfield_flow_metadata; > extern int rte_mbuf_dynfield_flow_metadata_offset; > extern const struct rte_mbuf_dynflag rte_mbuf_dynflag_flow_metadata; > extern int rte_mbuf_dynflag_flow_metadata_bitnum; >=20 > ...helpers to set/get metadata... > =3D=3D=3D >=20 > Centralizing the definitions of non-private dynamic fields/flags in > rte_mbuf_dyn may help other people to reuse a field that is well describe= d if > it match their use-case. Yes, centralizing is important, that's why MBUF_DYNF_METADATA_xxx placed in rte_mbuf_dyn.h. Do you think we should share the descriptors either? I have no idea why someone (but rte_flow_dynf_metadata_register()) might register metadata field directly. >=20 > In your case, what is carried by metadata? Could it be reused by others? = I > think some more description is needed. In my case, metadata is just opaquie rte_flow related 32-bit unsigned value= provided by mlx5 hardrware in rx datapath. I have no guess whether someone wishes to re= use. Brief summary of you comment (just to make sure I understood your proposal = in correct way): 1. drop all definitions MBUF_DYNF_METADATA_xxx, leave MBUF_DYNF_METADATA_NA= ME only 2. move the descriptor const struct rte_mbuf_dynfield desc_offs =3D {} to r= te_mbuf_dyn.c and make it global 3. provide helpers to access metadata [1] and [2] look OK in general. Although I think these ones make code less = flexible, restrict the potential compile time options. For now it is rather theoretical question, if you insist on your approach -= please, let me know, I'll address [1] and [2] and update.my patch. As for [3] - IMHO, the extra abstraction layer is not useful, and might be = even harmful. I tend not to complicate the code, at least, for now. With best regards, Slava =20 > Regards, > Olivier