jamal wrote:

On Mon, 2006-27-02 at 03:31 -0500, Shailabh Nagar wrote:


+#define TASKSTATS_LISTEN_GROUP 0x1

You do multicast to this group - does this mean there could be multiple
listeners subscribed for this event?
Yes, the current intent is to allow multiple listeners to receive the responses sent by the kernel.
In our earlier incarnation of the interface (which used connectors), Andrew
suggested that it could be multiplexed for use by other components such as Comprehensive System Accouting,
ELSA etc. that might also be interested in export per-task/process stats.

Since this interface (taskstats) is currently designed for that possibility, having multiple listeners, one for
each "component" such as delay accounting, is the model we're using.
We expect each component to have a pair of userspace programs, one for sending commands and the other to "listen" to all replies + data generated on task exits. The listener is expected to register/deregister interest through
TASKSTATS_CMD_LISTEN and IGNORE.

How does this correlate to TASKSTATS_CMD_LISTEN/IGNORE?
See above. Its mainly an optimization so that if no listener is present, there's no need to generate the data.

Typically, an equivalent of listen is when the first user space app
joins this group; and when it leaves, you have equivalency to ignore.
Unless i misunderstood what you are trying to do with this.
Yes, thats what we're trying to do, except that the "first" listener has no special connotation.



+
+/*
+ * Commands sent from userspace
+ * Not versioned. New commands should only be inserted at the enum's end
+ */
+
+enum {
+       TASKSTATS_CMD_UNSPEC,           /* Reserved */
+       TASKSTATS_CMD_NONE,             /* Not a valid cmd to send
+                                        * Marks data sent on task/tgid exit */

you should call it NONE just because it is unidirectional
(kernel->user). You should check in the kernel if someone sends it from
user space and/or set a command flag etc.
Good point. Should check for users sending it as a cmd and treat it as a noop. I'm just using
this as a placeholder for data thats returned without being requested.

Come to think of it, there's no real reason to have a genlmsghdr for returned data, is there ? Other than to copy the genlmsghdr that was sent so user can identify which command was sent
(and I'm doing that through the reply type, perhaps redundantly).


+       TASKSTATS_CMD_LISTEN,           /* Start listening */
+       TASKSTATS_CMD_IGNORE,           /* Stop listening */
+       TASKSTATS_CMD_PID,              /* Send stats for a pid */
+       TASKSTATS_CMD_TGID,             /* Send stats for a tgid */
+};
+
+/* Parameters for commands
+ * New parameters should only be inserted at the struct's end
+ */
+
+struct taskstats_cmd_param {
+       union {
+               pid_t   pid;
+               pid_t   tgid;
+       } id;
+};
+
+/*
+ * Reply sent from kernel
+ * Version number affects size/format of struct taskstats only
+ */
+
+struct taskstats_reply {
+       enum outtype {
+               TASKSTATS_REPLY_NONE = 1,       /* Control cmd response */
+               TASKSTATS_REPLY_PID,            /* per-pid data cmd response*/
+               TASKSTATS_REPLY_TGID,           /* per-tgid data cmd response*/
+               TASKSTATS_REPLY_EXIT_PID,       /* Exiting task's stats */
+               TASKSTATS_REPLY_EXIT_TGID,      /* Exiting tgid's stats
+                                                * (sent on each tid's exit) */
+       } outtype;
+       __u32 version;
+       __u32 err;
+       struct taskstats stats;                 /* Invalid if err != 0 */
+};
+

Make sure you have proper alignment of above for things like x86-64
Will do.

+/* NETLINK_GENERIC related info */
+
+#define TASKSTATS_GENL_NAME    "TASKSTATS"
+#define TASKSTATS_GENL_VERSION 0x1
+/* Following must be > NLMSG_MIN_TYPE */
+#define TASKSTATS_GENL_ID      0x123

You do not check whether you got this id on registration or not.
It is not guaranteed for you.

Actually, the next iteration of the code will move to dynamically generated ID. But yes,
will need to check for that.

Thanks for the review.
Couple of questions about general netlink:
is it intended to remain a size that will always be aligned to the NLMSG_ALIGNTO so that (NLMSG_DATA(nlhdr) + GENL_HDRLEN) can always be used as a pointer to the genlmsghdr ?

Adding some macros like genlmsg_data(nlh) would be handy (currently I just define and use it locally).


--Shailabh

cheers,
jamal


-
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to [EMAIL PROTECTED]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to