Hello Nithin,
Thanks for the review!
> 2. We'll have to get rid of the OvsDumpVportIoctl() sometime. We need not do
> it in this patch itself.
I think the best time to remove it is when the netlink part is done. We could
add some #ifdef so people know it's not used when the netlink device is used.
> 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.
I think you do not understand the functionality here: the dump next operation
yields a reply with the very next vport.
this code:
instance->dumpState.index[0] = outBucket;
instance->dumpState.index[1] = outIndex;
updates the dump state: we've got an array of lists, so we need an index into
the array (outBucket) and an index into the list therein (outIndex). One index
is not enough.
The code does update the indexes correctly.
> The caller of OvsCreateMsgFromVport() should be responsible for writing out
> the OVS_MESSAGE,
I do not agree with you on OvsCreateMsgFromVport: the name implies that its job
is to build an OVS_MESSAGE out of a given vport.
> I think we should avoid initializing 'nlBuffer' each time and initialize it
> only once in the caller
The dump next operation yields a message with the very next vport. nlBuffer
gets initialized maximum 1 time a dump next call.
nlBuffer is being initialized by OvsCreateMsgFromVport; if no vport is found
(or we've finished with the last one), nlBuffer will not be initialized.
> > + 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".
Yes, checks for the return values of NlMsgPutTailXxx need to be added.
> > + return STATUS_SUCCESS;
> > + }
> > +
> > + msgOut->ovsHdr.dp_ifindex = gOvsSwitchContext->dpNo;
>
> Shouldn't this come from BuildMsgOutFromMsgIn()?
No. The name states that it builds a msg out from a msg in. The dp_ifindex is
not part of a msgin. Besides, it would not make the BuildMsgOutFromMsgIn
function generic, because messages such as a netlink error message lack
dp_ifindex.
> 2. Also, the format of the message would look something like:
> OVS_MESSAGE - VPORT1 - VPORT2 - VPORT3, etc
The format actually looks like this:
OVS_MESSAGE - VPORT1 (dump next: msg1)
OVS_MESSAGE - VPORT2 (dump next: msg2)
OVS_MESSAGE - VPORT3 (dump next: msg3)
> Why are we breaking out of the loop here?
We are breaking the loop whenever we found a vport as "next". Because we reply
with maximum one vport at a time.
> 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.
I don't understand... is there an issue or it's fine?
If there is no vport to dump next, this line will be negated:
*replyLen = msgOut->nlMsg.nlmsgLen;
by making *replyLen be 0, so that the userspace knows it's dump done.
I could put an if there, but the point is that we need to set *reply = 0 to let
the userspace know that we're at dump done.
> 1. You need to handle the return value of OvsCreateMsgFromVport().
What do you want to be done if a dump next for a given vport failed: cancel all
the dump operation (set *replyLen = 0), or try our luck with the next vport
(ignore current vport dump next)?
Btw, I've just remembered: time ago we discussed that the index(es) will be
managed by the userspace.
The current method (having dump state preserved in the "instance" object in the
kernel) makes the dump state be handled by the kernel only, instead.
Should we change our methods back to the original design, or you think it's ok
to continue the way we did so far?
Thanks,
Sam
________________________________________
From: Nithin Raju [nit...@vmware.com]
Sent: Friday, September 19, 2014 2:21 AM
To: Samuel Ghinet
Cc: dev@openvswitch.org; Eitan Eliahu; Saurabh Shah; Ankur Sharma; Alin Serdean
Subject: Re: [PATCH v2] datapath-windows: Netlink command: vport dump
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
