hi Samuel,
Thanks for sending out the changes.
Some high level comments are:
1. You might have to rebase your change when Eitan's change gets checked in. He
has coalesced the declarations of the command handlers.
2. We'll have to get rid of the OvsDumpVportIoctl() sometime. We need not do it
in this patch itself.
I have noted the comments inline. Pls. have a look.
On Sep 17, 2014, at 6:08 AM, Samuel Ghinet <sghi...@cloudbasesolutions.com>
wrote:
> /* Netlink vport family. */
> +NETLINK_CMD nlVportFamilyCmdOps[] = {
> + { OVS_VPORT_CMD_GET, OvsGetVportCmdHandler,
> + OVS_WRITE_DEV_OP | OVS_READ_DEV_OP, FALSE }
I am 100% sure which is the correct approach. But other commands, have aligned
OVS_WRITE_DEV_OP with OVS_VPORT_CMD_GET.
Is the ValidateDpIndex FALSE? I'd think it is TRUE. Pls. see the following code
in dpif-linux.c
dpif_linux_port_dump_start__(const struct dpif_linux *dpif,
struct nl_dump *dump)
[...]
request.cmd = OVS_VPORT_CMD_GET;
request.dp_ifindex = dpif->dp_ifindex;
> +/* Netlink vport family. */
> /* XXX: Add commands here. */
You can get rid of the two lines above. The formatting followed is:
/* Netlink XYS family. */
<Command definitions>
<Family definition>
> @@ -966,6 +976,210 @@ OvsSetupDumpStart(POVS_USER_PARAMS_CONTEXT usrParamsCtx)
> }
>
> /*
> +* XXX: When "dump done" and "error" netlink types will be implemented, we
> will
> +* need a few more BuildMsgOut functions, which would ultimately call a
> generic
> +* BuildMsgOutFromMsgIn.
> +*/
> +
> +static VOID
> +BuildMsgOutFromMsgIn(POVS_MESSAGE msgIn, POVS_MESSAGE msgOut, UINT16 flags)
> +{
> + msgOut->nlMsg.nlmsgType = msgIn->nlMsg.nlmsgType;
> + msgOut->nlMsg.nlmsgFlags = flags;
> + msgOut->nlMsg.nlmsgSeq = msgIn->nlMsg.nlmsgSeq;
> + msgOut->nlMsg.nlmsgPid = msgIn->nlMsg.nlmsgPid;
> + msgOut->nlMsg.nlmsgLen = sizeof *msgOut;
> +
> + msgOut->genlMsg.cmd = msgIn->genlMsg.cmd;
> + msgOut->genlMsg.version = nlDatapathFamilyOps.version;
> + msgOut->genlMsg.reserved = 0;
> +}
> +
> +static NTSTATUS
> +OvsCreateMsgFromVport(POVS_VPORT_ENTRY vport,
> + POVS_MESSAGE msgOut,
> + UINT32 maxPayloadSize)
> +{
> + NL_BUFFER nlBuffer;
> + OVS_VPORT_FULL_STATS vportStats;
> +
> + NlBufInit(&nlBuffer, (PCHAR)msgOut + sizeof (*msgOut), maxPayloadSize);
I think we should avoid initializing 'nlBuffer' each time and initialize it
only once in the caller. Each call to OvsCreateMsgFromVport should continue
from the offset it was left off. The caller of OvsCreateMsgFromVport() should
be responsible for writing out the OVS_MESSAGE, and OvsCreateMsgFromVport
should only write out the VPORT data.
> +
> + NlMsgPutTailU32(&nlBuffer, OVS_VPORT_ATTR_PORT_NO, vport->portNo);
> + NlMsgPutTailU32(&nlBuffer, OVS_VPORT_ATTR_TYPE, vport->ovsType);
> +
> + NlMsgPutTailString(&nlBuffer, OVS_VPORT_ATTR_NAME, vport->ovsName);
> +
> + /*
> + * XXX: when we implement OVS_DP_ATTR_USER_FEATURES in datapath,
> + * we'll need to check the OVS_DP_F_VPORT_PIDS flag: if it is set,
> + * it means we have an array of pids, instead of a single pid.
> + * ATM we assume we have one pid only.
> + */
Multiple line comments should be formatted as:
/*
*
*/
and not
/*
*
*
*/
s/ATM/Currently,
> +
> + NlMsgPutTailU32(&nlBuffer, OVS_VPORT_ATTR_UPCALL_PID, vport->upcallPid);
> +
> + /*stats*/
> + vportStats.rxPackets = vport->stats.rxPackets;
> + vportStats.rxBytes = vport->stats.rxBytes;
> + vportStats.txPackets = vport->stats.txPackets;
> + vportStats.txBytes = vport->stats.txBytes;
> + vportStats.rxErrors = vport->errStats.rxErrors;
> + vportStats.txErrors = vport->errStats.txErrors;
> + vportStats.rxDropped = vport->errStats.rxDropped;
> + vportStats.txDropped = vport->errStats.txDropped;
> +
> + NlMsgPutTailUnspec(&nlBuffer, OVS_VPORT_ATTR_STATS, (PCHAR)&vportStats,
> + sizeof(OVS_VPORT_FULL_STATS));
How are you sure that there's enough space in the buffer. NlMsgPutTailUnspec()
might return FALSE. This needs to be translated into "end of this dump-next
operation".
> +
> + /*
> + * XXX: when vxlan udp dest port becomes configurable, we will also need
> + * to add vport options
> + */
> +
> + msgOut->nlMsg.nlmsgLen = NlBufSize(&nlBuffer) + sizeof(OVS_MESSAGE);
I think it is best of the caller of OvsCreateMsgFromVport() wrote out the
OVS_MESSAGE like what you are doing, and here you can just do. It is best if
the NlBufSize() includes the OVS_MESSAGE.
msgOut->nlMsg.nlmsgLen = NlBufSize(&nlBuffer);
> +
> + return STATUS_SUCCESS;
> +}
> +
> +static NTSTATUS
> +OvsGetVportDumpNext(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> + UINT32 *replyLen)
> +{
> + POVS_MESSAGE msgIn;
> + POVS_MESSAGE msgOut = (POVS_MESSAGE)usrParamsCtx->outputBuffer;
> + POVS_OPEN_INSTANCE instance =
> + (POVS_OPEN_INSTANCE)usrParamsCtx->ovsInstance;
> + LOCK_STATE_EX lockState;
> + UINT32 i = 0;
> +
> + /*
> + * XXX: this function shares some code with other dump command(s).
> + * In the future, we will need to refactor the dump functions
> + */
> +
> + ASSERT(usrParamsCtx->devOp == OVS_READ_DEV_OP);
> +
> + if (instance->dumpState.ovsMsg == NULL) {
> + ASSERT(FALSE);
> + return STATUS_INVALID_DEVICE_STATE;
> + }
> +
> + /* Output buffer has been validated while validating read dev op. */
> + ASSERT(msgOut != NULL && usrParamsCtx->outputLength >= sizeof *msgOut);
> +
> + msgIn = instance->dumpState.ovsMsg;
> + BuildMsgOutFromMsgIn(msgIn, msgOut, NLM_F_MULTI);
> +
> + OvsAcquireCtrlLock();
> + if (!gOvsSwitchContext) {
> + /* Treat this as a dump done. */
> + OvsReleaseCtrlLock();
> + *replyLen = 0;
We don't need to set *replyLen to 0, since it is initialized to 0 in
OvsDeviceControl(). I know there are other occurances of *replyLen = 0. We
should get rid of them too at some point.
> + FreeUserDumpState(instance);
This is an interesting situation that gOvsSwitchContext is NULL while the dump
state is setup. This would imply that the dump state was setup while
gOvsSwitchContext was != NULL, and before the dump operation could complete
gOvsSwitchContext became NULL. The best way to handle such a case is to clear
up the dump state in the place where we set gOvsSwitchContext to NULL (in the
NDIS detach handler). But, I am fine with this code for now.
> + return STATUS_SUCCESS;
> + }
> +
> + msgOut->ovsHdr.dp_ifindex = gOvsSwitchContext->dpNo;
Shouldn't this come from BuildMsgOutFromMsgIn()?
> +
> + /*
> + * XXX: when we implement OVS_DP_ATTR_USER_FEATURES in datapath,
> + * we'll need to check the OVS_DP_F_VPORT_PIDS flag: if it is set,
> + * it means we have an array of pids, instead of a single pid.
> + * ATM we assume we have one pid only.
> + */
> +
> + NdisAcquireRWLockRead(gOvsSwitchContext->dispatchLock, &lockState, 0);
> +
> + if (gOvsSwitchContext->numVports > 0) {
> + /* inBucket: the bucket, used for lookup */
> + UINT32 inBucket = instance->dumpState.index[0];
> + /* inIndex: index within the given bucket, used for lookup */
> + UINT32 inIndex = instance->dumpState.index[1];
> + /* the bucket to be used for the next dump operation */
> + UINT32 outBucket = 0;
> + /* the index within the outBucket to be used for the next dump */
> + UINT32 outIndex = 0;
> + UINT32 maxSize = usrParamsCtx->outputLength;
> +
> + for (i = inBucket; i < OVS_MAX_VPORT_ARRAY_SIZE; i++) {
> + PLIST_ENTRY head, link;
> + head = &(gOvsSwitchContext->portHashArray[i]);
> + POVS_VPORT_ENTRY vport = NULL;
> +
> + outIndex = 0;
> + LIST_FORALL(head, link) {
> +
> + /*
> + * if one or more dumps were previously done on this same
> bucket,
> + * inIndex will be > 0, so we'll need to reply with the
> + * inIndex + 1 vport from the bucket.
> + */
> + if (outIndex >= inIndex) {
> + vport = CONTAINING_RECORD(link, OVS_VPORT_ENTRY,
> portLink);
> + OvsCreateMsgFromVport(vport, msgOut, maxSize);
There's something missing here:
1. You need to handle the return value of OvsCreateMsgFromVport().
2. Also, the format of the message would look something like:
OVS_MESSAGE - VPORT1 - VPORT2 - VPORT3, etc
After writing out the OVS_MESSAGE using BuildMsgOutFromMsgIn(), and VPORT1, I
don't see you incrementing the offset when you call into
OvsCreateMsgFromVport(). When OvsCreateMsgFromVport() gets called for VPORT2,
you are again doing a NlMsgPutTailUnspec() and start writing at the same offset
as VPORT1 which will overwrite the data for VPORT1. Also, you should be
decrementing the value of 'maxSize' each time you send write out a VPORT. For
that matter, 'maxSize' should be initialized with
maxSize = usrParamsCtx->outputLength - sizeof *msgOut.
> + ++outIndex;
> + break;
Why are we breaking out of the loop here? Consider the following example, where
there are 5 ports in the bucket, and 'outIndex' and 'inIndex' are 0. We'll hit
the if block, and get into it. After adding a port, we'll break out of the
loop. We need to continue till all the 5 ports are added. Basically, you want
to do something like:
LIST_FORALL(head, link) {
if (outIndex < inIndex) {
/* Skip over all the ports till we get to 'inIndex'. */
/* Break out of the loop to store outIndex and outBucket if there's no
space in outBuffer. */
} else {
/* Add the port at outIndex */
}
outindex++;
}
> + }
> +
> + ++outIndex;
> + }
> +
> + if (vport) {
> + break;
> + }
> +
> + /*
> + * if no vport was found above, check the next bucket, beginning
> + * with the first (i.e. index 0) elem from within that bucket
> + */
> + inIndex = 0;
> + }
> +
> + outBucket = i;
> +
> + /* XXX: what about NLMSG_DONE (as msg type)? */
> + instance->dumpState.index[0] = outBucket;
> + instance->dumpState.index[1] = outIndex;
> + }
> +
> + NdisReleaseRWLock(gOvsSwitchContext->dispatchLock, &lockState);
> +
> + OvsReleaseCtrlLock();
> +
> + *replyLen = msgOut->nlMsg.nlmsgLen;
> +
> + /* if i is OVS_MAX_VPORT_ARRAY_SIZE => no vport was found => dump done*/
> + if (i == OVS_MAX_VPORT_ARRAY_SIZE) {
> + *replyLen = 0;
> + /* Free up the dump state, since there's no more data to continue. */
> + FreeUserDumpState(instance);
> + }
For the case where there are no ports, it could be optimized as:
if (gOvsSwitchContext->numVports > 0) {
} else {
/* Free up the dump state, since there's no more data to continue. */
FreeUserDumpState(instance);
< release locks >
< return >
}
There's one more issue:
Lets say, we added some ports but there's buffer space left. In such a case,
'i' would be 'OVS_MAX_VPORT_ARRAY_SIZE' - basically, we'd have looked through
the entire hash table, but found no ports. In such a case, you negating all the
data added in the output buffer by setting *replyLen = 0. The
FreeUserDumpState() is fine.
> +
> + return STATUS_SUCCESS;
> +}
> +
> +/*
> +* --------------------------------------------------------------------------
> +* Handler for the get vport command. The function handles the initial call
> to
> +* setup the dump state, as well as subsequent calls to continue dumping
> data.
> +* --------------------------------------------------------------------------
> +*/
> +static NTSTATUS
> +OvsGetVportCmdHandler(POVS_USER_PARAMS_CONTEXT usrParamsCtx,
> + UINT32 *replyLen)
> +{
> + if (usrParamsCtx->devOp == OVS_WRITE_DEV_OP) {
> + *replyLen = 0;
We don't need explicit '*replyLen = 0'.
_______________________________________________
dev mailing list
dev@openvswitch.org
http://openvswitch.org/mailman/listinfo/dev
